Merge pull request #7782 from mdehoog/michael/dont-increment-nonce-for-signer-failures

[txmgr] Avoid incrementing the nonce if tx signing fails

op-service/txmgr/txmgr.go

@@ -238,23 +238,15 @@ func (m *SimpleTxManager) craftTx(ctx context.Context, candidate TxCandidate) (*
 		rawTx.Gas = gas
 	}
 
-	// Avoid bumping the nonce if the gas estimation fails.
-	nonce, err := m.nextNonce(ctx)
-	if err != nil {
-		return nil, err
-	}
-	rawTx.Nonce = nonce
-
-	ctx, cancel := context.WithTimeout(ctx, m.cfg.NetworkTimeout)
-	defer cancel()
-	return m.cfg.Signer(ctx, m.cfg.From, types.NewTx(rawTx))
+	return m.signWithNextNonce(ctx, rawTx)
 }
 
-// nextNonce returns a nonce to use for the next transaction. It uses
-// eth_getTransactionCount with "latest" once, and then subsequent calls simply
-// increment this number. If the transaction manager is reset, it will query the
-// eth_getTransactionCount nonce again.
-func (m *SimpleTxManager) nextNonce(ctx context.Context) (uint64, error) {
+// signWithNextNonce returns a signed transaction with the next available nonce.
+// The nonce is fetched once using eth_getTransactionCount with "latest", and
+// then subsequent calls simply increment this number. If the transaction manager
+// is reset, it will query the eth_getTransactionCount nonce again. If signing
+// fails, the nonce is not incremented.
+func (m *SimpleTxManager) signWithNextNonce(ctx context.Context, rawTx *types.DynamicFeeTx) (*types.Transaction, error) {
 	m.nonceLock.Lock()
 	defer m.nonceLock.Unlock()
 
@@ -265,15 +257,25 @@ func (m *SimpleTxManager) nextNonce(ctx context.Context) (uint64, error) {
 		nonce, err := m.backend.NonceAt(childCtx, m.cfg.From, nil)
 		if err != nil {
 			m.metr.RPCError()
-			return 0, fmt.Errorf("failed to get nonce: %w", err)
+			return nil, fmt.Errorf("failed to get nonce: %w", err)
 		}
 		m.nonce = &nonce
 	} else {
 		*m.nonce++
 	}
 
+	rawTx.Nonce = *m.nonce
+	ctx, cancel := context.WithTimeout(ctx, m.cfg.NetworkTimeout)
+	defer cancel()
+	tx, err := m.cfg.Signer(ctx, m.cfg.From, types.NewTx(rawTx))
+	if err != nil {
+		// decrement the nonce, so we can retry signing with the same nonce next time
+		// signWithNextNonce is called
+		*m.nonce--
+	} else {
 		m.metr.RecordNonce(*m.nonce)
-	return *m.nonce, nil
+	}
+	return tx, err
 }
 
 // resetNonce resets the internal nonce tracking. This is called if any pending send

op-service/txmgr/txmgr_test.go

@@ -450,6 +450,40 @@ func TestTxMgr_EstimateGasFails(t *testing.T) {
 	require.Equal(t, lastNonce+1, tx.Nonce())
 }
 
+func TestTxMgr_SigningFails(t *testing.T) {
+	t.Parallel()
+	errorSigning := false
+	cfg := configWithNumConfs(1)
+	cfg.Signer = func(ctx context.Context, from common.Address, tx *types.Transaction) (*types.Transaction, error) {
+		if errorSigning {
+			return nil, fmt.Errorf("signer error")
+		} else {
+			return tx, nil
+		}
+	}
+	h := newTestHarnessWithConfig(t, cfg)
+	candidate := h.createTxCandidate()
+
+	// Set the gas limit to zero to trigger gas estimation.
+	candidate.GasLimit = 0
+
+	// Craft a successful transaction.
+	tx, err := h.mgr.craftTx(context.Background(), candidate)
+	require.Nil(t, err)
+	lastNonce := tx.Nonce()
+
+	// Mock signer failure.
+	errorSigning = true
+	_, err = h.mgr.craftTx(context.Background(), candidate)
+	require.ErrorContains(t, err, "signer error")
+
+	// Ensure successful craft uses the correct nonce
+	errorSigning = false
+	tx, err = h.mgr.craftTx(context.Background(), candidate)
+	require.Nil(t, err)
+	require.Equal(t, lastNonce+1, tx.Nonce())
+}
+
 // TestTxMgrOnlyOnePublicationSucceeds asserts that the tx manager will return a
 // receipt so long as at least one of the publications is able to succeed with a
 // simulated rpc failure.