fix(blockchain): do not cache a negative GetBlockIsMined answer - #1676
Open
ordishs wants to merge 1 commit into
Open
fix(blockchain): do not cache a negative GetBlockIsMined answer#1676ordishs wants to merge 1 commit into
ordishs wants to merge 1 commit into
Conversation
A poll that reads mined_set before the update lands cached false for the response-cache TTL (2 min). The svp2p bridge waits at most ~80 s for the parent's mined_set, so a stale negative stranded the tip+1 block on a live mainnet sync. Only a positive answer is stable; cache that alone. (cherry picked from commit ff21e7d)
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // Cache the mining status result | ||
| cacheOp.Set(isMined, s.cacheTTL) | ||
| if isMined { | ||
| cacheOp.Set(isMined, s.cacheTTL) |
Contributor
There was a problem hiding this comment.
[Minor] The fix is correct and eliminates the observed stale-negative race. One factual nuance: the PR description says "A positive is permanent," but mined_set can transition true→false via ClearBlockMinedSet, InvalidateBlock, and RevalidateBlock (fork/reorg handling). Caching true here is still safe because all three of those paths call ResetResponseCache() — worth relying on that fact rather than permanence.
Note the symmetric read-then-cache TOCTOU race still exists for the true case (a concurrent reorg reset landing between the DB read on line 75 and this Set, leaving a stale true for the TTL). That is far rarer and, for the svp2p IBD caller motivating this fix, a stale true is the benign direction — so this is not blocking, just noting the residual.
Contributor
|
🤖 Claude Code Review Status: Complete Current Review: The added test reproduces the exact race shape (read One [Minor] factual note left inline: the PR description’s "a positive is permanent" is not strictly true (positives can be reverted on reorg), and a symmetric — but far rarer and, for the motivating IBD caller, benign — stale- No blocking issues found. |
|
Contributor
Benchmark Comparison ReportBaseline: Current: Summary
All benchmark results (sec/op)
Threshold: >10% with p < 0.05 | Generated: 2026-08-31 17:15 UTC |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
stores/blockchain/sqlcachesGetBlockIsMinedresults in the response cache for the 2-minute TTL — includingfalse. A poll that readsmined_setjust before the update lands caches the negative, and every caller then sees a stalefalseuntil the TTL expires or another write resets the cache.Observed live on a mainnet IBD: the svp2p bridge waits ~80 s for the parent's
mined_setbefore failing the child block; a cached negative outlives that budget, so block 56 failed repeatedly and sync stalled for 4 hours.services/svp2p/bridge/handle_block.gopollsGetBlockIsMinedin exactly this shape, and the same wait exists for any caller that polls.Change
Cache only a
trueanswer. A positive is permanent; a negative is transient by nature and re-reading it is one indexed row lookup.Test
TestGetBlockIsMined_NegativeAnswerIsNotCachedreproduces the race shape (read false → updatemined_setdirectly → read again) and fails on the old code with the stale cachedfalse.go test ./stores/blockchain/sql/— ok.golangci-lint— 0 issues.