missing tx attempt bug fix (#12036)

* add code to handle if exceeds max fee is encountered

* add partial fix but has testing bugs

* separate tests

* remove fallthrough

* reword critical message

* fix lint error

common/txmgr/confirmer.go

@@ -718,7 +718,7 @@ func (ec *Confirmer[CHAIN_ID, HEAD, ADDR, TX_HASH, BLOCK_HASH, R, SEQ, FEE]) Fin
 				oldestBlocksBehind = blockNum - *oldestBlockNum
 			}
 		} else {
-			logger.Sugared(lggr).AssumptionViolationf("Expected tx for gas bump to have at least one attempt", "etxID", etx.ID, "blockNum", blockNum, "address", address)
+			logger.Sugared(lggr).AssumptionViolationw("Expected tx for gas bump to have at least one attempt", "etxID", etx.ID, "blockNum", blockNum, "address", address)
 		}
 		lggr.Infow(fmt.Sprintf("Found %d transactions to re-sent that have still not been confirmed after at least %d blocks. The oldest of these has not still not been confirmed after %d blocks. These transactions will have their gas price bumped. %s", len(etxBumps), gasBumpThreshold, oldestBlocksBehind, label.NodeConnectivityProblemWarning), "blockNum", blockNum, "address", address, "gasBumpThreshold", gasBumpThreshold)
 	}
@@ -858,10 +858,19 @@ func (ec *Confirmer[CHAIN_ID, HEAD, ADDR, TX_HASH, BLOCK_HASH, R, SEQ, FEE]) han
 		}
 		return ec.handleInProgressAttempt(ctx, lggr, etx, replacementAttempt, blockHeight)
 	case client.ExceedsMaxFee:
-		// Confirmer: The gas price was bumped too high. This transaction attempt cannot be accepted.
-		// Best thing we can do is to re-send the previous attempt at the old
-		// price and discard this bumped version.
-		fallthrough
+		// Confirmer: Note it is not guaranteed that all nodes share the same tx fee cap.
+		// So it is very likely that this attempt was successful on another node since
+		// it was already successfully broadcasted. So we assume it is successful and
+		// warn the operator that the RPC node is misconfigured.
+		// This failure scenario is a strong indication that the RPC node
+		// is misconfigured. This is a critical error and should be resolved by the
+		// node operator.
+		// If there is only one RPC node, or all RPC nodes have the same
+		// configured cap, this transaction will get stuck and keep repeating
+		// forever until the issue is resolved.
+		lggr.Criticalw(`RPC node rejected this tx as outside Fee Cap but it may have been accepted by another Node`, "attempt", attempt)
+		timeout := ec.dbConfig.DefaultQueryTimeout()
+		return ec.txStore.SaveSentAttempt(ctx, timeout, &attempt, now)
 	case client.Fatal:
 		// WARNING: This should never happen!
 		// Should NEVER be fatal this is an invariant violation. The

core/chains/evm/txmgr/confirmer_test.go

@@ -1719,6 +1719,73 @@ func TestEthConfirmer_RebroadcastWhereNecessary_WithConnectivityCheck(t *testing
 	})
 }
 
