Skip to content

fix: bound QGETDATA request tracking and reject requester-supplied nError #7519

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 1 commit into
base: develop
Choose a base branch
from
Open

fix: bound QGETDATA request tracking and reject requester-supplied nError #7519

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions src/Makefile.test.include
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,7 @@ BITCOIN_TESTS =\
test/llmq_commitment_tests.cpp \
test/llmq_hash_tests.cpp \
test/llmq_params_tests.cpp \
test/llmq_qgetdata_tests.cpp \
test/llmq_snapshot_tests.cpp \
test/llmq_utils_tests.cpp \
test/logging_tests.cpp \
Expand Down
46 changes: 36 additions & 10 deletions src/llmq/net_quorum.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,16 @@ void NetQuorum::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDataS
CQuorumDataRequest request;
vRecv >> request;

// nError is a response-only field on QDATA. Honest QGETDATA senders never
// emit it (serialization skips UNDEFINED). Accepting a requester-supplied
// value let an attacker pick QUORUM_VERIFICATION_VECTOR_MISSING /
// ENCRYPTED_CONTRIBUTIONS_MISSING and suppress the rate-limit ban while
// still forcing the expensive response construction path.
if (request.GetError() != CQuorumDataRequest::Errors::UNDEFINED) {
m_peer_manager->PeerMisbehaving(pfrom.GetId(), 10, "qgetdata with error field");
return;
}
Comment on lines +87 to +90

@PastaPastaPasta PastaPastaPasta Aug 3, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm; why only 10...?


auto sendQDATA = [&](CQuorumDataRequest::Errors nError,
bool request_limit_exceeded,
const CDataStream& body = CDataStream(SER_NETWORK, PROTOCOL_VERSION)) -> bool {
Expand All @@ -104,23 +114,38 @@ void NetQuorum::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDataS
return misbehave;
};

const CQuorumDataRequestKey key(pfrom.GetVerifiedProRegTxHash(), false, request.GetQuorumHash(), request.GetLLMQType());
const bool request_limit_exceeded = !m_qman.RegisterDataRequest(key, request, /*add_expiry_bias=*/false);

// Validate cheap, attacker-controlled fields before registering a tracking entry so
// garbage llmqType / unknown quorumHash values cannot grow mapQuorumDataRequests
// (and so rate-limit keys only cover requests that can reach the expensive path).
// Neither reply below is rate-limited or scored: they are reached before any
// tracking entry exists, so there is no limit state to report, and both are
// legitimate for an honest peer (an llmqType we do not know, or a block we have
// not synced yet). Cost is a ~93-byte reply to a ~93-byte request, so there is no
// amplification to gate.
if (!Params().GetLLMQ(request.GetLLMQType()).has_value()) {
if (sendQDATA(CQuorumDataRequest::Errors::QUORUM_TYPE_INVALID, request_limit_exceeded)) {
m_peer_manager->PeerMisbehaving(pfrom.GetId(), 25, "request limit exceeded");
}
sendQDATA(CQuorumDataRequest::Errors::QUORUM_TYPE_INVALID, /*request_limit_exceeded=*/false);
return;
}

const CBlockIndex* pQuorumBaseBlockIndex = WITH_LOCK(::cs_main, return m_chainman.m_blockman.LookupBlockIndex(request.GetQuorumHash()));
if (pQuorumBaseBlockIndex == nullptr) {
if (sendQDATA(CQuorumDataRequest::Errors::QUORUM_BLOCK_NOT_FOUND, request_limit_exceeded)) {
m_peer_manager->PeerMisbehaving(pfrom.GetId(), 25, "request limit exceeded");
}
sendQDATA(CQuorumDataRequest::Errors::QUORUM_BLOCK_NOT_FOUND, /*request_limit_exceeded=*/false);
return;
}

