Skip to content

Commit

Permalink
Fixed issue with in-progress tx during startup
Browse files Browse the repository at this point in the history
  • Loading branch information
amit-momin committed Mar 15, 2024
1 parent 0b22f2b commit a9d75e6
Show file tree
Hide file tree
Showing 5 changed files with 58 additions and 6 deletions.
4 changes: 2 additions & 2 deletions common/txmgr/broadcaster.go
Original file line number Diff line number Diff line change
Expand Up @@ -535,7 +535,7 @@ func (eb *Broadcaster[CHAIN_ID, HEAD, ADDR, TX_HASH, BLOCK_HASH, SEQ, FEE]) hand
return err, true
}
// Increment sequence if successfully broadcasted
eb.sequenceTracker.GenerateNextSequence(etx.FromAddress)
eb.sequenceTracker.GenerateNextSequence(etx.FromAddress, *etx.Sequence)
return err, true
case client.Underpriced:
return eb.tryAgainBumpingGas(ctx, lgr, err, etx, attempt, initialBroadcastAt)
Expand Down Expand Up @@ -587,7 +587,7 @@ func (eb *Broadcaster[CHAIN_ID, HEAD, ADDR, TX_HASH, BLOCK_HASH, SEQ, FEE]) hand
return err, true
}
// Increment sequence if successfully broadcasted
eb.sequenceTracker.GenerateNextSequence(etx.FromAddress)
eb.sequenceTracker.GenerateNextSequence(etx.FromAddress, *etx.Sequence)
return err, true
}
// Either the unknown error prevented the transaction from being mined, or
Expand Down
2 changes: 1 addition & 1 deletion common/txmgr/types/sequence_tracker.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ type SequenceTracker[
GetNextSequence(context.Context, ADDR) (SEQ, error)
// Signals the existing sequence has been used so generates and stores the next sequence
// Can be a no-op depending on the chain
GenerateNextSequence(ADDR)
GenerateNextSequence(ADDR, SEQ)
// Syncs the local sequence with the one on-chain in case the address as been used externally
// Can be a no-op depending on the chain
SyncSequence(context.Context, ADDR, services.StopChan)
Expand Down
40 changes: 40 additions & 0 deletions core/chains/evm/txmgr/broadcaster_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1759,6 +1759,46 @@ func TestEthBroadcaster_SyncNonce(t *testing.T) {
})
}

func TestEthBroadcaster_NonceTracker_InProgressTx(t *testing.T) {
t.Parallel()

db := pgtest.NewSqlxDB(t)
cfg := configtest.NewTestGeneralConfig(t)
txStore := cltest.NewTestTxStore(t, db, cfg.Database())
ethKeyStore := cltest.NewKeyStore(t, db, cfg.Database()).Eth()
_, fromAddress := cltest.MustInsertRandomKey(t, ethKeyStore)

ethClient := evmtest.NewEthClientMockWithDefaultChain(t)
evmcfg := evmtest.NewChainScopedConfig(t, cfg)
checkerFactory := &txmgr.CheckerFactory{Client: ethClient}

lggr := logger.Test(t)
ctx := testutils.Context(t)

t.Run("maintains the proper nonce if there is an in-progress tx during startup", func(t *testing.T) {
inProgressTxNonce := uint64(0)
ethClient.On("SendTransactionReturnCode", mock.Anything, mock.MatchedBy(func(tx *gethTypes.Transaction) bool {
return tx.Nonce() == inProgressTxNonce
}), fromAddress).Return(commonclient.Successful, nil).Once()

// Tx with nonce 0 in DB will set local nonce map to value to 1
mustInsertInProgressEthTxWithAttempt(t, txStore, evmtypes.Nonce(inProgressTxNonce), fromAddress)
nonceTracker := txmgr.NewNonceTracker(lggr, txStore, txmgr.NewEvmTxmClient(ethClient))
eb := NewTestEthBroadcaster(t, txStore, ethClient, ethKeyStore, evmcfg, checkerFactory, false, nonceTracker)

// Check the local nonce map was set to 1 higher than in-progress tx nonce
nonce := getLocalNextNonce(t, nonceTracker, fromAddress)
require.Equal(t, inProgressTxNonce + 1, nonce)

_, err := eb.ProcessUnstartedTxs(ctx, fromAddress)
require.NoError(t, err)

// Check the local nonce map maintained nonce 1 higher than in-progress tx nonce
nonce = getLocalNextNonce(t, nonceTracker, fromAddress)
require.Equal(t, inProgressTxNonce + 1, nonce)
})
}

type testCheckerFactory struct {
err error
}
Expand Down
16 changes: 14 additions & 2 deletions core/chains/evm/txmgr/nonce_tracker.go
Original file line number Diff line number Diff line change
Expand Up @@ -167,8 +167,20 @@ func (s *nonceTracker) GetNextSequence(ctx context.Context, address common.Addre
return foundSeq, nil
}

func (s *nonceTracker) GenerateNextSequence(address common.Address) {
func (s *nonceTracker) GenerateNextSequence(address common.Address, nonceUsed evmtypes.Nonce) {
s.sequenceLock.Lock()
defer s.sequenceLock.Unlock()
s.nextSequenceMap[address]++
currentNonce := s.nextSequenceMap[address]

// In most cases, currentNonce would equal nonceUsed
// There is a chance currentNonce is 1 ahead of nonceUsed if the DB contains an in-progress tx during startup
// Incrementing currentNonce, which is already set to the next usable nonce, could lead to a nonce gap. Set the map to the incremented nonceUsed instead.
if currentNonce == nonceUsed || currentNonce == nonceUsed + 1 {
s.nextSequenceMap[address] = nonceUsed + 1
return
}

// If currentNonce is ahead of even the incremented nonceUsed, maintain the unchanged currentNonce in the map
// This scenario should never occur but logging this discrepancy for visibility
s.lggr.Warnf("Local nonce map value %d for address %s is ahead of the nonce transmitted %d. Maintaining the existing value in the map without incrementing.", currentNonce, address.String(), nonceUsed)
}
2 changes: 1 addition & 1 deletion core/chains/evm/txmgr/nonce_tracker_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,7 @@ func TestNonceTracker_GenerateNextSequence(t *testing.T) {
require.NoError(t, err)
require.Equal(t, types.Nonce(randNonce+1), seq) // Local nonce should be highest nonce in DB + 1

nonceTracker.GenerateNextSequence(addr)
nonceTracker.GenerateNextSequence(addr, types.Nonce(randNonce+1))

seq, err = nonceTracker.GetNextSequence(ctx, addr)
require.NoError(t, err)
Expand Down

0 comments on commit a9d75e6

Please sign in to comment.