From 968aa803f4efec127b2df1470c1f5b8097b98ec6 Mon Sep 17 00:00:00 2001 From: Matt Curtis Date: Tue, 19 Mar 2024 11:07:46 +0000 Subject: [PATCH 1/3] Host L1 txs: one-at-a-time to remove nonce risk --- go/ethadapter/geth_rpc_client.go | 24 ++++++----- go/ethadapter/interface.go | 5 ++- go/host/l1/publisher.go | 42 ++++++++++++-------- integration/ethereummock/node.go | 9 +++-- integration/simulation/network/geth_utils.go | 9 +++-- integration/simulation/simulation.go | 2 +- 6 files changed, 55 insertions(+), 36 deletions(-) diff --git a/go/ethadapter/geth_rpc_client.go b/go/ethadapter/geth_rpc_client.go index 1d55dedbb1..17745cd903 100644 --- a/go/ethadapter/geth_rpc_client.go +++ b/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{ diff --git a/go/ethadapter/interface.go b/go/ethadapter/interface.go index 40a1bf7439..45f3d0e56f 100644 --- a/go/ethadapter/interface.go +++ b/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) diff --git a/go/host/l1/publisher.go b/go/host/l1/publisher.go index 6e2a0545b5..de821798fb 100644 --- a/go/host/l1/publisher.go +++ b/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 { diff --git a/integration/ethereummock/node.go b/integration/ethereummock/node.go index ef08ad230c..1531adffc3 100644 --- a/integration/ethereummock/node.go +++ b/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 { diff --git a/integration/simulation/network/geth_utils.go b/integration/simulation/network/geth_utils.go index 1d19df0737..39504b9fb7 100644 --- a/integration/simulation/network/geth_utils.go +++ b/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 } diff --git a/integration/simulation/simulation.go b/integration/simulation/simulation.go index 6576e94dec..c673bd1dd2 100644 --- a/integration/simulation/simulation.go +++ b/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) From dc76e9e85c03c00a01382fa591fcbdeddfa754ad Mon Sep 17 00:00:00 2001 From: Matt Curtis Date: Tue, 19 Mar 2024 11:31:43 +0000 Subject: [PATCH 2/3] lint --- integration/eth2network/eth2_network_test.go | 4 ++-- integration/manualtests/tx_test.go | 12 ++++-------- .../networktest/actions/l1/important_contracts.go | 2 +- 3 files changed, 7 insertions(+), 11 deletions(-) diff --git a/integration/eth2network/eth2_network_test.go b/integration/eth2network/eth2_network_test.go index 4874b44cb0..4eb712df21 100644 --- a/integration/eth2network/eth2_network_test.go +++ b/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) diff --git a/integration/manualtests/tx_test.go b/integration/manualtests/tx_test.go index 7dfd7a4477..2f65d84c34 100644 --- a/integration/manualtests/tx_test.go +++ b/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) diff --git a/integration/networktest/actions/l1/important_contracts.go b/integration/networktest/actions/l1/important_contracts.go index 654ef3b754..dfff1f7c97 100644 --- a/integration/networktest/actions/l1/important_contracts.go +++ b/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") } From 8c404aa032b831601904d5c1dfed767fa436be9b Mon Sep 17 00:00:00 2001 From: Matt Curtis Date: Tue, 19 Mar 2024 11:55:17 +0000 Subject: [PATCH 3/3] compilation misses --- integration/smartcontract/debug_wallet.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/integration/smartcontract/debug_wallet.go b/integration/smartcontract/debug_wallet.go index 0bcd7865e4..b8b0dc297f 100644 --- a/integration/smartcontract/debug_wallet.go +++ b/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