perf: deserialize DKG messages once (framing-only intake) - #7557
perf: deserialize DKG messages once (framing-only intake)#7557PastaPastaPasta wants to merge 4 commits into
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
WalkthroughDKG intake now validates payload framing before retention and defers BLS deserialization until worker processing. Pending-message quotas use sender proTxHash values instead of peer node IDs. Duplicate messages do not consume quota, and quota state clears between rounds. New fuzz, unit, and functional tests cover framing, malformed BLS data, reconnects, per-sender isolation, and stale-message cleanup. Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Peer
participant DKGIntake
participant CDKGPendingMessages
participant DKGWorker
Peer->>DKGIntake: send DKG payload
DKGIntake->>DKGIntake: validate wire framing
DKGIntake->>CDKGPendingMessages: retain raw payload with sender proTxHash
DKGWorker->>CDKGPendingMessages: retrieve pending payload
DKGWorker->>DKGWorker: deserialize BLS objects and validate structure
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
|
✅ Final review complete — no blockers (commit 5d06f2e) |
783af2b to
8389144
Compare
DKG intake previously deserialized a copy of each accepted payload (repeating BLS point decompression on the shared network thread) for structural validation, and the DKG worker then deserialized the retained bytes again. Replace the typed intake pass with a framing-only wire walk that validates CompactSize counts, dynamic bitsets (via the same ReadFixedBitSet the typed path uses), quorum-parameter bounds, truncation, and trailing bytes without decoding any BLS object. The worker is now the sole typed deserialization point, immediately followed by the same parameter-derived structural checks. The pre-existing per-peer pending-message quota is rekeyed from NodeId to the MNAuth-verified proTxHash and made cumulative for the round, so a sender can no longer reset its retention budget by reconnecting or by waiting for the worker to drain the queue. Own messages are enqueued under this node's own proTxHash and share the same quota path. Sender identities are pinned to the deterministic masternode list by MNAuth, so worst-case retention is bounded by (hostile MN count) x quota. Duplicate hashes are rejected before charging the quota, and quota-dropped messages are not marked seen so another peer with budget can re-deliver them. The llmqType/quorumHash prefix is peeked via SpanReader instead of read+Rewind, and short payloads are scored instead of throwing out of ProcessMessage. Leftover raw queues are discarded at round start without BLS work. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Intake framing validation and worker typed deserialization are two hand-maintained parsers over one wire format. The safety-critical direction is that framing must never reject a payload the worker would accept, otherwise honest DKG messages are silently dropped before retention and quorum formation degrades. Assert that direction over fuzzer-provided payloads for every configured LLMQ and both BLS schemes, plus a constructed well-formed message per input so serializer/framing drift is caught even from an empty corpus. The converse is intentionally not asserted: framing accepts undecodable BLS encodings so the worker can score the sender. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Unit tests pin the CDKGPendingMessages semantics: the per-proTx quota survives reconnects and is not refunded by drains, duplicates are rejected before charging, quotas are independent across proTxes, and own messages are charged under this node's own proTxHash. Functional tests cover trailing-byte rejection at intake, deferral of BLS decoding to the DKG worker (scored there, not at intake), quota persistence across reconnects under fresh NodeIds, and late-message retention cleared at round start without BLS work. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
8389144 to
11cd6c4
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The framing parser and proTxHash-keyed quota redesign are well covered, but moving malformed BLS detection to the DKG worker creates a deterministic penalty bypass when the originating peer disconnects before its queued message is processed, so changes are required. The new Dash-specific fuzz target must also be registered in the non-backported manifest to receive the intended lint coverage.
Source: Reviewer backend models: codex general gpt-5.6-sol and codex dash-core-commit-history gpt-5.6-sol; final verifier backend model: gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking | 🟡 1 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/llmq/net_dkg.cpp`:
- [BLOCKING] src/llmq/net_dkg.cpp:403-407: Deferred BLS failures lose their penalty after reconnect
The queued entry retains only the originating `NodeId`, while this PR moves malformed BLS detection from synchronous intake validation to the later DKG worker pass. A peer can send a requested, framing-valid payload with an invalid BLS encoding during `Initialized`, disconnect before the matching phase drains the queue, and reconnect under a new ID. `FinalizeNode()` removes the old ID from `PeerManagerImpl::m_peer_map`, so the later `PeerMisbehaving(nodeId, 100)` call finds no `PeerRef` and silently applies no score. This is a regression from the previous typed intake check, which detected this malformed encoding while the sender was still being processed. The proTxHash-keyed quota limits retained work but does not preserve punishment; retain enough authenticated sender metadata to apply the offense after disconnect, or otherwise ensure deferred validation keeps the originating peer punishable.
In `src/test/fuzz/dkg_message_framing.cpp`:
- [SUGGESTION] src/test/fuzz/dkg_message_framing.cpp:1: Register the new Dash-specific fuzz file as non-backported
`src/test/fuzz/dkg_message_framing.cpp` is a newly added Dash-specific source, but no pattern in `test/util/data/non-backported.txt` matches it; `src/test/llmq*.cpp` only covers files directly under `src/test`. The manifest feeds Dash-specific cppcheck and clang-diff-format coverage, so add the fuzz target's exact path to it.
| for (const auto& p : msgs) { | ||
| const NodeId& nodeId = p.first; | ||
| if (!p.second) { | ||
| LogPrint(BCLog::LLMQ_DKG, "%s -- failed to deserialize message, peer=%d\n", __func__, nodeId); | ||
| auto msg = DeserializeCheckedDKGMessage<Message>(*p.second, params, nodeId); | ||
| if (!msg) { | ||
| peerman.PeerMisbehaving(nodeId, 100); |
There was a problem hiding this comment.
🔴 Blocking: Deferred BLS failures lose their penalty after reconnect
The queued entry retains only the originating NodeId, while this PR moves malformed BLS detection from synchronous intake validation to the later DKG worker pass. A peer can send a requested, framing-valid payload with an invalid BLS encoding during Initialized, disconnect before the matching phase drains the queue, and reconnect under a new ID. FinalizeNode() removes the old ID from PeerManagerImpl::m_peer_map, so the later PeerMisbehaving(nodeId, 100) call finds no PeerRef and silently applies no score. This is a regression from the previous typed intake check, which detected this malformed encoding while the sender was still being processed. The proTxHash-keyed quota limits retained work but does not preserve punishment; retain enough authenticated sender metadata to apply the offense after disconnect, or otherwise ensure deferred validation keeps the originating peer punishable.
source: ['codex']
There was a problem hiding this comment.
Resolved in this update — Deferred BLS failures lose their penalty after reconnect 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.
| @@ -0,0 +1,157 @@ | |||
| // Copyright (c) 2025 The Dash Core developers | |||
There was a problem hiding this comment.
🟡 Suggestion: Register the new Dash-specific fuzz file as non-backported
src/test/fuzz/dkg_message_framing.cpp is a newly added Dash-specific source, but no pattern in test/util/data/non-backported.txt matches it; src/test/llmq*.cpp only covers files directly under src/test. The manifest feeds Dash-specific cppcheck and clang-diff-format coverage, so add the fuzz target's exact path to it.
source: ['codex']
There was a problem hiding this comment.
Resolved in this update — Register the new Dash-specific fuzz file as non-backported 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.
src/test/fuzz/dkg_message_framing.cpp is Dash-specific; add it to test/util/data/non-backported.txt so it receives Dash cppcheck and clang-format-diff coverage.
|
Thanks for the review. Taking the two findings in turn: Suggestion (non-backported.txt): accepted. Blocker (deferred BLS penalty lost after reconnect): declining. The mechanics of the finding are accurate —
If reviewers feel the residual gap is worth tracking, a follow-up issue for protx-keyed misbehavior accounting across the deferred DKG validation paths (all of them, not just decode failures) would be the coherent shape — but it is orthogonal to this PR's perf change and predates it. 🤖 Posted autonomously by Claude on behalf of pasta. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
The framing-only intake parser, worker-side typed validation, proTxHash-keyed cumulative quotas, and associated fuzz/unit/functional coverage are consistent at the exact head. The prior non-backported manifest omission is fixed, and the deferred peer-scoring disconnect window is bounded by this PR's durable quota and remains a broader pre-existing behavior rather than an in-scope blocker; no actionable findings remain.
Source: Reviewer backend models gpt-5.6-sol (Codex general and dash-core-commit-history) and claude-sonnet-5 (Claude general and dash-core-commit-history); final verifier backend model gpt-5.6-sol as the permitted fallback after both Claude Sonnet verifier attempts failed. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Review provenance
- Codex reviewers:
gpt-5.6-sol— dash-core-commit-history (completed),gpt-5.6-sol— general (completed) - Verifier:
gpt-5.6-sol— final-verifier (fallback for Sonnet verifier) - Sonnet reviewers:
claude-sonnet-5— general (completed),claude-sonnet-5— dash-core-commit-history (completed)
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/functional/feature_llmq_dkg_intake.py (1)
260-268: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
nodeparameter.
_send_late_qcontribdoes not usenode.♻️ Proposed change
- def _send_late_qcontrib(self, node, peer, nonce): + def _send_late_qcontrib(self, peer, nonce):Update the three call sites in
test_late_messages_boundedaccordingly.🤖 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 `@test/functional/feature_llmq_dkg_intake.py` around lines 260 - 268, Remove the unused node parameter from _send_late_qcontrib and update all three calls in test_late_messages_bounded to pass only peer and nonce.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@src/test/llmq_dkg_tests.cpp`:
- Line 51: Update all three CDKGPendingMessages constructor calls in the test to
use the declared parameter name _maxMessagesPerProTx in their argument comments,
or remove the comments entirely.
---
Nitpick comments:
In `@test/functional/feature_llmq_dkg_intake.py`:
- Around line 260-268: Remove the unused node parameter from _send_late_qcontrib
and update all three calls in test_late_messages_bounded to pass only peer and
nonce.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 835a2cc1-8eb6-4bba-87a0-67c4162b4173
📒 Files selected for processing (9)
src/Makefile.test.includesrc/llmq/dkgsessionhandler.cppsrc/llmq/dkgsessionhandler.hsrc/llmq/net_dkg.cppsrc/llmq/net_dkg.hsrc/test/fuzz/dkg_message_framing.cppsrc/test/llmq_dkg_tests.cpptest/functional/feature_llmq_dkg_intake.pytest/util/data/non-backported.txt
|
|
||
| // Own messages (from=-1) are enqueued under this node's proTxHash and | ||
| // charged like any other sender's. | ||
| CDKGPendingMessages pending{/*max_messages_per_protx=*/2}; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n 'bugprone-argument-comment' .clang-tidy src/.clang-tidy 2>/dev/nullRepository: dashpay/dash
Length of output: 241
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- constructor and call site ---'
rg -n -C 6 'CDKGPendingMessages|maxMessagesPerProTx|max_messages_per_protx' src/test/llmq_dkg_tests.cpp src --glob '*.{cpp,h}' | head -240
printf '%s\n' '--- clang-tidy configuration ---'
cat -n src/.clang-tidy | sed -n '1,35p'
printf '%s\n' '--- nearby tests ---'
sed -n '1,90p' src/test/llmq_dkg_tests.cppRepository: dashpay/dash
Length of output: 20902
🏁 Script executed:
#!/bin/bash
set -eu
if command -v clang-tidy >/dev/null 2>&1; then
clang-tidy --version
tmp="$(mktemp --suffix=.cpp)"
trap 'rm -f "$tmp"' EXIT
cat >"$tmp" <<'CPP'
class CDKGPendingMessages {
public:
explicit CDKGPendingMessages(size_t _maxMessagesPerProTx) {}
};
void test()
{
CDKGPendingMessages pending{/*max_messages_per_protx=*/2};
}
CPP
clang-tidy -checks=-*,bugprone-argument-comment -warnings-as-errors=bugprone-argument-comment "$tmp" -- -std=c++17 2>&1
else
echo "clang-tidy is not available"
fiRepository: dashpay/dash
Length of output: 791
🏁 Script executed:
#!/bin/bash
set -eu
tmp="$(mktemp --suffix=.cpp)"
trap 'rm -f "$tmp"' EXIT
cat >"$tmp" <<'CPP'
`#include` <cstddef>
class CDKGPendingMessages {
std::size_t maxMessagesPerProTx;
public:
explicit CDKGPendingMessages(std::size_t _maxMessagesPerProTx) :
maxMessagesPerProTx(_maxMessagesPerProTx) {}
};
void test()
{
CDKGPendingMessages pending{/*max_messages_per_protx=*/2};
}
CPP
clang-tidy -checks=-*,bugprone-argument-comment -warnings-as-errors=bugprone-argument-comment "$tmp" -- -std=c++17 2>&1Repository: dashpay/dash
Length of output: 757
Use the declared parameter name in the argument comment.
bugprone-argument-comment is enabled as an error in src/.clang-tidy. Replace /*max_messages_per_protx=*/ with /*_maxMessagesPerProTx=*/, or remove the comment. Apply this to all three constructor calls.
🤖 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/test/llmq_dkg_tests.cpp` at line 51, Update all three CDKGPendingMessages
constructor calls in the test to use the declared parameter name
_maxMessagesPerProTx in their argument comments, or remove the comments
entirely.
2 participants
Issue being fixed or feature implemented
DKG network message intake deserializes each accepted payload twice: once on a copied payload in the p2p message handler for structural validation, and again from the pending queue on the DKG worker thread. These messages carry BLS objects, so the redundant intake pass repeats elliptic-curve point decompression on the shared network thread during every DKG round.
Additionally, the per-peer pending-message quota in the same path is keyed by
NodeId, so a single misbehaving masternode can reset its retention budget just by reconnecting, defeating the bound that already exists.This is a from-scratch redesign of the approach in #7401, sharing its goals and test strategy but with a substantially smaller intake parser and a simpler retention model.
What was done?
CheckDKGMessageWireStructure(), a framing-only walk that validates CompactSize counts, dynamic bitsets, quorum-parameter bounds, truncation, and trailing bytes without decoding any BLS object. The walk is deliberately thin: fixed-size BLS encodings are skipped with a single bounds-checkedignore(), and dynamic bitsets are validated by calling the sameReadFixedBitSet()the typed deserializer uses, so truncation and padding-bit semantics cannot diverge.llmqType/quorumHashprefix is peeked viaSpanReaderinstead of read-then-Rewind, and short payloads are rejected up front instead of throwing out ofProcessMessage.maxMessagesPerProTx) instead ofNodeId, so reconnecting with a freshNodeIdno longer resets the budget, and the quota is cumulative for the round (draining the queue does not refund it). Own messages are enqueued under this node's own proTxHash and go through the same quota path -- no special case.payload || garbagewas accepted and hashed as a distinct inventory item.dkg_message_framing) that continuously checks the safety-critical equivalence direction across every configured LLMQ and both BLS schemes: the framing walk must never reject a payload that typed worker deserialization would accept. The converse is intentionally not asserted -- framing accepts undecodable BLS encodings so the worker can score the sender.How Has This Been Tested?
Validated on macOS arm64 with:
Breaking Changes
None.
Checklist: