fix(evm): proper nonce management in statedb #2130
Conversation
|
Warning Rate limit exceeded@Unique-Divine has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 31 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThis pull request encompasses changes across multiple files related to nonce management, transaction handling, and testing in the Ethereum backend. The modifications include adding new methods for nonce retrieval and transaction building, updating the changelog, and refining nonce handling in the EVM module. The changes primarily focus on improving the robustness of transaction processing and testing capabilities, with a particular emphasis on consistent nonce management across different components of the system. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Backend
participant EVMKeeper
participant StateDB
Client->>Backend: Send Transaction
Backend->>Backend: Get Current Nonce
Backend->>EVMKeeper: Apply EVM Message
EVMKeeper->>EVMKeeper: Reset Sender Nonce
EVMKeeper->>StateDB: Process Transaction
StateDB-->>EVMKeeper: Transaction Result
EVMKeeper->>EVMKeeper: Increment Nonce
EVMKeeper-->>Backend: Transaction Response
Backend-->>Client: Transaction Confirmation
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2130 +/- ##
==========================================
+ Coverage 65.13% 65.16% +0.02%
==========================================
Files 277 277
Lines 22168 22169 +1
==========================================
+ Hits 14440 14447 +7
+ Misses 6738 6732 -6
Partials 990 990
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
eth/rpc/backend/call_tx.go (1)
Line range hint
147-148: Handle nonce retrieval errors explicitlyThe current implementation ignores potential errors from
getAccountNonce. Given that this PR focuses on proper nonce management, we should handle these errors explicitly to prevent potential transaction failures.Consider modifying the code to handle errors:
- nonce, _ := b.getAccountNonce(*args.From, true, 0, b.logger) // #nosec G703s + nonce, err := b.getAccountNonce(*args.From, true, 0, b.logger) + if err != nil { + return args, fmt.Errorf("failed to get account nonce: %w", err) + } args.Nonce = (*hexutil.Uint64)(&nonce)
🧹 Nitpick comments (2)
eth/rpc/backend/nonce_test.go (1)
47-75: Consider adding error handling for gas estimationThe helper function
buildSDKTxWithEVMMessagescalculates total gas by summing individual transaction gas limits. This might not account for the overhead of processing multiple messages in a single transaction.Consider adding a buffer to the total gas calculation:
totalGas := uint64(0) for _, tx := range txs { totalGas += tx.Gas() } +// Add buffer for multi-msg overhead +totalGas += uint64(len(txs) * 10000)eth/rpc/backend/backend_suite_test.go (1)
237-238: Remove commented codeThere's a commented line that appears to be unused test data.
- //recipient := crypto.CreateAddress(s.fundedAccEthAddr, 29381)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
CHANGELOG.md(1 hunks)eth/rpc/backend/backend_suite_test.go(4 hunks)eth/rpc/backend/call_tx.go(2 hunks)eth/rpc/backend/nonce_test.go(1 hunks)x/evm/keeper/msg_server.go(2 hunks)x/evm/statedb/journal_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: integration-tests
🔇 Additional comments (4)
eth/rpc/backend/backend_suite_test.go (1)
192-202: LGTM! Well-structured nonce retrievalThe
getCurrentNoncefunction properly retrieves the current block number and uses it to get the accurate nonce. This centralization of nonce retrieval logic improves maintainability.x/evm/statedb/journal_test.go (1)
159-162: LGTM! Updated test expectationsThe test now correctly expects one dirty journal entry after a precompile call, aligning with the new nonce management behavior.
x/evm/keeper/msg_server.go (1)
338-341: LGTM! Improved nonce managementThe changes properly handle nonce management by:
- Resetting the nonce before EVM execution
- Incrementing it after processing, regardless of the outcome
- This fixes potential nonce inconsistencies and aligns with Ethereum's behavior
The implementation ensures that nonces are managed consistently, which is crucial for transaction ordering and replay protection.
Also applies to: 358-359
eth/rpc/backend/call_tx.go (1)
343-345: Consider the implications of exposing ClientCtxWhile this getter provides access to the client context needed for nonce management, exposing the entire client context could lead to potential misuse. Consider:
- Creating specific methods for the required functionality instead of exposing the entire context
- Documenting the intended usage and any thread-safety considerations
Let's verify the usage of this new method:
✅ Verification successful
ClientCtx() exposure is appropriately scoped
The method appears to be specifically added for testing purposes, with no production code dependencies. The current implementation is safe and follows testing patterns commonly used in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all calls to ClientCtx() to understand its usage pattern rg "ClientCtx\(\)" --type go # Look for any concurrent access patterns ast-grep --pattern 'go func() { $$$ $x.ClientCtx() $$$ }'Length of output: 592
Script:
#!/bin/bash # Search for interfaces that might require ClientCtx() ast-grep --pattern 'type $interface interface { $$$ ClientCtx() $return $$$ }' # Look for the Backend struct definition to understand its role ast-grep --pattern 'type Backend struct { $$$ }'Length of output: 756
| s.Assert().NotEqual(rsp.Code, 0) | ||
| s.Require().NoError(s.network.WaitForNextBlock()) |
There was a problem hiding this comment.
Assertion should check for success, not failure
The assertion is checking for non-zero response code, which typically indicates failure. For a successful transaction, the response code should be zero.
- s.Assert().NotEqual(rsp.Code, 0)
+ s.Assert().Equal(rsp.Code, 0)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| s.Assert().NotEqual(rsp.Code, 0) | |
| s.Require().NoError(s.network.WaitForNextBlock()) | |
| s.Assert().Equal(rsp.Code, 0) | |
| s.Require().NoError(s.network.WaitForNextBlock()) |
Summary by CodeRabbit
Documentation
New Features
Tests
Improvements