fix: remove never-invalidated mined-commitment cache from cbtx merkle root - #64
Draft
PastaPastaPasta wants to merge 4 commits into
Draft
fix: remove never-invalidated mined-commitment cache from cbtx merkle root#64PastaPastaPasta wants to merge 4 commits into
PastaPastaPasta wants to merge 4 commits into
Conversation
Add a unit test that warms CachedGetQcHashesQcIndexedHashes with commitment C1 for a quorum base hash, simulates a reorg that replaces the EvoDB mined-commitment row with a distinct but equally valid C2 (signers-only difference), and asserts CalcCbTxMerkleRootQuorums tracks C2 rather than the process-lifetime LRU hit on C1. Fails before the qc_hashes_cached invalidation fix.
qc_hashes_cached in CachedGetQcHashesQcIndexedHashes was process-lifetime and keyed only by the quorum base block hash. On reorg, UndoBlock erases the DB_MINED_COMMITMENT row and a competing chain may mine a different but valid CFinalCommitment for the same quorum (signers is part of SerializeHash but not of the signed commitmentHash). Sibling caches were already cleared when the active-quorum set changed; clear and re-init qc_hashes_cached in the same reset block so CalcCbTxMerkleRootQuorums cannot return a stale hash and reject the honest majority tip with bad-cbtx-quorummerkleroot. Consensus-safe: for any fixed chain tip the recomputed root is identical to a cold-process computation; only the reorg-stale path changes.
The previous commit cleared qc_hashes_cached in the quorum-set reset block. That is correct but leaves the cache provably dead: the clear is unconditional and sits above the cache's only read, so every lookup now misses. Within a single invocation all (llmqType, quorumHash) keys are distinct -- bad-qc-dup forbids two mined commitments for one quorum on a chain, and rotated types contribute one entry per quorumIndex -- so a hit is impossible. It costs a per-call map rebuild and an InitQuorumsCache walk to memoise nothing. Remove it instead. This restores the loop to its shape before 8958152 (perf: cache mined commitment for quorum merkle root calculation), the commit that introduced the defect, and drops the now-unused llmq/utils.h include. The key, not the invalidation, was the root cause: the quorum base block hash does not identify the value, because the DB_MINED_COMMITMENT row is mutable across a reorg. Leaving a cleared-but-present cache invites a future reader to re-add a lookup path under the same unsound key. The reset block now carries a comment stating why no cache may be keyed on the base block hash alone, and what a correct key would have to pin. Behaviour is unchanged for every chain: the value returned is what a cold process computes. The regression test is unchanged and still fails when the old LRU is restored.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 V0NN/U0NN labels were private working identifiers from a local review pass. They carry no meaning outside that pass, so they are removed while the surrounding technical rationale is kept.
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.
Audit finding V034 (high, CONFIRMED). Class: chain-split — worth noting this is not a plain DoS.
Issue
CachedGetQcHashesQcIndexedHashesinsrc/evo/cbtx.cppheld a process-lifetimeqc_hashes_cachedLRU memoising::SerializeHash(minedCommitment), keyed on the quorum base block hash. That key does not identify the value.The
DB_MINED_COMMITMENTrow for a givenquorumHashis mutable:CQuorumBlockProcessor::UndoBlockerases it on disconnect and re-adds the commitment as mineable, so after a reorg a competing chain can mine a different but equally validCFinalCommitmentfor the same quorum. The signedcommitmentHashcovers only(llmqType, quorumHash, validMembers, quorumPublicKey, quorumVvecHash)— not thesignersbitset — whilesignersdoes feed into::SerializeHash.Surviving that mutation, the cache returns the pre-reorg hash. The node then computes a quorum merkle root nobody else agrees on and rejects the honest-majority tip with
bad-cbtx-quorummerkleroot(BLOCK_CONSENSUS), marking itBLOCK_FAILED_VALIDon disk. A restart does not clear that — the split is permanent and requires reindex to recover.Fix
The cache is removed outright and the commitment hash computed directly from
GetMinedCommitment.The initial approach invalidated the LRU on quorum-set change, but review showed the cached value was then provably dead — the invalidation fires on exactly the paths that would have produced a hit, so the cache never served anything. Keeping it would have been complexity with no benefit and a live footgun.
A comment at the former cache site records why, so nobody reintroduces it: any such cache must be keyed on something that pins the commitment content (e.g. the mined block hash
GetMinedCommitmentalready returns alongside it), not on the quorum base block hash alone.Tests
test: prove mined-commitment cache survives reorg in cbtxprecedes the fix and fails without it.Review notes
qcHashes_cached/qcIndexedHashes_cachedcaches (keyed on the quorum set, and correctly invalidated) are untouched and still absorb the common case. If benchmarking shows the removed layer mattered, the correct fix is to re-add it keyed on the mined block hash — but correctness first.dashpay/dashdevelop @6d04c60ef36. Not rebase-tested against a newer tip; full functional suite not run.🤖 Generated with Claude Code