Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Host: avoid spamming stuck L1 transactions #1941

Merged
merged 2 commits into from
May 31, 2024
Merged

Host: avoid spamming stuck L1 transactions #1941

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3602908
Host: avoid spamming stuck L1 transactions
BedrockSquirrel May 30, 2024
390e8b1
fix in-mem test
BedrockSquirrel May 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 6 additions & 8 deletions go/ethadapter/geth_rpc_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,12 +246,16 @@ func (e *gethRPCClient) FetchLastBatchSeqNo(address gethcommon.Address) (*big.In

// 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)
nonce, err := e.EthClient().PendingNonceAt(ctx, from)
if err != nil {
return nil, fmt.Errorf("could not get nonce - %w", err)
}
return e.PrepareTransactionToRetry(ctx, txData, from, nonce, 0)
}

// 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(ctx context.Context, txData types.TxData, from gethcommon.Address, retryNumber int) (types.TxData, error) {
func (e *gethRPCClient) PrepareTransactionToRetry(ctx context.Context, txData types.TxData, from gethcommon.Address, nonce uint64, retryNumber int) (types.TxData, error) {
unEstimatedTx := types.NewTx(txData)
gasPrice, err := e.EthClient().SuggestGasPrice(ctx)
if err != nil {
Expand Down Expand Up @@ -279,12 +283,6 @@ func (e *gethRPCClient) PrepareTransactionToRetry(ctx context.Context, txData ty
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{
Nonce: nonce,
GasPrice: retryPrice,
Expand Down
2 changes: 1 addition & 1 deletion go/ethadapter/interface.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ type EthClient interface {

// PrepareTransactionToSend updates the tx with from address, current nonce and current estimates for the gas and the gas price
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)
PrepareTransactionToRetry(ctx context.Context, txData types.TxData, from gethcommon.Address, nonce uint64, retries int) (types.TxData, error)

FetchLastBatchSeqNo(address gethcommon.Address) (*big.Int, error)

Expand Down
9 changes: 8 additions & 1 deletion go/host/l1/publisher.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -391,12 +391,19 @@ func (p *Publisher) publishTransaction(tx types.TxData) error {

retries := -1

// we keep trying to send the transaction with this nonce until it is included in a block
// note: this is only safe because of the sendingLock guaranteeing only one transaction in-flight at a time
nonce, err := p.ethClient.EthClient().PendingNonceAt(p.sendingContext, p.hostWallet.Address())
if err != nil {
return fmt.Errorf("could not get nonce for L1 tx: %w", err)
}

// 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

// update the tx gas price before each attempt
tx, err := p.ethClient.PrepareTransactionToRetry(p.sendingContext, tx, p.hostWallet.Address(), retries)
tx, err := p.ethClient.PrepareTransactionToRetry(p.sendingContext, tx, p.hostWallet.Address(), nonce, retries)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to add some sort of timeout here? given the original impl choice was to avoid nonce reuse/ conflicts it looks like we might be putting that back in. Having a configurable timeout for how many times we want to try a nonce before we aquire a new one might be useful

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We avoid the nonce-conflicts with the synchronization. We block on the sendingLock whenever a transaction is in-flight. So long as the nonce is fetched after locking I think that's safe.

I do think we might want to revisit this at some point to make it more intuitive but I think it's very safe. The unintuitive bit is that the calls to publish rollups etc. are blocking and so the rollup timer won't fire again until the rollup it was trying to publish goes through, it never gives up.

But this bug where I was fetching the pending nonce on each attempt was causing the backing up, the later txs with higher gas prices weren't going through because the first one was stuck.

if err != nil {
return errors.Wrap(err, "could not estimate gas/gas price for L1 tx")
}
Expand Down
2 changes: 1 addition & 1 deletion integration/ethereummock/node.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ func (m *Node) PrepareTransactionToSend(_ context.Context, txData types.TxData,
}, nil
}

func (m *Node) PrepareTransactionToRetry(ctx context.Context, txData types.TxData, from gethcommon.Address, _ int) (types.TxData, error) {
func (m *Node) PrepareTransactionToRetry(ctx context.Context, txData types.TxData, from gethcommon.Address, _ uint64, _ int) (types.TxData, error) {
return m.PrepareTransactionToSend(ctx, txData, from)
}

Expand Down
Loading