Fix Nil pointer error in TXM stuck tx detector #14685
Conversation
WF: CI Core#b43bf21No errors found in this run. 🎉 |
There was a problem hiding this comment.
Left one comment about something that confused me in the logic. In addition, I was wondering - can we add a test case somewhere that triggers this logic? I think we are adding logic to cover an unusual corner case it'd be good to have a test covering it so someone doesn't accidentally come along and break the desired behavior.
| // 3. Check if the transaction has at least MinAttempts amount of broadcasted attempts | ||
| if broadcastedAttemptsCount < *d.cfg.MinAttempts() { | ||
| 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 { |
There was a problem hiding this comment.
if this is nil, shouldn't we log an error, similar to if oldestBroadcastAttempt is nil?
Would it make sense to move both checks up to line 220 before we do anything else? Although interestingly it looks like if oldest is nil we continue and if newest is nil we will add it to the stuck txns.
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
huangzhen1997
dismissed stale reviews from amit-momin, friedemannf, ilija42, and patrick-dowell
via
7e9c1bd
huangzhen1997
requested review from
patrick-dowell,
ilija42,
friedemannf and
amit-momin
|
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.
Description
The stuck transaction detector handler in Zircuit checks for both
quarantinetransaction using rpc call, and utilize existingdetectStuckTransactionsHeuristicfor potential overflow. ThefindBroadcastedAttemptsindetectStuckTransactionsHeuristiccan returns attempts with nil pointers that potentially cause panic.Changed the return type of
findBroadcastedAttemptsto be pointers and added nil pointer check.Reference:
https://chainlink-core.slack.com/archives/C06P81YRSN8/p1728379773038689