fix: invalidate CbTx quorum hash caches when mined commitments change - #7491
Conversation
CachedGetQcHashesQcIndexedHashes memoized into function-local statics: a whole-result cache keyed on the set of active quorum base blocks, and an LRU keyed on those base-block hashes. That left two defects. Keying: neither key identifies the CFinalCommitment actually mined for a base, so a disconnect that re-mines a different valid commitment for an unchanged base list left both layers serving hashes from the abandoned branch, and CalcCbTxMerkleRootQuorums could reject a valid replacement branch with bad-cbtx-quorummerkleroot. Lifetime: the caches lived for the whole process while the block index whose CBlockIndex* the outer cache stores is per-node, and the outer cache's hit test is a pointer comparison that a recycled address can make report a match between unrelated chains. Move both layers onto CQuorumBlockProcessor, which owns the mined-commitment state they derive from and is already passed to every caller. That removes the cross-instance hazard by construction: a fresh CQuorumBlockProcessor starts with empty caches, so no node or test fixture can observe another's entries. It also bounds the stored CBlockIndex* lifetime, though by teardown ordering rather than by construction: CBlockIndex objects are held by value in BlockManager::m_block_index and are never cleared or erased at runtime, so they are freed only with the ChainstateManager, and both teardown paths destroy the quorum block processor first. Owning the caches here also puts them in the same class as the only two sites that mutate mined-commitment state, so the keying defect is handled by dropping them there directly, as a private detail rather than a cross-module call. The drop happens only after the commitment's evoDb writes or erases are complete, so the caches cannot be repopulated from a half-updated view. Every caller holds cs_main today, which already rules that out, but the invalidation should not depend on a lock discipline enforced elsewhere. fJustCheck returns before the write, so speculative validation does not churn the caches. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Disconnecting the block that mined a commitment must make a replacement commitment for the same quorum base visible, even though the base-block list keying the caches is unchanged across the swap. Verified to fail when the UndoBlock cache drop is removed. No fixture-level cache reset is needed: the caches belong to the quorum block processor, so each fixture gets its own. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
✅ Final review complete — no blockers (commit 01f012a) |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
WalkthroughThe quorum block processor now owns quorum commitment hash map types and cache state, exposes Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant BlockProcessor
participant EvoDB
participant MerkleCalculator
BlockProcessor->>EvoDB: persist or undo mined commitment
BlockProcessor->>BlockProcessor: invalidate quorum hash cache
MerkleCalculator->>BlockProcessor: GetQcHashes
BlockProcessor->>EvoDB: read current commitment state
EvoDB-->>BlockProcessor: commitment hashes
BlockProcessor-->>MerkleCalculator: quorum hash mappings
MerkleCalculator-->>MerkleCalculator: calculate quorum merkle root
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
Verified the complete PR at head 01f012a. The cache ownership and invalidation changes correctly address stale commitment hashes across undo/replacement and instance-lifetime boundaries, and the focused regression test covers the actual failure mode; no actionable in-scope issues remain.
Source: reviewers codex/general=gpt-5.6-sol(completed); codex/dash-core-commit-history=gpt-5.6-sol(completed); claude/general=claude-sonnet-5(completed); claude/dash-core-commit-history=claude-sonnet-5(failed); claude/dash-core-commit-history=claude-sonnet-5(failed); claude/dash-core-commit-history=claude-sonnet-5(completed); verifier=codex/final-verifier=gpt-5.6-sol(completed) fallback_for_sonnet_verifier=true; coordinator=openclaw-agent/cliproxy/gpt-5.6-sol(orchestration-only).
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— final-verifier (fallback) - Sonnet reviewers:
claude-sonnet-5— general (completed),claude-sonnet-5— dash-core-commit-history (failed),claude-sonnet-5— dash-core-commit-history (failed),claude-sonnet-5— dash-core-commit-history (completed)
3 participants
Issue being fixed or feature implemented
CalcCbTxMerkleRootQuorumsmemoized into two function-local statics insrc/evo/cbtx.cpp: an outer whole-result cache keyed on the vector of active quorum baseCBlockIndex*, and an inner per-LLMQ-type LRU keyed on those base-block hashes.Keying defect. Neither key identifies which
CFinalCommitmentwas actually mined for a base on the active chain. Different valid branches can mine different valid commitments for the same base list. On a disconnect the evoDb state is rolled back correctly, but the caches were not invalidated, so both layers kept serving hashes from the abandoned branch.CalcCbTxMerkleRootQuorumscould then reject an otherwise valid replacement branch withbad-cbtx-quorummerkleroot.Concretely: branch A mines
QC_Afor baseBat height H. A reorg disconnects H and connects H' miningQC_Bfor the same baseB. Validating H'+1 uses a base-block list identical to branch A's, so the outer cache hits and returns branch A's hashes wholesale; even on an outer miss the LRU still returnshash(QC_A)for baseB.Triggerability on Dash is lower than in a pure PoS setting because of PoW and ChainLocks, but the keying is branch-dependent and therefore a correctness bug.
Lifetime defect. The caches lived for the whole process, while the block index whose
CBlockIndex*the outer cache stores is per-node. The outer cache's hit test is a pointer comparison, so after a chainstate teardown a recycled address can make two unrelated chains compare equal. In the unit-test binary this leaks state between fixtures, sinceTestChainSetup::CreateBlockpopulates the caches.This is an alternative to PR #7476, which fixes the keying defect only, via a process-global cache plus an exported invalidation function called from
UndoBlock.What was done?
Moved both cache layers onto
llmq::CQuorumBlockProcessorasmutablemembers guarded by a newm_qc_hashes_cache_mutex, and replaced the free helper withGetQcHashes(pindexPrev) const. Every caller already receives aCQuorumBlockProcessor&, so no signatures changed.Ownership removes the cross-instance hazard by construction: a fresh
CQuorumBlockProcessorstarts with empty caches, so no node or test fixture can observe another's entries, and the pointer-comparison hit test can no longer report a match between unrelated chains.It also bounds the stored
CBlockIndex*lifetime.CBlockIndexobjects are held by value inBlockManager::m_block_indexand are never cleared or erased at runtime, so they are freed only with theChainstateManager; both teardown paths destroy the quorum block processor first (init.cppcallsDashChainstateSetupClosebeforechainman.reset(), and in tests~TestingSetupruns before the base~ChainTestingSetupwhich resets chainman).It also puts the caches in the same class as the only two sites that mutate mined commitment state, so the keying defect is handled by a private
DropQcHashesCache()called from the write inProcessCommitmentand the erase inUndoBlock— a private detail rather than a cross-module call. The drop runs only after that commitment's evoDb writes/erases are complete, so the caches cannot be repopulated from a half-updated view; every caller holdscs_maintoday, but the invalidation should not depend on a lock discipline enforced elsewhere.Notes for review:
DB_MINED_COMMITMENTand the inverted-height index are always mutated as a pair, in those two functions only, so guarding both together is sufficient.fJustCheckreturns before the write, soTestBlockValidityand the miner's speculative validation do not churn the caches.GetMinedAndActiveCommitmentsUntilBlockis still called before the cache lock, and the only call made while holding it isGetMinedCommitment, a bare evoDb read that takes no other lock.Two commits: the fix, then the regression test.
How Has This Been Tested?
Built on macOS arm64 against the depends prefix with
--enable-debug --enable-crash-hooks --enable-werror../src/test/test_dash: 760 test cases, no errors../src/test/test_dash --run_test=evo_cbtx_tests: passes.UndoBlockcache drop removed, the newqc_hash_cache_invalidated_by_undoblockcase fails on a merkle-root mismatch; restored and re-confirmed green. The test therefore genuinely covers the bug rather than passing vacuously.test/lint/all-lint.py: onlylint-cppcheck-dash.pyfails, on files untouched by this change (llmq/params.h,evo/dmn_types.h,evo/netinfo.h); reproduced identically on an unmodified checkout.lint-circular-dependencies.pypasses.Functional tests were not run; the change is confined to the CbTx merkle-root cache path and is covered by unit tests.
Breaking Changes
None. Consensus rules are unchanged; this only affects caching of values that were already being computed.
Checklist: