fix(gas-oracle): nonce too low when resubmission #1712
Conversation
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 labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 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 (
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ 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. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
rollup/internal/orm/orm_test.go (1)
601-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)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
rollup/internal/orm/orm_test.go(1 hunks)rollup/internal/orm/pending_transaction.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- rollup/internal/orm/pending_transaction.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: check
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 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)
🚧 Files skipped from review as they are similar to previous changes (1)
- rollup/internal/orm/orm_test.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: tests
🔇 Additional comments (6)
rollup/internal/orm/pending_transaction.go (2)
6-6: LGTM!The
database/sqlimport is correctly added to support thesql.NullInt64type used in the newGetMaxNonceBySenderAddressmethod.
212-232: LGTM!The
GetMaxNonceBySenderAddressmethod is well-implemented with proper error handling and null value management. The use ofsql.NullInt64correctly 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 newinitializeNonce()method. The error handling is appropriate and the approach consolidates nonce initialization logic.
332-360: LGTM!The
initializeNoncemethod 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
resetNoncemethod improvements are excellent. The method now returns an error for proper error propagation and uses the newinitializeNoncemethod for consistency. This addresses the past review comment about applying the "maximum of both values" logic to theresetNoncefunction 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.
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
tagincommon/version.gobeen updated or have you addedbump-versionlabel to this PR?Breaking change label
Does this PR have the
breaking-changelabel?Summary by CodeRabbit
Summary by CodeRabbit
Bug Fixes
New Features
Chores