fix(sender): graceful restart #1567
Conversation
WalkthroughThe pull request introduces changes across several files, focusing on a version increment and enhancements to transaction handling. The version number in Changes
Possibly related PRs
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lint (1.62.2)level=warning msg="[runner] Can't run linter goanalysis_metalinter: buildir: failed to load package : could not load export data: no export data for "scroll-tech/rollup/mock_bridge"" 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: 1
🧹 Outside diff range and nitpick comments (1)
rollup/internal/controller/sender/sender.go (1)
625-641: Enhance error handling when rollback failsWhen both
SendTransactionand the rollback transaction fail, the code logs an error but does not handle the failure further. Consider implementing additional error handling to maintain system consistency, such as retrying the rollback or alerting system operators.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
common/types/db.go(2 hunks)common/version/version.go(1 hunks)rollup/internal/controller/sender/sender.go(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- common/version/version.go
🔇 Additional comments (4)
common/types/db.go (3)
313-314: LGTM: New transaction status enhances error handling granularity
The addition of TxStatusSentFailed helps distinguish between transactions that fail during sending versus those that fail after confirmation, which is crucial for implementing proper rollback mechanisms.
327-328: LGTM: String representation properly implemented
The String() method is correctly updated to handle the new status, maintaining consistency with the existing pattern.
Line range hint 313-328: Verify handling of the new status across the codebase
Since TxStatus is part of the public API, we should verify:
- All switch statements handling
TxStatusare updated - Database migrations if
TxStatusis stored as an integer - Error handling logic in the rollup relayer properly sets this status
✅ Verification successful
All switch statements and database handling properly account for the new TxStatusSentFailed status
The verification results show:
- The new
TxStatusSentFailedis properly used in the sender implementation (rollup/internal/controller/sender/sender.go) when transaction sending fails - Database schema migrations exist and handle transaction status (
bridge-history-api/internal/orm/migrate/migrations/) - All switch statements handling TxStatus in the codebase are for different status types (btypes.TxStatusType) and not related to this enum
- The status is properly defined in the enum and its String() method
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify complete handling of TxStatusSentFailed across the codebase
# Find all switch statements handling TxStatus
echo "Checking switch statements handling TxStatus..."
rg -A 10 "switch.*TxStatus"
# Find database schema references to TxStatus
echo "Checking database schema references..."
fd -e sql . | xargs rg -l "tx_status|transaction_status"
# Find all direct references to TxStatus constants
echo "Checking usage of TxStatus constants..."
rg "TxStatus(Unknown|Pending|Replaced|Confirmed|ConfirmedFailed|SentFailed)"
Length of output: 10391
rollup/internal/controller/sender/sender.go (1)
466-474: Confirm gas fee doubling logic is accurate
The added code correctly ensures that the adjusted gas fees (gasTipCap, gasFeeCap, blobGasFeeCap) are at least double the original values. This validation aligns with network requirements for fee escalation.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
common/types/db.go (1)
313-314: Clarify the distinction between TxStatusSentFailed and TxStatusConfirmedFailedThe new status
TxStatusSentFailedis well-documented, but its relationship withTxStatusConfirmedFailedcould be clearer. Consider enhancing the comment to explain:
- When exactly this status is set (e.g., network issues, gas estimation failures)
- How it differs from
TxStatusConfirmedFailed(which occurs after the transaction is mined)- Whether transactions in this status can be retried
// TxStatusSentFailed indicates that the transaction has failed to be sent. +// This status is set when the transaction fails during the sending process (e.g., +// network issues, gas estimation failures) and differs from TxStatusConfirmedFailed +// which occurs when a sent transaction fails during execution. TxStatusSentFailed
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
common/types/db.go(2 hunks)common/version/version.go(1 hunks)rollup/internal/controller/sender/sender.go(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- common/version/version.go
🔇 Additional comments (2)
common/types/db.go (1)
327-328: LGTM! String() method properly updated
The String() method has been correctly updated to handle the new status, maintaining consistency with the existing implementation.
rollup/internal/controller/sender/sender.go (1)
466-473: Validation for gas fees doubling is correctly implemented
The added code in lines 466-473 properly ensures that the new gas fees (gasTipCap, gasFeeCap, blobGasFeeCap) are at least double the original values. This validation aligns with the gas fee escalation policies and enhances transaction reliability.
colinlyguo
changed the title
colinlyguo
requested review from
omerfirmak,
Thegaram,
georgehao and
jonastheis
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
rollup/internal/orm/pending_transaction.go (1)
153-167: Consider documenting soft delete behaviorSince this method uses GORM's soft delete feature (via the
DeletedAtfield), it's important to document that deleted transactions are not physically removed from the database. This behavior is crucial for the "graceful restart" feature as it allows for potential recovery scenarios.Consider adding a comment like:
-// DeletePendingTransactionByTxHash deletes a pending transaction record from the database by transaction hash. +// DeletePendingTransactionByTxHash soft deletes a pending transaction record from the database by transaction hash. +// The record will be marked as deleted but retained in the database for potential recovery scenarios. func (o *PendingTransaction) DeletePendingTransactionByTxHash(ctx context.Context, hash common.Hash, dbTX ...*gorm.DB) error {rollup/internal/controller/sender/sender.go (1)
626-642: Consider improving error handling after rollbackWhile the rollback mechanism is well-implemented, the current implementation returns immediately after a successful rollback, which might be too aggressive. Consider continuing the loop to process other pending transactions even if one transaction fails.
Apply this diff to improve the error handling:
if rollbackErr := s.db.Transaction(func(tx *gorm.DB) error { // Restore original transaction status back to pending if updateErr := s.pendingTransactionOrm.UpdatePendingTransactionStatusByTxHash(s.ctx, originalTx.Hash(), types.TxStatusPending, tx); updateErr != nil { return fmt.Errorf("failed to rollback status of original transaction, err: %w", updateErr) } // Delete the new transaction that was inserted if updateErr := s.pendingTransactionOrm.DeletePendingTransactionByTxHash(s.ctx, newSignedTx.Hash(), tx); updateErr != nil { return fmt.Errorf("failed to delete new transaction, err: %w", updateErr) } return nil }); rollbackErr != nil { // Both SendTransaction and rollback failed log.Error("failed to rollback database after SendTransaction failed", "tx hash", newSignedTx.Hash().String(), "from", s.transactionSigner.GetAddr().String(), "nonce", newSignedTx.Nonce(), "sendTxErr", err, "rollbackErr", rollbackErr) - return + continue } log.Error("failed to send replacing tx", "tx hash", newSignedTx.Hash().String(), "from", s.transactionSigner.GetAddr().String(), "nonce", newSignedTx.Nonce(), "err", err) -return +continue
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
rollup/internal/controller/sender/sender.go(3 hunks)rollup/internal/orm/pending_transaction.go(1 hunks)
🔇 Additional comments (3)
rollup/internal/orm/pending_transaction.go (1)
153-167: Verify transaction status before deletion
Given that this PR involves "precheck for blob transaction fees", it would be prudent to add validation ensuring we only delete transactions in appropriate states.
Let's verify the transaction status handling in the codebase:
Consider adding status validation:
func (o *PendingTransaction) DeletePendingTransactionByTxHash(ctx context.Context, hash common.Hash, dbTX ...*gorm.DB) error {
db := o.db
if len(dbTX) > 0 && dbTX[0] != nil {
db = dbTX[0]
}
db = db.WithContext(ctx)
db = db.Model(&PendingTransaction{})
+ // Verify transaction is in a deletable state
+ var tx PendingTransaction
+ if err := db.Where("hash = ?", hash.String()).First(&tx).Error; err != nil {
+ return fmt.Errorf("failed to find pending transaction: %w", err)
+ }
+ if tx.Status == types.TxStatusConfirmed {
+ return fmt.Errorf("cannot delete confirmed transaction: %s", hash.String())
+ }
result := db.Where("hash = ?", hash.String()).Delete(&PendingTransaction{})
if result.Error != nil {
return fmt.Errorf("failed to delete pending transaction, err: %w", result.Error)
}
return nil
}rollup/internal/controller/sender/sender.go (2)
235-240: LGTM: Proper cleanup of failed transactions
The implementation correctly handles the cleanup of failed transactions by removing them from the pending transaction table, with appropriate error handling and logging.
467-474: LGTM: Robust validation of gas fee increments
The implementation correctly enforces the minimum gas fee increment requirement for transaction replacement, ensuring compliance with EIP-1559 and EIP-4844 specifications. The error logging is comprehensive, including all relevant fee values for debugging.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
rollup/internal/orm/pending_transaction.go (1)
153-154: Enhance method documentationConsider adding more details to the documentation about return values and the soft delete behavior:
-// DeletePendingTransactionByTxHash deletes a pending transaction record from the database by transaction hash. +// DeletePendingTransactionByTxHash soft deletes a pending transaction record from the database by transaction hash. +// Returns nil if the transaction was successfully deleted, an error if the transaction wasn't found, +// or a wrapped error if the database operation failed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
rollup/internal/orm/pending_transaction.go(1 hunks)
🔇 Additional comments (1)
rollup/internal/orm/pending_transaction.go (1)
162-169: LGTM! Robust error handling implementation
The implementation includes proper validation for both database errors and "not found" cases, making it easy to distinguish between different error scenarios. The soft delete operation (due to DeletedAt field) ensures data can be recovered if needed.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1567 +/- ##
===========================================
- Coverage 52.11% 52.06% -0.06%
===========================================
Files 157 157
Lines 12433 12479 +46
===========================================
+ Hits 6480 6497 +17
- Misses 5395 5421 +26
- Partials 558 561 +3
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/orm/orm_test.go (1)
598-609: Consider enhancing test assertions for deletion verification.While the test cases cover the main scenarios, consider adding more specific assertions:
// Test DeleteTransactionByTxHash err = pendingTransactionOrm.DeleteTransactionByTxHash(context.Background(), tx0.Hash()) assert.NoError(t, err) // Verify the transaction is deleted status, err = pendingTransactionOrm.GetTxStatusByTxHash(context.Background(), tx0.Hash()) -assert.NoError(t, err) -assert.Equal(t, types.TxStatusUnknown, status) // Should return unknown status for deleted transaction +assert.ErrorContains(t, err, "record not found") // Try to delete non-existent transaction err = pendingTransactionOrm.DeleteTransactionByTxHash(context.Background(), common.HexToHash("0x123")) -assert.Error(t, err) // Should return error for non-existent transaction +assert.ErrorContains(t, err, "no transaction found with hash")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
rollup/internal/orm/orm_test.go(3 hunks)rollup/internal/orm/pending_transaction.go(3 hunks)
🔇 Additional comments (3)
rollup/internal/orm/pending_transaction.go (2)
154-177: LGTM! Well-implemented transaction deletion.
The implementation is robust with proper error handling, logging, and validation. The use of permanent deletion is appropriate for preventing database bloat from repeated transaction failures.
Line range hint 179-191: LGTM! Method rename improves clarity.
The rename from UpdatePendingTransactionStatusByTxHash to UpdateTransactionStatusByTxHash better reflects the method's functionality, as it handles all transaction states, not just pending ones.
rollup/internal/orm/orm_test.go (1)
Line range hint 563-580: LGTM! Comprehensive test coverage for transaction status updates.
The test cases thoroughly verify the transaction status update functionality and proper retrieval of transaction metadata.
Purpose or design rationale of this PR
This PR adds:
Note that:
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
New Features
Bug Fixes
Improvements