Add nonce validation after broadcast for Hedera #13957

amit-momin · 2024-07-29T23:15:59Z

Added a validation to confirm the on-chain nonce incremented immediately after a transaction was broadcasted for Hedera. This check is only applicable to Hedera because it has instant finality and a scenario where a transaction can be included on chain with a receipt but fail to increment the nonce.
Added a new RevertReason field was added to the Receipt struct to track this additional message provided by Hedera.

…ailed-transactions

dimriou · 2024-07-30T11:53:04Z

common/txmgr/broadcaster.go

+		return errType, err
+	}
+	// Check that the transaction count has incremented on-chain to include the broadcasted transaction
+	// Insufficient transaction fee is a common scenario in which the sequence is not incremented by the chain


Let's be even more explicit about this

Suggested change

// Insufficient transaction fee is a common scenario in which the sequence is not incremented by the chain

// Insufficient transaction fee is a common scenario in which the sequence is not incremented by the chain even though we got a successful response

dhaidashenko · 2024-07-30T16:29:50Z

common/txmgr/broadcaster.go

+	// Check that the transaction count has incremented on-chain to include the broadcasted transaction
+	// Insufficient transaction fee is a common scenario in which the sequence is not incremented by the chain
+	// If the sequence failed to increment and hasn't reached the max retries, return the Underpriced error to try again with a bumped attempt
+	if nextSeqOnChain.Int64() == txSeq.Int64() && retryCount < maxHederaBroadcastRetries {


nit: in theory if nextSeqOnChain is < txSeq we can mark tx as Successful

nextSeqOnChain < txSeq I think would be an indicator of a nonce gap which wouldn't return a successful errType so we would have exited early. But for a belts and braces approach, I added the condition just in case and mark the tx as Fatal in this case.

Maybe @dhaidashenko you meant nextSeqOnChain > txSeq?

I agree that nextSeqOnChain < txSeq would be incorrect and caught by SendTransactionReturnCode early, and probably due to a reorg in this case ? otherwise not sure about how this is possible.

Ah if that's the case then classifying that as Successful is ok because we process TransactionAlreadyKnown as the same.
Also unsure if nextSeqOnChain < txSeq is a scenario we could realistically hit for this chain. I believe re-orgs aren't possible because of instant finality. But there could be some obscure scenario so this check should help.

Retryable is also another option for the nextSeqOnChain < txSeq case. There are two scenarios:

We have a nonce gap: fatal will alert us sooner, but we might miss reorg cases and it won't recover anyway

There was a reorg or RPC was lagging: retryable will handle this, although I found out that Hedera doesn't handle nonce too high errors gracefully, so it will throw a bunch of errors during this process.

…omment

huangzhen1997

lgtm except one small nit, but due to the limited knowledge/context I have now, I will just comment instead of approve :)

huangzhen1997 · 2024-07-30T20:58:30Z

common/txmgr/broadcaster.go

@@ -600,6 +615,44 @@ func (eb *Broadcaster[CHAIN_ID, HEAD, ADDR, TX_HASH, BLOCK_HASH, SEQ, FEE]) hand
 	}
 }

