fix(sender): graceful restart - #1564
Conversation
Sorry, something went wrong.
WalkthroughThe changes in this pull request involve an update to the version tag in the common/version/version.go file, changing it from "v4.4.74" to "v4.4.75". Additionally, modifications were made to the sender package, including renaming methods for clarity and improving error handling in transaction processing. The test suite for the sender package was also updated to reflect these method name changes and to enhance the clarity and robustness of transaction handling tests. Changes
Possibly related PRs
Suggested reviewers
📜 Recent review details Configuration used: CodeRabbit UI Reviewing files that changed from the base of the PR and between a9f6ffc and 334d3aa. 📒 Files selected for processing (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? ❤️ Share 🪧 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 (.coderabbit.yaml)
Documentation and Community
|
Sorry, something went wrong.
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)📜 Review details220-220: Consider renaming the metric for clarity
The metric sendTransactionFailureSendTx is incremented when createTx fails. To improve clarity and maintainability, consider renaming this metric to reflect that it tracks transaction creation failures, such as createTransactionFailureTotal.
238-240: Use robust error handling instead of string matching
Relying 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 resubmissions
When createTx fails during a resubmission, there is no metric being incremented to track this failure. Consider incrementing a metric, such as resubmitTransactionFailedTotal, 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 tracking
The metric sendTransactionFailureSendTx may 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.
Configuration used: CodeRabbit UI
Review profile: CHILL
Reviewing files that changed from the base of the PR and between 3ab5752 and 1e95eec.
📒 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)
- common/version/version.go
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
⚠️ Potential issue
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.
Sorry, something went wrong.
Codecov Report❌ Patch coverage is 35.00000% with 39 lines in your changes missing coverage. Please review.
@@ 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.
|
Sorry, something went wrong.
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)📜 Review details285-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
Configuration used: CodeRabbit UI
Review profile: CHILL
Reviewing files that changed from the base of the PR and between 00688d4 and a9f6ffc.
📒 Files selected for processing (1)- rollup/internal/controller/sender/sender_test.go (7 hunks)
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
Sorry, something went wrong.
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 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
New Features
Bug Fixes
Tests