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]

Walkthrough

DKG 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. Observer nodes ignore and do not request DKG payloads. New fuzz, unit, and functional tests cover these changes.

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

Loading

Possibly related PRs

• dashpay/dash#7401: Related DKG intake, quota, framing-validation, fuzzing, and test changes.
• dashpay/dash#7523: Earlier DKG intake framing and deserialization changes extended by this PR.
• dashpay/dash#7524: Related pending-message quota and duplicate-handling changes.

Suggested reviewers: udjinm6

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main change: moving DKG typed deserialization off intake and using framing-only validation.
Description check	✅ Passed	The description directly explains the DKG intake redesign, quota changes, added validation, tests, and testing performed.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

[thepastaclaw]

✅ Final review complete — no blockers (commit 8aa661a)

[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.

[PastaPastaPasta]

Thanks for the review. Taking the two findings in turn:

Suggestion (non-backported.txt): accepted. src/test/fuzz/dkg_message_framing.cpp is now registered in test/util/data/non-backported.txt (verified git ls-files resolves it), so it gets Dash-specific cppcheck and clang-format-diff coverage.

Blocker (deferred BLS penalty lost after reconnect): declining. The mechanics of the finding are accurate — PeerMisbehaving no-ops once FinalizeNode() has run, so a peer that disconnects before the phase drain escapes the score for a malformed BLS encoding. But this does not rise to a blocker:

1. The baseline being "regressed" is six weeks old, and the historical behavior is what this PR restores. Synchronous typed intake validation was introduced in 31142da (merged 2026-06-29). From the DKG's introduction until then, malformed BLS encodings were detected exactly where this PR detects them — on the DKG worker via PopAndDeserializeMessages returning nullptr — with the identical disconnect-evasion window. That commit's purpose was preventing unauthenticated retention and crashes; those protections are kept and strengthened here (MNAuth gate, framing walk, size caps, per-proTx quota).

2. The same window already exists — before and after this PR — for the sibling failure classes on this exact code path. A well-encoded message with a bad signature, or one failing PreVerifyMessage, is only punished at drain time (ProcessPendingMessageBatch / BatchVerifyMessageSigs), and always has been. The invariant "the sender is still connected when punished" was never held by deferred DKG validation in general; singling out the encoding-failure class doesn't change the security posture, because an attacker who wants to dodge the score can already do so today by sending a framing-valid, signature-invalid payload and disconnecting.

3. What the score protects is bounded by a mechanism that does survive reconnect. The DoS-relevant resources — queue retention and worker-side BLS decompression — are capped by the proTxHash-keyed quota this PR introduces, which is deliberately cumulative for the round, not refunded on pop, and unaffected by disconnect/reconnect. Worst case per masternode identity per phase is 2 × quorumSize framing-valid payloads decoded once each. Reaching the queue at all requires an MNAuth-verified, on-chain-registered masternode identity, which is Sybil-expensive; the peer score is a secondary, best-effort deterrent on top of that, as it is everywhere else in the codebase.

4. The proposed fix has worse semantics than the gap. Retaining sender identity and punishing "whoever currently holds that proTxHash" would discourage a fresh, MNAuth-verified connection for bytes attributed to a previous session — effectively reimplementing PoSe at the peer layer with confusing address-discouragement side effects. Identity-level accountability for DKG misconduct already lives at the session/PoSe layer, which is keyed by the durable identity and is the right place for it.

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]

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)

[thepastaclaw]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

test/functional/feature_llmq_dkg_intake.py (1)

260-268: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Remove the unused node parameter.

_send_late_qcontrib does not use node.

♻️ 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_bounded accordingly.

🤖 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

📥 Commits

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

📒 Files selected for processing (9)

• 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
• test/util/data/non-backported.txt

[thepastaclaw]

Preliminary review — Codex only

The active-masternode path has focused framing validation, worker-side typed validation, and strong quota and fuzz coverage, but observer mode retains accepted payloads without ever running the deferred validation or round cleanup. Two commit-history cleanup suggestions also remain.
Source: Reviewer backend models: gpt-5.6-sol (Codex general and dash-core-commit-history); 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 | 🟡 2 suggestion(s)

2 additional finding(s) omitted (not in diff).

