Actionable comments posted: 1

rollup/internal/controller/sender/sender.go (1)

625-641: Enhance error handling when rollback fails

When both SendTransaction and 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.

Configuration used: CodeRabbit UI
Review profile: CHILL

Reviewing files that changed from the base of the PR and between 438a9fb and 8371843.

• common/types/db.go (2 hunks)
• common/version/version.go (1 hunks)
• rollup/internal/controller/sender/sender.go (3 hunks)

• common/version/version.go

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:

1. All switch statements handling TxStatus are updated
2. Database migrations if TxStatus is stored as an integer
3. Error handling logic in the rollup relayer properly sets this status

All switch statements and database handling properly account for the new TxStatusSentFailed status

The verification results show:

1. The new TxStatusSentFailed is properly used in the sender implementation (rollup/internal/controller/sender/sender.go) when transaction sending fails
2. Database schema migrations exist and handle transaction status (bridge-history-api/internal/orm/migrate/migrations/)
3. All switch statements handling TxStatus in the codebase are for different status types (btypes.TxStatusType) and not related to this enum
4. The status is properly defined in the enum and its String() method

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

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.

Actionable comments posted: 2

common/types/db.go (1)

313-314: Clarify the distinction between TxStatusSentFailed and TxStatusConfirmedFailed

The new status TxStatusSentFailed is well-documented, but its relationship with TxStatusConfirmedFailed could 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

Configuration used: CodeRabbit UI
Review profile: CHILL

Reviewing files that changed from the base of the PR and between 438a9fb and 8371843.

• common/types/db.go (2 hunks)
• common/version/version.go (1 hunks)
• rollup/internal/controller/sender/sender.go (3 hunks)

• common/version/version.go

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.

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.

Actionable comments posted: 1

rollup/internal/orm/pending_transaction.go (1)

153-167: Consider documenting soft delete behavior

Since this method uses GORM's soft delete feature (via the DeletedAt field), 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 rollback

While 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

Configuration used: CodeRabbit UI
Review profile: CHILL

Reviewing files that changed from the base of the PR and between 8371843 and 2eb7266.

• rollup/internal/controller/sender/sender.go (3 hunks)
• rollup/internal/orm/pending_transaction.go (1 hunks)

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
 }

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.

Actionable comments posted: 0

rollup/internal/orm/pending_transaction.go (1)

153-154: Enhance method documentation

Consider 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.

Configuration used: CodeRabbit UI
Review profile: CHILL

Reviewing files that changed from the base of the PR and between 2eb7266 and 0866f99.

• rollup/internal/orm/pending_transaction.go (1 hunks)

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.

Actionable comments posted: 0

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")

Configuration used: CodeRabbit UI
Review profile: CHILL

Reviewing files that changed from the base of the PR and between feb3294 and b527cd5.

• rollup/internal/orm/orm_test.go (3 hunks)
• rollup/internal/orm/pending_transaction.go (3 hunks)

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.

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.