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

Update abandon tx functionality to drop related attempts #15616

Merged
merged 2 commits into from
Jan 10, 2025
+65 −5
Merged

Update abandon tx functionality to drop related attempts #15616

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dec2671
Updated abandon tx to drop related attemtps
amit-momin Dec 10, 2024
95e7537
Added changeset
amit-momin Dec 10, 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
5 changes: 5 additions & 0 deletions .changeset/gorgeous-ants-promise.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"chainlink": patch
---

Updated TXM abandon transaction functionality to drop related attempts. #updated
15 changes: 12 additions & 3 deletions core/chains/evm/txmgr/evm_tx_store.go
View file
Open in desktop
Copy link
Collaborator

@dimriou dimriou Dec 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think a bigger underlying design problem exists with how we treat purgeable attempts. Here's what will happen if we have multiple in-flight transactions, including a purgeable attempt. Purgeable attempts are classified as fatal but still treated as in-flight. If we have two transactions with nonce 10 and 11 (the first one is unconfirmed, the second one is purgeable) if we hit Abandon then only the first one will be marked as fatal and the second one won't be impacted. When the TXM restarts, it will try to sync the nonce, look at the DB and see the nonce of the purgeable attempt. Assuming the tx with nonce 10 never made it on-chain this will effectively create a nonce-gap.

Original file line number Diff line number Diff line change
Expand Up @@ -1786,8 +1786,17 @@ func (o *evmTxStore) Abandon(ctx context.Context, chainID *big.Int, addr common.
var cancel context.CancelFunc
ctx, cancel = o.stopCh.Ctx(ctx)
defer cancel()
_, err := o.q.ExecContext(ctx, `UPDATE evm.txes SET state='fatal_error', nonce = NULL, error = 'abandoned' WHERE state IN ('unconfirmed', 'in_progress', 'unstarted') AND evm_chain_id = $1 AND from_address = $2`, chainID.String(), addr)
return err
return o.Transact(ctx, false, func(orm *evmTxStore) error {
var abandonedIDs []string
err := orm.q.SelectContext(ctx, &abandonedIDs, `UPDATE evm.txes SET state='fatal_error', nonce = NULL, error = 'abandoned' WHERE state IN ('unconfirmed', 'in_progress', 'unstarted') AND evm_chain_id = $1 AND from_address = $2 RETURNING id`, chainID.String(), addr)
if err != nil {
return fmt.Errorf("failed to mark transactions as abandoned: %w", err)
}
if _, err := orm.q.ExecContext(ctx, `DELETE FROM evm.tx_attempts WHERE eth_tx_id = ANY($1)`, pq.Array(abandonedIDs)); err != nil {
return fmt.Errorf("failed to delete attempts related to abandoned transactions: %w", err)
}
return nil
})
}

