diff --git a/src/evo/cbtx.cpp b/src/evo/cbtx.cpp index 4e1b7986ad6c..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); @@ -69,13 +67,25 @@ 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. + // + // 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(); - if (qc_hashes_cached.empty()) { - llmq::utils::InitQuorumsCache(qc_hashes_cached, Params().GetConsensus()); - } for (const auto& [llmqType, vecBlockIndexes] : quorums) { const auto& llmq_params_opt = Params().GetLLMQ(llmqType); @@ -85,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 f7f33c823bfd..111f5696f1e6 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,95 @@ BOOST_FIXTURE_TEST_CASE(check_cbtx_best_chainlock_rejects_excessive_height_diff, BOOST_CHECK_EQUAL(state_big.GetRejectReason(), "bad-cbtx-cldiff"); } +// 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 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) +{ + 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()