From 1640c305f2ff4155aca20533faab955fbb72dfa0 Mon Sep 17 00:00:00 2001 From: Joe Huang Date: Tue, 8 Oct 2024 10:00:30 -0500 Subject: [PATCH 01/13] add nil ptr check for findBroadcastedAttempts --- core/chains/evm/txmgr/stuck_tx_detector.go | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/core/chains/evm/txmgr/stuck_tx_detector.go b/core/chains/evm/txmgr/stuck_tx_detector.go index 962be8afead..7b1300471e9 100644 --- a/core/chains/evm/txmgr/stuck_tx_detector.go +++ b/core/chains/evm/txmgr/stuck_tx_detector.go @@ -217,8 +217,10 @@ func (d *stuckTxDetector) detectStuckTransactionsHeuristic(ctx context.Context, } // Tx attempts are loaded from newest to oldest oldestBroadcastAttempt, newestBroadcastAttempt, broadcastedAttemptsCount := findBroadcastedAttempts(tx) + d.lggr.Debugf("found %v broadcasted attempts for tx id %v in stuck transaction heuristic", broadcastedAttemptsCount, tx) + // 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()) { + if oldestBroadcastAttempt != nil && *oldestBroadcastAttempt.BroadcastBeforeBlockNum > blockNum-int64(*d.cfg.Threshold()) { continue } // 3. Check if the transaction has at least MinAttempts amount of broadcasted attempts @@ -226,7 +228,7 @@ func (d *stuckTxDetector) detectStuckTransactionsHeuristic(ctx context.Context, continue } // 4. Check if the newest broadcasted attempt's gas price is higher than what our gas estimator's GetFee method returns - if compareGasFees(newestBroadcastAttempt.TxFee, marketGasPrice) <= 0 { + if newestBroadcastAttempt != nil && compareGasFees(newestBroadcastAttempt.TxFee, marketGasPrice) <= 0 { continue } // 5. Return the transaction since it is likely stuck due to overflow @@ -246,17 +248,17 @@ 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 { if attempt.State != types.TxAttemptBroadcast { continue } if !foundNewest { - newestAttempt = attempt + newestAttempt = &attempt foundNewest = true } - oldestAttempt = attempt + oldestAttempt = &attempt broadcastedCount++ } return From 50627456169eb0bbb6897b05484f89e489ae9580 Mon Sep 17 00:00:00 2001 From: Joe Huang Date: Tue, 8 Oct 2024 10:10:35 -0500 Subject: [PATCH 02/13] add changeset --- .changeset/chilled-plants-clap.md | 5 +++++ 1 file changed, 5 insertions(+) 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 From 4699d432c1af84dcb4cb91d986ad24a39500a916 Mon Sep 17 00:00:00 2001 From: Joe Huang Date: Tue, 8 Oct 2024 10:28:43 -0500 Subject: [PATCH 03/13] fix loop variable lint --- core/chains/evm/txmgr/stuck_tx_detector.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/core/chains/evm/txmgr/stuck_tx_detector.go b/core/chains/evm/txmgr/stuck_tx_detector.go index 7b1300471e9..14bed4de6c7 100644 --- a/core/chains/evm/txmgr/stuck_tx_detector.go +++ b/core/chains/evm/txmgr/stuck_tx_detector.go @@ -250,7 +250,8 @@ 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) { foundNewest := false - for _, attempt := range tx.TxAttempts { + for i := range tx.TxAttempts { + attempt := tx.TxAttempts[i] // Create a new variable for each loop iteration if attempt.State != types.TxAttemptBroadcast { continue } From 1a13a56e290a64e68be04be340cbda9d5ecc1e47 Mon Sep 17 00:00:00 2001 From: Joe Huang Date: Tue, 8 Oct 2024 11:07:02 -0500 Subject: [PATCH 04/13] remove comment --- core/chains/evm/txmgr/stuck_tx_detector.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/chains/evm/txmgr/stuck_tx_detector.go b/core/chains/evm/txmgr/stuck_tx_detector.go index 14bed4de6c7..359da4513b1 100644 --- a/core/chains/evm/txmgr/stuck_tx_detector.go +++ b/core/chains/evm/txmgr/stuck_tx_detector.go @@ -251,7 +251,7 @@ func compareGasFees(attemptGas gas.EvmFee, marketGas gas.EvmFee) int { func findBroadcastedAttempts(tx Tx) (oldestAttempt *TxAttempt, newestAttempt *TxAttempt, broadcastedCount uint32) { foundNewest := false for i := range tx.TxAttempts { - attempt := tx.TxAttempts[i] // Create a new variable for each loop iteration + attempt := tx.TxAttempts[i] if attempt.State != types.TxAttemptBroadcast { continue } From ba39f9cb1d24fdc192033a416b565a0dddd5e15d Mon Sep 17 00:00:00 2001 From: Joe Huang Date: Tue, 8 Oct 2024 11:25:46 -0500 Subject: [PATCH 05/13] add nil ptr check for BroadcastBeforeBlockNum --- core/chains/evm/txmgr/stuck_tx_detector.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/core/chains/evm/txmgr/stuck_tx_detector.go b/core/chains/evm/txmgr/stuck_tx_detector.go index 359da4513b1..744e504036c 100644 --- a/core/chains/evm/txmgr/stuck_tx_detector.go +++ b/core/chains/evm/txmgr/stuck_tx_detector.go @@ -220,9 +220,14 @@ func (d *stuckTxDetector) detectStuckTransactionsHeuristic(ctx context.Context, d.lggr.Debugf("found %v broadcasted attempts for tx id %v in stuck transaction heuristic", broadcastedAttemptsCount, tx) // 2. Check if Threshold amount of blocks have passed since the oldest attempt's broadcast block num - if oldestBroadcastAttempt != nil && *oldestBroadcastAttempt.BroadcastBeforeBlockNum > blockNum-int64(*d.cfg.Threshold()) { + if oldestBroadcastAttempt == nil || oldestBroadcastAttempt.BroadcastBeforeBlockNum == nil { continue } + + 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 From c5321cc3ff032ee73960dd1039d6fcb38f5ebd6f Mon Sep 17 00:00:00 2001 From: Joe Huang Date: Tue, 8 Oct 2024 11:29:22 -0500 Subject: [PATCH 06/13] add error log --- core/chains/evm/txmgr/stuck_tx_detector.go | 1 + 1 file changed, 1 insertion(+) diff --git a/core/chains/evm/txmgr/stuck_tx_detector.go b/core/chains/evm/txmgr/stuck_tx_detector.go index 744e504036c..e1bc5f78130 100644 --- a/core/chains/evm/txmgr/stuck_tx_detector.go +++ b/core/chains/evm/txmgr/stuck_tx_detector.go @@ -221,6 +221,7 @@ func (d *stuckTxDetector) detectStuckTransactionsHeuristic(ctx context.Context, // 2. Check if Threshold amount of blocks have passed since the oldest attempt's broadcast block num if oldestBroadcastAttempt == nil || oldestBroadcastAttempt.BroadcastBeforeBlockNum == nil { + d.lggr.Errorf("oldestBroadcastAttempt is nil or BroadcastBeforeBlockNum is nil for tx id %v in stuck transaction heuristic", tx) continue } From 3a1e84828929ef067c22f2172d1f4b54309a827b Mon Sep 17 00:00:00 2001 From: Joe Huang Date: Tue, 8 Oct 2024 11:33:10 -0500 Subject: [PATCH 07/13] update log fmt --- core/chains/evm/txmgr/stuck_tx_detector.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/chains/evm/txmgr/stuck_tx_detector.go b/core/chains/evm/txmgr/stuck_tx_detector.go index e1bc5f78130..fdfc334e142 100644 --- a/core/chains/evm/txmgr/stuck_tx_detector.go +++ b/core/chains/evm/txmgr/stuck_tx_detector.go @@ -217,11 +217,11 @@ func (d *stuckTxDetector) detectStuckTransactionsHeuristic(ctx context.Context, } // Tx attempts are loaded from newest to oldest oldestBroadcastAttempt, newestBroadcastAttempt, broadcastedAttemptsCount := findBroadcastedAttempts(tx) - d.lggr.Debugf("found %v broadcasted attempts for tx id %v in stuck transaction heuristic", broadcastedAttemptsCount, tx) + d.lggr.Debugf("found %d broadcasted attempts for tx id %d in stuck transaction heuristic", broadcastedAttemptsCount, tx.ID) // 2. Check if Threshold amount of blocks have passed since the oldest attempt's broadcast block num if oldestBroadcastAttempt == nil || oldestBroadcastAttempt.BroadcastBeforeBlockNum == nil { - d.lggr.Errorf("oldestBroadcastAttempt is nil or BroadcastBeforeBlockNum is nil for tx id %v in stuck transaction heuristic", tx) + d.lggr.Errorf("oldestBroadcastAttempt is nil or BroadcastBeforeBlockNum is nil for tx id %v in stuck transaction heuristic", tx.ID) continue } From 2ebc86618448afe6d873aa47ad7d053a16c39efd Mon Sep 17 00:00:00 2001 From: Joe Huang Date: Tue, 8 Oct 2024 11:47:41 -0500 Subject: [PATCH 08/13] break logs --- core/chains/evm/txmgr/stuck_tx_detector.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/core/chains/evm/txmgr/stuck_tx_detector.go b/core/chains/evm/txmgr/stuck_tx_detector.go index fdfc334e142..3ec17764785 100644 --- a/core/chains/evm/txmgr/stuck_tx_detector.go +++ b/core/chains/evm/txmgr/stuck_tx_detector.go @@ -220,8 +220,13 @@ func (d *stuckTxDetector) detectStuckTransactionsHeuristic(ctx context.Context, d.lggr.Debugf("found %d broadcasted attempts for tx id %d in stuck transaction heuristic", broadcastedAttemptsCount, tx.ID) // 2. Check if Threshold amount of blocks have passed since the oldest attempt's broadcast block num - if oldestBroadcastAttempt == nil || oldestBroadcastAttempt.BroadcastBeforeBlockNum == nil { - d.lggr.Errorf("oldestBroadcastAttempt is nil or BroadcastBeforeBlockNum is nil for tx id %v in stuck transaction heuristic", tx.ID) + if oldestBroadcastAttempt == nil { + d.lggr.Debugw("failed to find broadcast attempt for tx in stuck transaction heuristic", "tx", tx) + continue + } + + if oldestBroadcastAttempt.BroadcastBeforeBlockNum == nil { + d.lggr.Debugw("BroadcastBeforeBlockNum was not set for broadcast attempt in stuck transaction heuristic", "attempt", oldestBroadcastAttempt) continue } From ae9344c9435692e3f082a6aeea9b0d903ddabe5a Mon Sep 17 00:00:00 2001 From: Joe Huang Date: Tue, 8 Oct 2024 16:16:47 -0500 Subject: [PATCH 09/13] update sanity check --- core/chains/evm/txmgr/stuck_tx_detector.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/core/chains/evm/txmgr/stuck_tx_detector.go b/core/chains/evm/txmgr/stuck_tx_detector.go index 3ec17764785..56d55506bd4 100644 --- a/core/chains/evm/txmgr/stuck_tx_detector.go +++ b/core/chains/evm/txmgr/stuck_tx_detector.go @@ -219,8 +219,8 @@ func (d *stuckTxDetector) detectStuckTransactionsHeuristic(ctx context.Context, oldestBroadcastAttempt, newestBroadcastAttempt, broadcastedAttemptsCount := findBroadcastedAttempts(tx) d.lggr.Debugf("found %d broadcasted attempts for tx id %d in stuck transaction heuristic", broadcastedAttemptsCount, tx.ID) - // 2. Check if Threshold amount of blocks have passed since the oldest attempt's broadcast block num - if oldestBroadcastAttempt == nil { + // sanity checks + if oldestBroadcastAttempt == nil || newestBroadcastAttempt == nil { d.lggr.Debugw("failed to find broadcast attempt for tx in stuck transaction heuristic", "tx", tx) continue } @@ -230,6 +230,7 @@ func (d *stuckTxDetector) detectStuckTransactionsHeuristic(ctx context.Context, 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 } @@ -239,7 +240,7 @@ func (d *stuckTxDetector) detectStuckTransactionsHeuristic(ctx context.Context, continue } // 4. Check if the newest broadcasted attempt's gas price is higher than what our gas estimator's GetFee method returns - if newestBroadcastAttempt != nil && compareGasFees(newestBroadcastAttempt.TxFee, marketGasPrice) <= 0 { + if compareGasFees(newestBroadcastAttempt.TxFee, marketGasPrice) <= 0 { continue } // 5. Return the transaction since it is likely stuck due to overflow From ae4235c2fd77622ac777a81d62d71345e131c6d9 Mon Sep 17 00:00:00 2001 From: Joe Huang Date: Tue, 8 Oct 2024 16:26:38 -0500 Subject: [PATCH 10/13] add unit test coverage for empty BroadcastBeforeBlockNum --- .../evm/txmgr/stuck_tx_detector_test.go | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/core/chains/evm/txmgr/stuck_tx_detector_test.go b/core/chains/evm/txmgr/stuck_tx_detector_test.go index d87e13059b3..0de361f3808 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 that with empty attempts will be skipped", 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{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 From fe48a13fd740a3c9a9c7a2e1f12472c79dfc1c43 Mon Sep 17 00:00:00 2001 From: Joe Huang Date: Tue, 8 Oct 2024 16:34:20 -0500 Subject: [PATCH 11/13] update comments --- core/chains/evm/txmgr/stuck_tx_detector.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/core/chains/evm/txmgr/stuck_tx_detector.go b/core/chains/evm/txmgr/stuck_tx_detector.go index 56d55506bd4..303cb47a955 100644 --- a/core/chains/evm/txmgr/stuck_tx_detector.go +++ b/core/chains/evm/txmgr/stuck_tx_detector.go @@ -219,12 +219,13 @@ func (d *stuckTxDetector) detectStuckTransactionsHeuristic(ctx context.Context, oldestBroadcastAttempt, newestBroadcastAttempt, broadcastedAttemptsCount := findBroadcastedAttempts(tx) d.lggr.Debugf("found %d broadcasted attempts for tx id %d in stuck transaction heuristic", broadcastedAttemptsCount, tx.ID) - // sanity checks + // 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 From 07c8481e17aee583450e278afddb6f7a3778eff0 Mon Sep 17 00:00:00 2001 From: Joe Huang Date: Tue, 8 Oct 2024 16:43:03 -0500 Subject: [PATCH 12/13] fix test name --- core/chains/evm/txmgr/stuck_tx_detector_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/chains/evm/txmgr/stuck_tx_detector_test.go b/core/chains/evm/txmgr/stuck_tx_detector_test.go index 0de361f3808..187e16e9136 100644 --- a/core/chains/evm/txmgr/stuck_tx_detector_test.go +++ b/core/chains/evm/txmgr/stuck_tx_detector_test.go @@ -279,7 +279,7 @@ func TestStuckTxDetector_DetectStuckTransactionsHeuristic(t *testing.T) { require.Len(t, txs, 1) }) - t.Run("detects stuck transaction that with empty attempts will be skipped", func(t *testing.T) { + 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)) From 7e9c1bd781d8fb6cfe8cb3c1a676e71a7cb4f68e Mon Sep 17 00:00:00 2001 From: Joe Huang Date: Wed, 9 Oct 2024 12:55:43 -0500 Subject: [PATCH 13/13] fix make --- core/chains/evm/txmgr/stuck_tx_detector_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/chains/evm/txmgr/stuck_tx_detector_test.go b/core/chains/evm/txmgr/stuck_tx_detector_test.go index ea3d104d425..c6a0f5a7a41 100644 --- a/core/chains/evm/txmgr/stuck_tx_detector_test.go +++ b/core/chains/evm/txmgr/stuck_tx_detector_test.go @@ -555,7 +555,7 @@ func mustInsertUnconfirmedTxWithBroadcastAttemptsContainsEmptyBroadcastBeforeBlo attempt := cltest.NewLegacyEthTxAttempt(t, etx.ID) attempt.State = txmgrtypes.TxAttemptBroadcast attempt.BroadcastBeforeBlockNum = nil - attempt.TxFee = gas.EvmFee{Legacy: latestGasPrice.Sub(assets.NewWeiI(i))} + attempt.TxFee = gas.EvmFee{GasPrice: latestGasPrice.Sub(assets.NewWeiI(i))} require.NoError(t, txStore.InsertTxAttempt(ctx, &attempt)) } etx, err := txStore.FindTxWithAttempts(ctx, etx.ID)