Skip to content

Commit 7d4da8a

Browse files
dimrioudimriou
authored
Upgrade error handler (#408)
* Upgrade error handler * Address feedback
1 parent f6d1995 commit 7d4da8a

File tree

4 files changed

+57
-27
lines changed

4 files changed

+57
-27
lines changed

‎pkg/txm/clientwrappers/dualbroadcast/meta_error_handler.go‎

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,10 @@ package dualbroadcast
33
import (
44
"context"
55
"errors"
6-
"fmt"
76

87
"github.com/ethereum/go-ethereum/common"
98

9+
"github.com/smartcontractkit/chainlink-common/pkg/logger"
1010
"github.com/smartcontractkit/chainlink-evm/pkg/txm"
1111
"github.com/smartcontractkit/chainlink-evm/pkg/txm/types"
1212
)
@@ -17,15 +17,25 @@ func NewErrorHandler() *errorHandler {
1717
return &errorHandler{}
1818
}
1919

20-
func (e *errorHandler) HandleError(ctx context.Context, tx *types.Transaction, txErr error, txStore txm.TxStore, setNonce func(common.Address, uint64), isFromBroadcastMethod bool) error {
20+
func (e *errorHandler) HandleError(
21+
ctx context.Context,
22+
lggr logger.Logger,
23+
tx *types.Transaction,
24+
txErr error,
25+
txStore txm.TxStore,
26+
setNonce func(common.Address, uint64),
27+
) (noTransmission bool, err error) {
2128
// Mark the tx as fatal only if this is the first broadcast. In any other case, other txs might be included on-chain.
2229
if (errors.Is(txErr, ErrNoBids) || errors.Is(txErr, ErrAuction)) && tx.AttemptCount == 1 {
2330
if err := txStore.MarkTxFatal(ctx, tx, tx.FromAddress); err != nil {
24-
return err
31+
return false, err
2532
}
2633
setNonce(tx.FromAddress, *tx.Nonce)
27-
return fmt.Errorf("transaction with txID: %d marked as fatal", tx.ID)
34+
l := logger.Sugared(logger.Named(lggr, "Txm.MetaErrorHandler"))
35+
l.Infow("transaction marked as fatal", "txID", tx.ID, "transactionLifecycleID", tx.GetTransactionLifecycleID(l))
36+
return true, nil
2837
}
2938

30-
return txErr
39+
// If the error message is not recognized, we don't know if we didn't transmit so return false and continue with the standard execution path.
40+
return false, nil
3141
}

‎pkg/txm/clientwrappers/dualbroadcast/meta_error_handler_test.go‎

Lines changed: 17 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,10 @@ import (
77
"github.com/ethereum/go-ethereum/common"
88
"github.com/stretchr/testify/assert"
99
"github.com/stretchr/testify/require"
10+
"go.uber.org/zap"
1011

1112
"github.com/smartcontractkit/chainlink-common/pkg/logger"
13+
"github.com/smartcontractkit/chainlink-common/pkg/utils/tests"
1214
"github.com/smartcontractkit/chainlink-evm/pkg/assets"
1315
"github.com/smartcontractkit/chainlink-evm/pkg/gas"
1416
"github.com/smartcontractkit/chainlink-evm/pkg/testutils"
@@ -21,6 +23,7 @@ func TestMetaErrorHandler(t *testing.T) {
2123
require.NotNil(t, errorHandler)
2224

2325
t.Run("handles no bids error for first attempt", func(t *testing.T) {
26+
lggr, observedLogs := logger.TestObserved(t, zap.InfoLevel)
2427
nonce := uint64(1)
2528
address := testutils.NewAddress()
2629
txRequest := &types.TxRequest{
@@ -29,7 +32,7 @@ func TestMetaErrorHandler(t *testing.T) {
2932
ToAddress: testutils.NewAddress(),
3033
}
3134
setNonce := func(address common.Address, nonce uint64) {}
32-
txStoreManager := storage.NewInMemoryStoreManager(logger.Test(t), testutils.FixtureChainID)
35+
txStoreManager := storage.NewInMemoryStoreManager(lggr, testutils.FixtureChainID)
3336
require.NoError(t, txStoreManager.Add(address))
3437
txStore := txStoreManager.InMemoryStoreMap[address]
3538
_ = txStore.CreateTransaction(txRequest)
@@ -44,9 +47,10 @@ func TestMetaErrorHandler(t *testing.T) {
4447
_, err = txStore.AppendAttemptToTransaction(*tx.Nonce, attempt)
4548
require.NoError(t, err)
4649
tx, _ = txStore.FetchUnconfirmedTransactionAtNonceWithCount(nonce)
47-
err = errorHandler.HandleError(t.Context(), tx, ErrNoBids, txStoreManager, setNonce, false)
48-
require.Error(t, err)
49-
require.ErrorContains(t, err, "transaction with txID: 0 marked as fatal")
50+
noTransmission, err := errorHandler.HandleError(t.Context(), lggr, tx, ErrNoBids, txStoreManager, setNonce)
51+
require.NoError(t, err)
52+
require.True(t, noTransmission)
53+
tests.AssertLogEventually(t, observedLogs, "transaction marked as fatal")
5054
_, unconfirmedCount := txStore.FetchUnconfirmedTransactionAtNonceWithCount(nonce)
5155
assert.Equal(t, 0, unconfirmedCount)
5256
})
@@ -78,14 +82,15 @@ func TestMetaErrorHandler(t *testing.T) {
7882
_, err = txStore.AppendAttemptToTransaction(*tx.Nonce, attempt)
7983
require.NoError(t, err)
8084
tx, _ = txStore.FetchUnconfirmedTransactionAtNonceWithCount(nonce)
81-
err = errorHandler.HandleError(t.Context(), tx, txErr, txStoreManager, setNonce, false)
82-
require.Error(t, err)
83-
require.ErrorIs(t, err, txErr)
85+
noTransmission, err := errorHandler.HandleError(t.Context(), logger.Test(t), tx, txErr, txStoreManager, setNonce)
86+
require.NoError(t, err)
87+
require.False(t, noTransmission)
8488
_, unconfirmedCount := txStore.FetchUnconfirmedTransactionAtNonceWithCount(nonce)
8589
assert.Equal(t, 1, unconfirmedCount)
8690
})
8791

8892
t.Run("handles auction error for first attempt", func(t *testing.T) {
93+
lggr, observedLogs := logger.TestObserved(t, zap.InfoLevel)
8994
nonce := uint64(1)
9095
address := testutils.NewAddress()
9196
txRequest := &types.TxRequest{
@@ -95,7 +100,7 @@ func TestMetaErrorHandler(t *testing.T) {
95100
}
96101
txErr := ErrAuction
97102
setNonce := func(address common.Address, nonce uint64) {}
98-
txStoreManager := storage.NewInMemoryStoreManager(logger.Test(t), testutils.FixtureChainID)
103+
txStoreManager := storage.NewInMemoryStoreManager(lggr, testutils.FixtureChainID)
99104
require.NoError(t, txStoreManager.Add(address))
100105
txStore := txStoreManager.InMemoryStoreMap[address]
101106
_ = txStore.CreateTransaction(txRequest)
@@ -110,9 +115,10 @@ func TestMetaErrorHandler(t *testing.T) {
110115
_, err = txStore.AppendAttemptToTransaction(*tx.Nonce, attempt)
111116
require.NoError(t, err)
112117
tx, _ = txStore.FetchUnconfirmedTransactionAtNonceWithCount(nonce)
113-
err = errorHandler.HandleError(t.Context(), tx, txErr, txStoreManager, setNonce, false)
114-
require.Error(t, err)
115-
require.ErrorContains(t, err, "transaction with txID: 0 marked as fatal")
118+
noTransmission, err := errorHandler.HandleError(t.Context(), lggr, tx, txErr, txStoreManager, setNonce)
119+
require.NoError(t, err)
120+
require.True(t, noTransmission)
121+
tests.AssertLogEventually(t, observedLogs, "transaction marked as fatal")
116122
_, unconfirmedCount := txStore.FetchUnconfirmedTransactionAtNonceWithCount(nonce)
117123
assert.Equal(t, 0, unconfirmedCount)
118124
})

‎pkg/txm/txm.go‎

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,9 @@ type AttemptBuilder interface {
5757
}
5858

5959
type ErrorHandler interface {
60-
HandleError(context.Context, *types.Transaction, error, TxStore, func(common.Address, uint64), bool) (err error)
60+
// HandleError tries to decide if there was a successful transmission by parsing the error message. If it can't decide, it returns control to
61+
// the standard execution path.
62+
HandleError(context.Context, logger.Logger, *types.Transaction, error, TxStore, func(common.Address, uint64)) (noTransmission bool, err error)
6163
}
6264

6365
type StuckTxDetector interface {
@@ -337,11 +339,17 @@ func (t *Txm) sendTransactionWithError(ctx context.Context, tx *types.Transactio
337339
start := time.Now()
338340
txErr := t.client.SendTransaction(ctx, tx, attempt)
339341
t.lggr.Infow("Broadcasted attempt", "tx", tx, "attempt", attempt, "transactionLifecycleID", tx.GetTransactionLifecycleID(t.lggr), "duration", time.Since(start), "txErr", txErr)
340-
if txErr != nil && t.errorHandler != nil {
341-
if err = t.errorHandler.HandleError(ctx, tx, txErr, t.txStore, t.SetNonce, false); err != nil {
342-
return
342+
if txErr != nil {
343+
// If ErrorHandler is set, try to decide if there was a successful transmission by parsing the error message. If there wasn't, we return early.
344+
if t.errorHandler != nil {
345+
noTransmission, hErr := t.errorHandler.HandleError(ctx, t.lggr, tx, txErr, t.txStore, t.SetNonce)
346+
if hErr != nil {
347+
return hErr
348+
}
349+
if noTransmission {
350+
return nil
351+
}
343352
}
344-
} else if txErr != nil {
345353
pendingNonce, pErr := t.client.PendingNonceAt(ctx, fromAddress)
346354
if pErr != nil {
347355
return pErr

‎pkg/txm/txm_test.go‎

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -512,16 +512,17 @@ func TestFlow_ErrorHandler(t *testing.T) {
512512
t.Parallel()
513513

514514
client := txm.NewMockClient(t)
515-
txStoreManager := storage.NewInMemoryStoreManager(logger.Test(t), testutils.FixtureChainID)
515+
lggr, observedLogs := logger.TestObserved(t, zap.InfoLevel)
516+
txStoreManager := storage.NewInMemoryStoreManager(lggr, testutils.FixtureChainID)
516517
address := testutils.NewAddress()
517518
require.NoError(t, txStoreManager.Add(address))
518519
config := txm.Config{EIP1559: true, EmptyTxLimitDefault: 22000, RetryBlockThreshold: 0, BlockTime: 2 * time.Second}
519520
mockEstimator := mocks.NewEvmFeeEstimator(t)
520521
keystore := &keystest.FakeChainStore{}
521522
attemptBuilder := txm.NewAttemptBuilder(func(address common.Address) *assets.Wei { return assets.NewWeiI(1) }, mockEstimator, keystore, 22000)
522-
stuckTxDetector := txm.NewStuckTxDetector(logger.Test(t), "", txm.StuckTxDetectorConfig{BlockTime: config.BlockTime, StuckTxBlockThreshold: uint32(config.RetryBlockThreshold + 1)})
523+
stuckTxDetector := txm.NewStuckTxDetector(lggr, "", txm.StuckTxDetectorConfig{BlockTime: config.BlockTime, StuckTxBlockThreshold: uint32(config.RetryBlockThreshold + 1)})
523524
errorHandler := dualbroadcast.NewErrorHandler()
524-
tm := txm.NewTxm(logger.Test(t), testutils.FixtureChainID, client, attemptBuilder, txStoreManager, stuckTxDetector, config, keystore, errorHandler)
525+
tm := txm.NewTxm(lggr, testutils.FixtureChainID, client, attemptBuilder, txStoreManager, stuckTxDetector, config, keystore, errorHandler)
525526
metrics, err := txm.NewTxmMetrics(testutils.FixtureChainID)
526527
require.NoError(t, err)
527528
tm.Metrics = metrics
@@ -544,8 +545,11 @@ func TestFlow_ErrorHandler(t *testing.T) {
544545
Return(gas.EvmFee{DynamicFee: gas.DynamicFee{GasTipCap: assets.NewWeiI(5), GasFeeCap: assets.NewWeiI(10)}}, defaultGasLimit, nil).Once()
545546
client.On("SendTransaction", mock.Anything, mock.Anything, mock.Anything).Return(dualbroadcast.ErrNoBids).Once()
546547
_, err = tm.BroadcastTransaction(t.Context(), address)
547-
require.Error(t, err)
548-
require.ErrorContains(t, err, "transaction with txID: 0 marked as fatal")
548+
require.NoError(t, err)
549+
tests.AssertLogEventually(t, observedLogs, "transaction marked as fatal")
550+
_, count, err := txStoreManager.FetchUnconfirmedTransactionAtNonceWithCount(t.Context(), 0, address)
551+
require.NoError(t, err)
552+
require.Equal(t, 0, count)
549553

550554
// Create transaction 2
551555
IDK2 := "IDK2"
@@ -575,9 +579,11 @@ func TestFlow_ErrorHandler(t *testing.T) {
575579
mockEstimator.On("BumpFee", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).
576580
Return(gas.EvmFee{DynamicFee: gas.DynamicFee{GasTipCap: assets.NewWeiI(6), GasFeeCap: assets.NewWeiI(12)}}, defaultGasLimit, nil).Once()
577581
client.On("SendTransaction", mock.Anything, mock.Anything, mock.Anything).Return(dualbroadcast.ErrNoBids).Once()
582+
client.On("PendingNonceAt", mock.Anything, address).Return(initialNonce, nil).Once()
578583
err = tm.BackfillTransactions(t.Context(), address) // retry
579584
require.Error(t, err)
580-
require.ErrorContains(t, err, dualbroadcast.ErrNoBids.Error())
585+
require.ErrorContains(t, err, "pending nonce for txID: 1 didn't increase")
586+
require.ErrorIs(t, err, dualbroadcast.ErrNoBids)
581587
tx, count, err = txStoreManager.FetchUnconfirmedTransactionAtNonceWithCount(t.Context(), 0, address) // same transaction is still in the store
582588
require.NoError(t, err)
583589
require.Equal(t, 1, count)

0 commit comments

Comments
 (0)