fix(gas-oracle): nonce too low when resubmission - #1712
Conversation
Sorry, something went wrong.
WalkthroughThis change updates the version tag to "v4.5.36" and improves nonce initialization by considering both database and client pending nonces. It adds handling for "nonce too low" errors during transaction resubmission by marking the original and replacement transactions as failed. Additionally, it introduces a method to retrieve the maximum nonce by sender address from the database and adds a test for this method. Changes
Sequence Diagram(s)sequenceDiagram
participant Sender
participant ORM
participant DB
Sender->>ORM: GetMaxNonceBySenderAddress(sender)
ORM->>DB: Query max nonce for sender
DB-->>ORM: Return max nonce
ORM-->>Sender: Return max nonce
Sender->>Sender: initializeNonce() = max(DB max nonce + 1, client pending nonce)
Sender->>Sender: Log nonce initialization/reset
Sender->>Sender: SendTransaction()
alt Error contains "nonce too low"
Sender->>Sender: resetNonce()
Sender->>Sender: Log nonce reset
Sender->>Sender: Return error if reset fails
end
Sender->>Sender: checkPendingTransaction()
alt Resubmission "nonce too low" error
Sender->>Sender: Log warning
Sender->>DB: Update original and replacement tx status to ConfirmedFailed
Sender-->>Sender: Return early (no rollback)
else Other errors
Sender->>Sender: Existing rollback/error handling
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested labelsbump-version Suggested reviewers
Poem
📜 Recent review details Configuration used: CodeRabbit UI Reviewing files that changed from the base of the PR and between 167ce20 and 1115c51. 📒 Files selected for processing (2)
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. ❤️ Share 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (.coderabbit.yaml)
Documentation and Community
|
Sorry, something went wrong.
Codecov Report❌ Patch coverage is 44.15584% with 43 lines in your changes missing coverage. Please review.
@@ Coverage Diff @@
## develop #1712 +/- ##
===========================================
+ Coverage 39.55% 39.65% +0.09%
===========================================
Files 237 237
Lines 18999 19033 +34
===========================================
+ Hits 7516 7547 +31
+ Misses 10743 10742 -1
- Partials 740 744 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry.
|
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)rollup/internal/orm/orm_test.go (1)📜 Review details601-657: Well-structured test with good coverage of basic scenarios.
The test properly covers both the empty state (no transactions) and populated state (multiple transactions with different nonces). The test setup is clean and follows established patterns in the file.
Consider adding these test cases for more comprehensive coverage:
- Multiple transactions with the same nonce for the same sender
- Transactions for different sender addresses to verify proper filtering
- Testing with nonce 0 to ensure it's handled correctly
Example of additional test cases:
+ // Test with different sender address to ensure proper filtering + differentSender := &SenderMeta{ + Name: "differentSender", + Service: "testService", + Address: common.HexToAddress("0xbeefcafe"), + Type: types.SenderTypeCommitBatch, + } + tx2 := gethTypes.NewTx(&gethTypes.DynamicFeeTx{ + Nonce: 5, + To: &common.Address{}, + Data: []byte{}, + Gas: 23000, + AccessList: gethTypes.AccessList{}, + Value: big.NewInt(0), + ChainID: big.NewInt(1), + GasTipCap: big.NewInt(2), + GasFeeCap: big.NewInt(3), + V: big.NewInt(0), + R: big.NewInt(0), + S: big.NewInt(0), + }) + err = pendingTransactionOrm.InsertPendingTransaction(context.Background(), "test", differentSender, tx2, 0) + assert.NoError(t, err) + + // Original sender should still have max nonce 3 + maxNonce, err = pendingTransactionOrm.GetMaxNonceBySenderAddress(context.Background(), senderMeta.Address.String()) + assert.NoError(t, err) + assert.Equal(t, uint64(3), maxNonce) + + // Different sender should have max nonce 5 + maxNonce, err = pendingTransactionOrm.GetMaxNonceBySenderAddress(context.Background(), differentSender.Address.String()) + assert.NoError(t, err) + assert.Equal(t, uint64(5), maxNonce)
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Reviewing files that changed from the base of the PR and between 637578a and 22a1108.
📒 Files selected for processing (2)- rollup/internal/orm/orm_test.go (1 hunks)
- rollup/internal/orm/pending_transaction.go (1 hunks)
- rollup/internal/orm/pending_transaction.go
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: check
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review detailsConfiguration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Reviewing files that changed from the base of the PR and between 22a1108 and 86b18d5.
📒 Files selected for processing (3)- rollup/internal/controller/sender/sender.go (6 hunks)
- rollup/internal/orm/orm_test.go (1 hunks)
- rollup/internal/orm/pending_transaction.go (2 hunks)
- rollup/internal/orm/orm_test.go
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: tests
rollup/internal/orm/pending_transaction.go (2)6-6: LGTM!
The database/sql import is correctly added to support the sql.NullInt64 type used in the new GetMaxNonceBySenderAddress method.
212-232: LGTM!
The GetMaxNonceBySenderAddress method is well-implemented with proper error handling and null value management. The use of sql.NullInt64 correctly handles the case where no transactions exist for the given address, and returning -1 is appropriately documented and consistent with its usage in the sender logic.
rollup/internal/controller/sender/sender.go (4)108-134: LGTM!
The constructor changes improve nonce initialization by using the resetNonce() method, which now considers both database and client nonces through the new initializeNonce() method. The error handling is appropriate and the approach consolidates nonce initialization logic.
332-360: LGTM!
The initializeNonce method implements robust nonce initialization logic by considering both database and client states. The approach of taking the maximum between pending nonce and (db nonce + 1) ensures consistency, and the comprehensive logging aids in debugging. The error handling is appropriate with descriptive messages.
363-372: LGTM!
The resetNonce method improvements are excellent. The method now returns an error for proper error propagation and uses the new initializeNonce method for consistency. This addresses the past review comment about applying the "maximum of both values" logic to the resetNonce function as well.
244-247: LGTM!
The error handling for the resetNonce() call is appropriate since the method signature now returns an error. The early return on failure and descriptive error messaging maintain consistency with the existing error handling patterns in this method.
Sorry, something went wrong.
Purpose or design rationale of this PR
When resubmission receives "nonce too low" error, it means the transaction has already been mined. In such cases sender can skip resubmitting such transactions.
This PR also adds another robustness fix about initializing/resetting nonce by setting it as max(pending nonce from the nonce, max db's nonce + 1).
PR title
Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:
Deployment tag versioning
Has tag in common/version.go been updated or have you added bump-version label to this PR?
Breaking change label
Does this PR have the breaking-change label?
Summary by CodeRabbit
Summary by CodeRabbit
Bug Fixes
New Features
Chores