fix: invalidate quorum commitment cache on disconnect - #7476
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
✅ Final review complete — no blockers (commit acb611a) |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThe CbTx quorum commitment hash cache now uses mutex-protected process-lifetime state with whole-result caches and per-LMQ-type LRU entries. A public invalidation helper clears cached results and LRU contents. Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant CQuorumBlockProcessor
participant UndoBlock
participant InvalidateCachedQcHashes
participant CachedGetQcHashesQcIndexedHashes
CQuorumBlockProcessor->>UndoBlock: undo block commitments
UndoBlock->>InvalidateCachedQcHashes: invalidate after non-null commitment undo
InvalidateCachedQcHashes-->>CachedGetQcHashesQcIndexedHashes: clear cached QC hash state
CachedGetQcHashesQcIndexedHashes->>GetMinedCommitment: reload commitment data on cache miss
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
The five container/cache failures in run 29848453760 are the known Each failing job stops at checkout before compiling this branch because the reusable workflow is loaded from base |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
This PR fixes a real correctness bug where process-lifetime CbTx quorum-commitment hash caches were keyed only by base-block identity, not by the mined CFinalCommitment, allowing a disconnect/reorg to leave stale commitment hashes and incorrectly reject a valid replacement branch with bad-cbtx-quorummerkleroot. I verified the fix directly: InvalidateCachedQcHashes() correctly clears both the outer whole-result cache and the per-quorum LRU under g_qc_hashes_cache_mutex, and is called from CQuorumBlockProcessor::UndoBlock whenever a non-null commitment is undone; the new unit tests demonstrate both the bug (stale hash surviving an evoDb swap) and the fix. The only issue is commit hygiene: the second and third commits are pure iteration on the first commit's own comments/tests within the same PR (one deletes an UndoBlock comment, the next re-adds a shorter version of the same comment one commit later), which is confirmed noise for permanent git blame/bisect history and should be squashed.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
claude-sonnet-5— verifier - Sonnet reviewers:
claude-sonnet-5— general (failed),claude-sonnet-5— dash-core-commit-history (failed),claude-sonnet-5— general (completed),claude-sonnet-5— dash-core-commit-history (completed)
🟡 1 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/llmq/blockprocessor.cpp`:
- [SUGGESTION] src/llmq/blockprocessor.cpp:417-424: Squash the two iterative follow-up commits into the primary fix commit
I diffed all three commits directly. 5a142a8 introduces the cache-invalidation fix, an explanatory comment at the UndoBlock call site, and the regression tests. c39393a then trims that same comment down and simplifies the tests it just added (including removing an EraseMinedCommitment helper and inlining CBlockIndex/genesis setup). 52abbf3 immediately re-adds a shortened version of the identical comment c39393a had just deleted, word-for-word restating the same rationale ("branch-dependent even when the active quorum base-block list is unchanged"). None of these three commits is independently meaningful in permanent history: c39393a's comment removal is undone one commit later, and 52abbf3 exists solely to patch what c39393a just deleted. A future `git blame`/`git log -p` on this line will show a comment added, deleted, and re-added within the same PR — pure noise for bisect/blame with no reviewable content difference. Squash all three into one commit via interactive rebase before merge.
| AddMineableCommitment(qc); | ||
| } | ||
|
|
||
| // Drop both CbTx qc-hash cache layers: mined commitments are branch-dependent | ||
| // even when the active quorum base-block list is unchanged. | ||
| if (undone_commitment) { | ||
| InvalidateCachedQcHashes(); | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Squash the two iterative follow-up commits into the primary fix commit
I diffed all three commits directly. 5a142a8 introduces the cache-invalidation fix, an explanatory comment at the UndoBlock call site, and the regression tests. c39393a then trims that same comment down and simplifies the tests it just added (including removing an EraseMinedCommitment helper and inlining CBlockIndex/genesis setup). 52abbf3 immediately re-adds a shortened version of the identical comment c39393a had just deleted, word-for-word restating the same rationale ("branch-dependent even when the active quorum base-block list is unchanged"). None of these three commits is independently meaningful in permanent history: c39393a's comment removal is undone one commit later, and 52abbf3 exists solely to patch what c39393a just deleted. A future git blame/git log -p on this line will show a comment added, deleted, and re-added within the same PR — pure noise for bisect/blame with no reviewable content difference. Squash all three into one commit via interactive rebase before merge.
source: ['claude', 'codex']
There was a problem hiding this comment.
Fixed in 8a897dee04f: squashed the two iterative follow-ups into the primary fix, leaving one signed commit. The resulting tree is byte-identical to the reviewed 52abbf390d3d head; evo_cbtx_tests and git diff --check pass.
52abbf3 to
8a897de
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/test/evo_cbtx_tests.cpp`:
- Around line 165-218: Add a local RAII cleanup guard to both
qc_hash_cache_invalidated_on_commitment_branch_change
(src/test/evo_cbtx_tests.cpp lines 165-218) and the UndoBlock test
(src/test/evo_cbtx_tests.cpp lines 232-279) so InvalidateCachedQcHashes() runs
when each test exits, while retaining the existing entry invalidation; ensure
the guard restores the process-global cache even on early failure or exceptions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 020a5135-daf9-4f95-8d64-2c6c715c31d3
📒 Files selected for processing (4)
src/evo/cbtx.cppsrc/evo/cbtx.hsrc/llmq/blockprocessor.cppsrc/test/evo_cbtx_tests.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/evo/cbtx.cpp
CachedGetQcHashesQcIndexedHashes keyed both process-lifetime layers only by quorum base blocks/hashes, not by the mined CFinalCommitment. Different valid branches can therefore produce different commitment hashes for the same base list. evoDb rolled back on disconnect, but the caches did not, which can make CalcCbTxMerkleRootQuorums reject a valid replacement branch with bad-cbtx-quorummerkleroot. Invalidate both the outer whole-result cache and the per-quorum LRU from CQuorumBlockProcessor::UndoBlock when a non-null commitment is undone, and add unit coverage for the branch-change and UndoBlock invalidation seams. Co-Authored-By: Claude <noreply@anthropic.com>
8a897de to
acb611a
Compare
|
Addressed the latest review and current-head CI findings in amended commit
Local validation: changed translation units syntax-check cleanly, circular-dependency/include-guard/whitespace lints pass, and independent Codex review found no actionable regression. CI has been re-triggered by the push. |
…tments change 01f012a test: cover post-undo qc-hash cache visibility (UdjinM6) 8e92667 fix: own the CbTx quorum hash caches in CQuorumBlockProcessor (UdjinM6) Pull request description: ## Issue being fixed or feature implemented `CalcCbTxMerkleRootQuorums` memoized into two function-local statics in `src/evo/cbtx.cpp`: an outer whole-result cache keyed on the vector of active quorum base `CBlockIndex*`, and an inner per-LLMQ-type LRU keyed on those base-block hashes. **Keying defect.** Neither key identifies which `CFinalCommitment` was 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. `CalcCbTxMerkleRootQuorums` could then reject an otherwise valid replacement branch with `bad-cbtx-quorummerkleroot`. Concretely: branch A mines `QC_A` for base `B` at height H. A reorg disconnects H and connects H' mining `QC_B` for the same base `B`. 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 returns `hash(QC_A)` for base `B`. 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, since `TestChainSetup::CreateBlock` populates 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::CQuorumBlockProcessor` as `mutable` members guarded by a new `m_qc_hashes_cache_mutex`, and replaced the free helper with `GetQcHashes(pindexPrev) const`. Every caller already receives a `CQuorumBlockProcessor&`, so no signatures changed. Ownership 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, and the pointer-comparison hit test can no longer report a match between unrelated chains. It also bounds the stored `CBlockIndex*` lifetime. `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`; both teardown paths destroy the quorum block processor first (`init.cpp` calls `DashChainstateSetupClose` before `chainman.reset()`, and in tests `~TestingSetup` runs before the base `~ChainTestingSetup` which 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 in `ProcessCommitment` and the erase in `UndoBlock` — 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 holds `cs_main` today, but the invalidation should not depend on a lock discipline enforced elsewhere. Notes for review: - `DB_MINED_COMMITMENT` and the inverted-height index are always mutated as a pair, in those two functions only, so guarding both together is sufficient. - `fJustCheck` returns before the write, so `TestBlockValidity` and the miner's speculative validation do not churn the caches. - Lock ordering is unchanged: `GetMinedAndActiveCommitmentsUntilBlock` is still called before the cache lock, and the only call made while holding it is `GetMinedCommitment`, a bare evoDb read that takes no other lock. - A rolled-back evoDb transaction leaves an empty cache that repopulates from committed state; it can never retain uncommitted commitment hashes. 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`. - Full unit suite `./src/test/test_dash`: 760 test cases, no errors. - `./src/test/test_dash --run_test=evo_cbtx_tests`: passes. - The first commit was checked out and built on its own to confirm it compiles and passes the full suite independently. - Falsification: with the `UndoBlock` cache drop removed, the new `qc_hash_cache_invalidated_by_undoblock` case 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`: only `lint-cppcheck-dash.py` fails, 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.py` passes. 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: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone ACKs for top commit: PastaPastaPasta: utACK 01f012a Tree-SHA512: df2ebf07db13f1f305fbcfb30082b2d1c36e8614e3000984a2f2c1529b1743ae4418a99cd66deb6a4ab2f48c2793af5611cba800da69f53319c2459844088af9
2 participants
Issue being fixed or feature implemented
CalcCbTxMerkleRootQuorumsmemoizes quorum commitment hashes in two layers. The inner LRU is keyed only by quorum base-block hash, not by the serializedCFinalCommitmentmined for that base. Different valid branches can mine different valid commitments for the same base, so after evoDb rolls back a disconnect, the LRU can still return the old branch's commitment hash and make validation reject a valid replacement branch withbad-cbtx-quorummerkleroot.The outer whole-result cache is not itself the stale layer: it is keyed by the active quorum base-block list, and undoing the commitment removes the inversed-height entry, so that list changes and the outer cache misses. The stale value is reintroduced when that rebuild consults the surviving inner LRU.
Dash triggerability is lower than in a pure PoS setting because of PoW and ChainLocks, but the cache key remains branch-insufficient and therefore a correctness bug.
Out of scope: automatic reconsideration of a chainlocked branch already marked failed.
What was done?
CQuorumBlockProcessor, which owns the mined-commitment evoDb state they derive from.CQuorumBlockProcessor::UndoBlockwhenever a non-null mined commitment is undone.evo/cbtx_cache.hAPI.UndoBlockperforms the invalidation, and make duplicated private DB-key assumptions fail loudly through the public reader.How Has This Been Tested?
src/test/test_dashin the worktree-local debug build../src/test/test_dash --run_test=evo_cbtx_tests --catch_system_errors=no(pass: 4 cases).UndoBlockinvalidation and confirmed the regression test fails, then restored it and confirmed the suite passes.test/lint/lint-circular-dependencies.py,test/lint/lint-includes.py, andtest/lint/lint-python.py(pass).validation_chainstate_testsrun crashes identically at the prior PR head, so that failure is pre-existing and unrelated to this change.Breaking Changes
None.
Checklist: