From c5e9f932ca2115b833675d9c098e96676c2b6b58 Mon Sep 17 00:00:00 2001 From: Joe Huang Date: Wed, 9 Oct 2024 14:12:17 -0500 Subject: [PATCH] Fix Nil pointer error in TXM stuck tx detector (#14685) * add nil ptr check for findBroadcastedAttempts * add changeset * fix loop variable lint * remove comment * add nil ptr check for BroadcastBeforeBlockNum * add error log * update log fmt * break logs * update sanity check * add unit test coverage for empty BroadcastBeforeBlockNum * update comments * fix test name * fix make --- .changeset/chilled-plants-clap.md | 5 ++++ core/chains/evm/txmgr/stuck_tx_detector.go | 24 ++++++++++++++--- .../evm/txmgr/stuck_tx_detector_test.go | 26 +++++++++++++++++++ 3 files changed, 51 insertions(+), 4 deletions(-) create mode 100644 .changeset/chilled-plants-clap.md diff --git a/.changeset/chilled-plants-clap.md b/.changeset/chilled-plants-clap.md new file mode 100644 index 00000000000..2a23b0960f1 --- /dev/null +++ b/.changeset/chilled-plants-clap.md @@ -0,0 +1,5 @@ +--- +"chainlink": patch +--- + +The findBroadcastedAttempts in detectStuckTransactionsHeuristic can returns uninitialized struct that potentially cause nil pointer error. Changed the return type of findBroadcastedAttempts to be pointers and added nil pointer check. #bugfix diff --git a/core/chains/evm/txmgr/stuck_tx_detector.go b/core/chains/evm/txmgr/stuck_tx_detector.go index a3c4d9eb4b0..a8bb18af416 100644 --- a/core/chains/evm/txmgr/stuck_tx_detector.go +++ b/core/chains/evm/txmgr/stuck_tx_detector.go @@ -217,10 +217,25 @@ 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 } + // 3. Check if the transaction has at least MinAttempts amount of broadcasted attempts if broadcastedAttemptsCount < *d.cfg.MinAttempts() { continue @@ -246,17 +261,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 diff --git a/core/chains/evm/txmgr/stuck_tx_detector_test.go b/core/chains/evm/txmgr/stuck_tx_detector_test.go index 18b044b141c..c6a0f5a7a41 100644 --- a/core/chains/evm/txmgr/stuck_tx_detector_test.go +++ b/core/chains/evm/txmgr/stuck_tx_detector_test.go @@ -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_DetectStuckTransactionsZircuit(t *testing.T) { @@ -537,6 +546,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{GasPrice: 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