From 8e9266753e1d332a6eb5d617ce7382533c20e402 Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Mon, 27 Jul 2026 12:16:25 +0300 Subject: [PATCH 1/2] fix: own the CbTx quorum hash caches in CQuorumBlockProcessor 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 --- src/evo/cbtx.cpp | 68 +++-------------------------------- src/llmq/blockprocessor.cpp | 70 +++++++++++++++++++++++++++++++++++++ src/llmq/blockprocessor.h | 35 +++++++++++++++++-- 3 files changed, 106 insertions(+), 67 deletions(-) diff --git a/src/evo/cbtx.cpp b/src/evo/cbtx.cpp index 4e1b7986ad6c..856d8d74ee6c 100644 --- a/src/evo/cbtx.cpp +++ b/src/evo/cbtx.cpp @@ -46,68 +46,8 @@ bool CheckCbTx(const CCbTx& cbTx, const CBlockIndex* pindexPrev, TxValidationSta return true; } -using QcHashMap = std::map>; -using QcIndexedHashMap = std::map>; - -/** - * Handles the calculation or caching of qcHashes and qcIndexedHashes - * @param pindexPrev The const CBlockIndex* (ie a block) of a block. Both the Quorum list and quorum rotation activation status will be retrieved based on this block. - * @return nullopt if quorumCommitment was unable to be found, otherwise returns the qcHashes and qcIndexedHashes that were calculated or cached - */ -auto CachedGetQcHashesQcIndexedHashes(const CBlockIndex* pindexPrev, const llmq::CQuorumBlockProcessor& quorum_block_processor) -> - std::optional> { - auto quorums = quorum_block_processor.GetMinedAndActiveCommitmentsUntilBlock(pindexPrev); - - 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); - - LOCK(cs_cache); - if (quorums == quorums_cached) { - return std::make_pair(qcHashes_cached, qcIndexedHashes_cached); - } - - // Quorums set is different, reset cached values - 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); - assert(llmq_params_opt.has_value()); - bool rotation_enabled = llmq::IsQuorumRotationEnabled(llmq_params_opt.value(), pindexPrev); - auto& vec_hashes = qcHashes_cached[llmqType]; - 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); - } - if (rotation_enabled) { - map_indexed_hashes[qc_hash.second] = qc_hash.first; - } else { - vec_hashes.emplace_back(qc_hash.first); - } - } - } - std::swap(quorums_cached, quorums); - return std::make_pair(qcHashes_cached, qcIndexedHashes_cached); -} +using llmq::QcHashMap; +using llmq::QcIndexedHashMap; auto CalcHashCountFromQCHashes(const QcHashMap& qcHashes) { @@ -124,7 +64,7 @@ bool CalcCbTxMerkleRootQuorums(const CBlock& block, const CBlockIndex* pindexPre int64_t nTime1 = GetTimeMicros(); - auto retVal = CachedGetQcHashesQcIndexedHashes(pindexPrev, quorum_block_processor); + auto retVal = quorum_block_processor.GetQcHashes(pindexPrev); if (!retVal) { return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "commitment-not-found"); } @@ -132,7 +72,7 @@ bool CalcCbTxMerkleRootQuorums(const CBlock& block, const CBlockIndex* pindexPre auto [qcHashes, qcIndexedHashes] = retVal.value(); int64_t nTime2 = GetTimeMicros(); nTimeMined += nTime2 - nTime1; - LogPrint(BCLog::BENCHMARK, " - CachedGetQcHashesQcIndexedHashes: %.2fms [%.2fs]\n", 0.001 * (nTime2 - nTime1), nTimeMined * 0.000001); + LogPrint(BCLog::BENCHMARK, " - GetQcHashes: %.2fms [%.2fs]\n", 0.001 * (nTime2 - nTime1), nTimeMined * 0.000001); // now add the commitments from the current block, which are not returned by GetMinedAndActiveCommitmentsUntilBlock // due to the use of pindexPrev (we don't have the tip index here) diff --git a/src/llmq/blockprocessor.cpp b/src/llmq/blockprocessor.cpp index a4793f0df3d8..c901bfbcc3ab 100644 --- a/src/llmq/blockprocessor.cpp +++ b/src/llmq/blockprocessor.cpp @@ -367,6 +367,11 @@ bool CQuorumBlockProcessor::ProcessCommitment(int nHeight, const uint256& blockH m_evoDb.Write(BuildInversedHeightKey(llmq_params.type, nHeight), pQuorumBaseBlockIndex->nHeight); } + // Only once this commitment's state change is complete, so the caches can never be + // repopulated from a half-updated view. Callers all hold cs_main today, but the + // invalidation should not depend on that. + DropQcHashesCache(); + { LOCK(minableCommitmentsCs); mapHasMinedCommitmentCache[qc.llmqType].erase(qc.quorumHash); @@ -380,6 +385,68 @@ bool CQuorumBlockProcessor::ProcessCommitment(int nHeight, const uint256& blockH return true; } +void CQuorumBlockProcessor::DropQcHashesCache() +{ + LOCK(m_qc_hashes_cache_mutex); + m_quorums_cached.clear(); + m_qc_hashes_cached.clear(); + m_qc_indexed_hashes_cached.clear(); + // Clear per-type LRU contents but keep the map entries so InitQuorumsCache is not + // required on every subsequent miss. + for (auto& [_, cache] : m_qc_hashes_lru) { + cache.clear(); + } +} + +std::optional> CQuorumBlockProcessor::GetQcHashes(const CBlockIndex* pindexPrev) const +{ + auto quorums = GetMinedAndActiveCommitmentsUntilBlock(pindexPrev); + + LOCK(m_qc_hashes_cache_mutex); + if (quorums == m_quorums_cached) { + return std::make_pair(m_qc_hashes_cached, m_qc_indexed_hashes_cached); + } + + // Quorums set is different, reset cached values + m_quorums_cached.clear(); + m_qc_hashes_cached.clear(); + m_qc_indexed_hashes_cached.clear(); + if (m_qc_hashes_lru.empty()) { + utils::InitQuorumsCache(m_qc_hashes_lru, Params().GetConsensus()); + } + + for (const auto& [llmqType, vecBlockIndexes] : quorums) { + const auto& llmq_params_opt = Params().GetLLMQ(llmqType); + assert(llmq_params_opt.has_value()); + bool rotation_enabled = IsQuorumRotationEnabled(llmq_params_opt.value(), pindexPrev); + auto& vec_hashes = m_qc_hashes_cached[llmqType]; + vec_hashes.reserve(vecBlockIndexes.size()); + auto& map_indexed_hashes = m_qc_indexed_hashes_cached[llmqType]; + for (const auto& blockIndex : vecBlockIndexes) { + uint256 block_hash{blockIndex->GetBlockHash()}; + + std::pair qc_hash; + if (!m_qc_hashes_lru[llmqType].get(block_hash, qc_hash)) { + auto [pqc, dummy_hash] = 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; + m_qc_hashes_lru[llmqType].insert(block_hash, qc_hash); + } + if (rotation_enabled) { + map_indexed_hashes[qc_hash.second] = qc_hash.first; + } else { + vec_hashes.emplace_back(qc_hash.first); + } + } + } + std::swap(m_quorums_cached, quorums); + return std::make_pair(m_qc_hashes_cached, m_qc_indexed_hashes_cached); +} + bool CQuorumBlockProcessor::UndoBlock(const CBlock& block, gsl::not_null pindex) { AssertLockHeld(::cs_main); @@ -408,6 +475,9 @@ bool CQuorumBlockProcessor::UndoBlock(const CBlock& block, gsl::not_nullnHeight)); } + // Only once this commitment's state change is complete; see ProcessCommitment. + DropQcHashesCache(); + WITH_LOCK(minableCommitmentsCs, mapHasMinedCommitmentCache[qc.llmqType].erase(qc.quorumHash)); // if a reorg happened, we should allow to mine this commitment later diff --git a/src/llmq/blockprocessor.h b/src/llmq/blockprocessor.h index 4cf2598b3f44..6aa82bad75ce 100644 --- a/src/llmq/blockprocessor.h +++ b/src/llmq/blockprocessor.h @@ -38,6 +38,11 @@ namespace llmq class CFinalCommitment; class CQuorumSnapshotManager; +//! Serialized hashes of the commitments mined for the active quorums, by LLMQ type. +using QcHashMap = std::map>; +//! As above, but keyed by quorumIndex, for rotation-enabled types. +using QcIndexedHashMap = std::map>; + class CQuorumBlockProcessor { private: @@ -54,6 +59,18 @@ class CQuorumBlockProcessor mutable std::map> mapHasMinedCommitmentCache GUARDED_BY(minableCommitmentsCs); + // Memoizes GetQcHashes(). The whole-result cache is keyed on the set of active + // quorum base blocks, the LRU on those base-block hashes; neither key identifies + // which CFinalCommitment was mined for a base, so both are dropped whenever mined + // commitment state changes (see DropQcHashesCache). Owning them here keeps that + // invalidation next to the writes it has to follow, and ties their lifetime to the + // block index whose CBlockIndex* the outer cache stores. + mutable Mutex m_qc_hashes_cache_mutex; + mutable std::map> m_quorums_cached GUARDED_BY(m_qc_hashes_cache_mutex); + mutable std::map>> m_qc_hashes_lru GUARDED_BY(m_qc_hashes_cache_mutex); + mutable QcHashMap m_qc_hashes_cached GUARDED_BY(m_qc_hashes_cache_mutex); + mutable QcIndexedHashMap m_qc_indexed_hashes_cached GUARDED_BY(m_qc_hashes_cache_mutex); + public: CQuorumBlockProcessor() = delete; CQuorumBlockProcessor(const CQuorumBlockProcessor&) = delete; @@ -66,9 +83,9 @@ class CQuorumBlockProcessor EXCLUSIVE_LOCKS_REQUIRED(!minableCommitmentsCs); bool ProcessBlock(const CBlock& block, gsl::not_null pindex, BlockValidationState& state, - bool fJustCheck, bool fBLSChecks) EXCLUSIVE_LOCKS_REQUIRED(::cs_main, !minableCommitmentsCs); + bool fJustCheck, bool fBLSChecks) EXCLUSIVE_LOCKS_REQUIRED(::cs_main, !minableCommitmentsCs, !m_qc_hashes_cache_mutex); bool UndoBlock(const CBlock& block, gsl::not_null pindex) - EXCLUSIVE_LOCKS_REQUIRED(::cs_main, !minableCommitmentsCs); + EXCLUSIVE_LOCKS_REQUIRED(::cs_main, !minableCommitmentsCs, !m_qc_hashes_cache_mutex); //! it returns hash of commitment if it should be relay, otherwise nullopt std::optional AddMineableCommitment(const CFinalCommitment& fqc) EXCLUSIVE_LOCKS_REQUIRED(!minableCommitmentsCs); @@ -84,6 +101,15 @@ class CQuorumBlockProcessor EXCLUSIVE_LOCKS_REQUIRED(!minableCommitmentsCs); std::pair GetMinedCommitment(Consensus::LLMQType llmqType, const uint256& quorumHash) const; + /** + * Serialized hashes of the commitments mined for the quorums active as of pindexPrev. + * + * Memoized; returns nullopt if a commitment recorded as mined could not be read back, + * which should never happen. + */ + std::optional> GetQcHashes(const CBlockIndex* pindexPrev) const + EXCLUSIVE_LOCKS_REQUIRED(!m_qc_hashes_cache_mutex); + std::vector GetMinedCommitmentsUntilBlock(Consensus::LLMQType llmqType, gsl::not_null pindex, size_t maxCount) const; std::map> GetMinedAndActiveCommitmentsUntilBlock(gsl::not_null pindex) const; @@ -93,9 +119,12 @@ class CQuorumBlockProcessor size_t cycle) const; std::optional GetLastMinedCommitmentsByQuorumIndexUntilBlock(Consensus::LLMQType llmqType, const CBlockIndex* pindex, int quorumIndex, size_t cycle) const; private: + //! Called from every site that writes or erases mined commitment state. + void DropQcHashesCache() EXCLUSIVE_LOCKS_REQUIRED(!m_qc_hashes_cache_mutex); + static bool GetCommitmentsFromBlock(const CBlock& block, gsl::not_null pindex, std::multimap& ret, BlockValidationState& state) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); bool ProcessCommitment(int nHeight, const uint256& blockHash, const CFinalCommitment& qc, BlockValidationState& state, - bool fJustCheck) EXCLUSIVE_LOCKS_REQUIRED(::cs_main, !minableCommitmentsCs); + bool fJustCheck) EXCLUSIVE_LOCKS_REQUIRED(::cs_main, !minableCommitmentsCs, !m_qc_hashes_cache_mutex); size_t GetNumCommitmentsRequired(const Consensus::LLMQParams& llmqParams, int nHeight) const EXCLUSIVE_LOCKS_REQUIRED(::cs_main, !minableCommitmentsCs); static uint256 GetQuorumBlockHash(const Consensus::LLMQParams& llmqParams, const CChain& active_chain, int nHeight, int quorumIndex) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); From 01f012a90bb53583a0e8ca3d8cc6d54256816c9a Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Mon, 27 Jul 2026 12:19:24 +0300 Subject: [PATCH 2/2] test: cover post-undo qc-hash cache visibility 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 --- src/test/evo_cbtx_tests.cpp | 153 +++++++++++++++++++++++++++++++++++- 1 file changed, 152 insertions(+), 1 deletion(-) diff --git a/src/test/evo_cbtx_tests.cpp b/src/test/evo_cbtx_tests.cpp index f7f33c823bfd..2f37ccf18f08 100644 --- a/src/test/evo_cbtx_tests.cpp +++ b/src/test/evo_cbtx_tests.cpp @@ -2,25 +2,42 @@ // 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 +#include +#include #include +using namespace llmq; +using namespace llmq::testutils; + BOOST_AUTO_TEST_SUITE(evo_cbtx_tests) // Out-of-range bestCLHeightDiff (>= pindex->nHeight) must be rejected with @@ -67,4 +84,138 @@ BOOST_FIXTURE_TEST_CASE(check_cbtx_best_chainlock_rejects_excessive_height_diff, BOOST_CHECK_EQUAL(state_big.GetRejectReason(), "bad-cbtx-cldiff"); } +namespace { +// Mirrors private DB keys in llmq/blockprocessor.cpp so tests can install +// mined-commitment state without a full DKG/mining path. +static const std::string DB_MINED_COMMITMENT = "q_mc"; +static const std::string DB_MINED_COMMITMENT_BY_INVERSED_HEIGHT = "q_mcih"; + +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)); +} + +// Store a mined commitment as if it was mined at `mined_height` for the genesis +// quorum base (quorumHeight 0). GetMinedCommitmentsUntilBlock iterates inverted- +// height keys in [pindex->nHeight, 0), so scan height must be >= mined_height +// and mined_height must be > 0 for the entry to be returned. +void WriteMinedCommitment(CEvoDB& evoDb, const CFinalCommitment& qc, const uint256& mined_block_hash, int mined_height) +{ + assert(mined_height > 0); + evoDb.Write(std::make_pair(DB_MINED_COMMITMENT, std::make_pair(qc.llmqType, qc.quorumHash)), + std::make_pair(qc, mined_block_hash)); + evoDb.Write(BuildInversedHeightKey(qc.llmqType, mined_height), /*quorumHeight=*/0); +} + +CTransactionRef MakeCommitmentTx(const CFinalCommitment& qc, int height) +{ + CFinalCommitmentTxPayload payload; + payload.nHeight = height; + payload.commitment = qc; + + CMutableTransaction tx; + tx.nVersion = 3; + tx.nType = TRANSACTION_QUORUM_COMMITMENT; + SetTxPayload(tx, payload); + return MakeTransactionRef(std::move(tx)); +} + +uint256 CalcQuorumMerkleRootForCommitment(const CFinalCommitment& qc) +{ + std::vector hashes{::SerializeHash(qc)}; + bool mutated{false}; + return ComputeMerkleRoot(hashes, &mutated); +} + +CFinalCommitment MakeDistinctCommitment(const Consensus::LLMQParams& params, const uint256& quorum_hash, uint8_t salt) +{ + CFinalCommitment qc = CreateValidCommitment(params, quorum_hash); + // Force a deterministic difference even if random BLS material collides. + qc.quorumVvecHash = uint256{std::vector(32, salt)}; + return qc; +} + +CBlock MakeEmptyBlock() +{ + CBlock block; + block.vtx.emplace_back(MakeTransactionRef(CMutableTransaction{})); + return block; +} + +void ExpectQuorumMerkleRoot(const CBlock& block, const CBlockIndex* pindex, const CQuorumBlockProcessor& qblockman, + const CFinalCommitment& qc) +{ + uint256 merkle_root; + BlockValidationState state; + BOOST_REQUIRE(CalcCbTxMerkleRootQuorums(block, pindex, qblockman, merkle_root, state)); + BOOST_CHECK_EQUAL(merkle_root.ToString(), CalcQuorumMerkleRootForCommitment(qc).ToString()); +} + +const CBlockIndex* GenesisIndex(const node::NodeContext& node) +{ + LOCK(cs_main); + return node.chainman->ActiveChain()[0]; +} +} // anonymous namespace + +// Activate DIP0003 immediately so GetCommitmentsFromBlock accepts the payload +// at a low height without a long fake chain. +struct Dip3ActiveSetup : public RegTestingSetup { + Dip3ActiveSetup() : + RegTestingSetup({"-dip3params=1:1"}) + { + } +}; + +// End to end: disconnecting the block that mined a commitment must make a replacement +// commitment for the same quorum base visible, even though the active base-block list +// that keys the caches is unchanged across the swap. +BOOST_FIXTURE_TEST_CASE(qc_hash_cache_invalidated_by_undoblock, Dip3ActiveSetup) +{ + auto& evoDb = *Assert(m_node.evodb); + auto& qblockman = *Assert(m_node.llmq_ctx)->quorum_block_processor; + const auto& params = GetLLMQParams(Consensus::LLMQType::LLMQ_TEST); + + const CBlockIndex* pindex_genesis = GenesisIndex(m_node); + BOOST_REQUIRE(pindex_genesis != nullptr); + const uint256 quorum_hash = pindex_genesis->GetBlockHash(); + + const CFinalCommitment qc_a = MakeDistinctCommitment(params, quorum_hash, /*salt=*/0x33); + const CFinalCommitment qc_b = MakeDistinctCommitment(params, quorum_hash, /*salt=*/0x44); + BOOST_REQUIRE(::SerializeHash(qc_a) != ::SerializeHash(qc_b)); + + const uint256 mined_hash_a = GetTestBlockHash(11); + const uint256 mined_hash_b = GetTestBlockHash(12); + constexpr int mined_height = 1; + + { + auto dbTx = evoDb.BeginTransaction(); + WriteMinedCommitment(evoDb, qc_a, mined_hash_a, mined_height); + dbTx->Commit(); + } + + CBlockIndex pindex_mined; + pindex_mined.nHeight = mined_height; + pindex_mined.pprev = const_cast(pindex_genesis); + pindex_mined.phashBlock = &mined_hash_a; + + CBlock block_with_qc = MakeEmptyBlock(); + block_with_qc.vtx.emplace_back(MakeCommitmentTx(qc_a, mined_height)); + const CBlock empty_block = MakeEmptyBlock(); + + ExpectQuorumMerkleRoot(empty_block, &pindex_mined, qblockman, qc_a); + + { + LOCK(cs_main); + auto dbTx = evoDb.BeginTransaction(); + BOOST_REQUIRE(qblockman.UndoBlock(block_with_qc, &pindex_mined)); + // Install the replacement while the disconnect transaction is still open. + WriteMinedCommitment(evoDb, qc_b, mined_hash_b, mined_height); + dbTx->Commit(); + } + + ExpectQuorumMerkleRoot(empty_block, &pindex_mined, qblockman, qc_b); +} + BOOST_AUTO_TEST_SUITE_END()