const CQuorumDataRequestKey key(pfrom.GetVerifiedProRegTxHash(), false, request.GetQuorumHash(), request.GetLLMQType());
const auto registered = m_qman.RegisterDataRequest(key, request, /*add_expiry_bias=*/false);
if (!registered.has_value()) {
// Per-identity tracking budget exhausted: this peer has too many live requests
// outstanding. Score it and drop without doing any of the response work below --
// an honest peer never reaches this, and the rate limit alone cannot bound the
// map because a fresh quorumHash is always a fresh key.
LogPrint(BCLog::LLMQ, "NetQuorum::%s -- %s: inbound request budget exhausted, from peer=%d\n",
__func__, msg_type, pfrom.GetId());
m_peer_manager->PeerMisbehaving(pfrom.GetId(), 25, "too many quorum data requests");
return;
}
const bool request_limit_exceeded = !*registered;

const auto pQuorum = m_qman.GetQuorum(request.GetLLMQType(), request.GetQuorumHash());
if (pQuorum == nullptr) {
Expand Down Expand Up @@ -319,7 +344,8 @@ DataRequestStatus NetQuorum::RequestQuorumData(CNode& peer, const CQuorum& quoru
quorum.m_quorum_base_block_index->GetBlockHash(), quorum.qc->llmqType);
const CQuorumDataRequest request(quorum.qc->llmqType, quorum.m_quorum_base_block_index->GetBlockHash(),
nDataMask, proTxHash);
if (!m_qman.RegisterDataRequest(key, request)) {
// We initiated this request, so the inbound budget cannot apply and the result is engaged.
if (m_qman.RegisterDataRequest(key, request) != std::optional<bool>{true}) {
return m_qman.GetDataRequestStatus(peer.GetVerifiedProRegTxHash(), /*we_requested=*/true,
quorum.m_quorum_base_block_index->GetBlockHash(), quorum.qc->llmqType);
}
Expand Down
36 changes: 28 additions & 8 deletions src/llmq/quorumsman.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,6 +308,13 @@ void CQuorumManager::CleanupExpiredDataRequests() const
auto it = mapQuorumDataRequests.begin();
while (it != mapQuorumDataRequests.end()) {
if (it->second.IsExpired(/*add_bias=*/true)) {
if (!it->first.m_we_requested) {
// Release the entry's per-identity budget, else a peer stays locked out forever.
if (auto count_it = m_inbound_request_counts.find(it->first.proRegTx);
count_it != m_inbound_request_counts.end() && --count_it->second == 0) {
m_inbound_request_counts.erase(count_it);
}
}
it = mapQuorumDataRequests.erase(it);
} else {
++it;
Expand Down Expand Up @@ -362,19 +369,32 @@ CQuorumCPtr CQuorumManager::GetQuorum(Consensus::LLMQType llmqType, gsl::not_nul
return BuildQuorumFromCommitment(llmqType, pQuorumBaseBlockIndex, populate_cache);
}

bool CQuorumManager::RegisterDataRequest(const CQuorumDataRequestKey& key, const CQuorumDataRequest& request,
bool add_expiry_bias) const
std::optional<bool> CQuorumManager::RegisterDataRequest(const CQuorumDataRequestKey& key,
const CQuorumDataRequest& request,
bool add_expiry_bias) const
{
LOCK(cs_data_requests);
// A peer-initiated request for an unseen key consumes tracking budget. The rate limit cannot
// bound the map on its own: a fresh quorumHash is always a fresh key, so it is never "already
// pending". Re-requests of an existing key fall through to the rate limit below instead.
if (!key.m_we_requested && !mapQuorumDataRequests.count(key)) {
if (auto it = m_inbound_request_counts.find(key.proRegTx);
it != m_inbound_request_counts.end() && it->second >= MAX_INBOUND_DATA_REQUESTS) {
return std::nullopt;
}
}
auto [old_pair, inserted] = mapQuorumDataRequests.emplace(key, request);
if (!inserted) {
if (old_pair->second.IsExpired(add_expiry_bias)) {
old_pair->second = request;
return true;
if (inserted) {
if (!key.m_we_requested) {
++m_inbound_request_counts[key.proRegTx];
}
return false;
return true;
}
return true;
if (old_pair->second.IsExpired(add_expiry_bias)) {
old_pair->second = request;
return true;
}
return false;
}

CQuorumManager::DataResponseValidation CQuorumManager::ValidateDataResponse(
Expand Down
26 changes: 23 additions & 3 deletions src/llmq/quorumsman.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
#include <deque>
#include <map>
#include <memory>
#include <optional>
#include <thread>

class CBLSSignature;
Expand Down Expand Up @@ -48,6 +49,18 @@ class CDKGSessionManager;
class CQuorumBlockProcessor;
class CQuorumSnapshotManager;

//! Per-identity budget for live peer-initiated QGETDATA tracking entries.
//!
//! Entries are keyed on the attacker-chosen quorumHash, so without a cap a peer that never
//! repeats a hash is never rate-limited and grows mapQuorumDataRequests unboundedly for the
//! 300s+bias expiry window (cleanup only runs per-block, and not at all during IBD).
//! The budget is per requesting identity rather than global so one peer cannot evict or
//! starve another; all qwatch peers share the null proRegTx identity and therefore one budget,
//! matching the existing rate-limit behaviour for that class of peer.
//! An honest peer requests at most vvec+contributions for a handful of quorums it is recovering,
//! so this is orders of magnitude above legitimate use.
static constexpr size_t MAX_INBOUND_DATA_REQUESTS{64};

/**
* The quorum manager maintains quorums which were mined on chain. When a quorum is requested from the manager,
* it will lookup the commitment (through CQuorumBlockProcessor) and build a CQuorum object from it.
Expand All @@ -70,6 +83,10 @@ class CQuorumManager final
mutable Mutex cs_data_requests;
mutable std::unordered_map<CQuorumDataRequestKey, CQuorumDataRequest, StaticSaltedHasher> mapQuorumDataRequests
GUARDED_BY(cs_data_requests);
//! Live peer-initiated entries in mapQuorumDataRequests, counted per requesting identity so
//! the per-identity budget can be enforced without scanning the whole map.
mutable std::unordered_map<uint256, size_t, StaticSaltedHasher> m_inbound_request_counts
GUARDED_BY(cs_data_requests);

mutable Mutex m_cs_maps;
mutable std::map<Consensus::LLMQType, Uint256LruHashMap<CQuorumPtr>> mapQuorumsCache
Expand Down Expand Up @@ -131,9 +148,12 @@ class CQuorumManager final
bool IsMasternode() const;
bool IsWatching() const;

//! Request tracking for QGETDATA/QDATA — used by NetQuorum and RPC
bool RegisterDataRequest(const CQuorumDataRequestKey& key, const CQuorumDataRequest& request,
bool add_expiry_bias = true) const
//! Request tracking for QGETDATA/QDATA — used by NetQuorum and RPC.
//! Returns nullopt when a peer-initiated request would exceed that identity's tracking
//! budget, true when the entry was created or refreshed, and false when an unexpired entry
//! already exists (the rate limit applies).
std::optional<bool> RegisterDataRequest(const CQuorumDataRequestKey& key, const CQuorumDataRequest& request,

@chatgpt-codex-connector chatgpt-codex-connector Bot Aug 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve duplicate suppression in the RPC caller

When quorum getdata is invoked again for the same peer and quorum before the request expires, this method now returns std::optional<bool>{false}. The unchanged caller in src/rpc/quorums.cpp:935 applies ! to the optional itself, which tests whether it is engaged rather than its contained value, so it proceeds to send the duplicate QGETDATA and reports success. The responder then treats that duplicate as rate-limit abuse and scores this node; repeated RPC calls can ultimately disconnect it. Update that caller to inspect the contained boolean just as RequestQuorumData now does.

Useful? React with 👍 / 👎.

bool add_expiry_bias = true) const
EXCLUSIVE_LOCKS_REQUIRED(!cs_data_requests);
Comment on lines +151 to 157

@coderabbitai coderabbitai Bot Aug 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: Find all callers of CQuorumManager::RegisterDataRequest to confirm they handle std::optional<bool>.
rg -nP -C5 '\bRegisterDataRequest\s*\(' --type=cpp

Repository: dashpay/dash

Length of output: 150

🏁 Script executed:

#!/bin/bash
set -u

echo "Tracked files matching quorumsman.h name:"
git ls-files | rg '(^|/)quorumsman\.h$|src/llmq/net_quorum\.cpp$|src/llmq/.*quorum.*data.*|quorums.*data|quorum.*data' || true

echo
echo "Search all tracked files for RegisterDataRequest and CQuorumDataRequestKey:"
rg -n -C4 '\bRegisterDataRequest\s*\(|\bCQuorumDataRequestKey\b' || true

echo
echo "Locate RPC files mentioning QGETDATA/QDATA or CQuorumDataRequest:"
rg -n -C3 '\b(QGETDATA|QDATA|CQuorumDataRequest|quorumdata)\b' --glob '*.{cpp,h,json,py,js}' . || true

echo
echo "Current CQuorumManager declaration context if available:"
file="$(git ls-files | rg '(^|/)quorumsman\.h$' | head -n1)"
if [ -n "$file" ]; then
  wc -l "$file"
  sed -n '1,260p' "$file" | nl -ba
fi

Repository: dashpay/dash

Length of output: 350

🏁 Script executed:

#!/bin/bash
set -u

echo "Exact identifier search:"
python3 - <<'PY'
from pathlib import Path
for p in Path('.').rglob('*'):
    if p.is_file() and not any(part in {'.git','build','vendor','node_modules'} for part in p.parts):
        try:
            s=p.read_text(errors='ignore')
        except Exception:
            continue
        if 'RegisterDataRequest' in s or 'CQuorumDataRequestKey' in s:
            print(p)
            for i,line in enumerate(s.splitlines(),1):
                if 'RegisterDataRequest' in line or 'CQuorumDataRequestKey' in line:
                    print(f"  {i}: {line}")
PY

echo
echo "NetQuorum source relevant functions if present:"
sed -n '1,520p' src/llmq/net_quorum.cpp | nl -ba | sed -n '110,170p;328,365p'

echo
echo "Any QGETDATA/QDATA/CQuorumDataRequest mentions:"
python3 - <<'PY'
from pathlib import Path
terms = ['QGETDATA','QDATA','CQuorumDataRequest','quorumdata']
for p in Path('.').rglob('*'):
    if p.is_file() and p.suffix in {'.cpp','.h','.py','.json',''}:
        try:
            s=p.read_text(errors='ignore').splitlines()
        except Exception:
            continue
        hits=[(i,line) for i,line in enumerate(s,1) if any(t in line for t in terms)]
        if hits:
            print(p)
            for i,line in hits[:40]:
                print(f"  {i}: {line[:220]}")
PY

Repository: dashpay/dash

Length of output: 15154

Fix the RPC call to use std::optional<bool> semantics.

src/rpc/quorums.cpp:935 still treats RegisterDataRequest as returning bool: if (!llmq_ctx.qman->RegisterDataRequest(key, request)) return false;. This misinterprets the nullopt case and can send a QGETDATA message while the request budget is exceeded.

Use the NetQuorum RPC pattern with RegisterDataRequest(key, request) == std::optional<bool>{true} before PushMessage(...).

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/llmq/quorumsman.h` around lines 151 - 157, Update the RPC flow around
RegisterDataRequest to require an explicit std::optional<bool>{true} result
before calling PushMessage. Preserve the existing failure return path, ensuring
nullopt and false both prevent sending QGETDATA.

enum class DataResponseValidation : uint8_t { OK, NotRequested, AlreadyReceived, Mismatch };
DataResponseValidation ValidateDataResponse(const CQuorumDataRequestKey& key,
Expand Down
Loading
Loading