From acb611aacfcc95c04fcbf2c67d12bd4bdf91c2b7 Mon Sep 17 00:00:00 2001 From: PastaClaw Date: Tue, 21 Jul 2026 11:21:12 -0500 Subject: [PATCH] fix: invalidate quorum commitment cache on disconnect 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 --- src/Makefile.am | 1 + src/evo/cbtx.cpp | 68 +++++++---- src/evo/cbtx_cache.h | 17 +++ src/llmq/blockprocessor.cpp | 9 ++ src/test/evo_cbtx_tests.cpp | 220 +++++++++++++++++++++++++++++++++++- 5 files changed, 293 insertions(+), 22 deletions(-) create mode 100644 src/evo/cbtx_cache.h diff --git a/src/Makefile.am b/src/Makefile.am index 3395d9e602fb..a53e4a99742f 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -217,6 +217,7 @@ BITCOIN_CORE_H = \ dsnotificationinterface.h \ evo/assetlocktx.h \ evo/cbtx.h \ + evo/cbtx_cache.h \ evo/chainhelper.h \ evo/creditpool.h \ evo/deterministicmns.h \ diff --git a/src/evo/cbtx.cpp b/src/evo/cbtx.cpp index 4e1b7986ad6c..36810a9121d7 100644 --- a/src/evo/cbtx.cpp +++ b/src/evo/cbtx.cpp @@ -4,6 +4,8 @@ #include +#include + #include #include #include @@ -49,6 +51,35 @@ bool CheckCbTx(const CCbTx& cbTx, const CBlockIndex* pindexPrev, TxValidationSta using QcHashMap = std::map>; using QcIndexedHashMap = std::map>; +// Process-lifetime caches for CalcCbTxMerkleRootQuorums. +// +// The outer whole-result cache is keyed only by the set of active quorum *base* +// blocks. The inner LRU is keyed only by those base-block hashes. Neither key +// includes the serialized CFinalCommitment that was actually mined for that +// base on the active chain. Different valid branches can therefore mine +// different commitments for the same base list, so these caches must be dropped +// whenever mined commitment state is undone (see InvalidateCachedQcHashes). +namespace { +GlobalMutex g_qc_hashes_cache_mutex; +std::map> g_quorums_cached GUARDED_BY(g_qc_hashes_cache_mutex); +std::map>> g_qc_hashes_lru GUARDED_BY(g_qc_hashes_cache_mutex); +QcHashMap g_qcHashes_cached GUARDED_BY(g_qc_hashes_cache_mutex); +QcIndexedHashMap g_qcIndexedHashes_cached GUARDED_BY(g_qc_hashes_cache_mutex); +} // anonymous namespace + +void InvalidateCachedQcHashes() +{ + LOCK(g_qc_hashes_cache_mutex); + g_quorums_cached.clear(); + g_qcHashes_cached.clear(); + g_qcIndexedHashes_cached.clear(); + // Clear per-type LRU contents but keep the map entries so InitQuorumsCache is + // not required on every post-invalidation miss. + for (auto& [_, cache] : g_qc_hashes_lru) { + cache.clear(); + } +} + /** * 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. @@ -58,37 +89,32 @@ auto CachedGetQcHashesQcIndexedHashes(const CBlockIndex* pindexPrev, const llmq: 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); + LOCK(g_qc_hashes_cache_mutex); + if (quorums == g_quorums_cached) { + return std::make_pair(g_qcHashes_cached, g_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()); + // Quorums set changed: rebuild whole-result caches. Keep the per-base LRU; + // branch-dependent staleness is handled by InvalidateCachedQcHashes(). + g_quorums_cached.clear(); + g_qcHashes_cached.clear(); + g_qcIndexedHashes_cached.clear(); + if (g_qc_hashes_lru.empty()) { + llmq::utils::InitQuorumsCache(g_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 = llmq::IsQuorumRotationEnabled(llmq_params_opt.value(), pindexPrev); - auto& vec_hashes = qcHashes_cached[llmqType]; + auto& vec_hashes = g_qcHashes_cached[llmqType]; vec_hashes.reserve(vecBlockIndexes.size()); - auto& map_indexed_hashes = qcIndexedHashes_cached[llmqType]; + auto& map_indexed_hashes = g_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)) { + if (!g_qc_hashes_lru[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 @@ -96,7 +122,7 @@ auto CachedGetQcHashesQcIndexedHashes(const CBlockIndex* pindexPrev, const llmq: } qc_hash.first = ::SerializeHash(pqc); qc_hash.second = rotation_enabled ? pqc.quorumIndex : 0; - qc_hashes_cached[llmqType].insert(block_hash, qc_hash); + g_qc_hashes_lru[llmqType].insert(block_hash, qc_hash); } if (rotation_enabled) { map_indexed_hashes[qc_hash.second] = qc_hash.first; @@ -105,8 +131,8 @@ auto CachedGetQcHashesQcIndexedHashes(const CBlockIndex* pindexPrev, const llmq: } } } - std::swap(quorums_cached, quorums); - return std::make_pair(qcHashes_cached, qcIndexedHashes_cached); + std::swap(g_quorums_cached, quorums); + return std::make_pair(g_qcHashes_cached, g_qcIndexedHashes_cached); } auto CalcHashCountFromQCHashes(const QcHashMap& qcHashes) diff --git a/src/evo/cbtx_cache.h b/src/evo/cbtx_cache.h new file mode 100644 index 000000000000..9d6143a00d3b --- /dev/null +++ b/src/evo/cbtx_cache.h @@ -0,0 +1,17 @@ +// Copyright (c) 2026 The Dash Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#ifndef BITCOIN_EVO_CBTX_CACHE_H +#define BITCOIN_EVO_CBTX_CACHE_H + +/** + * Drop process-lifetime CbTx quorum-commitment hash caches. + * + * Required when mined commitment data for a quorum base block can change without the + * active base-block list changing (e.g. disconnect/reorg of the block that mined a + * different valid CFinalCommitment for the same quorumHash). + */ +void InvalidateCachedQcHashes(); + +#endif // BITCOIN_EVO_CBTX_CACHE_H diff --git a/src/llmq/blockprocessor.cpp b/src/llmq/blockprocessor.cpp index a4793f0df3d8..8d7b035db61c 100644 --- a/src/llmq/blockprocessor.cpp +++ b/src/llmq/blockprocessor.cpp @@ -4,6 +4,7 @@ #include +#include #include #include #include @@ -391,12 +392,14 @@ bool CQuorumBlockProcessor::UndoBlock(const CBlock& block, gsl::not_nullpprev->GetBlockHash()); return true; diff --git a/src/test/evo_cbtx_tests.cpp b/src/test/evo_cbtx_tests.cpp index f7f33c823bfd..7652960c860d 100644 --- a/src/test/evo_cbtx_tests.cpp +++ b/src/test/evo_cbtx_tests.cpp @@ -2,25 +2,43 @@ // 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 #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 +85,204 @@ 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]; +} + +struct QcHashCacheCleanupGuard { + ~QcHashCacheCleanupGuard() { InvalidateCachedQcHashes(); } +}; +} // anonymous namespace + +// Outer cache keys on active quorum base blocks; inner LRU keys on base hashes. +// Neither includes the mined CFinalCommitment, so without InvalidateCachedQcHashes +// a stale hash survives an evoDb commitment swap for the same base list. +BOOST_FIXTURE_TEST_CASE(qc_hash_cache_invalidated_on_commitment_branch_change, RegTestingSetup) +{ + InvalidateCachedQcHashes(); + const QcHashCacheCleanupGuard cache_cleanup; + + 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=*/0x11); + const CFinalCommitment qc_b = MakeDistinctCommitment(params, quorum_hash, /*salt=*/0x22); + BOOST_REQUIRE(::SerializeHash(qc_a) != ::SerializeHash(qc_b)); + + const uint256 mined_hash_a = GetTestBlockHash(1); + const uint256 mined_hash_b = GetTestBlockHash(2); + const uint256 scan_hash = GetTestBlockHash(3); + constexpr int mined_height = 1; + + CBlockIndex pindex_scan; + pindex_scan.nHeight = mined_height; + pindex_scan.pprev = const_cast(pindex_genesis); + pindex_scan.phashBlock = &scan_hash; + + { + auto dbTx = evoDb.BeginTransaction(); + WriteMinedCommitment(evoDb, qc_a, mined_hash_a, mined_height); + dbTx->Commit(); + } + + const CBlock block = MakeEmptyBlock(); + ExpectQuorumMerkleRoot(block, &pindex_scan, qblockman, qc_a); + + // Swap evoDb to commitment B without changing the active base-block list. + { + auto dbTx = evoDb.BeginTransaction(); + WriteMinedCommitment(evoDb, qc_b, mined_hash_b, mined_height); + dbTx->Commit(); + } + + // Without invalidation, both cache layers still serve commitment A. + { + uint256 merkle_root; + BlockValidationState state; + BOOST_REQUIRE(CalcCbTxMerkleRootQuorums(block, &pindex_scan, qblockman, merkle_root, state)); + BOOST_CHECK_EQUAL(merkle_root.ToString(), CalcQuorumMerkleRootForCommitment(qc_a).ToString()); + BOOST_CHECK(merkle_root != CalcQuorumMerkleRootForCommitment(qc_b)); + } + + InvalidateCachedQcHashes(); + ExpectQuorumMerkleRoot(block, &pindex_scan, qblockman, qc_b); +} + +// UndoBlock must invalidate the process-lifetime caches so a replacement +// commitment is observed after disconnect. +// +// 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"}) + { + } +}; + +BOOST_FIXTURE_TEST_CASE(qc_hash_cache_invalidated_by_undoblock, Dip3ActiveSetup) +{ + InvalidateCachedQcHashes(); + const QcHashCacheCleanupGuard cache_cleanup; + + 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()