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 7691dbf19286..9e0025e254aa 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); } }; @@ -216,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 8ed6c16da5d2..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) @@ -245,6 +240,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 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"])