+func TestEthConfirmer_RebroadcastWhereNecessary_MaxFeeScenario(t *testing.T) {
+	t.Parallel()
+
+	db := pgtest.NewSqlxDB(t)
+	cfg := configtest.NewGeneralConfig(t, func(c *chainlink.Config, s *chainlink.Secrets) {
+		c.EVM[0].GasEstimator.PriceMax = assets.GWei(500)
+	})
+	txStore := cltest.NewTestTxStore(t, db, cfg.Database())
+
+	ethClient := evmtest.NewEthClientMockWithDefaultChain(t)
+	ethKeyStore := cltest.NewKeyStore(t, db, cfg.Database()).Eth()
+
+	evmcfg := evmtest.NewChainScopedConfig(t, cfg)
+
+	_, _ = cltest.MustInsertRandomKeyReturningState(t, ethKeyStore)
+	_, fromAddress := cltest.MustInsertRandomKeyReturningState(t, ethKeyStore)
+
+	kst := ksmocks.NewEth(t)
+	addresses := []gethCommon.Address{fromAddress}
+	kst.On("EnabledAddressesForChain", &cltest.FixtureChainID).Return(addresses, nil).Maybe()
+	// Use a mock keystore for this test
+	ec := newEthConfirmer(t, txStore, ethClient, evmcfg, kst, nil)
+	currentHead := int64(30)
+	oldEnough := int64(19)
+	nonce := int64(0)
+
+	originalBroadcastAt := time.Unix(1616509100, 0)
+	etx := cltest.MustInsertUnconfirmedEthTxWithBroadcastLegacyAttempt(t, txStore, nonce, fromAddress, originalBroadcastAt)
+	attempt1_1 := etx.TxAttempts[0]
+	var dbAttempt txmgr.DbEthTxAttempt
+	require.NoError(t, db.Get(&dbAttempt, `UPDATE evm.tx_attempts SET broadcast_before_block_num=$1 WHERE id=$2 RETURNING *`, oldEnough, attempt1_1.ID))
+
+	t.Run("treats an exceeds max fee attempt as a success", func(t *testing.T) {
+		ethTx := *types.NewTx(&types.LegacyTx{})
+		kst.On("SignTx",
+			fromAddress,
+			mock.MatchedBy(func(tx *types.Transaction) bool {
+				if tx.Nonce() != uint64(*etx.Sequence) {
+					return false
+				}
+				ethTx = *tx
+				return true
+			}),
+			mock.MatchedBy(func(chainID *big.Int) bool {
+				return chainID.Cmp(evmcfg.EVM().ChainID()) == 0
+			})).Return(&ethTx, nil).Once()
+
+		// Once for the bumped attempt which exceeds limit
+		ethClient.On("SendTransactionReturnCode", mock.Anything, mock.MatchedBy(func(tx *types.Transaction) bool {
+			return tx.Nonce() == uint64(*etx.Sequence) && tx.GasPrice().Int64() == int64(20000000000)
+		}), fromAddress).Return(commonclient.ExceedsMaxFee, errors.New("tx fee (1.10 ether) exceeds the configured cap (1.00 ether)")).Once()
+
+		// Do the thing
+		require.NoError(t, ec.RebroadcastWhereNecessary(testutils.Context(t), currentHead))
+		var err error
+		etx, err = txStore.FindTxWithAttempts(etx.ID)
+		require.NoError(t, err)
+
+		// Check that the attempt is saved
+		require.Len(t, etx.TxAttempts, 2)
+
+		// broadcast_at did change
+		require.Greater(t, etx.BroadcastAt.Unix(), originalBroadcastAt.Unix())
+		require.Equal(t, etx.InitialBroadcastAt.Unix(), originalBroadcastAt.Unix())
+	})
+}
+
 func TestEthConfirmer_RebroadcastWhereNecessary(t *testing.T) {
 	t.Parallel()
 
@@ -1803,43 +1870,6 @@ func TestEthConfirmer_RebroadcastWhereNecessary(t *testing.T) {
 		require.Len(t, etx.TxAttempts, 1)
 	})
 
-	ethClient = evmtest.NewEthClientMockWithDefaultChain(t)
-	ec.XXXTestSetClient(txmgr.NewEvmTxmClient(ethClient))
-
-	t.Run("does nothing and continues if bumped attempt transaction was too expensive", func(t *testing.T) {
-		ethTx := *types.NewTx(&types.LegacyTx{})
-		kst.On("SignTx",
-			fromAddress,
-			mock.MatchedBy(func(tx *types.Transaction) bool {
-				if tx.Nonce() != uint64(*etx.Sequence) {
-					return false
-				}
-				ethTx = *tx
-				return true
-			}),
-			mock.MatchedBy(func(chainID *big.Int) bool {
-				return chainID.Cmp(evmcfg.EVM().ChainID()) == 0
-			})).Return(&ethTx, nil).Once()
-
-		// Once for the bumped attempt which exceeds limit
-		ethClient.On("SendTransactionReturnCode", mock.Anything, mock.MatchedBy(func(tx *types.Transaction) bool {
-			return tx.Nonce() == uint64(*etx.Sequence) && tx.GasPrice().Int64() == int64(20000000000)
-		}), fromAddress).Return(commonclient.ExceedsMaxFee, errors.New("tx fee (1.10 ether) exceeds the configured cap (1.00 ether)")).Once()
-
-		// Do the thing
-		require.NoError(t, ec.RebroadcastWhereNecessary(testutils.Context(t), currentHead))
-		var err error
-		etx, err = txStore.FindTxWithAttempts(etx.ID)
-		require.NoError(t, err)
-
-		// Did not create an additional attempt
-		require.Len(t, etx.TxAttempts, 1)
-
-		// broadcast_at did not change
-		require.Equal(t, etx.BroadcastAt.Unix(), originalBroadcastAt.Unix())
-		require.Equal(t, etx.InitialBroadcastAt.Unix(), originalBroadcastAt.Unix())
-	})
-
 	var attempt1_2 txmgr.TxAttempt
 	ethClient = evmtest.NewEthClientMockWithDefaultChain(t)
 	ec.XXXTestSetClient(txmgr.NewEvmTxmClient(ethClient))