Host L1 txs: one-at-a-time to remove nonce risk (#1839)

go/ethadapter/geth_rpc_client.go

@@ -244,18 +244,18 @@ func (e *gethRPCClient) FetchLastBatchSeqNo(address gethcommon.Address) (*big.In
 	return contract.LastBatchSeqNo(&bind.CallOpts{})
 }
 
-// PrepareTransactionToSend takes a txData type and overrides the From, Nonce, Gas and Gas Price field with current values
-func (e *gethRPCClient) PrepareTransactionToSend(txData types.TxData, from gethcommon.Address, nonce uint64) (types.TxData, error) {
-	return e.PrepareTransactionToRetry(txData, from, nonce, 0)
+// PrepareTransactionToSend takes a txData type and overrides the From, Gas and Gas Price field with current values
+func (e *gethRPCClient) PrepareTransactionToSend(ctx context.Context, txData types.TxData, from gethcommon.Address) (types.TxData, error) {
+	return e.PrepareTransactionToRetry(ctx, txData, from, 0)
 }
 
-// PrepareTransactionToRetry takes a txData type and overrides the From, Nonce, Gas and Gas Price field with current values
+// PrepareTransactionToRetry takes a txData type and overrides the From, Gas and Gas Price field with current values
 // it bumps the price by a multiplier for retries. retryNumber is zero on first attempt (no multiplier on price)
-func (e *gethRPCClient) PrepareTransactionToRetry(txData types.TxData, from gethcommon.Address, nonce uint64, retryNumber int) (types.TxData, error) {
+func (e *gethRPCClient) PrepareTransactionToRetry(ctx context.Context, txData types.TxData, from gethcommon.Address, retryNumber int) (types.TxData, error) {
 	unEstimatedTx := types.NewTx(txData)
-	gasPrice, err := e.EthClient().SuggestGasPrice(context.Background())
+	gasPrice, err := e.EthClient().SuggestGasPrice(ctx)
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("could not suggest gas price - %w", err)
 	}
 
 	// it should never happen but to avoid any risk of repeated price increases we cap the possible retry price bumps to 5
@@ -269,14 +269,20 @@ func (e *gethRPCClient) PrepareTransactionToRetry(txData types.TxData, from geth
 	// prices aren't big enough for float error to matter
 	retryPrice, _ := retryPriceFloat.Int(nil)
 
-	gasLimit, err := e.EthClient().EstimateGas(context.Background(), ethereum.CallMsg{
+	gasLimit, err := e.EthClient().EstimateGas(ctx, ethereum.CallMsg{
 		From:  from,
 		To:    unEstimatedTx.To(),
 		Value: unEstimatedTx.Value(),
 		Data:  unEstimatedTx.Data(),
 	})
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("could not estimate gas - %w", err)
+	}
+
+	// we fetch the current nonce on every retry to avoid any risk of nonce reuse/conflicts
+	nonce, err := e.EthClient().PendingNonceAt(ctx, from)
+	if err != nil {
+		return nil, fmt.Errorf("could not fetch nonce - %w", err)
 	}
 
 	return &types.LegacyTx{

go/ethadapter/interface.go

@@ -1,6 +1,7 @@
 package ethadapter
 
 import (
+	"context"
 	"errors"
 	"math/big"
 
@@ -37,8 +38,8 @@ type EthClient interface {
 	CallContract(msg ethereum.CallMsg) ([]byte, error) // Runs the provided call message on the latest block.
 
 	// PrepareTransactionToSend updates the tx with from address, current nonce and current estimates for the gas and the gas price
-	PrepareTransactionToSend(txData types.TxData, from gethcommon.Address, nonce uint64) (types.TxData, error)
-	PrepareTransactionToRetry(txData types.TxData, from gethcommon.Address, nonce uint64, retries int) (types.TxData, error)
+	PrepareTransactionToSend(ctx context.Context, txData types.TxData, from gethcommon.Address) (types.TxData, error)
+	PrepareTransactionToRetry(ctx context.Context, txData types.TxData, from gethcommon.Address, retries int) (types.TxData, error)
 
 	FetchLastBatchSeqNo(address gethcommon.Address) (*big.Int, error)
 

go/host/l1/publisher.go

@@ -1,6 +1,7 @@
 package l1
 
 import (
+	"context"
 	"encoding/json"
 	"fmt"
 	"math/big"
@@ -40,6 +41,12 @@ type Publisher struct {
 
 	maxWaitForL1Receipt       time.Duration
 	retryIntervalForL1Receipt time.Duration
+
+	// we only allow one transaction in-flight at a time to avoid nonce conflicts
+	// We also have a context to cancel the tx if host stops
+	sendingLock      sync.Mutex
+	sendingContext   context.Context
+	sendingCtxCancel context.CancelFunc
 }
 
 func NewL1Publisher(
@@ -53,6 +60,7 @@ func NewL1Publisher(
 	maxWaitForL1Receipt time.Duration,
 	retryIntervalForL1Receipt time.Duration,
 ) *Publisher {
+	sendingCtx, cancelSendingCtx := context.WithCancel(context.Background())
 	return &Publisher{
 		hostData:                  hostData,
 		hostWallet:                hostWallet,
@@ -66,6 +74,10 @@ func NewL1Publisher(
 
 		importantContractAddresses: map[string]gethcommon.Address{},
 		importantAddressesMutex:    sync.RWMutex{},
+
+		sendingLock:      sync.Mutex{},
+		sendingContext:   sendingCtx,
+		sendingCtxCancel: cancelSendingCtx,
 	}
 }
 
@@ -81,6 +93,7 @@ func (p *Publisher) Start() error {
 }
 
 func (p *Publisher) Stop() error {
+	p.sendingCtxCancel()
 	return nil
 }
 
@@ -303,42 +316,36 @@ func (p *Publisher) ResyncImportantContracts() error {
 }
 
 // publishTransaction will keep trying unless the L1 seems to be unavailable or the tx is otherwise rejected
-// It is responsible for keeping the nonce accurate, according to the following rules:
-// - Caller should not increment the wallet nonce before this method is called
-// - This method will increment the wallet nonce only if the transaction is successfully broadcast
-// - This method will continue to resend the tx using latest gas price until it is successfully broadcast or the L1 is unavailable/this service is shutdown
-// - **ONLY** the L1 publisher service is publishing transactions for this wallet (to avoid nonce conflicts)
+// this method is guarded by a lock to ensure that only one transaction is attempted at a time to avoid nonce conflicts
 // todo (@matt) this method should take a context so we can try to cancel if the tx is no longer required
 func (p *Publisher) publishTransaction(tx types.TxData) error {
-	// the nonce to be used for this tx attempt
-	nonce := p.hostWallet.GetNonceAndIncrement()
+	// this log message seems superfluous but is useful to debug deadlock issues, we expect 'Host issuing l1 tx' soon
+	// after unless we're stuck blocking.
+	p.logger.Info("Host preparing to issue L1 tx")
+
+	p.sendingLock.Lock()
+	defer p.sendingLock.Unlock()
+
 	retries := -1
 
 	// while the publisher service is still alive we keep trying to get the transaction into the L1
 	for !p.hostStopper.IsStopping() {
 		retries++ // count each attempt so we can increase gas price
 
-		// make sure an earlier tx hasn't been abandoned
-		if nonce > p.hostWallet.GetNonce() {
-			return errors.New("earlier transaction has failed to complete, we need to abort this transaction")
-		}
 		// update the tx gas price before each attempt
-		tx, err := p.ethClient.PrepareTransactionToRetry(tx, p.hostWallet.Address(), nonce, retries)
+		tx, err := p.ethClient.PrepareTransactionToRetry(p.sendingContext, tx, p.hostWallet.Address(), retries)
 		if err != nil {
-			p.hostWallet.SetNonce(nonce) // revert the wallet nonce because we failed to complete the transaction
 			return errors.Wrap(err, "could not estimate gas/gas price for L1 tx")
 		}
 
 		signedTx, err := p.hostWallet.SignTransaction(tx)
 		if err != nil {
-			p.hostWallet.SetNonce(nonce) // revert the wallet nonce because we failed to complete the transaction
 			return errors.Wrap(err, "could not sign L1 tx")
 		}
 
 		p.logger.Info("Host issuing l1 tx", log.TxKey, signedTx.Hash(), "size", signedTx.Size()/1024, "retries", retries)
 		err = p.ethClient.SendTransaction(signedTx)
 		if err != nil {
-			p.hostWallet.SetNonce(nonce) // revert the wallet nonce because we failed to complete the transaction
 			return errors.Wrap(err, "could not broadcast L1 tx")
 		}
 		p.logger.Info("Successfully submitted tx to L1", "txHash", signedTx.Hash())
@@ -347,6 +354,9 @@ func (p *Publisher) publishTransaction(tx types.TxData) error {
 		// retry until receipt is found
 		err = retry.Do(
 			func() error {
+				if p.hostStopper.IsStopping() {
+					return retry.FailFast(errors.New("host is stopping"))
+				}
 				receipt, err = p.ethClient.TransactionReceipt(signedTx.Hash())
 				if err != nil {
 					return fmt.Errorf("could not get receipt for L1 tx=%s: %w", signedTx.Hash(), err)
@@ -357,7 +367,7 @@ func (p *Publisher) publishTransaction(tx types.TxData) error {
 		)
 		if err != nil {
 			p.logger.Info("Receipt not found for transaction, we will re-attempt", log.ErrKey, err)
-			continue // try again on the same nonce, with updated gas price
+			continue // try again with updated gas price
 		}
 
 		if err == nil && receipt.Status != types.ReceiptStatusSuccessful {

integration/eth2network/eth2_network_test.go

@@ -148,10 +148,10 @@ func txsAreMinted(t *testing.T, wallets []wallet.Wallet) {
 		w := wallets[i]
 
 		toAddr := datagenerator.RandomAddress()
-		estimatedTx, err := ethClient.PrepareTransactionToSend(&types.LegacyTx{
+		estimatedTx, err := ethClient.PrepareTransactionToSend(context.Background(), &types.LegacyTx{
 			To:    &toAddr,
 			Value: big.NewInt(100),
-		}, w.Address(), w.GetNonceAndIncrement())
+		}, w.Address())
 		assert.Nil(t, err)
 
 		signedTx, err := w.SignTransaction(estimatedTx)

integration/ethereummock/node.go

@@ -2,6 +2,7 @@ package ethereummock
 
 import (
 	"bytes"
+	"context"
 	"errors"
 	"fmt"
 	"math/big"
@@ -86,10 +87,10 @@ type Node struct {
 	logger gethlog.Logger
 }
 
-func (m *Node) PrepareTransactionToSend(txData types.TxData, _ gethcommon.Address, nonce uint64) (types.TxData, error) {
+func (m *Node) PrepareTransactionToSend(_ context.Context, txData types.TxData, _ gethcommon.Address) (types.TxData, error) {
 	tx := types.NewTx(txData)
 	return &types.LegacyTx{
-		Nonce:    nonce,
+		Nonce:    123,
 		GasPrice: tx.GasPrice(),
 		Gas:      tx.Gas(),
 		To:       tx.To(),
@@ -98,8 +99,8 @@ func (m *Node) PrepareTransactionToSend(txData types.TxData, _ gethcommon.Addres
 	}, nil
 }
 
-func (m *Node) PrepareTransactionToRetry(txData types.TxData, from gethcommon.Address, nonce uint64, _ int) (types.TxData, error) {
-	return m.PrepareTransactionToSend(txData, from, nonce)
+func (m *Node) PrepareTransactionToRetry(ctx context.Context, txData types.TxData, from gethcommon.Address, _ int) (types.TxData, error) {
+	return m.PrepareTransactionToSend(ctx, txData, from)
 }
 
 func (m *Node) SendTransaction(tx *types.Transaction) error {

integration/manualtests/tx_test.go

@@ -63,13 +63,9 @@ func TestL1IssueContractInteractWaitReceipt(t *testing.T) {
 
 	storeContractBytecode := "0x608060405234801561001057600080fd5b5061020b806100206000396000f3fe608060405234801561001057600080fd5b50600436106100365760003560e01c80632e64cec11461003b57806370ef6e0b14610059575b600080fd5b610043610075565b60405161005091906100ab565b60405180910390f35b610073600480360381019061006e9190610161565b61007e565b005b60008054905090565b836000819055508260008190555050505050565b6000819050919050565b6100a581610092565b82525050565b60006020820190506100c0600083018461009c565b92915050565b600080fd5b600080fd5b6100d981610092565b81146100e457600080fd5b50565b6000813590506100f6816100d0565b92915050565b600080fd5b600080fd5b600080fd5b60008083601f840112610121576101206100fc565b5b8235905067ffffffffffffffff81111561013e5761013d610101565b5b60208301915083600182028301111561015a57610159610106565b5b9250929050565b6000806000806060858703121561017b5761017a6100c6565b5b6000610189878288016100e7565b945050602061019a878288016100e7565b935050604085013567ffffffffffffffff8111156101bb576101ba6100cb565b5b6101c78782880161010b565b92509250509295919450925056fea2646970667358221220eda68578fb741c32f26000b6c0273945f8322dd35f536c918e3d5a6193aaf62564736f6c63430008120033"
 
-	nonce, err := ethClient.Nonce(l1Wallet.Address())
-	require.NoError(t, err)
-
-	l1Wallet.SetNonce(nonce)
-	estimatedTx, err := ethClient.PrepareTransactionToSend(&types.LegacyTx{
+	estimatedTx, err := ethClient.PrepareTransactionToSend(context.Background(), &types.LegacyTx{
 		Data: gethcommon.FromHex(storeContractBytecode),
-	}, l1Wallet.Address(), l1Wallet.GetNonceAndIncrement())
+	}, l1Wallet.Address())
 	require.NoError(t, err)
 
 	signedTx, err := l1Wallet.SignTransaction(estimatedTx)
@@ -109,10 +105,10 @@ func TestL1IssueTxWaitReceipt(t *testing.T) {
 	require.NoError(t, err)
 
 	l1Wallet.SetNonce(nonce)
-	estimatedTx, err := ethClient.PrepareTransactionToSend(&types.LegacyTx{
+	estimatedTx, err := ethClient.PrepareTransactionToSend(context.Background(), &types.LegacyTx{
 		To:    &toAddr,
 		Value: big.NewInt(100),
-	}, l1Wallet.Address(), l1Wallet.GetNonceAndIncrement())
+	}, l1Wallet.Address())
 	require.NoError(t, err)
 
 	signedTx, err := l1Wallet.SignTransaction(estimatedTx)

integration/networktest/actions/l1/important_contracts.go

@@ -63,7 +63,7 @@ func (s *setImportantContract) Run(ctx context.Context, network networktest.Netw
 	// !! Important note !!
 	// The ownerOnly check in the contract doesn't like the gas estimate in here, to test you may need to hardcode a
 	// the gas value when the estimate errors
-	tx, err := l1Client.PrepareTransactionToSend(txData, networkCfg.ManagementContractAddress, mcOwner.GetNonceAndIncrement())
+	tx, err := l1Client.PrepareTransactionToSend(ctx, txData, networkCfg.ManagementContractAddress)
 	if err != nil {
 		return ctx, errors.Wrap(err, "failed to prepare tx")
 	}

integration/simulation/network/geth_utils.go

@@ -214,11 +214,12 @@ func InitializeContract(workerClient ethadapter.EthClient, w wallet.Wallet, cont
 // DeployContract returns receipt of deployment
 // todo (@matt) - this should live somewhere else
 func DeployContract(workerClient ethadapter.EthClient, w wallet.Wallet, contractBytes []byte) (*types.Receipt, error) {
-	deployContractTx, err := workerClient.PrepareTransactionToSend(&types.LegacyTx{
-		Data: contractBytes,
-	}, w.Address(), w.GetNonceAndIncrement())
+	deployContractTx, err := workerClient.PrepareTransactionToSend(
+		context.Background(),
+		&types.LegacyTx{Data: contractBytes},
+		w.Address(),
+	)
 	if err != nil {
-		w.SetNonce(w.GetNonce() - 1)
 		return nil, err
 	}
 

integration/simulation/simulation.go

@@ -276,7 +276,7 @@ func (s *Simulation) prefundL1Accounts() {
 			Sender:        &ownerAddr,
 		}
 		tx := s.Params.ERC20ContractLib.CreateDepositTx(txData)
-		estimatedTx, err := ethClient.PrepareTransactionToSend(tx, tokenOwner.Address(), tokenOwner.GetNonceAndIncrement())
+		estimatedTx, err := ethClient.PrepareTransactionToSend(s.ctx, tx, tokenOwner.Address())
 		if err != nil {
 			// ignore txs that are not able to be estimated/execute
 			testlog.Logger().Error("unable to estimate tx", log.ErrKey, err)

integration/smartcontract/debug_wallet.go

@@ -28,7 +28,7 @@ func newDebugWallet(w wallet.Wallet) *debugWallet {
 func (w *debugWallet) AwaitedSignAndSendTransaction(client ethadapter.EthClient, txData types.TxData) (*types.Transaction, *types.Receipt, error) {
 	var err error
 
-	txData, err = client.PrepareTransactionToSend(txData, w.Address(), w.GetNonceAndIncrement())
+	txData, err = client.PrepareTransactionToSend(context.Background(), txData, w.Address())
 	if err != nil {
 		w.SetNonce(w.GetNonce() - 1)
 		return nil, nil, err