fix(sender): graceful restart #1564
Conversation
WalkthroughThe changes in this pull request involve an update to the version tag in the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
![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
🧹 Outside diff range and nitpick comments (4)
rollup/internal/controller/sender/sender.go (4)
220-220: Consider renaming the metric for clarityThe metric
sendTransactionFailureSendTxis incremented whencreateTxfails. To improve clarity and maintainability, consider renaming this metric to reflect that it tracks transaction creation failures, such ascreateTransactionFailureTotal.
238-240: Use robust error handling instead of string matchingRelying on string matching with
strings.Contains(err.Error(), "nonce too low")can be fragile, as error messages may change. Consider checking for specific error types or error codes to handle the "nonce too low" error more reliably.Apply this diff to improve error handling:
-if strings.Contains(err.Error(), "nonce too low") { +if isNonceTooLowError(err) { s.resetNonce(context.Background()) }And define the error checking function:
func isNonceTooLowError(err error) bool { // Implement error type assertion or error code checking here // Example for illustrative purposes: return errors.Is(err, ethereum.NonceTooLowError) }
476-478: Add metric for failed transaction resubmissionsWhen
createTxfails during a resubmission, there is no metric being incremented to track this failure. Consider incrementing a metric, such asresubmitTransactionFailedTotal, to monitor failed resubmission attempts.Apply this diff to include the metric:
if err != nil { + s.metrics.resubmitTransactionFailedTotal.WithLabelValues(s.service, s.name).Inc() log.Error("failed to create signed tx (resubmit case)", "from", s.transactionSigner.GetAddr().String(), "nonce", nonce, "err", err) return nil, err }
220-220: Ensure consistent metric naming for error trackingThe metric
sendTransactionFailureSendTxmay not accurately reflect that it's tracking failures in transaction creation. For consistency and clarity, consider renaming it to align with other failure metrics used elsewhere in the codebase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
common/version/version.go(1 hunks)rollup/internal/controller/sender/sender.go(7 hunks)rollup/internal/controller/sender/sender_test.go(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- common/version/version.go
🔇 Additional comments (7)
rollup/internal/controller/sender/sender_test.go (6)
285-289: LGTM! Improved separation of concerns.
The separation of transaction creation and sending into distinct steps improves code clarity and follows the single responsibility principle.
293-295: LGTM! Consistent method naming and explicit transaction handling.
The renaming to createReplacingTransaction better describes its purpose, and the explicit transaction sending maintains consistency with the new pattern.
376-380: LGTM! Consistent implementation of the new pattern.
The changes maintain consistency with the new transaction handling pattern across test cases.
381-383: LGTM! Consistent method naming and transaction handling.
The changes maintain consistency in the transaction replacement pattern.
423-427: LGTM! Maintains consistency in transaction creation pattern.
The changes follow the established pattern of separate transaction creation and sending.
477-479: LGTM! Consistent implementation across test scenarios.
The changes maintain consistency in transaction replacement handling across different test cases (dynamic fee and blob transactions).
Also applies to: 528-530
rollup/internal/controller/sender/sender.go (1)
565-565: 🛠️ Refactor suggestion
Avoid premature function exit in error handling within loops
Using return inside the loop will exit the checkPendingTransaction function upon encountering an error, which could prevent the processing of subsequent transactions. Consider using continue to proceed with the next transaction or handle the error appropriately to ensure all transactions are checked.
Apply this diff to fix the issue:
if err != nil {
log.Error("failed to get transaction status by tx hash", "hash", originalTx.Hash().String(), "err", err)
- return
+ continue
}Likely invalid or redundant comment.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1564 +/- ##
===========================================
- Coverage 52.48% 52.39% -0.10%
===========================================
Files 157 157
Lines 12643 12647 +4
===========================================
- Hits 6636 6626 -10
- Misses 5432 5447 +15
+ Partials 575 574 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
rollup/internal/controller/sender/sender_test.go (1)
285-289: LGTM! Improved transaction handling clarity.The refactoring improves code clarity by:
- Separating transaction creation from sending with clear method names
- Making the transaction lifecycle more explicit
- Maintaining consistency across all test cases
Also applies to: 293-295, 376-383, 423-430, 477-479, 528-530
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
rollup/internal/controller/sender/sender_test.go(7 hunks)
🔇 Additional comments (1)
rollup/internal/controller/sender/sender_test.go (1)
488-488: LGTM! Comprehensive fee verification.
The test assertions thoroughly verify:
- Gas fee cap calculations for regular transactions
- Both gas fee cap and blob gas fee cap for blob transactions
Also applies to: 545-546
colinlyguo
changed the title
Purpose or design rationale of this PR
This PR fixes a bug found during testing scroll-sdk.
Issue: the service restarts after sending the txn, and before adding the txn in db, so the txn's status will not be tracked or updated.
Fix: this PR first stores the txn in db, then sends the txn. if the txn fails to be sent, it will be covered and resubmitted by the resending logic.
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
New Features
Bug Fixes
Tests