Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
8 changes: 8 additions & 0 deletions src/llmq/net_quorum.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,14 @@ void NetQuorum::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDataS
}

CQuorumDataRequest request;
// An honest QGETDATA is exactly the fixed-size request encoding; nError is
// response-only (QDATA). A smuggled value used to select the *_MISSING branches
// that skip the rate-limit ban, so reject any longer payload by length — this
// also catches an explicitly serialized UNDEFINED byte and trailing garbage.
if (vRecv.size() > GetSerializeSize(request, vRecv.GetVersion())) {
m_peer_manager->PeerMisbehaving(pfrom.GetId(), 100, "oversized qgetdata");
return;
}
vRecv >> request;

auto sendQDATA = [&](CQuorumDataRequest::Errors nError,
Expand Down
15 changes: 15 additions & 0 deletions test/functional/p2p_quorum_data.py
Original file line number Diff line number Diff line change
Expand Up @@ -376,6 +376,19 @@ def send_bad_qdata_expect_disconnect(bad_qdata):
self.restart_mn(mn1)
self.wait_for_quorum_data([mn1], 100, quorum_hash, recover=False)

# Requester-supplied nError on QGETDATA is rejected (+100) and disconnects.
# Independent of cleanup / rate-limit state — one poisoned message is enough.
def test_qgetdata_rejects_requester_error():
self.log.info("Test QGETDATA with requester-supplied nError is disconnected")
p2p_mn = p2p_connection(mn2.get_node(self))
id_p2p_mn = get_p2p_id(mn2.get_node(self))
mnauth(mn2.get_node(self), id_p2p_mn, fake_mnauth_2[0], fake_mnauth_2[1])
wait_for_banscore(mn2.get_node(self), id_p2p_mn, 0)
poisoned = msg_qgetdata(quorum_hash_int, 100, 0x01, error=ENCRYPTED_CONTRIBUTIONS_MISSING)
p2p_mn.send_message(poisoned)
self.wait_until(lambda: not p2p_mn.is_connected, timeout=10)
mn2.get_node(self).disconnect_p2ps()

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Suggestion: Exercise presence-based rejection for both special error bytes

The PR explicitly requires rejection based on field presence, including serialized NONE (0x00) and UNDEFINED (0xFF), but the test sends only ENCRYPTED_CONTRIBUTIONS_MISSING. It would therefore pass with a weaker value-based check that rejects ordinary nonzero errors while accepting one or both special values. Exercise both boundary values on fresh peers, while retaining the existing nonzero case, so the regression test distinguishes the size-based implementation from every decoded-value gate.

Suggested change
p2p_mn = p2p_connection(mn2.get_node(self))
id_p2p_mn = get_p2p_id(mn2.get_node(self))
mnauth(mn2.get_node(self), id_p2p_mn, fake_mnauth_2[0], fake_mnauth_2[1])
wait_for_banscore(mn2.get_node(self), id_p2p_mn, 0)
poisoned = msg_qgetdata(quorum_hash_int, 100, 0x01, error=ENCRYPTED_CONTRIBUTIONS_MISSING)
p2p_mn.send_message(poisoned)
self.wait_until(lambda: not p2p_mn.is_connected, timeout=10)
mn2.get_node(self).disconnect_p2ps()
for error in (0, 0xFF, ENCRYPTED_CONTRIBUTIONS_MISSING):
p2p_mn = p2p_connection(mn2.get_node(self))
id_p2p_mn = get_p2p_id(mn2.get_node(self))
mnauth(mn2.get_node(self), id_p2p_mn, fake_mnauth_2[0], fake_mnauth_2[1])
wait_for_banscore(mn2.get_node(self), id_p2p_mn, 0)
poisoned = msg_qgetdata(quorum_hash_int, 100, 0x01, error=error)
p2p_mn.send_message(poisoned)
self.wait_until(lambda: not p2p_mn.is_connected, timeout=10)
mn2.get_node(self).disconnect_p2ps()

source: ['codex', 'coderabbit']

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Resolved in 1a32e0eExercise presence-based rejection for both special error bytes no longer present.

Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.


Comment thread
coderabbitai[bot] marked this conversation as resolved.
# Test request limiting / banscore increase
def test_request_limit():

Expand Down Expand Up @@ -531,6 +544,8 @@ def test_qsigshares_inv_oom():
test_watchquorums()
test_rpc_quorum_getdata_protx_hash()

# Once: nError rejection does not depend on request-expiry cleanup.
test_qgetdata_rejects_requester_error()
test_qsigshares_inv_oom()


Expand Down
18 changes: 14 additions & 4 deletions test/functional/test_framework/messages.py
Original file line number Diff line number Diff line change
Expand Up @@ -2565,35 +2565,45 @@ def __repr__(self):


class msg_qgetdata:
__slots__ = ("quorum_hash", "quorum_type", "data_mask", "protx_hash")
__slots__ = ("quorum_hash", "quorum_type", "data_mask", "protx_hash", "error")
msgtype = b"qgetdata"

def __init__(self, quorum_hash=0, quorum_type=-1, data_mask=0, protx_hash=0):
def __init__(self, quorum_hash=0, quorum_type=-1, data_mask=0, protx_hash=0, error=None):
self.quorum_hash = quorum_hash
self.quorum_type = quorum_type
self.data_mask = data_mask
self.protx_hash = protx_hash
# error is response-only on the wire. Honest requesters leave it None so
# it is not serialized. Attackers can set it to smuggle a QDATA error
# code into a request (see CQuorumDataRequest SERIALIZE_METHODS).
self.error = error

def deserialize(self, f):
self.quorum_type = struct.unpack("<B", f.read(1))[0]
self.quorum_hash = deser_uint256(f)
self.data_mask = struct.unpack("<H", f.read(2))[0]
self.protx_hash = deser_uint256(f)
# Optional trailing byte (present on QDATA responses; may be smuggled on requests).
extra = f.read(1)
self.error = struct.unpack("<B", extra)[0] if extra else None

def serialize(self):
r = b""
r += struct.pack("<B", self.quorum_type)
r += ser_uint256(self.quorum_hash)
r += struct.pack("<H", self.data_mask)
r += ser_uint256(self.protx_hash)
if self.error is not None:
r += struct.pack("<B", self.error)
return r

def __repr__(self):
return "msg_qgetdata(quorum_hash=%064x, quorum_type=%d, data_mask=%d, protx_hash=%064x)" % (
return "msg_qgetdata(quorum_hash=%064x, quorum_type=%d, data_mask=%d, protx_hash=%064x, error=%s)" % (
self.quorum_hash,
self.quorum_type,
self.data_mask,
self.protx_hash)
self.protx_hash,
self.error)


class msg_qdata:
Expand Down
Loading