Handle terminally stuck transactions on send #14127
Conversation
common/txmgr/confirmer.go
Outdated
| @@ -999,6 +999,21 @@ func (ec *Confirmer[CHAIN_ID, HEAD, ADDR, TX_HASH, BLOCK_HASH, R, SEQ, FEE]) han | |||
| ec.SvcErrBuffer.Append(sendError) | |||
| // This will loop continuously on every new head so it must be handled manually by the node operator! | |||
| return ec.txStore.DeleteInProgressAttempt(ctx, attempt) | |||
| case client.TerminallyStuck: | |||
| // A transaction could broadcast successfully but then be considered terminally stuck on another attempt | |||
| // Even though the transaction can succeeed under different circumstances, we want to purge this transaction as soon as we get this error | |||
There was a problem hiding this comment.
| // Even though the transaction can succeeed under different circumstances, we want to purge this transaction as soon as we get this error | |
| // Even though the transaction can succeed under different circumstances, we want to purge this transaction as soon as we get this error |
common/txmgr/confirmer.go
Outdated
| case client.TerminallyStuck: | ||
| // A transaction could broadcast successfully but then be considered terminally stuck on another attempt | ||
| // Even though the transaction can succeeed under different circumstances, we want to purge this transaction as soon as we get this error | ||
| lggr.Errorw("terminally stuck transaction detected", "err", sendError.Error()) |
There was a problem hiding this comment.
whats the criteria for a Error log vs a Critical log?
There was a problem hiding this comment.
from my understanding this (tx stuck due to overflow, not enough keccak counters to continue the execution) is expected behavior, as least for zkSync. When it happens we just need to cancel/purge the existing tx by reprocessing, and it's not critical/fatal issue related to chainlink node that we need to raise alert, critial/fatal one for example: Invariant violation: fatal error while re-attempting transaction should not happen
There was a problem hiding this comment.
That's my understanding as well. Since the TXM resolves this on its own, we don't have to raise a signal for NOPs to take any actions.
There was a problem hiding this comment.
Agree. I would say, change log level also to a warn.
The Tx is bad, and is nothing wrong with the TXM.
| return tx.Nonce() == uint64(346) && tx.Value().Cmp(big.NewInt(243)) == 0 | ||
| }), fromAddress).Return(commonclient.Fatal, errors.New(terminallyStuckError)).Once() | ||
|
|
||
| // Do the thing |
There was a problem hiding this comment.
nit: replace with more descriptive comment
There was a problem hiding this comment.
lol I notice we have a lot Do the thing lines in many test files, confirmer_test.go for example
There was a problem hiding this comment.
Ya this was just copied from another broadcaster test haha. But never too late to update at least the new tests to say something better.
amit-momin
dismissed stale reviews from poopoothegorilla and huangzhen1997
via
321c21a
core/chains/evm/client/errors.go
Outdated
| @@ -596,6 +596,11 @@ func ClassifySendError(err error, clientErrors config.ClientErrors, lggr logger. | |||
| ) | |||
| return commonclient.ExceedsMaxFee | |||
| } | |||
| if sendError.IsTerminallyStuckConfigError(configErrors) { | |||
| lggr.Criticalw("Transaction that would have been terminally stuck in the mempool detected on send. Marking as fatal error.", "err", sendError, "etx", tx) | |||
There was a problem hiding this comment.
Shouldn't be critical log. Should be a warning.
Even the Errorw() log is for cases where we clearly see a failure, although ones that we can recover from.
From example, an important RPC failed, or database writing failed.
A stuck tx is now an expected behavior, so warn log is enough.
There was a problem hiding this comment.
Will do! I was just trying to match the behavior for Fatal. Think we're planning to rework logs in the near future so that might change anyways
common/txmgr/confirmer.go
Outdated
| case client.TerminallyStuck: | ||
| // A transaction could broadcast successfully but then be considered terminally stuck on another attempt | ||
| // Even though the transaction can succeeed under different circumstances, we want to purge this transaction as soon as we get this error | ||
| lggr.Errorw("terminally stuck transaction detected", "err", sendError.Error()) |
There was a problem hiding this comment.
Agree. I would say, change log level also to a warn.
The Tx is bad, and is nothing wrong with the TXM.
|
prashantkumar1982
deleted the
BCI-3014-Handle-ZK-overflow-error-on-send-transaction
branch
Cherry-pick of smartcontractkit/chainlink#14127 Required to successfully handle zk overflows on Polygon zkEVM and X Layer. Co-authored-by: amit-momin <[email protected]> Co-authored-by: Rens Rooimans <[email protected]>
BCI-3014