Repository navigation
fix: improve handleConsistency to wait for tracker polls instead of bailing - #2326
Conversation
PR Summary by QodoFix handleConsistency to wait for tracker polls before bailing
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
Code Review by Qodo
1. Conservative poll interval
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| trackerPollInterval := averageBlockTime / (chaintracker.MostFrequentPollingMultiplier / 4) | ||
| noTimeForATrackerPoll := ok && halfTimeLeft < trackerPollInterval | ||
| if (blockGap > blockLagForQosSync*2 || (blockGap > 1 && noTimeForATrackerPoll)) && (seenBlock >= latestBlock) { |
There was a problem hiding this comment.
1. Conservative poll interval 🐞 Bug ≡ Correctness
handleConsistency computes trackerPollInterval as averageBlockTime/4 and bails when halfTimeLeft is smaller, but the ChainTracker’s scheduling can poll as frequently as averageBlockTime/16. This can return ConsistencyError even when a tracker poll could happen within the wait budget and the wait loop could succeed.
Agent Prompt
### Issue description
`handleConsistency` uses `trackerPollInterval := averageBlockTime / (chaintracker.MostFrequentPollingMultiplier / 4)` (i.e. `averageBlockTime/4`) to decide whether there is time for at least one chain-tracker poll. However, the chain tracker can poll at `averageBlockTime/chaintracker.MostFrequentPollingMultiplier` (i.e. `averageBlockTime/16`) in its initial and steady-state scheduling.
This makes `noTimeForATrackerPoll` up to 4x too conservative and can cause premature `ConsistencyError` bails when `averageBlockTime/16 <= halfTimeLeft < averageBlockTime/4`.
### Issue Context
The PR goal is to avoid bailing when blocks already exist and the tracker can catch up on its next poll. The current calculation can still bail in cases where a poll could plausibly occur within the remaining wait budget.
### Fix Focus Areas
- protocol/rpcprovider/rpcprovider_server.go[1328-1330]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 4 files with indirect coverage changes 🚀 New features to boost your workflow:
|
nimrod-teich
left a comment
nimrod-teich
left a comment
There was a problem hiding this comment.
LGTM. Rebase to main and push.
…production
handleConsistency's early bail used a Poisson model of block production to
decide whether the provider could reach the consumer's seenBlock. That model
does not describe what this code path waits for.
Every path that reaches the bail targets a block <= seenBlock (blockGap is
clamped to seenBlock - latestBlock just above), and seenBlock was already
served to the consumer by another provider — so those blocks already exist on
chain. The provider is not waiting for the chain to produce blocks, it is
waiting for its own ChainTracker to poll blocks that are already there, which
takes at most averageBlockTime/(MostFrequentPollingMultiplier/4) — well under
one block time.
Modelling it as block production makes the bail fire whenever the spec's
average_block_time overstates the chain's real block time. On NEAR (spec says
1200ms, chain runs at ~600ms since the May 2025 optimistic-block upgrade)
probabilityBlockError evaluates to 0.62-0.97 for every realistic combination
of blockGap >= 2 and relay timeout, so the bail was unconditional and the
catch-up wait below it — which would have closed a 2-3 block gap on the next
poll — was unreachable.
Keep probabilityBlockError for observability and gate the early bail on
whether waiting can help at all:
- blockGap beyond blockLagForQosSync*2: the node is genuinely behind and
polling won't fix it, bail immediately (unchanged)
- not enough time left for a single tracker poll: waiting is pointless
Otherwise fall through to the existing bounded wait, which already re-checks
the tracker every 50ms and whose post-wait check still returns
ConsistencyError if the provider truly did not catch up.
Trade-off: a provider whose node is genuinely behind by <= blockLagForQosSync*2
now spends up to halfTimeLeft before failing instead of failing instantly. That
budget was already spent this way for blockGap == 1, and the consumer races
providers in parallel, so it does not serialize onto end-user latency.
Adds halfTimeLeft and trackerPollInterval to the bail warning so operators can
see which term fired.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YCMc3dBY9QkwHkX8H4iK1Q
0d1fb2f to
fa15ab3
Compare
|
Rebased onto #2322 landed in the meantime, which made this a real rebase rather than a fast-forward. The diff is unchanged at +85/−2, and Verified locally: One open item, flagging rather than silently carrying it: Qodo's Generated by Claude Code |
|
What failed. Why it isn't this PR.
A package that is green on both parents and red on a merge that changes no Go code is a flake. One caveat on my own evidence, in the interest of not overstating it: I could not reproduce this locally in a way that discriminates. If the re-run reproduces it, that changes the picture and I'll dig in properly rather than re-running again. Generated by Claude Code |
|
Correcting my previous comment: the re-run reproduced the failure, so this is not a flake. What the evidence now says.
The Go code at The mechanism, as best I can establish it. This change replaces an instant bail with entry into the existing wait loop, which sleeps up to That is precisely the fail-fast trade-off noted when this PR was opened. CI is now putting a number on it rather than leaving it theoretical. What I have not established, and won't assert. I cannot reproduce this locally in a way that discriminates — Where I think this should go. The most promising fix is to bound the wait by what it is actually waiting for. We established the provider is waiting for one ChainTracker poll, not for block production — so sleeping up to That is a design change to a PR that is already approved, and it interacts with Qodo's Generated by Claude Code |
protocol/integration times out under `go test -timeout 15m` with no output and
no report file, taking the whole test-protocol job down with it. The hang is in
the first line of the first test, in uniqueAddressGenerator.GetAddress.
Two defects combine:
isPortInUse treats every error from resolve/listen as "this port is taken". A
host without IPv6 loopback fails the [::1] bind with EAFNOSUPPORT, so every port
reports busy even though none of them are.
GetAddress then loops on that, and its `currentPort > maxPort` guard sits after
the loop rather than inside it. When every probe says "in use" the loop never
terminates and never reaches the guard, so the intended
panic("all ports have been exhausted") is unreachable and the package spins
until the test binary is killed.
Fixes both: probe failures that mean the address family is unavailable
(EAFNOSUPPORT, EADDRNOTAVAIL) now skip that address instead of condemning the
port, and the bound is checked inside the loop so exhaustion panics with a
usable message instead of hanging.
Verified on a host without IPv6 loopback, where this reproduces every time:
TestConsumerProviderBasic went from a 15m timeout to passing in 2.09s, and the
full protocol/integration suite from timing out to green in 37s.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YCMc3dBY9QkwHkX8H4iK1Q
|
Found the actual cause, and it is neither this change nor an interaction with #2325. My previous two comments were both wrong; disregard them. Fix pushed as The hang is in the test helper, on the first line of the first test. Goroutine dump from the timeout: Two defects in
Measured, on a host where this reproduces every time:
The 37s run was at One thing I still cannot prove. I reproduced deterministically on a host with no IPv6 loopback. I don't know the runner's IPv6 state, so I can't say for certain the CI hang enters the loop by that same door — the unbounded loop is reachable any time probes keep reporting "in use", and Happy to split this into its own PR if you'd rather keep this one to the consistency change — it is independent, and it fixes a latent hang that can take down Generated by Claude Code |
|
A different job went red on the same commit: A 10-minute timeout, hung in a cosmos-sdk test-network gRPC
Worth noting it is the same class of problem as the one just fixed — test infrastructure standing up listeners on a constrained runner and hanging rather than failing fast. Different package and different owner, so I'm not touching it here, but Re-run of the failed job is currently rejected with Generated by Claude Code |
Description
Fixes #2324
Fixes the
handleConsistencylogic to distinguish between two failure modes:The previous implementation used
probabilityBlockError(which models block production) to decide whether to bail. This caused false positives when the spec'saverage_block_timeoverstates the chain's real block time, leading to premature bailouts even when the requested blocks already exist on-chain and the tracker would pick them up on its next poll.The fix gates the early bail on whether there's enough time for at least one tracker poll (
trackerPollInterval), which is much faster than block production. This allows the wait loop to run and catch up in cases like NEAR where small gaps (2-3 blocks) are transient.The spec-level half of #2324 was fixed separately in #2325 (NEAR/NEART
average_block_time1200 -> 600). That change alone is not sufficient: at 600 ablockGapof 2 still bails on 10-CU methods and 3 still bails everywhere. This PR closes the remaining gap.Changes
probabilityBlockError > 0.4condition withnoTimeForATrackerPollcheck that compares available time againsttrackerPollIntervalTestHandleConsistency_SmallGapWaitsInsteadOfBailing: Verifies that small gaps (2-3 blocks) wait instead of bail when the tracker can catch upTestHandleConsistency_NoTimeToPollBails: Verifies that we still bail when there's insufficient time for even one tracker pollAuthor Checklist
mainbranchReviewers Checklist
https://claude.ai/code/session_01YCMc3dBY9QkwHkX8H4iK1Q