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); 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()