Skip to content

Commit

Permalink
Mark nonce gap error as retryable
Browse files Browse the repository at this point in the history
  • Loading branch information
dimriou committed Feb 7, 2023
1 parent 7bf5e43 commit cd91bf6
Show file tree
Hide file tree
Showing 4 changed files with 74 additions and 4 deletions.
12 changes: 10 additions & 2 deletions core/chains/evm/client/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,9 @@ func (s *SendError) CauseStr() string {

const (
NonceTooLow = iota
// Nethermind specific error. Nethermind throws a NonceGap error when the tx nonce is greater than current_nonce + tx_count_in_mempool, instead of keeping the tx in mempool.
// See: https://github.com/NethermindEth/nethermind/blob/master/src/Nethermind/Nethermind.TxPool/Filters/GapNonceFilter.cs
NonceTooHigh
ReplacementTransactionUnderpriced
LimitReached
TransactionAlreadyInMempool
Expand Down Expand Up @@ -167,10 +170,11 @@ var klaytn = ClientErrors{
// Nethermind
// All errors: https://github.com/NethermindEth/nethermind/blob/master/src/Nethermind/Nethermind.TxPool/AcceptTxResult.cs
// All filters: https://github.com/NethermindEth/nethermind/tree/9b68ec048c65f4b44fb863164c0dec3f7780d820/src/Nethermind/Nethermind.TxPool/Filters
var nethermindFatal = regexp.MustCompile(`(: |^)(SenderIsContract|Invalid(, transaction Hash is null)?|Int256Overflow|FailedToResolveSender|GasLimitExceeded(, Gas limit: \d+, gas limit of rejected tx: \d+)?|NonceGap(, Future nonce. Expected nonce: \d+)?)$`)
var nethermindFatal = regexp.MustCompile(`(: |^)(SenderIsContract|Invalid(, transaction Hash is null)?|Int256Overflow|FailedToResolveSender|GasLimitExceeded(, Gas limit: \d+, gas limit of rejected tx: \d+)?)$`)
var nethermind = ClientErrors{
// OldNonce: The EOA (externally owned account) that signed this transaction (sender) has already signed and executed a transaction with the same nonce.
NonceTooLow: regexp.MustCompile(`(: |^)OldNonce(, Current nonce: \d+, nonce of rejected tx: \d+)?$`),
NonceTooLow: regexp.MustCompile(`(: |^)OldNonce(, Current nonce: \d+, nonce of rejected tx: \d+)?$`),
NonceTooHigh: regexp.MustCompile(`(: |^)NonceGap(, Future nonce. Expected nonce: \d+)?$`),

// FeeTooLow/FeeTooLowToCompete: Fee paid by this transaction is not enough to be accepted in the mempool.
TerminallyUnderpriced: regexp.MustCompile(`(: |^)(FeeTooLow(, MaxFeePerGas too low. MaxFeePerGas: \d+, BaseFee: \d+, MaxPriorityFeePerGas:\d+, Block number: \d+|` +
Expand Down Expand Up @@ -224,6 +228,10 @@ func (s *SendError) IsNonceTooLowError() bool {
return s.is(NonceTooLow)
}

func (s *SendError) IsNonceTooHighError() bool {
return s.is(NonceTooHigh)
}

// IsTransactionAlreadyMined - Harmony returns this error if the transaction has already been mined
func (s *SendError) IsTransactionAlreadyMined() bool {
return s.is(TransactionAlreadyMined)
Expand Down
17 changes: 15 additions & 2 deletions core/chains/evm/client/errors_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,21 @@ func Test_Eth_Errors(t *testing.T) {
}
})

t.Run("IsNonceTooHigh", func(t *testing.T) {

tests := []errorCase{
{"call failed: NonceGap", true, "Nethermind"},
{"call failed: NonceGap, Future nonce. Expected nonce: 10", true, "Nethermind"},
}

for _, test := range tests {
err = evmclient.NewSendErrorS(test.message)
assert.Equal(t, err.IsNonceTooHighError(), test.expect)
err = newSendErrorWrapped(test.message)
assert.Equal(t, err.IsNonceTooHighError(), test.expect)
}
})

t.Run("IsTransactionAlreadyMined", func(t *testing.T) {
assert.False(t, randomError.IsTransactionAlreadyMined())

Expand Down Expand Up @@ -316,8 +331,6 @@ func Test_Eth_Errors_Fatal(t *testing.T) {
{"call failed: FailedToResolveSender", true, "Nethermind"},
{"call failed: GasLimitExceeded", true, "Nethermind"},
{"call failed: GasLimitExceeded, Gas limit: 100, gas limit of rejected tx: 150", true, "Nethermind"},
{"call failed: NonceGap", true, "Nethermind"},
{"call failed: NonceGap, Future nonce. Expected nonce: 10", true, "Nethermind"},

{"invalid shard", true, "Harmony"},
{"`to` address of transaction in blacklist", true, "Harmony"},
Expand Down
10 changes: 10 additions & 0 deletions core/chains/evm/txmgr/eth_broadcaster.go
Original file line number Diff line number Diff line change
Expand Up @@ -557,6 +557,16 @@ func (eb *EthBroadcaster) handleInProgressEthTx(ctx context.Context, etx EthTx,
return errors.Wrap(sendError, "this error type only handled for L2s"), false
}

if sendError.IsNonceTooHighError() {
// Nethermind specific error. Nethermind throws a NonceGap error 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 assume success for now and let the eth_confirmer retry until
// the nonce gap gets filled by the previous transactions.
lgr.Warnw("Transaction has a nonce gap.", "err", sendError.Error())
return sendError, true
}

if sendError.IsTemporarilyUnderpriced() {
// If we can't even get the transaction into the mempool at all, assume
// success (even though the transaction will never confirm) and hand
Expand Down
39 changes: 39 additions & 0 deletions core/chains/evm/txmgr/eth_broadcaster_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"fmt"
"math/big"
"math/rand"
"strconv"
"testing"
"time"

Expand Down Expand Up @@ -1594,6 +1595,44 @@ func TestEthBroadcaster_ProcessUnstartedEthTxs_Errors(t *testing.T) {

pgtest.MustExec(t, db, `DELETE FROM eth_txes`)

t.Run("eth tx is left in progress if nonce is too high", func(t *testing.T) {
localNextNonce := getLocalNextNonce(t, ethKeyStore, fromAddress)
nonceGapError := "NonceGap, Future nonce. Expected nonce: " + strconv.FormatUint(localNextNonce, 10)
etx := txmgr.EthTx{
FromAddress: fromAddress,
ToAddress: toAddress,
EncodedPayload: encodedPayload,
Value: value,
GasLimit: gasLimit,
State: txmgr.EthTxUnstarted,
}
require.NoError(t, borm.InsertEthTx(&etx))

ethClient.On("SendTransaction", mock.Anything, mock.MatchedBy(func(tx *gethTypes.Transaction) bool {
return tx.Nonce() == localNextNonce
})).Return(errors.New(nonceGapError)).Once()

err, retryable := eb.ProcessUnstartedEthTxs(testutils.Context(t), keyState)
require.Error(t, err)
assert.Contains(t, err.Error(), nonceGapError)
assert.True(t, retryable)

etx, err = borm.FindEthTxWithAttempts(etx.ID)
require.NoError(t, err)

assert.Nil(t, etx.BroadcastAt)
assert.Nil(t, etx.InitialBroadcastAt)
require.NotNil(t, etx.Nonce)
assert.False(t, etx.Error.Valid)
assert.Equal(t, txmgr.EthTxInProgress, etx.State)
require.Len(t, etx.EthTxAttempts, 1)
attempt := etx.EthTxAttempts[0]
assert.Equal(t, txmgr.EthTxAttemptInProgress, attempt.State)
assert.Nil(t, attempt.BroadcastBeforeBlockNum)

pgtest.MustExec(t, db, `DELETE FROM eth_txes`)
})

t.Run("eth node returns underpriced transaction and bumping gas doesn't increase it in EIP-1559 mode", func(t *testing.T) {
// This happens if a transaction's gas price is below the minimum
// configured for the transaction pool.
Expand Down

0 comments on commit cd91bf6

Please sign in to comment.