fix(core): keep blocksHashCache entries unique per height - #2548
Conversation
UpdateBlocksHashCache appended unconditionally, so refreshing the cache for a block that was already cached — which the known-block path now does when it promotes a stored block — grew the per-height slice with duplicate hashes. Skip the append when the hash is already tracked; distinct fork hashes at the same height are still kept.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🟢 Approval recommended
The focused fix correctly prevents duplicate entries and includes regression coverage.
Pull request overview
Prevents unbounded duplicate hashes in the per-height block hash cache.
Changes:
- Skip insertion when a block hash is already cached at its height.
- Test repeated updates while preserving fork hashes.
File summaries
| File | Description |
|---|---|
core/blockchain.go |
Adds duplicate detection before cache insertion. |
core/blockchain_test.go |
Verifies repeated updates retain one hash entry. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Summary
UpdateBlocksHashCacheappended unconditionally when a per-height entry already existed, so refreshing the cache for a block that was already cached produced duplicate hash entries in the per-height slice. The cache tracks the hashes of all competing blocks at a height, and its LRU limit (blocksHashCacheLimit = 900) bounds the number of height entries, not the length of a single slice, so duplicates grew without bound.Reachability
Before the known-block work this defect was latent:
SetHeadpurges the cache on rollback, but slow reorgs into the pruned region re-import old blocks viarecoverAncestors, and the follow-upwriteBlockWithStaterefreshes the cache again, appending a duplicate. The upcoming known-block promotion PR refreshes the cache on every re-delivery of a known segment, which a remote peer can trigger cheaply and repeatedly, so this fix must land before (or together with) that PR.Change
Skip the append when the hash is already tracked. Distinct fork hashes at the same height are still kept, preserving the multi-fork behavior covered by the existing fork subtest of
TestBlocksHashCacheUpdate.Testing
TestBlocksHashCacheUpdate: repeatedUpdateBlocksHashCachecalls for the same block keep exactly one entry per hash.go test ./core -run 'TestBlocksHashCacheUpdate|TestAreTwoBlocksSamePath' -count=1passes;gofmt,go vet,go build ./core/...clean.Compatibility
No schema, migration, or wire-protocol impact; cache is in-memory only. Labels
core/dbsuggested.