// Find transactions by a field in the TxMeta blob and transaction states
Expand Down Expand Up @@ -1916,7 +1925,7 @@ func (o *evmTxStore) FindAttemptsRequiringReceiptFetch(ctx context.Context, chai
query := `
SELECT evm.tx_attempts.* FROM evm.tx_attempts
JOIN evm.txes ON evm.txes.ID = evm.tx_attempts.eth_tx_id
WHERE evm.tx_attempts.state = 'broadcast' AND evm.txes.state IN ('confirmed', 'confirmed_missing_receipt', 'fatal_error') AND evm.txes.evm_chain_id = $1 AND evm.txes.ID NOT IN (
WHERE evm.tx_attempts.state = 'broadcast' AND evm.txes.nonce IS NOT NULL AND evm.txes.state IN ('confirmed', 'confirmed_missing_receipt', 'fatal_error') AND evm.txes.evm_chain_id = $1 AND evm.txes.ID NOT IN (
SELECT DISTINCT evm.txes.ID FROM evm.txes
JOIN evm.tx_attempts ON evm.tx_attempts.eth_tx_id = evm.txes.ID
JOIN evm.receipts ON evm.receipts.tx_hash = evm.tx_attempts.hash
Expand Down
50 changes: 48 additions & 2 deletions core/chains/evm/txmgr/evm_tx_store_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1796,11 +1796,16 @@ func TestORM_FindAttemptsRequiringReceiptFetch(t *testing.T) {
// Terminally stuck transaction with receipt should NOT be picked up for receipt fetch
stuckTx := mustInsertTerminallyStuckTxWithAttempt(t, txStore, fromAddress, 1, blockNum)
mustInsertEthReceipt(t, txStore, blockNum, utils.NewHash(), stuckTx.TxAttempts[0].Hash)
// Fatal transactions with nil nonce and stored attempts should NOT be picked up for receipt fetch
fatalTxWithAttempt := mustInsertFatalErrorEthTx(t, txStore, fromAddress)
attempt := newBroadcastLegacyEthTxAttempt(t, fatalTxWithAttempt.ID)
err := txStore.InsertTxAttempt(ctx, &attempt)
require.NoError(t, err)

// Confirmed transaction without receipt should be picked up for receipt fetch
confirmedTx := mustInsertConfirmedEthTx(t, txStore, 0, fromAddress)
attempt := newBroadcastLegacyEthTxAttempt(t, confirmedTx.ID)
err := txStore.InsertTxAttempt(ctx, &attempt)
attempt = newBroadcastLegacyEthTxAttempt(t, confirmedTx.ID)
err = txStore.InsertTxAttempt(ctx, &attempt)
require.NoError(t, err)

attempts, err := txStore.FindAttemptsRequiringReceiptFetch(ctx, testutils.FixtureChainID)
Expand All @@ -1823,6 +1828,12 @@ func TestORM_FindAttemptsRequiringReceiptFetch(t *testing.T) {
// Terminally stuck transaction with receipt should NOT be picked up for receipt fetch
stuckTxWithReceipt := mustInsertTerminallyStuckTxWithAttempt(t, txStore, fromAddress, 1, blockNum)
mustInsertEthReceipt(t, txStore, blockNum, utils.NewHash(), stuckTxWithReceipt.TxAttempts[0].Hash)
// Fatal transactions with nil nonce and stored attempts should NOT be picked up for receipt fetch
fatalTxWithAttempt := mustInsertFatalErrorEthTx(t, txStore, fromAddress)
attempt := newBroadcastLegacyEthTxAttempt(t, fatalTxWithAttempt.ID)
err := txStore.InsertTxAttempt(ctx, &attempt)
require.NoError(t, err)

// Terminally stuck transaction without receipt should be picked up for receipt fetch
stuckTxWoutReceipt := mustInsertTerminallyStuckTxWithAttempt(t, txStore, fromAddress, 0, blockNum)

Expand Down Expand Up @@ -2008,6 +2019,41 @@ func TestORM_DeleteReceiptsByTxHash(t *testing.T) {
require.Len(t, etx2.TxAttempts[0].Receipts, 1)
}

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

db := pgtest.NewSqlxDB(t)
txStore := cltest.NewTestTxStore(t, db)
ctx := tests.Context(t)
ethKeyStore := cltest.NewKeyStore(t, db).Eth()
_, fromAddress := cltest.MustInsertRandomKeyReturningState(t, ethKeyStore)
etx1 := mustCreateUnstartedGeneratedTx(t, txStore, fromAddress, testutils.FixtureChainID)
etx2 := mustInsertInProgressEthTxWithAttempt(t, txStore, 1, fromAddress)
etx3 := mustInsertUnconfirmedEthTxWithAttemptState(t, txStore, 0, fromAddress, txmgrtypes.TxAttemptBroadcast)

err := txStore.Abandon(ctx, testutils.FixtureChainID, fromAddress)
require.NoError(t, err)

// transactions marked as fatal error with abandon reason, nil sequence, and no attempts
etx1, err = txStore.FindTxWithAttempts(ctx, etx1.ID)
require.NoError(t, err)
require.Equal(t, txmgrcommon.TxFatalError, etx1.State)
require.Nil(t, etx1.Sequence)
require.Empty(t, etx1.TxAttempts)

etx2, err = txStore.FindTxWithAttempts(ctx, etx2.ID)
require.NoError(t, err)
require.Equal(t, txmgrcommon.TxFatalError, etx2.State)
require.Nil(t, etx2.Sequence)
require.Empty(t, etx2.TxAttempts)

etx3, err = txStore.FindTxWithAttempts(ctx, etx3.ID)
require.NoError(t, err)
require.Equal(t, txmgrcommon.TxFatalError, etx3.State)
require.Nil(t, etx3.Sequence)
require.Empty(t, etx3.TxAttempts)
}

func mustInsertTerminallyStuckTxWithAttempt(t *testing.T, txStore txmgr.TestEvmTxStore, fromAddress common.Address, nonceInt int64, broadcastBeforeBlockNum int64) txmgr.Tx {
ctx := tests.Context(t)
broadcast := time.Now()
Expand Down
Loading