Fix batch tx send encoding (#11500)

* Changed signed tx encoding to use MarshalBinary

* Added tests and CHANGELOG entry

* Fixed linting

* Cleaned up ClassifySendError method signature

* Fixed linting

* Consolidated error logs to avoid duplicate logging by caller

* Isolated encoding change for batch resend logic

* Reverted models test changes

* Added comments

common/txmgr/broadcaster.go

@@ -647,15 +647,15 @@ func (eb *Broadcaster[CHAIN_ID, HEAD, ADDR, TX_HASH, BLOCK_HASH, SEQ, FEE]) hand
 		// 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.
-		lgr.Criticalw(`RPC node rejected this tx as outside Fee Cap`)
+		lgr.Criticalw(`RPC node rejected this tx as outside Fee Cap`, "attempt", attempt)
 		fallthrough
 	default:
 		// Every error that doesn't fall under one of the above categories will be treated as Unknown.
 		fallthrough
 	case client.Unknown:
 		eb.SvcErrBuffer.Append(err)
 		lgr.Criticalw(`Unknown error occurred while handling tx queue in ProcessUnstartedTxs. This chain/RPC client may not be supported. `+
-			`Urgent resolution required, Chainlink is currently operating in a degraded state and may miss transactions`, "err", err, "etx", etx, "attempt", attempt)
+			`Urgent resolution required, Chainlink is currently operating in a degraded state and may miss transactions`, "attempt", attempt)
 		nextSequence, e := eb.client.PendingSequenceAt(ctx, etx.FromAddress)
 		if e != nil {
 			err = multierr.Combine(e, err)

common/txmgr/confirmer.go

@@ -824,7 +824,7 @@ func (ec *Confirmer[CHAIN_ID, HEAD, ADDR, TX_HASH, BLOCK_HASH, R, SEQ, FEE]) han
 	case client.Underpriced:
 		// This should really not ever happen in normal operation since we
 		// already bumped above the required minimum in broadcaster.
-		ec.lggr.Warnw("Got terminally underpriced error for gas bump, this should never happen unless the remote RPC node changed its configuration on the fly, or you are using multiple RPC nodes with different minimum gas price requirements. This is not recommended", "err", sendError, "attempt", attempt)
+		ec.lggr.Warnw("Got terminally underpriced error for gas bump, this should never happen unless the remote RPC node changed its configuration on the fly, or you are using multiple RPC nodes with different minimum gas price requirements. This is not recommended", "attempt", attempt)
 		// "Lazily" load attempts here since the overwhelmingly common case is
 		// that we don't need them unless we enter this path
 		if err := ec.txStore.LoadTxAttempts(ctx, &etx); err != nil {
@@ -867,7 +867,6 @@ func (ec *Confirmer[CHAIN_ID, HEAD, ADDR, TX_HASH, BLOCK_HASH, R, SEQ, FEE]) han
 		// Broadcaster can never create a TxAttempt that will
 		// fatally error.
 		lggr.Criticalw("Invariant violation: fatal error while re-attempting transaction",
-			"err", sendError,
 			"fee", attempt.TxFee,
 			"feeLimit", etx.FeeLimit,
 			"signedRawTx", utils.EnsureHexPrefix(hex.EncodeToString(attempt.SignedRawTx)),
@@ -879,7 +878,7 @@ func (ec *Confirmer[CHAIN_ID, HEAD, ADDR, TX_HASH, BLOCK_HASH, R, SEQ, FEE]) han
 	case client.TransactionAlreadyKnown:
 		// Sequence too low indicated that a transaction at this sequence was confirmed already.
 		// Mark confirmed_missing_receipt and wait for the next cycle to try to get a receipt
-		lggr.Debugw("Sequence already used", "txAttemptID", attempt.ID, "txHash", attempt.Hash.String(), "err", sendError)
+		lggr.Debugw("Sequence already used", "txAttemptID", attempt.ID, "txHash", attempt.Hash.String())
 		timeout := ec.dbConfig.DefaultQueryTimeout()
 		return ec.txStore.SaveConfirmedMissingReceiptAttempt(ctx, timeout, &attempt, now)
 	case client.InsufficientFunds:

core/chains/evm/client/chain_client.go

@@ -216,7 +216,8 @@ func (c *chainClient) SendTransaction(ctx context.Context, tx *types.Transaction
 
 func (c *chainClient) SendTransactionReturnCode(ctx context.Context, tx *types.Transaction, fromAddress common.Address) (commonclient.SendTxReturnCode, error) {
 	err := c.SendTransaction(ctx, tx)
-	return ClassifySendError(err, c.logger, tx, fromAddress, c.IsL2())
+	returnCode := ClassifySendError(err, c.logger, tx, fromAddress, c.IsL2())
+	return returnCode, err
 }
 
 func (c *chainClient) SequenceAt(ctx context.Context, account common.Address, blockNumber *big.Int) (evmtypes.Nonce, error) {

core/chains/evm/client/client.go

@@ -220,7 +220,8 @@ func (client *client) HeaderByHash(ctx context.Context, h common.Hash) (*types.H
 
 func (client *client) SendTransactionReturnCode(ctx context.Context, tx *types.Transaction, fromAddress common.Address) (commonclient.SendTxReturnCode, error) {
 	err := client.SendTransaction(ctx, tx)
-	return ClassifySendError(err, client.logger, tx, fromAddress, client.pool.ChainType().IsL2())
+	returnCode := ClassifySendError(err, client.logger, tx, fromAddress, client.pool.ChainType().IsL2())
+	return returnCode, err
 }
 
 // SendTransaction also uses the sendonly HTTP RPC URLs if set

core/chains/evm/client/client_test.go

@@ -417,7 +417,7 @@ func TestEthClient_HeaderByNumber(t *testing.T) {
 func TestEthClient_SendTransaction_NoSecondaryURL(t *testing.T) {
 	t.Parallel()
 
-	tx := types.NewTransaction(uint64(42), testutils.NewAddress(), big.NewInt(142), 242, big.NewInt(342), []byte{1, 2, 3})
+	tx := cltest.NewLegacyTransaction(uint64(42), testutils.NewAddress(), big.NewInt(142), 242, big.NewInt(342), []byte{1, 2, 3})
 
 	wsURL := testutils.NewWSServer(t, &cltest.FixtureChainID, func(method string, params gjson.Result) (resp testutils.JSONRPCResponse) {
 		switch method {
@@ -449,7 +449,7 @@ func TestEthClient_SendTransaction_NoSecondaryURL(t *testing.T) {
 func TestEthClient_SendTransaction_WithSecondaryURLs(t *testing.T) {
 	t.Parallel()
 
-	tx := types.NewTransaction(uint64(42), testutils.NewAddress(), big.NewInt(142), 242, big.NewInt(342), []byte{1, 2, 3})
+	tx := cltest.NewLegacyTransaction(uint64(42), testutils.NewAddress(), big.NewInt(142), 242, big.NewInt(342), []byte{1, 2, 3})
 
 	wsURL := testutils.NewWSServer(t, &cltest.FixtureChainID, func(method string, params gjson.Result) (resp testutils.JSONRPCResponse) {
 		switch method {
@@ -494,7 +494,7 @@ func TestEthClient_SendTransactionReturnCode(t *testing.T) {
 	t.Parallel()
 
 	fromAddress := testutils.NewAddress()
-	tx := types.NewTransaction(uint64(42), testutils.NewAddress(), big.NewInt(142), 242, big.NewInt(342), []byte{1, 2, 3})
+	tx := cltest.NewLegacyTransaction(uint64(42), testutils.NewAddress(), big.NewInt(142), 242, big.NewInt(342), []byte{1, 2, 3})
 
 	t.Run("returns Fatal error type when error message is fatal", func(t *testing.T) {
 		wsURL := testutils.NewWSServer(t, &cltest.FixtureChainID, func(method string, params gjson.Result) (resp testutils.JSONRPCResponse) {

core/chains/evm/client/errors.go

@@ -413,73 +413,78 @@ func ExtractRPCError(baseErr error) (*JsonError, error) {
 	return &jErr, nil
 }
 
-func ClassifySendError(err error, lggr logger.SugaredLogger, tx *types.Transaction, fromAddress common.Address, isL2 bool) (commonclient.SendTxReturnCode, error) {
+func ClassifySendError(err error, lggr logger.SugaredLogger, tx *types.Transaction, fromAddress common.Address, isL2 bool) commonclient.SendTxReturnCode {
 	sendError := NewSendError(err)
 	if sendError == nil {
-		return commonclient.Successful, err
+		return commonclient.Successful
 	}
 	if sendError.Fatal() {
 		lggr.Criticalw("Fatal error sending transaction", "err", sendError, "etx", tx)
 		// Attempt is thrown away in this case; we don't need it since it never got accepted by a node
-		return commonclient.Fatal, err
+		return commonclient.Fatal
 	}
 	if sendError.IsNonceTooLowError() || sendError.IsTransactionAlreadyMined() {
+		lggr.Debugw("Transaction already confirmed for this nonce: %d", tx.Nonce(), "err", sendError, "etx", tx)
 		// Nonce too low indicated that a transaction at this nonce was confirmed already.
 		// Mark it as TransactionAlreadyKnown.
-		return commonclient.TransactionAlreadyKnown, err
+		return commonclient.TransactionAlreadyKnown
 	}
 	if sendError.IsReplacementUnderpriced() {
 		lggr.Errorw(fmt.Sprintf("Replacement transaction underpriced for eth_tx %x. "+
-			"Eth node returned error: '%s'. "+
 			"Please note that using your node's private keys outside of the chainlink node is NOT SUPPORTED and can lead to missed transactions.",
-			tx.Hash(), err), "gasPrice", tx.GasPrice, "gasTipCap", tx.GasTipCap, "gasFeeCap", tx.GasFeeCap)
+			tx.Hash()), "gasPrice", tx.GasPrice, "gasTipCap", tx.GasTipCap, "gasFeeCap", tx.GasFeeCap, "err", sendError, "etx", tx)
 
 		// Assume success and hand off to the next cycle.
-		return commonclient.Successful, err
+		return commonclient.Successful
 	}
 	if sendError.IsTransactionAlreadyInMempool() {
-		lggr.Debugw("Transaction already in mempool", "txHash", tx.Hash, "nodeErr", sendError.Error())
-		return commonclient.Successful, err
+		lggr.Debugw("Transaction already in mempool", "etx", tx, "err", sendError)
+		return commonclient.Successful
 	}
 	if sendError.IsTemporarilyUnderpriced() {
-		lggr.Infow("Transaction temporarily underpriced", "err", sendError.Error())
-		return commonclient.Successful, err
+		lggr.Infow("Transaction temporarily underpriced", "err", sendError)
+		return commonclient.Successful
 	}
 	if sendError.IsTerminallyUnderpriced() {
-		return commonclient.Underpriced, err
+		lggr.Errorw("Transaction terminally underpriced", "etx", tx, "err", sendError)
+		return commonclient.Underpriced
 	}
 	if sendError.L2FeeTooLow() || sendError.IsL2FeeTooHigh() || sendError.IsL2Full() {
 		if isL2 {
-			return commonclient.FeeOutOfValidRange, err
+			lggr.Errorw("Transaction fee out of range", "err", sendError, "etx", tx)
+			return commonclient.FeeOutOfValidRange
 		}
-		return commonclient.Unsupported, errors.Wrap(sendError, "this error type only handled for L2s")
+		lggr.Errorw("this error type only handled for L2s", "err", sendError, "etx", tx)
+		return commonclient.Unsupported
 	}
 	if sendError.IsNonceTooHighError() {
 		// This error occurs when the tx nonce is greater than current_nonce + tx_count_in_mempool,
 		// instead of keeping the tx in mempool. This can happen if previous transactions haven't
 		// reached the client yet. The correct thing to do is to mark it as retryable.
-		lggr.Warnw("Transaction has a nonce gap.", "err", err)
-		return commonclient.Retryable, err
+		lggr.Warnw("Transaction has a nonce gap.", "err", sendError, "etx", tx)
+		return commonclient.Retryable
 	}
 	if sendError.IsInsufficientEth() {
 		lggr.Criticalw(fmt.Sprintf("Tx %x with type 0x%d was rejected due to insufficient eth: %s\n"+
 			"ACTION REQUIRED: Chainlink wallet with address 0x%x is OUT OF FUNDS",
 			tx.Hash(), tx.Type(), sendError.Error(), fromAddress,
-		), "err", sendError)
-		return commonclient.InsufficientFunds, err
+		), "err", sendError, "etx", tx)
+		return commonclient.InsufficientFunds
 	}
 	if sendError.IsTimeout() {
-		return commonclient.Retryable, errors.Wrapf(sendError, "timeout while sending transaction %s", tx.Hash().Hex())
+		lggr.Errorw("timeout while sending transaction %x", tx.Hash(), "err", sendError, "etx", tx)
+		return commonclient.Retryable
 	}
 	if sendError.IsTxFeeExceedsCap() {
 		lggr.Criticalw(fmt.Sprintf("Sending transaction failed: %s", label.RPCTxFeeCapConfiguredIncorrectlyWarning),
 			"etx", tx,
 			"err", sendError,
 			"id", "RPCTxFeeCapExceeded",
 		)
-		return commonclient.ExceedsMaxFee, err
+		return commonclient.ExceedsMaxFee
 	}
-	return commonclient.Unknown, err
+	lggr.Errorw("Unknown error encountered when sending transaction", "err", err, "etx", tx)
+	return commonclient.Unknown
 }
 
 // ClassifySendOnlyError handles SendOnly nodes error codes. In that case, we don't assume there is another transaction that will be correctly

core/chains/evm/client/send_only_node_test.go

@@ -22,6 +22,7 @@ import (
 	"github.com/smartcontractkit/chainlink/v2/core/chains/evm/client"
 	evmclient "github.com/smartcontractkit/chainlink/v2/core/chains/evm/client"
 	"github.com/smartcontractkit/chainlink/v2/core/chains/evm/client/mocks"
+	"github.com/smartcontractkit/chainlink/v2/core/internal/cltest"
 	"github.com/smartcontractkit/chainlink/v2/core/internal/testutils"
 )
 
@@ -95,7 +96,7 @@ func createSignedTx(t *testing.T, chainID *big.Int, nonce uint64, data []byte) *
 	require.NoError(t, err)
 	sender, err := bind.NewKeyedTransactorWithChainID(key, chainID)
 	require.NoError(t, err)
-	tx := types.NewTransaction(
+	tx := cltest.NewLegacyTransaction(
 		nonce, sender.From,
 		assets.Ether(100).ToInt(),
 		21000, big.NewInt(1000000000), data,

core/chains/evm/txmgr/attempts.go

@@ -3,6 +3,7 @@ package txmgr
 import (
 	"bytes"
 	"context"
+	"fmt"
 	"math/big"
 
 	"github.com/ethereum/go-ethereum/common"
@@ -119,9 +120,17 @@ func (c *evmTxAttemptBuilder) NewEmptyTxAttempt(nonce evmtypes.Nonce, feeLimit u
 		return attempt, errors.New("NewEmptyTranscation: legacy fee cannot be nil")
 	}
 
-	tx := types.NewTransaction(uint64(nonce), fromAddress, value, uint64(feeLimit), fee.Legacy.ToInt(), payload)
+	tx := newLegacyTransaction(
+		uint64(nonce),
+		fromAddress,
+		value,
+		uint32(feeLimit),
+		fee.Legacy,
+		payload,
+	)
 
-	hash, signedTxBytes, err := c.SignTx(fromAddress, tx)
+	transaction := types.NewTx(&tx)
+	hash, signedTxBytes, err := c.SignTx(fromAddress, transaction)
 	if err != nil {
 		return attempt, errors.Wrapf(err, "error using account %s to sign empty transaction", fromAddress.String())
 	}
@@ -295,7 +304,7 @@ func newLegacyTransaction(nonce uint64, to common.Address, value *big.Int, gasLi
 func (c *evmTxAttemptBuilder) SignTx(address common.Address, tx *types.Transaction) (common.Hash, []byte, error) {
 	signedTx, err := c.keystore.SignTx(address, tx, &c.chainID)
 	if err != nil {
-		return common.Hash{}, nil, errors.Wrap(err, "SignTx failed")
+		return common.Hash{}, nil, fmt.Errorf("failed to sign tx: %w", err)
 	}
 	rlp := new(bytes.Buffer)
 	if err := signedTx.EncodeRLP(rlp); err != nil {

core/chains/evm/txmgr/attempts_test.go

@@ -6,6 +6,7 @@ import (
 	"testing"
 
 	gethcommon "github.com/ethereum/go-ethereum/common"
+	"github.com/ethereum/go-ethereum/common/hexutil"
 	"github.com/ethereum/go-ethereum/core/types"
 	gethtypes "github.com/ethereum/go-ethereum/core/types"
 	"github.com/pkg/errors"
@@ -85,6 +86,44 @@ func TestTxm_SignTx(t *testing.T) {
 		require.NotNil(t, rawBytes)
 		require.Equal(t, "0xdd68f554373fdea7ec6713a6e437e7646465d553a6aa0b43233093366cc87ef0", hash.String())
 	})
+	t.Run("can properly encoded and decode raw transaction for LegacyTx", func(t *testing.T) {
+		chainID := big.NewInt(1)
+		kst := ksmocks.NewEth(t)
+		kst.On("SignTx", to, tx, chainID).Return(tx, nil).Once()
+		cks := txmgr.NewEvmTxAttemptBuilder(*chainID, newFeeConfig(), kst, nil)
+
+		_, rawBytes, err := cks.SignTx(addr, tx)
+		require.NoError(t, err)
+		require.NotNil(t, rawBytes)
+		require.Equal(t, "0xe42a82015681f294b921f7763960b296b9cbad586ff066a18d749724818e83010203808080", hexutil.Encode(rawBytes))
+
+		var decodedTx *gethtypes.Transaction
+		decodedTx, err = txmgr.GetGethSignedTx(rawBytes)
+		require.NoError(t, err)
+		require.Equal(t, tx.Hash(), decodedTx.Hash())
+	})
+	t.Run("can properly encoded and decode raw transaction for DynamicFeeTx", func(t *testing.T) {
+		chainID := big.NewInt(1)
+		kst := ksmocks.NewEth(t)
+		typedTx := gethtypes.NewTx(&gethtypes.DynamicFeeTx{
+			Nonce: 42,
+			To:    &to,
+			Value: big.NewInt(142),
+			Gas:   242,
+			Data:  []byte{1, 2, 3},
+		})
+		kst.On("SignTx", to, typedTx, chainID).Return(typedTx, nil).Once()
+		cks := txmgr.NewEvmTxAttemptBuilder(*chainID, newFeeConfig(), kst, nil)
+		_, rawBytes, err := cks.SignTx(addr, typedTx)
+		require.NoError(t, err)
+		require.NotNil(t, rawBytes)
+		require.Equal(t, "0xa702e5802a808081f294b921f7763960b296b9cbad586ff066a18d749724818e83010203c0808080", hexutil.Encode(rawBytes))
+
+		var decodedTx *gethtypes.Transaction
+		decodedTx, err = txmgr.GetGethSignedTx(rawBytes)
+		require.NoError(t, err)
+		require.Equal(t, typedTx.Hash(), decodedTx.Hash())
+	})
 }
 
 func TestTxm_NewDynamicFeeTx(t *testing.T) {

core/chains/evm/txmgr/client.go

@@ -77,10 +77,14 @@ func (c *evmTxmClient) BatchSendTransactions(
 			// convert to tx for logging purposes - exits early if error occurs
 			tx, signedErr := GetGethSignedTx(attempts[i].SignedRawTx)
 			if signedErr != nil {
-				processingErr[i] = fmt.Errorf("failed to process tx (index %d): %w", i, signedErr)
+				signedErrMsg := fmt.Sprintf("failed to process tx (index %d)", i)
+				lggr.Errorw(signedErrMsg, "err", signedErr)
+				processingErr[i] = fmt.Errorf("%s: %w", signedErrMsg, signedErr)
 				return
 			}
-			codes[i], txErrs[i] = client.ClassifySendError(reqs[i].Error, lggr, tx, attempts[i].Tx.FromAddress, c.client.IsL2())
+			sendErr := reqs[i].Error
+			codes[i] = client.ClassifySendError(sendErr, lggr, tx, attempts[i].Tx.FromAddress, c.client.IsL2())
+			txErrs[i] = sendErr
 		}(index)
 	}
 	wg.Wait()

core/chains/evm/txmgr/common.go

@@ -35,25 +35,36 @@ func batchSendTransactions(
 	reqs := make([]rpc.BatchElem, len(attempts))
 	ethTxIDs := make([]int64, len(attempts))
 	hashes := make([]string, len(attempts))
+	now := time.Now()
+	successfulBroadcast := []int64{}
 	for i, attempt := range attempts {
 		ethTxIDs[i] = attempt.TxID
 		hashes[i] = attempt.Hash.String()
+		// Decode the signed raw tx back into a Transaction object
+		signedTx, decodeErr := GetGethSignedTx(attempt.SignedRawTx)
+		if decodeErr != nil {
+			return reqs, now, successfulBroadcast, fmt.Errorf("failed to decode signed raw tx into Transaction object: %w", decodeErr)
+		}
+		// Get the canonical encoding of the Transaction object needed for the eth_sendRawTransaction request
+		// The signed raw tx cannot be used directly because it uses a different encoding
+		txBytes, marshalErr := signedTx.MarshalBinary()
+		if marshalErr != nil {
+			return reqs, now, successfulBroadcast, fmt.Errorf("failed to marshal tx into canonical encoding: %w", marshalErr)
+		}
 		req := rpc.BatchElem{
 			Method: "eth_sendRawTransaction",
-			Args:   []interface{}{hexutil.Encode(attempt.SignedRawTx)},
+			Args:   []interface{}{hexutil.Encode(txBytes)},
 			Result: &common.Hash{},
 		}
 		reqs[i] = req
 	}
 
 	logger.Debugw(fmt.Sprintf("Batch sending %d unconfirmed transactions.", len(attempts)), "n", len(attempts), "ethTxIDs", ethTxIDs, "hashes", hashes)
 
-	now := time.Now()
 	if batchSize == 0 {
 		batchSize = len(reqs)
 	}
 
-	successfulBroadcast := []int64{}
 	for i := 0; i < len(reqs); i += batchSize {
 		j := i + batchSize
 		if j > len(reqs) {