fix(rpc): guard TxsResults length before indexing by block tx position - #2175
fix(rpc): guard TxsResults length before indexing by block tx position#2175JayT106 wants to merge 9 commits into
TxsResults length before indexing by block tx position#2175Conversation
📝 WalkthroughWalkthroughThe RPC block detail path validates transaction and transaction-result counts before indexing results. It returns nil result fields on related errors. Tests cover matching counts and both mismatch directions. The changelog records the fix. ChangesRPC transaction results validation
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
01412b3 to
5cae236
Compare
|
@claude review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
x/cronos/rpc/api_test.go (1)
12-48: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftAdd a regression test through
getBlockDetail.The current tests call
checkTxsResultsLengthdirectly. They would still pass ifgetBlockDetailstopped calling the guard. Add a backend-stub or fixture test that supplies mismatched lengths throughgetBlockDetailand verifies that it returns an error before BaseFee calculation or transaction-result indexing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@x/cronos/rpc/api_test.go` around lines 12 - 48, The current tests only cover checkTxsResultsLength directly; add a regression test that exercises the public getBlockDetail flow with mismatched transaction and result counts. Use a backend stub or fixture to provide the mismatch, then assert getBlockDetail returns the expected error before BaseFee calculation or transaction-result indexing occurs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@x/cronos/rpc/api_test.go`:
- Around line 12-48: Restructure TestCheckTxsResultsLength as a suite.Suite test
with a testCases table containing name, malleate func(), and expected outcomes,
and add the required doc comment for the exported test function. Move
newResBlock and newBlockRes into setup helpers that call t.Helper() or
s.T().Helper(), while preserving the existing matching, fewer-results, and
extra-results assertions.
- Around line 38-47: Update both checkTxsResultsLength test cases to assert the
complete expected error message, including the block height and both transaction
counts, rather than only checking error presence or a generic substring. Keep
the existing fewer-results and more-results scenarios unchanged while using
exact error comparisons for each case.
---
Nitpick comments:
In `@x/cronos/rpc/api_test.go`:
- Around line 12-48: The current tests only cover checkTxsResultsLength
directly; add a regression test that exercises the public getBlockDetail flow
with mismatched transaction and result counts. Use a backend stub or fixture to
provide the mismatch, then assert getBlockDetail returns the expected error
before BaseFee calculation or transaction-result indexing occurs.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2ebde0a1-a775-44b6-a969-30a6b1d7361e
📒 Files selected for processing (3)
CHANGELOG.mdx/cronos/rpc/api.gox/cronos/rpc/api_test.go
|
Claude finished @JayT106's task in 1m 7s —— View job Code Review
SummaryThe fix is well-targeted and correct. The panic vector is real: No state-machine breaking changes — this is purely an RPC-layer guard with no impact on consensus or module state. Issues FoundMinor — CHANGELOG references wrong PR number The CHANGELOG entry at line 11 links to * [#2174](...) fix(rpc): guard `TxsResults` length ...Should be Positive Notes
Nits
|
Signed-off-by: JayT106 <JayT106@users.noreply.github.com>
| api.logger.Debug("failed to retrieve block results", "height", blockNumber, "error", err.Error()) | ||
| return resBlock, blockNumber, blockHash, blockRes, baseFee, err | ||
| } | ||
| if err = checkTxsResultsLength(resBlock, blockRes); err != nil { |
There was a problem hiding this comment.
You are still returning resBlock and blockRes which could be missuses, perhaps some refactoring could be done to clarify the semantic of resBlock and resBlock ?
Do we allow them to have different length or not?
There was a problem hiding this comment.
renamed to block and blockResults, and for the returning only return the err info.
This is to match with the consensus raw data check, the length should always the same. if the data mismatch, meaning we need to migrate/patch the data like fix-unlucky-tx in v0.7 before.
…ultsLength naming
Signed-off-by: JayT106 <JayT106@users.noreply.github.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
x/cronos/rpc/api_test.go (1)
70-79: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCover the production call site.
Add a
getBlockDetailtest with mismatched counts. Assert that it returns the validation error before receipt andReplayBlockprocessing. Otherwise, the helper can remain covered while the RPC panic fix regresses.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@x/cronos/rpc/api_test.go` around lines 70 - 79, Add a test for the production getBlockDetail call path using mismatched transaction and result counts, and assert it returns the expected validation error before receipt handling or ReplayBlock processing occurs. Keep the existing checkTxsResultsLength tests unchanged while ensuring the RPC-level behavior and panic fix are covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@x/cronos/rpc/api_test.go`:
- Around line 70-79: Add a test for the production getBlockDetail call path
using mismatched transaction and result counts, and assert it returns the
expected validation error before receipt handling or ReplayBlock processing
occurs. Keep the existing checkTxsResultsLength tests unchanged while ensuring
the RPC-level behavior and panic fix are covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b6652b95-85c3-4f8f-9c77-4a61bfa0fcd0
📒 Files selected for processing (3)
CHANGELOG.mdx/cronos/rpc/api.gox/cronos/rpc/api_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- CHANGELOG.md
- x/cronos/rpc/api.go
2 participants
What
x/cronos/rpcpaired two independent RPC results —resBlock.Block.TxsandblockRes.TxsResults— then indexedTxsResults[i]positionally in both the receipt loop and theReplayBlockloop, with no length check. A shortTxsResultspanics the RPC handler with an index-out-of-range.Solution
checkTxsResultsLength, called once ingetBlockDetail— the only place the two results are paired and the sole feeder of both loops.state/execution.goenforceslen(block.Txs) == len(TxResults)before commit, so an unequal pair can only come from an inconsistent RPC layer (proxy/multiplexer, or a re-sync/pruning race between the two calls).Summary by CodeRabbit
Bug Fixes
Tests