From ad446c8b49696c70300f6f9411f7e369f7ebf5c5 Mon Sep 17 00:00:00 2001 From: pasta Date: Thu, 20 Aug 2026 11:04:09 -0500 Subject: [PATCH 1/2] fix(net): bound getqrinfo baseBlockHashes on deserialization CGetQuorumRotationInfo::baseBlockHashes had no size limit, so the wire format accepted MAX_PROTOCOL_MESSAGE_LENGTH / sizeof(uint256) = 98304 entries from an unauthenticated peer in a single 3 MiB message. BuildQuorumRotationInfo() then walks that list once per constructed CSimplifiedMNListDiff while holding cs_main. Route it through LIMITED_VECTOR with a 4096 cap. A client can only usefully hold the bases a response hands it, and the server appends at most 3 * signingActiveQuorumCount snapshot bases plus the target cycles and the tip (~101 for llmq_60_75) per response, so the limit is roughly 40x real usage. LimitedVectorFormatter emits the ordinary vector wire format, so senders are unaffected. --- src/llmq/snapshot.h | 14 +++++++++++++- src/test/llmq_snapshot_tests.cpp | 28 ++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 1 deletion(-) diff --git a/src/llmq/snapshot.h b/src/llmq/snapshot.h index 7691dbf19286..721ebf5ea240 100644 --- a/src/llmq/snapshot.h +++ b/src/llmq/snapshot.h @@ -89,6 +89,18 @@ class CQuorumSnapshot [[nodiscard]] UniValue ToJson() const; }; +/** Upper bound on the diff bases a peer may attach to a GETQUORUMROTATIONINFO request. + * + * A response only ever references a bounded set of work indexes: BuildQuorumRotationInfo + * itself appends at most 3 * signingActiveQuorumCount snapshot bases (96 for llmq_60_75) + * plus the target cycles and the tip, so even a client that echoes back everything it has + * ever learned stays in the low hundreds. Without a limit the wire format allows + * MAX_PROTOCOL_MESSAGE_LENGTH / sizeof(uint256) = 98304 entries, which an unauthenticated + * peer can use to make the server walk that list once per constructed diff while holding + * cs_main. The limit is deliberately generous relative to real usage; it only has to keep + * the list small enough that traversing it is free. */ +static constexpr size_t MAX_BASE_BLOCK_HASHES{4096}; + class CGetQuorumRotationInfo { public: @@ -98,7 +110,7 @@ class CGetQuorumRotationInfo SERIALIZE_METHODS(CGetQuorumRotationInfo, obj) { - READWRITE(obj.baseBlockHashes, obj.blockRequestHash, obj.extraShare); + READWRITE(LIMITED_VECTOR(obj.baseBlockHashes, MAX_BASE_BLOCK_HASHES), obj.blockRequestHash, obj.extraShare); } }; diff --git a/src/test/llmq_snapshot_tests.cpp b/src/test/llmq_snapshot_tests.cpp index 8ed6c16da5d2..2487f93dac8c 100644 --- a/src/test/llmq_snapshot_tests.cpp +++ b/src/test/llmq_snapshot_tests.cpp @@ -245,6 +245,34 @@ BOOST_AUTO_TEST_CASE(get_quorum_rotation_info_serialization_test) BOOST_CHECK(TestSerializationRoundtrip(emptyInfo)); } +BOOST_AUTO_TEST_CASE(get_quorum_rotation_info_base_block_hashes_limit_test) +{ + const auto serialize_with_count{[](size_t count) { + CGetQuorumRotationInfo info; + info.blockRequestHash = GetTestBlockHash(100); + info.baseBlockHashes.assign(count, GetTestBlockHash(1)); + CDataStream ss(SER_NETWORK, PROTOCOL_VERSION); + ss << info; + return ss; + }}; + + // A request at the limit still round-trips. + { + CDataStream ss{serialize_with_count(MAX_BASE_BLOCK_HASHES)}; + CGetQuorumRotationInfo parsed; + BOOST_CHECK_NO_THROW(ss >> parsed); + BOOST_CHECK_EQUAL(parsed.baseBlockHashes.size(), MAX_BASE_BLOCK_HASHES); + } + + // One past it is rejected before any element is decoded, so an unauthenticated peer + // cannot make the server hold a list sized by MAX_PROTOCOL_MESSAGE_LENGTH. + { + CDataStream ss{serialize_with_count(MAX_BASE_BLOCK_HASHES + 1)}; + CGetQuorumRotationInfo parsed; + BOOST_CHECK_THROW(ss >> parsed, std::ios_base::failure); + } +} + BOOST_AUTO_TEST_CASE(quorum_rotation_info_serialization_test) { // Note: mnListDiff{smth} testing requires proper CSimplifiedMNListDiff setup From 282a149bccdc877dba4667adfae8de73cbb6452e Mon Sep 17 00:00:00 2001 From: pasta Date: Thu, 20 Aug 2026 11:04:32 -0500 Subject: [PATCH 2/2] fix(net): stop re-sorting the getqrinfo base block list on every lookup On the non-legacy construction path GetLastBaseBlockHash() sorted the whole base list on each call, and BuildQuorumRotationInfo() calls it once per constructed CSimplifiedMNListDiff - roughly 10-30 times per request for llmq_60_75. Each comparison dereferences a CBlockIndex*, so the cost was k * n log n cache-hostile work under cs_main with n chosen by the requesting peer. Make the ordering an input precondition instead: the two mid-construction append sites now insert at std::upper_bound via InsertBaseBlockSorted() rather than push_back, so the list stays sorted at O(n) pointer moves. GetLastBaseBlockHash() drops both the sort and the now-unused use_legacy_construction parameter, and takes Span so it can no longer mutate the caller's list. Output is unchanged. The legacy path never sorted inside the getter and never appends. On the non-legacy path the list was already sorted and deduplicated before the first call and every append was followed by a re-sort, so inserting in position yields the same sequence; equal heights can only be the same active-chain block, so ties resolve identically. --- src/llmq/snapshot.cpp | 42 +++++++++++++----------- src/llmq/snapshot.h | 6 ++-- src/test/llmq_snapshot_tests.cpp | 31 ++++++++--------- test/functional/feature_llmq_rotation.py | 10 ++++++ 4 files changed, 49 insertions(+), 40 deletions(-) diff --git a/src/llmq/snapshot.cpp b/src/llmq/snapshot.cpp index 51d3a44007b8..c8380bfc1adf 100644 --- a/src/llmq/snapshot.cpp +++ b/src/llmq/snapshot.cpp @@ -60,6 +60,18 @@ std::optional ConstructCycle(llmq::CQuorumSnapshotManager& qsna } return ret; } + +//! Add a base block, keeping the by-height ordering GetLastBaseBlockHash() depends on. +//! Inserting in place is O(n) pointer moves; re-sorting after every append would be +//! O(n log n) block-index dereferences on a list whose size the requesting peer chooses. +void InsertBaseBlockSorted(std::vector& baseBlockIndexes, const CBlockIndex* blockIndex) +{ + const auto pos{std::upper_bound(baseBlockIndexes.begin(), baseBlockIndexes.end(), blockIndex, + [](const CBlockIndex* a, const CBlockIndex* b) { + return a->nHeight < b->nHeight; + })}; + baseBlockIndexes.insert(pos, blockIndex); +} } // anonymous namespace namespace llmq { @@ -147,8 +159,7 @@ bool BuildQuorumRotationInfo(CDeterministicMNManager& dmnman, CQuorumSnapshotMan if (use_legacy_construction) { // Build MN list Diff always with highest baseblock if (!BuildSimplifiedMNListDiff(dmnman, chainman, qblockman, qman, - GetLastBaseBlockHash(baseBlockIndexes, cycle_base_opt->m_work_index, - use_legacy_construction), + GetLastBaseBlockHash(baseBlockIndexes, cycle_base_opt->m_work_index), cycle_base_opt->m_work_index->GetBlockHash(), response.mnListDiffH, errorRet)) { return false; } @@ -167,8 +178,7 @@ bool BuildQuorumRotationInfo(CDeterministicMNManager& dmnman, CQuorumSnapshotMan } if (use_legacy_construction) { if (!BuildSimplifiedMNListDiff(dmnman, chainman, qblockman, qman, - GetLastBaseBlockHash(baseBlockIndexes, cycle_opt->m_work_index, - use_legacy_construction), + GetLastBaseBlockHash(baseBlockIndexes, cycle_opt->m_work_index), cycle_opt->m_work_index->GetBlockHash(), cycle_opt->m_diff, errorRet)) { return false; } @@ -204,13 +214,12 @@ bool BuildQuorumRotationInfo(CDeterministicMNManager& dmnman, CQuorumSnapshotMan response.quorumSnapshotList.push_back(cycle_opt->m_snap); CSimplifiedMNListDiff mnhneeded; if (!BuildSimplifiedMNListDiff(dmnman, chainman, qblockman, qman, - GetLastBaseBlockHash(baseBlockIndexes, cycle_opt->m_work_index, - use_legacy_construction), + GetLastBaseBlockHash(baseBlockIndexes, cycle_opt->m_work_index), cycle_opt->m_work_index->GetBlockHash(), mnhneeded, errorRet)) { return false; } if (!use_legacy_construction) { - baseBlockIndexes.push_back(cycle_opt->m_work_index); + InsertBaseBlockSorted(baseBlockIndexes, cycle_opt->m_work_index); } response.mnListDiffList.push_back(mnhneeded); } @@ -219,24 +228,22 @@ bool BuildQuorumRotationInfo(CDeterministicMNManager& dmnman, CQuorumSnapshotMan for (size_t n = target_cycles.size(); n > 0; --n) { auto* cycle{target_cycles[n - 1]}; if (!BuildSimplifiedMNListDiff(dmnman, chainman, qblockman, qman, - GetLastBaseBlockHash(baseBlockIndexes, cycle->m_work_index, - use_legacy_construction), + GetLastBaseBlockHash(baseBlockIndexes, cycle->m_work_index), cycle->m_work_index->GetBlockHash(), cycle->m_diff, errorRet)) { return false; } - baseBlockIndexes.push_back(cycle->m_work_index); + InsertBaseBlockSorted(baseBlockIndexes, cycle->m_work_index); } if (!BuildSimplifiedMNListDiff(dmnman, chainman, qblockman, qman, - GetLastBaseBlockHash(baseBlockIndexes, cycle_base_opt->m_work_index, - use_legacy_construction), + GetLastBaseBlockHash(baseBlockIndexes, cycle_base_opt->m_work_index), cycle_base_opt->m_work_index->GetBlockHash(), response.mnListDiffH, errorRet)) { return false; } - baseBlockIndexes.push_back(cycle_base_opt->m_work_index); + InsertBaseBlockSorted(baseBlockIndexes, cycle_base_opt->m_work_index); if (!BuildSimplifiedMNListDiff(dmnman, chainman, qblockman, qman, - GetLastBaseBlockHash(baseBlockIndexes, tipBlockIndex, use_legacy_construction), + GetLastBaseBlockHash(baseBlockIndexes, tipBlockIndex), tipBlockIndex->GetBlockHash(), response.mnListDiffTip, errorRet)) { return false; } @@ -244,13 +251,8 @@ bool BuildQuorumRotationInfo(CDeterministicMNManager& dmnman, CQuorumSnapshotMan return true; } -uint256 GetLastBaseBlockHash(Span baseBlockIndexes, const CBlockIndex* blockIndex, - bool use_legacy_construction) +uint256 GetLastBaseBlockHash(Span baseBlockIndexes, const CBlockIndex* blockIndex) { - if (!use_legacy_construction) { - std::sort(baseBlockIndexes.begin(), baseBlockIndexes.end(), - [](const CBlockIndex* a, const CBlockIndex* b) { return a->nHeight < b->nHeight; }); - } // default to genesis block uint256 hash{Params().GenesisBlock().GetHash()}; for (const auto baseBlock : baseBlockIndexes) { diff --git a/src/llmq/snapshot.h b/src/llmq/snapshot.h index 721ebf5ea240..9e0025e254aa 100644 --- a/src/llmq/snapshot.h +++ b/src/llmq/snapshot.h @@ -228,8 +228,10 @@ bool BuildQuorumRotationInfo(CDeterministicMNManager& dmnman, CQuorumSnapshotMan const CQuorumBlockProcessor& qblockman, const CGetQuorumRotationInfo& request, bool use_legacy_construction, CQuorumRotationInfo& response, std::string& errorRet) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); -uint256 GetLastBaseBlockHash(Span baseBlockIndexes, const CBlockIndex* blockIndex, - bool use_legacy_construction); +//! Highest base block at or below blockIndex, or the genesis hash if there is none. +//! baseBlockIndexes must already be sorted by height: the list is sized by the requesting +//! peer, so sorting it here (once per constructed diff) would be attacker-controlled work. +uint256 GetLastBaseBlockHash(Span baseBlockIndexes, const CBlockIndex* blockIndex); class CQuorumSnapshotManager { diff --git a/src/test/llmq_snapshot_tests.cpp b/src/test/llmq_snapshot_tests.cpp index 2487f93dac8c..45bef288c2f4 100644 --- a/src/test/llmq_snapshot_tests.cpp +++ b/src/test/llmq_snapshot_tests.cpp @@ -6,6 +6,7 @@ #include #include +#include #include #include @@ -191,17 +192,7 @@ BOOST_AUTO_TEST_CASE(get_last_base_block_hash_repeated_base_blocks_test) blocks[i].phashBlock = &hashes[i]; } - // Non-legacy: sorts internally, so unsorted input with duplicates is fine. - std::vector unsorted_repeated_base_blocks{ - &blocks[2], - &blocks[0], - &blocks[1], - &blocks[1], - }; - BOOST_CHECK(GetLastBaseBlockHash(unsorted_repeated_base_blocks, &blocks[3], false) == hashes[2]); - BOOST_CHECK(GetLastBaseBlockHash(unsorted_repeated_base_blocks, &blocks[1], false) == hashes[1]); - - // Legacy: relies on caller-supplied sort and tolerates duplicates as a no-op. + // GetLastBaseBlockHash() requires sorted input and tolerates duplicates as a no-op. // BuildQuorumRotationInfo deliberately does NOT deduplicate in the legacy path so // the wire response to older peers stays bit-for-bit identical; these checks // demonstrate that the duplicate is harmless to GetLastBaseBlockHash's output. @@ -216,13 +207,17 @@ BOOST_AUTO_TEST_CASE(get_last_base_block_hash_repeated_base_blocks_test) &blocks[1], &blocks[2], }; - BOOST_CHECK(GetLastBaseBlockHash(sorted_repeated_base_blocks, &blocks[3], true) == hashes[2]); - BOOST_CHECK(GetLastBaseBlockHash(sorted_repeated_base_blocks, &blocks[1], true) == hashes[1]); - // Legacy no-op proof: duplicate vs unique input produces the same hash. - BOOST_CHECK(GetLastBaseBlockHash(sorted_repeated_base_blocks, &blocks[3], true) == - GetLastBaseBlockHash(sorted_unique_base_blocks, &blocks[3], true)); - BOOST_CHECK(GetLastBaseBlockHash(sorted_repeated_base_blocks, &blocks[1], true) == - GetLastBaseBlockHash(sorted_unique_base_blocks, &blocks[1], true)); + BOOST_CHECK(GetLastBaseBlockHash(sorted_repeated_base_blocks, &blocks[3]) == hashes[2]); + BOOST_CHECK(GetLastBaseBlockHash(sorted_repeated_base_blocks, &blocks[1]) == hashes[1]); + // Duplicate vs unique input produces the same hash. + BOOST_CHECK(GetLastBaseBlockHash(sorted_repeated_base_blocks, &blocks[3]) == + GetLastBaseBlockHash(sorted_unique_base_blocks, &blocks[3])); + BOOST_CHECK(GetLastBaseBlockHash(sorted_repeated_base_blocks, &blocks[1]) == + GetLastBaseBlockHash(sorted_unique_base_blocks, &blocks[1])); + + // No base at or below the target falls back to the genesis hash. + std::vector above_target{&blocks[2], &blocks[3]}; + BOOST_CHECK(GetLastBaseBlockHash(above_target, &blocks[0]) == Params().GenesisBlock().GetHash()); } BOOST_AUTO_TEST_CASE(get_quorum_rotation_info_serialization_test) diff --git a/test/functional/feature_llmq_rotation.py b/test/functional/feature_llmq_rotation.py index 76b72a6238fe..eac5f6929dd4 100755 --- a/test/functional/feature_llmq_rotation.py +++ b/test/functional/feature_llmq_rotation.py @@ -238,6 +238,16 @@ def run_test(self): rpc_qr_info_repeated_base = self.nodes[0].quorum("rotationinfo", best_block_hash, False, [hmc_base_blockhash, hmc_base_blockhash]) assert_equal(rpc_qr_info_repeated_base, rpc_qr_info) + # Base blocks are ordered by height server-side, so the request order must not matter. + early_base_blockhash = self.nodes[0].getblockhash(1) + rpc_qr_info_two_bases = self.nodes[0].quorum("rotationinfo", best_block_hash, False, + [hmc_base_blockhash, early_base_blockhash]) + assert_equal(rpc_qr_info_two_bases, + self.nodes[0].quorum("rotationinfo", best_block_hash, False, + [early_base_blockhash, hmc_base_blockhash])) + # ...and the extra base is not inert: it displaces the genesis fallback below H-3C. + assert_equal(rpc_qr_info["mnListDiffAtHMinus3C"]["baseBlockHash"], genesis_blockhash) + assert_equal(rpc_qr_info_two_bases["mnListDiffAtHMinus3C"]["baseBlockHash"], early_base_blockhash) assert_equal(rpc_qr_info["mnListDiffTip"]["blockHash"], best_block_hash) assert_equal(rpc_qr_info["mnListDiffTip"]["baseBlockHash"], rpc_qr_info["mnListDiffH"]["blockHash"]) assert_equal(rpc_qr_info["mnListDiffH"]["baseBlockHash"], rpc_qr_info["mnListDiffAtHMinusC"]["blockHash"])