Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Nil pointer error in TXM stuck tx detector (CCIP 1.5) #1499

Open
wants to merge 1 commit into
base: release/2.14.0-ccip1.5
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 19 additions & 4 deletions core/chains/evm/txmgr/stuck_tx_detector.go
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,20 @@ func (d *stuckTxDetector) detectStuckTransactionsHeuristic(ctx context.Context,
}
// Tx attempts are loaded from newest to oldest
oldestBroadcastAttempt, newestBroadcastAttempt, broadcastedAttemptsCount := findBroadcastedAttempts(tx)
d.lggr.Debugf("found %d broadcasted attempts for tx id %d in stuck transaction heuristic", broadcastedAttemptsCount, tx.ID)

// attempt shouldn't be nil as we validated in FindUnconfirmedTxWithLowestNonce, but added anyway for a "belts and braces" approach
if oldestBroadcastAttempt == nil || newestBroadcastAttempt == nil {
d.lggr.Debugw("failed to find broadcast attempt for tx in stuck transaction heuristic", "tx", tx)
continue
}

// sanity check
if oldestBroadcastAttempt.BroadcastBeforeBlockNum == nil {
d.lggr.Debugw("BroadcastBeforeBlockNum was not set for broadcast attempt in stuck transaction heuristic", "attempt", oldestBroadcastAttempt)
continue
}

// 2. Check if Threshold amount of blocks have passed since the oldest attempt's broadcast block num
if *oldestBroadcastAttempt.BroadcastBeforeBlockNum > blockNum-int64(*d.cfg.Threshold()) {
continue
Expand Down Expand Up @@ -244,17 +258,18 @@ func compareGasFees(attemptGas gas.EvmFee, marketGas gas.EvmFee) int {
}

// Assumes tx attempts are loaded newest to oldest
func findBroadcastedAttempts(tx Tx) (oldestAttempt TxAttempt, newestAttempt TxAttempt, broadcastedCount uint32) {
func findBroadcastedAttempts(tx Tx) (oldestAttempt *TxAttempt, newestAttempt *TxAttempt, broadcastedCount uint32) {
foundNewest := false
for _, attempt := range tx.TxAttempts {
for i := range tx.TxAttempts {
attempt := tx.TxAttempts[i]
if attempt.State != types.TxAttemptBroadcast {
continue
}
if !foundNewest {
newestAttempt = attempt
newestAttempt = &attempt
foundNewest = true
}
oldestAttempt = attempt
oldestAttempt = &attempt
broadcastedCount++
}
return
Expand Down
26 changes: 26 additions & 0 deletions core/chains/evm/txmgr/stuck_tx_detector_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -278,6 +278,15 @@ func TestStuckTxDetector_DetectStuckTransactionsHeuristic(t *testing.T) {
require.NoError(t, err)
require.Len(t, txs, 1)
})

t.Run("detects stuck transaction with empty BroadcastBeforeBlockNum in attempts will be skipped without panic", func(t *testing.T) {
_, fromAddress := cltest.MustInsertRandomKey(t, ethKeyStore)
enabledAddresses := []common.Address{fromAddress}
mustInsertUnconfirmedTxWithBroadcastAttemptsContainsEmptyBroadcastBeforeBlockNum(t, txStore, 0, fromAddress, autoPurgeMinAttempts, marketGasPrice.Add(oneGwei))
txs, err := stuckTxDetector.DetectStuckTransactions(ctx, enabledAddresses, blockNum)
require.NoError(t, err)
require.Len(t, txs, 0)
})
}

func TestStuckTxDetector_DetectStuckTransactionsZkEVM(t *testing.T) {
Expand Down Expand Up @@ -435,6 +444,23 @@ func mustInsertUnconfirmedTxWithBroadcastAttempts(t *testing.T, txStore txmgr.Te
return etx
}

// helper function for edge case where broadcast attempt contains empty pointer
func mustInsertUnconfirmedTxWithBroadcastAttemptsContainsEmptyBroadcastBeforeBlockNum(t *testing.T, txStore txmgr.TestEvmTxStore, nonce int64, fromAddress common.Address, numAttempts uint32, latestGasPrice *assets.Wei) txmgr.Tx {
ctx := tests.Context(t)
etx := cltest.MustInsertUnconfirmedEthTx(t, txStore, nonce, fromAddress)
// Insert attempts from oldest to newest
for i := int64(numAttempts - 1); i >= 0; i-- {
attempt := cltest.NewLegacyEthTxAttempt(t, etx.ID)
attempt.State = txmgrtypes.TxAttemptBroadcast
attempt.BroadcastBeforeBlockNum = nil
attempt.TxFee = gas.EvmFee{Legacy: latestGasPrice.Sub(assets.NewWeiI(i))}
require.NoError(t, txStore.InsertTxAttempt(ctx, &attempt))
}
etx, err := txStore.FindTxWithAttempts(ctx, etx.ID)
require.NoError(t, err)
return etx
}

func mustInsertFatalErrorTxWithError(t *testing.T, txStore txmgr.TestEvmTxStore, nonce int64, fromAddress common.Address, blockNum int64) txmgr.Tx {
etx := cltest.NewEthTx(fromAddress)
etx.State = txmgrcommon.TxFatalError
Expand Down
Loading