+func (eb *Broadcaster[CHAIN_ID, HEAD, ADDR, TX_HASH, BLOCK_HASH, SEQ, FEE]) validateOnChainSequence(ctx context.Context, lgr logger.SugaredLogger, errType client.SendTxReturnCode, err error, etx txmgrtypes.Tx[CHAIN_ID, ADDR, TX_HASH, BLOCK_HASH, SEQ, FEE], retryCount int) (client.SendTxReturnCode, error) {


shall we rename this function to imply it's only meant for Hedara, or it's better to be generic ?

I think it's alright either way. I considered that actually but opted to leave it implicit. I'm not against changing it though if others agree

huangzhen1997 · 2024-07-30T21:27:53Z

common/txmgr/broadcaster.go

+	// Check that the transaction count has incremented on-chain to include the broadcasted transaction
+	// Insufficient transaction fee is a common scenario in which the sequence is not incremented by the chain
+	// If the sequence failed to increment and hasn't reached the max retries, return the Underpriced error to try again with a bumped attempt
+	if nextSeqOnChain.Int64() == txSeq.Int64() && retryCount < maxHederaBroadcastRetries {


Maybe @dhaidashenko you meant nextSeqOnChain > txSeq?

I agree that nextSeqOnChain < txSeq would be incorrect and caught by SendTransactionReturnCode early, and probably due to a reorg in this case ? otherwise not sure about how this is possible.

…ailed-transactions

cl-sonarqube-production · 2024-07-31T19:10:38Z

Quality Gate passed

Issues
3 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
76.6% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube

prashantkumar1982 · 2024-08-01T15:37:34Z

common/txmgr/confirmer.go

-				l.Warnw("transaction reverted on-chain unable to extract revert reason", "hash", receipt.GetTxHash(), "err", err)
+				rpcError, errExtract := ec.client.CallContract(ctx, attempt, receipt.GetBlockNumber())
+				if errExtract == nil {
+					l.Warnw("transaction reverted on-chain", "hash", receipt.GetTxHash(), "rpcError", rpcError.String())


nit: It might make sense to set this reason back on the receipt struct right?

I think it makes sense to leave this as a log instead of adding it to the Receipt struct. Since the TXM is indifferent to a tx reverting, I don't think we should try to store this to avoid adding another column.

## Motivation Cherry-pick of smartcontractkit/chainlink#14021 cherry-pick of smartcontractkit/chainlink#13957 cherry-pick of smartcontractkit/chainlink#13876 cherry-pick of smartcontractkit/chainlink#13907

* Added post-broadcast nonce validation for Hedera * Added changeset * Updated chaintype docs for Hedera * Fixed lint errors * Added condition to handle on-chain seq less than tx seq and updated comment

amit-momin added 2 commits July 29, 2024 18:05

Added post-broadcast nonce validation for Hedera

4625cfa

Added changeset

21b4a91

amit-momin temporarily deployed to sdlc July 29, 2024 23:16 — with GitHub Actions Inactive

Updated chaintype docs for Hedera

f0ce40f

amit-momin temporarily deployed to sdlc July 29, 2024 23:29 — with GitHub Actions Inactive

Fixed lint errors

5f0f849

amit-momin temporarily deployed to sdlc July 29, 2024 23:53 — with GitHub Actions Inactive

amit-momin marked this pull request as ready for review July 30, 2024 00:17

amit-momin requested review from a team as code owners July 30, 2024 00:17

Merge branch 'develop' into BCI-3843-Use-on-chain-nonce-to-identify-f…

d13cb3d

…ailed-transactions

amit-momin temporarily deployed to sdlc July 30, 2024 00:52 — with GitHub Actions Inactive

dimriou reviewed Jul 30, 2024

View reviewed changes

dhaidashenko reviewed Jul 30, 2024

View reviewed changes

Added condition to handle on-chain seq less than tx seq and updated c…

e7d0678

…omment

amit-momin temporarily deployed to sdlc July 30, 2024 17:14 — with GitHub Actions Inactive

dhaidashenko previously approved these changes Jul 30, 2024

View reviewed changes

huangzhen1997 reviewed Jul 30, 2024

View reviewed changes

dimriou previously approved these changes Jul 31, 2024

View reviewed changes

Merge branch 'develop' into BCI-3843-Use-on-chain-nonce-to-identify-f…

8dd90fc

…ailed-transactions

amit-momin dismissed stale reviews from dimriou and dhaidashenko via 8dd90fc July 31, 2024 18:56

amit-momin temporarily deployed to sdlc July 31, 2024 18:56 — with GitHub Actions Inactive

dimriou approved these changes Aug 1, 2024

View reviewed changes

dhaidashenko approved these changes Aug 1, 2024

View reviewed changes

dimriou added this pull request to the merge queue Aug 1, 2024

Merged via the queue into develop with commit 20dbba8 Aug 1, 2024
121 checks passed

dimriou deleted the BCI-3843-Use-on-chain-nonce-to-identify-failed-transactions branch August 1, 2024 14:01

github-actions bot mentioned this pull request Aug 1, 2024

[DO NOT MERGE] Changeset Release Preview - v2.19.0 #13148

Draft

prashantkumar1982 reviewed Aug 1, 2024

View reviewed changes

This was referenced Aug 18, 2024

[DO NOT MERGE] Changeset Release Preview - v2.18.0 picoinnetwork/chainlink#1

Draft

[DO NOT MERGE] Changeset Release Preview - v2.16.0 chaingence/chainlink#1

Draft

simsonraj mentioned this pull request Aug 19, 2024

Astar & Hedera changes cherry-picked smartcontractkit/ccip#1324

Merged

This was referenced Aug 21, 2024

[DO NOT MERGE] Changeset Release Preview - v2.17.0 YoutacRandS-VA/chainlink#23

Draft

[DO NOT MERGE] Changeset Release Preview - v2.19.0 EthanAnro/chainlink#2

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add nonce validation after broadcast for Hedera #13957

Add nonce validation after broadcast for Hedera #13957

amit-momin commented Jul 29, 2024 •

edited by jira bot

Loading

dimriou Jul 30, 2024

dhaidashenko Jul 30, 2024

amit-momin Jul 30, 2024

huangzhen1997 Jul 30, 2024 •

edited

Loading

amit-momin Jul 30, 2024

dimriou Jul 31, 2024 •

edited

Loading

huangzhen1997 left a comment

huangzhen1997 Jul 30, 2024

amit-momin Jul 30, 2024

huangzhen1997 Jul 30, 2024 •

edited

Loading

cl-sonarqube-production bot commented Jul 31, 2024

prashantkumar1982 Aug 1, 2024

amit-momin Aug 1, 2024

	// Insufficient transaction fee is a common scenario in which the sequence is not incremented by the chain
	// Insufficient transaction fee is a common scenario in which the sequence is not incremented by the chain even though we got a successful response

Add nonce validation after broadcast for Hedera #13957

Add nonce validation after broadcast for Hedera #13957

Conversation

amit-momin commented Jul 29, 2024 • edited by jira bot Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huangzhen1997 Jul 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimriou Jul 31, 2024 • edited Loading

Choose a reason for hiding this comment

huangzhen1997 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huangzhen1997 Jul 30, 2024 • edited Loading

Choose a reason for hiding this comment

cl-sonarqube-production bot commented Jul 31, 2024

Quality Gate passed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amit-momin commented Jul 29, 2024 •

edited by jira bot

Loading

huangzhen1997 Jul 30, 2024 •

edited

Loading

dimriou Jul 31, 2024 •

edited

Loading

huangzhen1997 Jul 30, 2024 •

edited

Loading