fix: Prevent Hedera same-nonce spam retries for successful txs - #108
fix: Prevent Hedera same-nonce spam retries for successful txs#108HelloKashif wants to merge 5 commits into
Conversation
After a successful send, validateOnChainSequence only checked the mined (latest) nonce. On Hedera that count can lag mempool acceptance by several seconds, which triggered gas-bump retries on the same nonce. Also consult PendingSequenceAt so a tx accepted into the mempool is treated as successful even before latest advances.
|
👋 HelloKashif, thanks for creating this pull request! To help reviewers, please consider creating future PRs as drafts first. This allows you to self-review and make any final changes before notifying the team. Once you're ready, you can mark it as "Ready for review" to request feedback. Thanks! |
✅ API Diff Results -
|
HelloKashif
changed the title
HelloKashif
changed the title
HelloKashif
changed the title
d9a5c57 to
c0f8853
Compare
| maxHederaBroadcastRetries = 3 | ||
|
|
||
| // hederaDefaultSequencePollInterval is the delay between Hedera SequenceAt re-polls after a successful send. | ||
| hederaDefaultSequencePollInterval = 10 * time.Second |
There was a problem hiding this comment.
Not PR feedback really but since you're the Hedera expert, is there any fear that legitimate Underpriced errors would slow down the TXM with these configs? In the worst case, 30s could be too much for a product like Data Feeds
There was a problem hiding this comment.
Yeah for this, I checked and unfortunately the way Hedera works it's difficult to detect the error. Hedera accepts even underpriced txs and then reverts on chain so there is no way to detect if a tx was due to nonce issue or underpricing until the tx gets included. And in my tests I found that even without a mempool (allegedly) the latest vs pending tags are lagging about 10 seconds. So even in the best case there will be a 10 second delay to confirm tx problem which is unavoidable for DF.
I also confirmed that this is not just a CCIP issue, even DF writes are seeing the same WRONG_NONCE reverts for eg here for Hedera feeds. So technically each DF call is already about 30sec delayed due to bumping retries
- https://hashscan.io/mainnet/contract/0.0.7887545/calls (3 WRONG_NONCE reverts for every one good tx)
- https://hashscan.io/mainnet/contract/0.0.7710674/calls
@amit-momin would you prefer if we can make this configurable via toml in chainlink-evm?
There was a problem hiding this comment.
I see so we need this delay for DF even. Although I assume, they may be more open to spending the extra gas for wrong_nonce reverts if that means they catch true underpriced issues quicker.
A separate concern I still have after looking at this code again is the 10s poll interval.If the nonce advances at the 11s, we would still wait till 20s to check. Since this is just a simple NonceAt call, we should increase how frequently we check for the nonce. Something like every second or 2 seconds. Then the cut off instead of number of retries (hederaDefaultSequencePollRetries) should be a overall timeout. Hope this doesn't complicate things even more.
Then I think we may want to make that overall timeout configurable and set the default to maybe 10s so we're still bumping prices as soon as possible in the happy path for DF if that latest v pending lag is usually 10s. Then CCIP can set it to something like 30s so yall are extra sure it's a true underpriced issue. What do you think?
There was a problem hiding this comment.
yeah agreed, its best to make this fully backwards compatible so no change for DF by default. and expose this as config.
Polling is disabled unless txConfig implements the new optional interface with a positive timeout, preserving legacy single-check behavior for existing nodes. When enabled, poll every 2s by default until the configured overall timeout elapses instead of using a fixed retry count.
There was a problem hiding this comment.
Pull request overview
This PR introduces a Hedera-specific post-broadcast nonce (sequence) polling mechanism to reduce false-positive “underpriced” retries when eth_getTransactionCount(..., "latest") lags behind RPC acceptance, while also adding configurability for polling behavior.
Changes:
- Added
HederaBroadcastConfiginterface to allow configuring Hedera sequence polling timeout/interval. - Updated Hedera post-send validation to use a polling helper (
pollSequenceAtAfterBroadcast) before falling back to the existing bumped-fee retry path. - Added unit tests covering the polling helper’s behavior (legacy single-check, success after advance, timeout, and context cancellation).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| chains/txmgr/types/config.go | Adds an optional config interface for Hedera sequence polling timeout/interval. |
| chains/txmgr/broadcaster.go | Implements Hedera post-broadcast SequenceAt polling and wiring into sequence validation. |
| chains/txmgr/broadcaster_hedera_test.go | Adds unit tests for the new polling helper function. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| select { | ||
| case <-ctx.Done(): | ||
| return nextSeqOnChain, ctx.Err() | ||
| case <-time.After(wait): | ||
| } |
| // hederaDefaultSequencePollInterval is the delay between Hedera SequenceAt re-polls when polling is enabled | ||
| // and no interval is configured. | ||
| hederaDefaultSequencePollInterval = 2 * time.Second |
| require.NoError(t, err) | ||
| assert.Equal(t, int64(86), got) | ||
| assert.Equal(t, int32(1), calls.Load()) | ||
| assert.Less(t, time.Since(start), pollInterval) |
| require.NoError(t, err) | ||
| assert.Equal(t, int64(85), got) | ||
| assert.GreaterOrEqual(t, calls.Load(), int32(2)) | ||
| assert.LessOrEqual(t, time.Since(start), pollTimeout+pollInterval) |
Problem
On Hedera, after a successful
eth_sendRawTransaction, TXM runsvalidateOnChainSequence. That reads the mined nonce viaSequenceAt(eth_getTransactionCountwithlatest). If the count is still equal to the tx nonce, TXM treats the send as underpriced and immediately gas-bumps with the same nonce — up to 3 times, no sleep between attempts.Where this happens in code:
handleInProgressTx→validateOnChainSequenceSequenceAtonce, immediately after sendUnderpricedOn Hedera, the mined nonce can lag several seconds behind RPC acceptance. The immediate check is too eager.
This matches what we see on Hedera mainnet: bursts of same-nonce OffRamp txs (WRONG_NONCE) a second or two apart.
What we tested
We sent a zero-value self-transfer and checked
latestandpendingimmediately after the RPC accepted the send.Testnet (tx nonce 227):
latest: 227 — unchangedpending: 228 — bumpedlatestcaught up to 228 after ~7.4sMainnet (tx nonce 85):
latest: 85 — unchangedpending: 86 — bumpedlatestcaught up to 86 after ~9.7sSame pattern on both: RPC accepts the tx right away, but
latestlags by several seconds.Fix
Before falling back to the gas-bump path, poll
SequenceAtwith backoff:latestadvances during polling → treat broadcast as successful (no resend)Underpriced/ gas-bump retry pathImplemented in
sequenceAtAfterBroadcastWithRetries.Note: why not
pending?We also tried accepting the broadcast when
eth_getTransactionCount(addr, "pending")advanced beforelatestdid. That fixed the false-positive retries in our repro, but we did not ship it.If we treat a bumped
pendingas success, we skip the gas-bump retry path entirely. In cases where the tx was not actually accepted — bothpendingandlateststay flat — we would never reachUnderpricedand the tx could sit inin_progressindefinitely instead of recovering via a bumped resend.Polling
latestwith backoff keeps the existing escape hatch: wait out normal Hedera lag, but still gas-bump if the mined nonce never moves.