Skip to content

Commit

Permalink
Fix Nil pointer error in TXM stuck tx detector (CCIP 1.5) (#1499)
Browse files Browse the repository at this point in the history
An NPE bug was identified in the Stuck Tx Detector component. The fix
was addressed in this
smartcontractkit/chainlink#14685 in core but
needs to be applied to CCIP as well. Changes could not be cherry-picked
because of other changes made to the component in between so changes
were back ported manually.
  • Loading branch information
amit-momin authored Oct 15, 2024
1 parent 7711683 commit a917e9a
Show file tree
Hide file tree
Showing 2 changed files with 45 additions and 4 deletions.
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

0 comments on commit a917e9a

Please sign in to comment.