Skip to content

fix: bound DKG contribution blob intake #7408

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

Merged
PastaPastaPasta merged 2 commits into from
Jul 7, 2026
Merged

fix: bound DKG contribution blob intake #7408

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
35d4480
fix: bound DKG contribution blob intake
PastaPastaPasta Jul 6, 2026
439fb7d
test: cover undersized DKG contribution intake
PastaPastaPasta Jul 6, 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
5 changes: 4 additions & 1 deletion src/llmq/net_dkg.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,14 +82,17 @@ size_t MaxDKGMessageSize(std::string_view msg_type, const Consensus::LLMQParams&
bool CheckDKGMessageStructure(std::string_view msg_type, const CDataStream& vRecv, const Consensus::LLMQParams& params)
{
const size_t size = params.size > 0 ? static_cast<size_t>(params.size) : 0;
const size_t min_size = params.minSize > 0 ? static_cast<size_t>(params.minSize) : 0;
const size_t threshold = params.threshold > 0 ? static_cast<size_t>(params.threshold) : 0;
try {
CDataStream s(vRecv); // copy; deserialization does not advance the caller's stream
if (msg_type == NetMsgType::QCONTRIB) {
CDKGContribution qc;
s >> qc;
return qc.vvec != nullptr && qc.vvec->size() == threshold &&
qc.contributions != nullptr && qc.contributions->blobs.size() <= size;
qc.contributions != nullptr &&
qc.contributions->blobs.size() >= min_size &&
Comment on lines +85 to +94

@thepastaclaw thepastaclaw Jul 6, 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.

🟡 Suggestion: Production min_size clamp is landing in a test-prefixed commit

Commit 439fb7d has subject test: cover undersized DKG contribution intake but its diff also edits src/llmq/net_dkg.cpp: it introduces const size_t min_size = params.minSize > 0 ? static_cast<size_t>(params.minSize) : 0; and rewrites the intake check to consume that local, replacing the raw static_cast<size_t>(params.minSize) that fix commit 35d4480 introduced. That is a defensive production behavior change (clamping negative/zero params.minSize) unrelated to the test being added. Because Dash keeps PR commits unsquashed on merge, git log --oneline and git blame on this consensus-adjacent intake path will report a test: commit for a real code change — a bisect/blame hazard. Two clean options: squash the net_dkg.cpp hunk into 35d4480 so the fix commit is self-contained with its final clamped form, or split it into its own commit (e.g. fix: clamp negative params.minSize in DKG intake check) and leave the test: commit touching only test/functional/feature_llmq_dkg_intake.py.

source: ['claude', 'codex']

qc.contributions->blobs.size() <= size;
Comment on lines 92 to +95

@thepastaclaw thepastaclaw Jul 6, 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.

💬 Nitpick: Cast of signed params.minSize is inconsistent with adjacent clamping

Lines 84-85 of the same function defensively clamp params.size/params.threshold before casting to size_t, and MaxDKGMessageSize (lines 51-52) does the same. Line 93 instead bare-casts params.minSize to size_t. Every configured minSize in src/llmq/params.h is >= 2, so this is safe today, but the local style divergence stands out and a future misconfigured 0/negative value would silently accept anything (0) or become unreachable (huge). A std::max(0, params.minSize)-style clamp would match the surrounding pattern with no observable behavior change. Not required for this PR.

source: ['claude-general']

@thepastaclaw thepastaclaw Jul 6, 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.

Resolved in this update — Cast of signed params.minSize is inconsistent with adjacent clamping 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.

} else if (msg_type == NetMsgType::QCOMPLAINT) {
CDKGComplaint qc;
s >> qc;
Expand Down
28 changes: 27 additions & 1 deletion test/functional/feature_llmq_dkg_intake.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
The node must not crash; the sending peer must be scored (Misbehaving).
"""

from test_framework.messages import ser_uint256
from test_framework.messages import ser_compact_size, ser_uint256
from test_framework.p2p import P2PInterface
from test_framework.test_framework import DashTestFramework
from test_framework.util import wait_until_helper
Expand Down Expand Up @@ -83,6 +83,21 @@ def quorum_hash_prefix(self):
# real in-progress quorum and reach the size/structural checks.
return bytes([LLMQ_TEST]) + ser_uint256(int(self.quorum_hash, 16))

def qcontrib_payload(self, blob_count):
# CDKGContribution: llmqType, quorumHash, proTxHash, vvec, contributions, sig.
# LLMQ_TEST uses threshold=2/minSize=2 by default, so blob_count=1 is
# well-formed enough to deserialize but below the contribution lower bound.
r = self.quorum_hash_prefix()
r += ser_uint256(0) # proTxHash
r += ser_compact_size(2) + b"\x00" * (2 * 48) # BLSVerificationVector
r += b"\x00" * 48 # CBLSIESMultiRecipientBlobs::ephemeralPubKey
r += b"\x00" * 32 # CBLSIESMultiRecipientBlobs::ivSeed
r += ser_compact_size(blob_count)
for _ in range(blob_count):
r += ser_compact_size(32) + b"\x00" * 32
r += b"\x00" * 96 # sig
return r

def add_verified_peer(self, node):
peer = node.add_p2p_connection(P2PInterface())
peer_id = get_p2p_id(node)
Expand All @@ -103,6 +118,7 @@ def run_test(self):
self.test_unverified_sender_rejected(mn_node)
self.test_oversized_rejected(mn_node)
self.test_malformed_rejected(mn_node)
self.test_under_min_contribution_blobs_rejected(mn_node)

def test_unverified_sender_rejected(self, node):
self.log.info("Pushed DKG messages from a non-verified peer are rejected (Misbehaving 10 each)")
Expand Down Expand Up @@ -144,6 +160,16 @@ def test_malformed_rejected(self, node):
wait_for_banscore(node, peer_id, 100)
node.disconnect_p2ps()

def test_under_min_contribution_blobs_rejected(self, node):
self.log.info("QCONTRIB with fewer than minSize encrypted blobs is rejected (Misbehaving 100)")
peer, peer_id = self.add_verified_peer(node)
wait_for_banscore(node, peer_id, 0)
with node.assert_debug_log(["malformed DKG message"]):
peer.send_message(msg_dkg_raw(b"qcontrib", self.qcontrib_payload(blob_count=1)))
peer.sync_with_ping()
wait_for_banscore(node, peer_id, 100)
node.disconnect_p2ps()


if __name__ == '__main__':
DkgIntakeTest().main()
Loading