Conversation
|
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: Advanced 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.
🟡 Changes recommended
Cached results can publish the pre-M2 block hash, and the tests do not reproduce differing hashes.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Aligns prepared-block cache keys with HashNoValidator so XDPoS precomputation can be reused.
Changes:
- Updates prepared-result and in-progress cache lookups.
- Adds regression tests for both cache probes.
File summaries
| File | Description |
|---|---|
core/blockchain.go |
Uses validator-independent cache keys. |
core/blockchain_test.go |
Adds prepared-block cache tests. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
e14f092 to
dd7d60e
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Concurrent insertions can reuse and mutate the same cached result and state.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
PrepareBlock stores the result of preparing a block in resultProcess, and asks both resultProcess and calculatingBlock whether that work is already under way before it starts one. All three of those look-ups asked by Hash, while getResultBlock - which computes a result and is what insertBlock reads one back with - stores and looks the block up by HashNoValidator. XDPoS signs a header with a validator signature that takes no part in its execution, so the two hashes differ, and every one of those look-ups missed: the precomputation was never reused, and a block whose calculation was already running was calculated a second time. Ask by HashNoValidator in all three, the key the two caches are written with. A result is prepared from the block as it was propagated, before the signature is added, and the signature is part of the hash the block is inserted under - so a reused result is stamped with the block it is inserted for before it is handed out: its receipts, and the logs they hold, would otherwise carry the pre-signature hash, which is the hash of a block that is written nowhere and that the subscribers of those logs cannot resolve. The look-ups being fixed is what makes a reused result possible, so the two go together. Pre-existing: the look-ups have been keyed this way since the caches were introduced. It was found while reviewing the insertion-error work for XinFinOrg#2534. The blocks the tests prepare carry a validator signature, so that the hash a block is inserted under and the one the caches ask for differ in them: an unsigned block answers both look-ups alike, and a test over one passes whatever key the caches are written with.
dd7d60e to
5c82750
Compare
PrepareBlock stores the result of preparing a block in
resultProcess, and asks bothresultProcessandcalculatingBlockwhether that work is already under way before it starts one. All three of those look-ups asked byHash, whilegetResultBlock- which computes a result and is whatinsertBlockreads one back with - stores and looks the block up byHashNoValidator. XDPoS signs a header with a validator signature that takes no part in its execution, so the two hashes differ, and every one of those look-ups missed: the precomputation was never reused, and a block whose calculation was already running was calculated a second time.Ask by
HashNoValidatorin all three, the key the two caches are written with.Pre-existing. The look-ups have been keyed this way since the caches were introduced. It was found while reviewing the insertion-error work for #2534 and is independent of it - this PR does not close either issue, and #2534/#2535 do not depend on it.
Upstream: none.
PrepareBlockandHashNoValidatorhave no counterpart in go-ethereum - both are XDPoS-specific - so there is no upstream fix to port.Behavioural effect. The caches now answer, so a block that was prepared or is being prepared is no longer prepared again. The worst case for a lookup that hits is a skipped precomputation, which
insertBlockthen performs itself throughgetResultBlock; no path starts to trust a result it did not already trust. A reused result is stamped with the hash of the block it is inserted under, which the validator signature is part of and the key the caches ask for is not. The cached result is copied for that instead of being stamped in place, so two blocks sharing its key - the same block signed twice, say - reuse it without overwriting each other's hash.Tests.
core/blockchain_test.gogains four cases:TestPrepareBlockStoresItsResultUnderTheLookupKeypins the key a prepared result is stored under, and showsgetResultBlock- the look-upinsertBlockperforms - reusing it instead of computing the block again.TestPrepareBlockSkipsABlockBeingCalculatedshows a blockgetResultBlockhas already recorded is not prepared a second time.TestAReusedResultIsStampedWithTheBlockItIsInsertedUndershows a result prepared from the block as it was propagated is stamped with the hash of the signed block that reuses it, so the receipts and the logs it publishes do not carry the pre-signature hash.TestConcurrentReusesOfAPreparedResultCarryTheirOwnHashshows two blocks sharing the key of one prepared result that reuse it at the same time are stamped with a hash each, and that neither stamps the cached result in place.