From ed704b13d987514abc5165b23694cc0e5e023532 Mon Sep 17 00:00:00 2001 From: pasta Date: Sat, 25 Jul 2026 12:41:52 -0500 Subject: [PATCH 1/4] test: prove mined-commitment cache survives reorg in cbtx 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. --- src/test/evo_cbtx_tests.cpp | 158 ++++++++++++++++++++++++++++++++++++ 1 file changed, 158 insertions(+) diff --git a/src/test/evo_cbtx_tests.cpp b/src/test/evo_cbtx_tests.cpp index f7f33c823bfd..3490b54774e9 100644 --- a/src/test/evo_cbtx_tests.cpp +++ b/src/test/evo_cbtx_tests.cpp @@ -2,25 +2,93 @@ // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. +#include #include #include #include #include #include +#include +#include #include #include +#include #include +#include +#include +#include #include +#include #include +#include #include #include #include #include +#include +#include +#include #include +// Keys mirror the file-local constants in src/llmq/blockprocessor.cpp so unit tests can +// seed mined-commitment rows without going through full DKG/mining validation. +static const std::string DB_MINED_COMMITMENT = "q_mc"; +static const std::string DB_MINED_COMMITMENT_BY_INVERSED_HEIGHT = "q_mcih"; + +static std::tuple BuildInversedHeightKey(Consensus::LLMQType llmqType, + int nMinedHeight) +{ + return std::make_tuple(DB_MINED_COMMITMENT_BY_INVERSED_HEIGHT, llmqType, + htobe32_internal(std::numeric_limits::max() - nMinedHeight)); +} + +static void WriteMinedCommitment(CEvoDB& evo_db, const llmq::CFinalCommitment& qc, const uint256& mined_block_hash, + int mined_height, int quorum_height) +{ + const auto cache_key = std::make_pair(qc.llmqType, qc.quorumHash); + evo_db.Write(std::make_pair(DB_MINED_COMMITMENT, cache_key), std::make_pair(qc, mined_block_hash)); + evo_db.Write(BuildInversedHeightKey(qc.llmqType, mined_height), quorum_height); +} + +static void EraseMinedCommitment(CEvoDB& evo_db, Consensus::LLMQType llmq_type, const uint256& quorum_hash, + int mined_height) +{ + evo_db.Erase(std::make_pair(DB_MINED_COMMITMENT, std::make_pair(llmq_type, quorum_hash))); + evo_db.Erase(BuildInversedHeightKey(llmq_type, mined_height)); +} + +static llmq::CFinalCommitment MakeDistinctCommitment(const Consensus::LLMQParams& params, const uint256& quorum_hash, + bool flip_last_signer) +{ + auto commitment = llmq::testutils::CreateValidCommitment(params, quorum_hash); + // signers is part of SERIALIZE_METHODS / SerializeHash but is not covered by the signed + // commitmentHash, so two valid commitments for the same quorum can differ only here. + BOOST_REQUIRE(!commitment.signers.empty()); + if (flip_last_signer) { + commitment.signers.back() = !commitment.signers.back(); + commitment.membersSig = llmq::testutils::CreateRandomBLSSignature(); + } + return commitment; +} + +static uint256 CalcQuorumMerkleRoot(const CBlockIndex* pindex_prev, const llmq::CQuorumBlockProcessor& qbp) +{ + CBlock block; + // Coinbase only — current-block commitments are intentionally absent so the result is + // driven solely by CachedGetQcHashesQcIndexedHashes / GetMinedCommitment. + CMutableTransaction coinbase; + coinbase.vin.emplace_back(); + coinbase.vout.emplace_back(); + block.vtx.emplace_back(MakeTransactionRef(std::move(coinbase))); + uint256 merkle_root; + BlockValidationState state; + BOOST_REQUIRE(CalcCbTxMerkleRootQuorums(block, pindex_prev, qbp, merkle_root, state)); + return merkle_root; +} + BOOST_AUTO_TEST_SUITE(evo_cbtx_tests) // Out-of-range bestCLHeightDiff (>= pindex->nHeight) must be rejected with @@ -67,4 +135,94 @@ BOOST_FIXTURE_TEST_CASE(check_cbtx_best_chainlock_rejects_excessive_height_diff, BOOST_CHECK_EQUAL(state_big.GetRejectReason(), "bad-cbtx-cldiff"); } +// V034: qc_hashes_cached in CachedGetQcHashesQcIndexedHashes is a process-lifetime LRU keyed +// only by the quorum *base* block hash. On reorg, UndoBlock erases the EvoDB mined-commitment +// row and a competing chain may mine a different but equally valid CFinalCommitment for the +// same quorum (signers is serialized into SerializeHash but not into the signed commitment +// hash). Sibling caches are cleared when the active-quorum set changes; qc_hashes_cached is +// not. A subsequent CalcCbTxMerkleRootQuorums call then returns the stale hash and rejects +// the honest majority tip with bad-cbtx-quorummerkleroot (BLOCK_CONSENSUS). +// +// This test seeds EvoDB directly (no full DKG) to isolate the cache defect: +// 1. Populate the LRU with SerializeHash(C1) for quorum base H_Q. +// 2. Force the outer quorums_cached short-circuit to miss (as intermediate reorg blocks do) +// by evaluating against a tip below the mined height. +// 3. Replace the EvoDB row with C2 (simulating UndoBlock + ConnectBlock of the competing tip). +// 4. Recompute: must agree with a fresh SerializeHash(C2) root, not the stale C1 value. +BOOST_FIXTURE_TEST_CASE(cbtx_quorum_merkle_root_reorg_invalidates_mined_commitment_cache, TestChain100Setup) +{ + auto& evo_db = *Assert(m_node.evodb); + auto& qbp = *Assert(m_node.llmq_ctx)->quorum_block_processor; + const auto& params = llmq::testutils::GetLLMQParams(Consensus::LLMQType::LLMQ_TEST); + + const CBlockIndex* pindex_tip; + const CBlockIndex* pindex_quorum; + const CBlockIndex* pindex_mined; + const CBlockIndex* pindex_before_mined; + { + LOCK(::cs_main); + const CChain& chain = m_node.chainman->ActiveChain(); + BOOST_REQUIRE_GE(chain.Height(), 30); + pindex_tip = chain.Tip(); + // Quorum base and the height at which the commitment is recorded as mined. The + // inverse-height index is only visible for pindex->nHeight >= mined_height. + pindex_quorum = chain[10]; + pindex_mined = chain[20]; + pindex_before_mined = chain[19]; + BOOST_REQUIRE(pindex_quorum && pindex_mined && pindex_before_mined); + } + + const uint256 quorum_hash = pindex_quorum->GetBlockHash(); + const uint256 mined_block_hash = pindex_mined->GetBlockHash(); + const int quorum_height = pindex_quorum->nHeight; + const int mined_height = pindex_mined->nHeight; + + auto c1 = MakeDistinctCommitment(params, quorum_hash, /*flip_last_signer=*/false); + auto c2 = MakeDistinctCommitment(params, quorum_hash, /*flip_last_signer=*/true); + const uint256 hash_c1 = ::SerializeHash(c1); + const uint256 hash_c2 = ::SerializeHash(c2); + BOOST_REQUIRE(hash_c1 != hash_c2); + + // Independent expected roots (single active commitment → single leaf). + const uint256 expected_root_c1 = ComputeMerkleRoot(std::vector{hash_c1}); + const uint256 expected_root_c2 = ComputeMerkleRoot(std::vector{hash_c2}); + BOOST_REQUIRE(expected_root_c1 != expected_root_c2); + + // 1. Chain A mines C1. Warm qc_hashes_cached with SerializeHash(C1). + WriteMinedCommitment(evo_db, c1, mined_block_hash, mined_height, quorum_height); + { + const auto [got, got_mined] = qbp.GetMinedCommitment(params.type, quorum_hash); + BOOST_REQUIRE(got_mined == mined_block_hash); + BOOST_REQUIRE(::SerializeHash(got) == hash_c1); + } + const uint256 root_after_c1 = CalcQuorumMerkleRoot(pindex_tip, qbp); + BOOST_CHECK_EQUAL(root_after_c1, expected_root_c1); + + // 2. Intermediate reorg block: active-quorum set differs (commitment not yet visible + // below mined_height). This clears the outer quorums/qcHashes caches the way a real + // reorg does, but must NOT leave a process-lifetime stale entry for H_Q. + { + const uint256 root_empty = CalcQuorumMerkleRoot(pindex_before_mined, qbp); + BOOST_CHECK(root_empty != expected_root_c1); + BOOST_CHECK(root_empty != expected_root_c2); + } + + // 3. Competing chain B: UndoBlock erases C1, ConnectBlock writes C2 for the same H_Q. + EraseMinedCommitment(evo_db, params.type, quorum_hash, mined_height); + WriteMinedCommitment(evo_db, c2, mined_block_hash, mined_height, quorum_height); + { + const auto [got, got_mined] = qbp.GetMinedCommitment(params.type, quorum_hash); + BOOST_REQUIRE(got_mined == mined_block_hash); + BOOST_REQUIRE(::SerializeHash(got) == hash_c2); + } + + // 4. Recompute against the same tip. Must track C2 (fresh EvoDB), not the LRU hit on C1. + const uint256 root_after_reorg = CalcQuorumMerkleRoot(pindex_tip, qbp); + BOOST_CHECK_EQUAL(root_after_reorg, expected_root_c2); + BOOST_CHECK(root_after_reorg != expected_root_c1); + + // Cleanup so later cases in this process do not observe our inverse-height row. + EraseMinedCommitment(evo_db, params.type, quorum_hash, mined_height); +} + BOOST_AUTO_TEST_SUITE_END() From 6a5d51e8f08b1d282dedf47ba056277c7079b7b2 Mon Sep 17 00:00:00 2001 From: pasta Date: Sat, 25 Jul 2026 12:41:58 -0500 Subject: [PATCH 2/4] fix: invalidate mined-commitment LRU on quorum set change 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. --- src/evo/cbtx.cpp | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/evo/cbtx.cpp b/src/evo/cbtx.cpp index 4e1b7986ad6c..131b8bb9c060 100644 --- a/src/evo/cbtx.cpp +++ b/src/evo/cbtx.cpp @@ -69,13 +69,19 @@ auto CachedGetQcHashesQcIndexedHashes(const CBlockIndex* pindexPrev, const llmq: return std::make_pair(qcHashes_cached, qcIndexedHashes_cached); } - // Quorums set is different, reset cached values + // Quorums set is different, reset cached values. + // qc_hashes_cached must be cleared too: it is keyed only by the quorum *base* + // block hash, but the underlying mined commitment (SerializeHash of the + // CFinalCommitment stored under DB_MINED_COMMITMENT) is not immutable for a + // given quorumHash — UndoBlock erases it and a competing chain may mine a + // different but equally valid commitment (signers is serialized into the + // hash but not into the signed commitmentHash). Leaving the LRU intact + // across that mutation produces a permanent bad-cbtx-quorummerkleroot split. quorums_cached.clear(); qcHashes_cached.clear(); qcIndexedHashes_cached.clear(); - if (qc_hashes_cached.empty()) { - llmq::utils::InitQuorumsCache(qc_hashes_cached, Params().GetConsensus()); - } + qc_hashes_cached.clear(); + llmq::utils::InitQuorumsCache(qc_hashes_cached, Params().GetConsensus()); for (const auto& [llmqType, vecBlockIndexes] : quorums) { const auto& llmq_params_opt = Params().GetLLMQ(llmqType); From 10033188f146a1edd69444158ffc55cb11f56e66 Mon Sep 17 00:00:00 2001 From: pasta Date: Sat, 25 Jul 2026 17:16:16 -0500 Subject: [PATCH 3/4] fix: remove unsalvageable mined-commitment LRU from cbtx merkle root 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 89581527f28 (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. --- src/evo/cbtx.cpp | 49 ++++++++++++++++++------------------- src/test/evo_cbtx_tests.cpp | 27 ++++++++++---------- 2 files changed, 38 insertions(+), 38 deletions(-) diff --git a/src/evo/cbtx.cpp b/src/evo/cbtx.cpp index 131b8bb9c060..80e792482daf 100644 --- a/src/evo/cbtx.cpp +++ b/src/evo/cbtx.cpp @@ -9,7 +9,6 @@ #include #include #include -#include #include #include @@ -60,7 +59,6 @@ auto CachedGetQcHashesQcIndexedHashes(const CBlockIndex* pindexPrev, const llmq: static Mutex cs_cache; static std::map> quorums_cached GUARDED_BY(cs_cache); - static std::map>> qc_hashes_cached GUARDED_BY(cs_cache); static QcHashMap qcHashes_cached GUARDED_BY(cs_cache); static QcIndexedHashMap qcIndexedHashes_cached GUARDED_BY(cs_cache); @@ -70,18 +68,24 @@ auto CachedGetQcHashesQcIndexedHashes(const CBlockIndex* pindexPrev, const llmq: } // Quorums set is different, reset cached values. - // qc_hashes_cached must be cleared too: it is keyed only by the quorum *base* - // block hash, but the underlying mined commitment (SerializeHash of the - // CFinalCommitment stored under DB_MINED_COMMITMENT) is not immutable for a - // given quorumHash — UndoBlock erases it and a competing chain may mine a - // different but equally valid commitment (signers is serialized into the - // hash but not into the signed commitmentHash). Leaving the LRU intact - // across that mutation produces a permanent bad-cbtx-quorummerkleroot split. + // + // Do NOT reintroduce a cache keyed on the quorum *base* block hash here (there used to be a + // process-lifetime `qc_hashes_cached` LRU memoising ::SerializeHash(minedCommitment)). That key + // is not sufficient to identify the value: the DB_MINED_COMMITMENT row for a given quorumHash is + // mutable. CQuorumBlockProcessor::UndoBlock erases it on disconnect and re-adds the commitment as + // mineable, so a competing chain can mine a *different but equally valid* CFinalCommitment for the + // same quorum -- the signed commitmentHash covers only (llmqType, quorumHash, validMembers, + // quorumPublicKey, quorumVvecHash) and not the `signers` bitset, while `signers` does feed into + // ::SerializeHash. Surviving that mutation, such a cache returns the pre-reorg hash, this function + // computes a merkle root nobody else agrees on, and ConnectBlock rejects the honest majority tip + // with bad-cbtx-quorummerkleroot (BLOCK_CONSENSUS) -- marking it BLOCK_FAILED_VALID on disk, which + // a restart does not clear. That is a permanent chain split. + // + // Any such cache must be keyed on something that pins the commitment content (e.g. the mined block + // hash returned alongside it by GetMinedCommitment), not on the quorum base block hash alone. quorums_cached.clear(); qcHashes_cached.clear(); qcIndexedHashes_cached.clear(); - qc_hashes_cached.clear(); - llmq::utils::InitQuorumsCache(qc_hashes_cached, Params().GetConsensus()); for (const auto& [llmqType, vecBlockIndexes] : quorums) { const auto& llmq_params_opt = Params().GetLLMQ(llmqType); @@ -91,23 +95,18 @@ auto CachedGetQcHashesQcIndexedHashes(const CBlockIndex* pindexPrev, const llmq: vec_hashes.reserve(vecBlockIndexes.size()); auto& map_indexed_hashes = qcIndexedHashes_cached[llmqType]; for (const auto& blockIndex : vecBlockIndexes) { - uint256 block_hash{blockIndex->GetBlockHash()}; - - std::pair qc_hash; - if (!qc_hashes_cached[llmqType].get(block_hash, qc_hash)) { - auto [pqc, dummy_hash] = quorum_block_processor.GetMinedCommitment(llmqType, block_hash); - if (dummy_hash == uint256::ZERO) { - // this should never happen - return std::nullopt; - } - qc_hash.first = ::SerializeHash(pqc); - qc_hash.second = rotation_enabled ? pqc.quorumIndex : 0; - qc_hashes_cached[llmqType].insert(block_hash, qc_hash); + const uint256 block_hash{blockIndex->GetBlockHash()}; + + const auto [pqc, mined_block_hash] = quorum_block_processor.GetMinedCommitment(llmqType, block_hash); + if (mined_block_hash == uint256::ZERO) { + // this should never happen + return std::nullopt; } + const uint256 qc_hash{::SerializeHash(pqc)}; if (rotation_enabled) { - map_indexed_hashes[qc_hash.second] = qc_hash.first; + map_indexed_hashes[pqc.quorumIndex] = qc_hash; } else { - vec_hashes.emplace_back(qc_hash.first); + vec_hashes.emplace_back(qc_hash); } } } diff --git a/src/test/evo_cbtx_tests.cpp b/src/test/evo_cbtx_tests.cpp index 3490b54774e9..73330fa688f8 100644 --- a/src/test/evo_cbtx_tests.cpp +++ b/src/test/evo_cbtx_tests.cpp @@ -77,7 +77,7 @@ static llmq::CFinalCommitment MakeDistinctCommitment(const Consensus::LLMQParams static uint256 CalcQuorumMerkleRoot(const CBlockIndex* pindex_prev, const llmq::CQuorumBlockProcessor& qbp) { CBlock block; - // Coinbase only — current-block commitments are intentionally absent so the result is + // Coinbase only: current-block commitments are intentionally absent so the result is // driven solely by CachedGetQcHashesQcIndexedHashes / GetMinedCommitment. CMutableTransaction coinbase; coinbase.vin.emplace_back(); @@ -135,18 +135,19 @@ BOOST_FIXTURE_TEST_CASE(check_cbtx_best_chainlock_rejects_excessive_height_diff, BOOST_CHECK_EQUAL(state_big.GetRejectReason(), "bad-cbtx-cldiff"); } -// V034: qc_hashes_cached in CachedGetQcHashesQcIndexedHashes is a process-lifetime LRU keyed -// only by the quorum *base* block hash. On reorg, UndoBlock erases the EvoDB mined-commitment -// row and a competing chain may mine a different but equally valid CFinalCommitment for the -// same quorum (signers is serialized into SerializeHash but not into the signed commitment -// hash). Sibling caches are cleared when the active-quorum set changes; qc_hashes_cached is -// not. A subsequent CalcCbTxMerkleRootQuorums call then returns the stale hash and rejects -// the honest majority tip with bad-cbtx-quorummerkleroot (BLOCK_CONSENSUS). +// V034: CachedGetQcHashesQcIndexedHashes must never memoise ::SerializeHash(minedCommitment) +// under a key that is only the quorum *base* block hash. The EvoDB mined-commitment row for a +// given quorumHash is mutable: on reorg UndoBlock erases it and a competing chain may mine a +// different but equally valid CFinalCommitment for the same quorum (`signers` feeds into +// SerializeHash but is not covered by the signed commitmentHash). A cache surviving that +// mutation makes CalcCbTxMerkleRootQuorums return a root derived from the pre-reorg commitment, +// rejecting the honest majority tip with bad-cbtx-quorummerkleroot (BLOCK_CONSENSUS) -- which is +// persisted as BLOCK_FAILED_VALID and therefore survives restart. Permanent chain split. // -// This test seeds EvoDB directly (no full DKG) to isolate the cache defect: -// 1. Populate the LRU with SerializeHash(C1) for quorum base H_Q. -// 2. Force the outer quorums_cached short-circuit to miss (as intermediate reorg blocks do) -// by evaluating against a tip below the mined height. +// This test seeds EvoDB directly (no full DKG) to pin that invariant: +// 1. Chain A mines C1 for quorum base H_Q; compute the root (warms any cache keyed on H_Q). +// 2. Evaluate against a tip below the mined height, so the outer quorums_cached short-circuit +// misses exactly as it does for intermediate blocks of a real reorg. // 3. Replace the EvoDB row with C2 (simulating UndoBlock + ConnectBlock of the competing tip). // 4. Recompute: must agree with a fresh SerializeHash(C2) root, not the stale C1 value. BOOST_FIXTURE_TEST_CASE(cbtx_quorum_merkle_root_reorg_invalidates_mined_commitment_cache, TestChain100Setup) @@ -183,7 +184,7 @@ BOOST_FIXTURE_TEST_CASE(cbtx_quorum_merkle_root_reorg_invalidates_mined_commitme const uint256 hash_c2 = ::SerializeHash(c2); BOOST_REQUIRE(hash_c1 != hash_c2); - // Independent expected roots (single active commitment → single leaf). + // Independent expected roots (single active commitment -> single leaf). const uint256 expected_root_c1 = ComputeMerkleRoot(std::vector{hash_c1}); const uint256 expected_root_c2 = ComputeMerkleRoot(std::vector{hash_c2}); BOOST_REQUIRE(expected_root_c1 != expected_root_c2); From 2884e57f72c0e0dbc67bf137cfb01a99dd5bea64 Mon Sep 17 00:00:00 2001 From: pasta Date: Sun, 26 Jul 2026 16:42:27 -0500 Subject: [PATCH 4/4] docs: drop internal audit finding IDs from comments 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. --- src/test/evo_cbtx_tests.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/evo_cbtx_tests.cpp b/src/test/evo_cbtx_tests.cpp index 73330fa688f8..111f5696f1e6 100644 --- a/src/test/evo_cbtx_tests.cpp +++ b/src/test/evo_cbtx_tests.cpp @@ -135,7 +135,7 @@ BOOST_FIXTURE_TEST_CASE(check_cbtx_best_chainlock_rejects_excessive_height_diff, BOOST_CHECK_EQUAL(state_big.GetRejectReason(), "bad-cbtx-cldiff"); } -// V034: CachedGetQcHashesQcIndexedHashes must never memoise ::SerializeHash(minedCommitment) +// CachedGetQcHashesQcIndexedHashes must never memoise ::SerializeHash(minedCommitment) // under a key that is only the quorum *base* block hash. The EvoDB mined-commitment row for a // given quorumHash is mutable: on reorg UndoBlock erases it and a competing chain may mine a // different but equally valid CFinalCommitment for the same quorum (`signers` feeds into