Skip to content

fix(net): bound and stop re-sorting getqrinfo base block hashes #7629

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
PastaPastaPasta wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

fix(net): bound and stop re-sorting getqrinfo base block hashes #7629

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ad446c8
fix(net): bound getqrinfo baseBlockHashes on deserialization
PastaPastaPasta Aug 20, 2026
282a149
fix(net): stop re-sorting the getqrinfo base block list on every lookup
PastaPastaPasta Aug 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 22 additions & 20 deletions src/llmq/snapshot.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,18 @@ std::optional<llmq::CycleData> 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<const CBlockIndex*>& 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 {
Expand Down Expand Up @@ -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;
}
Expand All @@ -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;
}
Expand Down Expand Up @@ -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);
}
Expand All @@ -219,38 +228,31 @@ 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;
}
}
return true;
}

uint256 GetLastBaseBlockHash(Span<const CBlockIndex*> baseBlockIndexes, const CBlockIndex* blockIndex,
bool use_legacy_construction)
uint256 GetLastBaseBlockHash(Span<const CBlockIndex* const> 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) {
Expand Down
20 changes: 17 additions & 3 deletions src/llmq/snapshot.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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);
}
};

Expand Down Expand Up @@ -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<const CBlockIndex*> 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<const CBlockIndex* const> baseBlockIndexes, const CBlockIndex* blockIndex);

class CQuorumSnapshotManager
{
Expand Down
59 changes: 41 additions & 18 deletions src/test/llmq_snapshot_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#include <test/util/setup_common.h>

#include <chain.h>
#include <chainparams.h>
#include <streams.h>
#include <univalue.h>

Expand Down Expand Up @@ -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<const CBlockIndex*> 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.
Expand All @@ -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<const CBlockIndex*> 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)
Expand All @@ -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
Expand Down
10 changes: 10 additions & 0 deletions test/functional/feature_llmq_rotation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"])
Expand Down
Loading