perf: deserialize DKG messages once (framing-only intake)

[PastaPastaPasta]

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?

• Incoming contributions, complaints, justifications, and premature commitments are retained as exact raw wire bytes in the existing per-message-type pending queues.
• The typed intake deserialize is replaced by 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-checked ignore(), and dynamic bitsets are validated by calling the same ReadFixedBitSet() the typed deserializer uses, so truncation and padding-bit semantics cannot diverge.
• The common llmqType/quorumHash prefix is peeked via SpanReader instead of read-then-Rewind, and short payloads are rejected up front instead of throwing out of ProcessMessage.
• The DKG worker is the sole typed deserialization point (BLS decompression, canonical checks, active-scheme handling happen exactly once), immediately followed by the same parameter-derived structural checks and normal preverification. Failures are scored 100 as before.
• The pre-existing per-peer retention quota (twice the quorum size, per message type) is now keyed by proTxHash (maxMessagesPerProTx) instead of NodeId, so reconnecting with a fresh NodeId no 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.
• Threat model note: sender identities are pinned to the deterministic masternode list by MNAuth, so each additional quota is gated by masternode collateral. Under the assumption that only a small number of masternodes are malicious, the per-proTx quota alone bounds worst-case retention to (hostile MN count) x (2 x quorum size) messages per type; no queue-wide cap is introduced.
• Duplicate wire hashes are rejected before charging the quota; messages dropped for quota are not marked seen, so they can be announced and delivered again by a peer with remaining budget.
• Inventory hashes are computed over the exact original wire bytes; own-message queueing and malformed-message scoring are preserved.
• At round start, leftover raw queues are discarded without any typed or BLS deserialization, so stale messages cannot delay next-round initialization.
• Deliberate strengthening vs. develop worth calling out: trailing bytes after a structurally complete message are now rejected (at intake by the framing walk, and defensively on the worker). Previously neither pass checked for them, so payload || garbage was accepted and hashed as a distinct inventory item.
• Added a fuzz target (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.
• Unit tests pin the quota semantics (reconnect persistence, no refund on drain, dedup-before-quota, per-proTx independence, own messages sharing the quota path). Functional tests cover trailing-byte rejection at intake, deferred BLS scoring on the worker, proTxHash-keyed quotas across reconnects, and late-message retention cleared at round start without BLS decoding.

How Has This Been Tested?

Validated on macOS arm64 with:

make -C src -j15 dashd test/test_dash
./src/test/test_dash --run_test=llmq_dkg_tests --catch_system_errors=no
test/functional/test_runner.py feature_llmq_dkg_intake.py
FUZZ=dkg_message_framing ./src/test/fuzz/fuzz <seeds>  # 402-seed replay
python3 test/lint/lint-includes.py
python3 test/lint/lint-circular-dependencies.py
python3 test/lint/lint-python.py
test/lint/lint-whitespace.py

Breaking Changes

None.

Checklist:

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have added or updated relevant unit/integration/functional/e2e tests
• I have made corresponding changes to the documentation
• I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[coderabbitai]

Warning

Review limit reached

@PastaPastaPasta, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 4 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 261e658f-0e71-4890-ba5a-3e512c462fa2

📥 Commits

Reviewing files that changed from the base of the PR and between 4dfe036 and 8389144.

📒 Files selected for processing (8)

• src/Makefile.test.include
• src/llmq/dkgsessionhandler.cpp
• src/llmq/dkgsessionhandler.h
• src/llmq/net_dkg.cpp
• src/llmq/net_dkg.h
• src/test/fuzz/dkg_message_framing.cpp
• src/test/llmq_dkg_tests.cpp
• test/functional/feature_llmq_dkg_intake.py

---

_{Comment @coderabbitai help to get the list of available commands.}

[thepastaclaw]

⛔ Blockers found — Opus deferred (commit 11cd6c4)
Canonical validated blockers: 1

[github-actions]

Potential PR merge conflicts

This is advisory only. It does not block CI, but it marks PRs that will likely need a rebase depending on merge order.

If this PR merges first

These open PRs will likely need a rebase:

• #7401: perf: deserialize DKG messages once Changed files: src/Makefile.test.include, src/llmq/dkgsessionhandler.cpp, src/llmq/dkgsessionhandler.h, src/llmq/net_dkg.cpp, src/llmq/net_dkg.h, src/test/fuzz/dkg_message_framing.cpp, src/test/llmq_dkg_tests.cpp, test/functional/feature_llmq_dkg_intake.py.
• #7524: fix: bound DKG pending message queues across NodeId reconnects Changed files: src/Makefile.test.include, src/llmq/dkgsessionhandler.cpp, src/llmq/dkgsessionhandler.h, src/llmq/net_dkg.cpp, src/llmq/net_dkg.h.

[thepastaclaw]

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.

[thepastaclaw]

🔴 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']}

[thepastaclaw]

🟡 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']}