🤖 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:591-595: Observer mode has no worker to perform the deferred BLS validation
  This framing-only check is also used by watch-only observer instances, but those instances have no path that performs the promised deferred typed validation. The observer constructor installs plain `CDKGSessionHandler` objects, `NetDKG::Start()` returns immediately when `m_active` is null, and `ClearPendingMessages()` is only called by the active-only `HandleDKGRound()`. A requested payload with framing-valid but invalid BLS encodings is therefore retained indefinitely without calling `DeserializeCheckedDKGMessage()`, scoring its sender, or clearing its bytes, seen hash, and proTxHash quota. Because membership validation is likewise worker-side, any MNAuth-verified masternode can fill these per-handler queues even when it is not a member of the targeted quorum. This regresses the previous synchronous typed intake rejection for malformed BLS encodings and makes the new nominally per-round proTxHash quota process-lifetime state for observers. Keep typed validation in observer mode, avoid retaining DKG payloads there, or add an observer lifecycle that validates or clears every queue at round boundaries.

In `test/util/data/non-backported.txt`:
- [SUGGESTION] test/util/data/non-backported.txt:1: Squash the manifest correction into the fuzz-target commit
  Commit `5d06f2e30f44` only adds the required non-backported manifest entry for `src/test/fuzz/dkg_message_framing.cpp`, which commit `863d53f08696` introduced. This is part of making that new Dash-specific target complete rather than a separate logical change. Squash `5d06f2e30f44` into `863d53f08696` so the fuzz target enters history with its required lint registration.

In `<commit:5f3ec10b2df>`:
- [SUGGESTION] <commit:5f3ec10b2df>:1: Fold the review-comment cleanup into the test commit
  Commit `5f3ec10b2dff` consists solely of corrections to tests introduced by `11cd6c4cb1a4`: it aligns three argument comments with the constructor parameter and removes an unused Python helper parameter. Its subject depends on temporary review context and does not represent an independent logical change. Fold it into `11cd6c4cb1a4` so the tests enter permanent history in their final form.

[PastaPastaPasta]

Applied the two commit-history suggestions as part of the force-with-lease update:

• test/util/data/non-backported.txt is now part of the fuzz-target commit 446c7c212084.
• The argument-comment and unused-helper-parameter cleanup is now part of the test commit 8aa661a14f12.

The PR now has three self-contained commits. The final tree is unchanged apart from the observer-mode fix and regression coverage, and passed the DKG unit suite plus feature_llmq_dkg_intake.py locally.

---

🤖 Posted autonomously by Codex on behalf of pasta.

[coderabbitai]

Actionable comments posted: 1

🤖 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 `@test/functional/feature_llmq_dkg_intake.py`:
- Line 130: Update the extra_args construction to use iterable unpacking instead
of list concatenation, preserving one entry with “-watchquorums=1” and three
entries containing common_args while resolving Ruff RUF005.

🪄 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: 192e95e0-779e-4190-8ee4-7eecc6261f21

📥 Commits

Reviewing files that changed from the base of the PR and between 5d06f2e and 8aa661a.

📒 Files selected for processing (4)

• src/llmq/net_dkg.cpp
• src/llmq/net_dkg.h
• src/test/llmq_dkg_tests.cpp
• test/functional/feature_llmq_dkg_intake.py

🚧 Files skipped from review as they are similar to previous changes (3)

• src/test/llmq_dkg_tests.cpp
• src/llmq/net_dkg.h
• src/llmq/net_dkg.cpp

[thepastaclaw]

Final validation — Codex/Sol only (Phase 2 disabled)

The observer-mode retention defect and both prior commit-history issues are fixed at the exact head. One non-blocking commit-message issue remains: the production and test commits omit the newly added observer-mode behavior and regression coverage. The CodeRabbit RUF005 comment is not applicable to this repository's flake8/mypy lint configuration.
Source: Reviewer backend models: gpt-5.6-sol (Codex general and dash-core-commit-history); final verifier backend model: gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.

Validated zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.

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/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
• Secondary pass: disabled (temporary_phase2_sonnet_disable)

🟡 1 suggestion(s)

1 additional finding(s) omitted (not in diff).

🤖 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 `<commit:6aa2f7833b6>`:
- [SUGGESTION] <commit:6aa2f7833b6>:1: Document the observer-mode behavior in the commit messages
  Commit `6aa2f7833b6` now makes observer nodes report DKG round inventory as already known and ignore directly delivered DKG payloads because they have no worker to validate or drain them. This is a substantive part of making deferred deserialization safe, but the production commit message only says that the worker becomes the sole typed-deserialization point. Commit `8aa661a14f1` also adds the corresponding observer regression test without mentioning it in its detailed test summary. Amend the production commit message to explain why observers no longer request or retain round payloads, and mention the observer coverage in the test commit message.