perf: deserialize DKG messages once - #7401
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
✅ Final review complete — no blockers (commit 0c255ab) |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
WalkthroughThis PR changes DKG pending-message deduplication and sender/queue capacity handling, validates serialized payload structure before retention, defers typed deserialization until worker processing, and updates round processing. Fuzz, unit, and functional tests cover framing, malformed BLS keys, duplicate accounting, queue bounds, worker rejection, and late-message clearing. Estimated code review effort: 4 (Complex) | ~50 minutes Sequence Diagram(s)sequenceDiagram
participant Peer
participant NetDKG
participant CDKGPendingMessages
participant ProcessPendingMessageBatch
participant PreVerifyMessage
Peer->>NetDKG: ProcessMessage serialized payload
NetDKG->>NetDKG: Validate size and wire structure
NetDKG->>CDKGPendingMessages: Enqueue serialized payload
ProcessPendingMessageBatch->>CDKGPendingMessages: Pop serialized payload
ProcessPendingMessageBatch->>ProcessPendingMessageBatch: Deserialize and validate
ProcessPendingMessageBatch->>PreVerifyMessage: Forward accepted message
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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)
src/bench/dkg_deserialize.cpp (1)
115-120: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMirror the worker allocation in the double-deserialize benchmark.
The single-deserialize path includes
std::make_shared, but the simulated old worker deserialize uses stack objects. If the removed worker path materialized shared typed messages, usemake_sharedhere too so the comparison measures only the intended deserialize-count delta.Proposed adjustment
- llmq::CDKGContribution qc; - s >> qc; - ankerl::nanobench::doNotOptimizeAway(qc.contributions->blobs.size()); + auto qc = std::make_shared<llmq::CDKGContribution>(); + s >> *qc; + ankerl::nanobench::doNotOptimizeAway(qc->contributions->blobs.size());- llmq::CDKGPrematureCommitment qc; - s >> qc; - ankerl::nanobench::doNotOptimizeAway(qc.validMembers.size()); + auto qc = std::make_shared<llmq::CDKGPrematureCommitment>(); + s >> *qc; + ankerl::nanobench::doNotOptimizeAway(qc->validMembers.size());Also applies to: 151-155
🤖 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/bench/dkg_deserialize.cpp` around lines 115 - 120, The old worker deserialize benchmark path in the double-deserialize setup is not mirroring the shared-allocation behavior used by the single-deserialize path. Update the worker-side deserialize blocks in the benchmark function(s) that use CDataStream and llmq::CDKGContribution so they materialize the typed message with std::make_shared, matching the removed worker path’s allocation pattern. Keep the deserialize-count comparison focused on the intended cost difference by aligning the allocation behavior between the single and double-deserialize cases.
🤖 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/llmq/dkgsessionhandler.h`:
- Around line 84-94: In CDKGPendingMessages, the duplicate-hash check currently
happens after incrementing messagesPerNode[from], so repeated duplicates can
consume the peer quota without adding work. Move the seenMessages.emplace(hash)
check ahead of the quota accounting in the same flow, then only increment
messagesPerNode[from] and continue enqueueing when the hash is new; keep the
existing logging and early returns in CDKGPendingMessages::AddMessage (or the
corresponding message-handling method).
---
Nitpick comments:
In `@src/bench/dkg_deserialize.cpp`:
- Around line 115-120: The old worker deserialize benchmark path in the
double-deserialize setup is not mirroring the shared-allocation behavior used by
the single-deserialize path. Update the worker-side deserialize blocks in the
benchmark function(s) that use CDataStream and llmq::CDKGContribution so they
materialize the typed message with std::make_shared, matching the removed worker
path’s allocation pattern. Keep the deserialize-count comparison focused on the
intended cost difference by aligning the allocation behavior between the single
and double-deserialize cases.
🪄 Autofix (Beta)
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
Run ID: 7c05047c-80ac-49ea-b4c5-0035e6ec8d3f
📥 Commits
Reviewing files that changed from the base of the PR and between 59d36a9 and c7e3cad4935c2e957f8806b8583716d82ca842da.
📒 Files selected for processing (5)
src/Makefile.bench.includesrc/bench/dkg_deserialize.cppsrc/llmq/dkgsessionhandler.cppsrc/llmq/dkgsessionhandler.hsrc/llmq/net_dkg.cpp
💤 Files with no reviewable changes (1)
- src/llmq/dkgsessionhandler.cpp
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Clean, well-scoped perf refactor eliminating a redundant DKG deserialize. All intake invariants (oversize ban, malformed ban, hash-over-wire-bytes, per-node/duplicate suppression, own-message enqueue form) are preserved and independently verified by multiple reviewers. Only remaining suggestion is a Dash-specific lint tracking update for the new benchmark file.
Source: reviewers: opus (general), claude-sonnet-5 (general), gpt-5.5[high] (general), opus (dash-core-commit-history), claude-sonnet-5 (dash-core-commit-history), gpt-5.5[high] (dash-core-commit-history); verifier: opus; specialists: dash-core-commit-history
🟡 1 suggestion
Findings not posted inline (1)
These findings could not be anchored to the current diff, but they are still part of this review.
- [SUGGESTION]
test/util/data/non-backported.txt:4: Add the new Dash-specific benchmark to non-backported tracking — This PR addssrc/bench/dkg_deserialize.cpp, a Dash-specific benchmark that is not matched by the existingsrc/bench/bls*.cppglob.test/lint/lint-cppcheck-dash.py(line 69-73) builds its Dash-specific file list exclusively from patterns in this file viagit ls-files, so the new benchmark will be silently skipped by that extra lint coverage until it is added. Add a matching entry to keep Dash-specific bench files under cppcheck.
🤖 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 `test/util/data/non-backported.txt`:
- [SUGGESTION] test/util/data/non-backported.txt:4: Add the new Dash-specific benchmark to non-backported tracking
This PR adds `src/bench/dkg_deserialize.cpp`, a Dash-specific benchmark that is not matched by the existing `src/bench/bls*.cpp` glob. `test/lint/lint-cppcheck-dash.py` (line 69-73) builds its Dash-specific file list exclusively from patterns in this file via `git ls-files`, so the new benchmark will be silently skipped by that extra lint coverage until it is added. Add a matching entry to keep Dash-specific bench files under cppcheck.
There was a problem hiding this comment.
Code Review
Two suggestions remain against head 029fcc228b. New finding in the latest delta: the dedupe-before-quota reorder correctly fixes duplicate resend quota charging, but a quota-exceeded drop now records the hash in seenMessages, so the same message can be poisoned for later legitimate delivery paths in the round. Carried-forward prior finding: src/bench/dkg_deserialize.cpp is still missing from test/util/data/non-backported.txt, so the extra cppcheck-dash lint pass will silently skip the new Dash-specific benchmark. No blocking issues; the benchmark allocation-shape fix is otherwise correct and well-scoped.
Source: reviewers: opus (general), claude-sonnet-5 (general), gpt-5.5[high] (general, failed), opus (dash-core-commit-history), claude-sonnet-5 (dash-core-commit-history), gpt-5.5[high] (dash-core-commit-history, failed); verifier: opus; specialists: dash-core-commit-history
🟡 2 suggestion(s)
Findings not posted inline (1)
These findings could not be anchored to the current diff, but they are still part of this review.
- [SUGGESTION]
test/util/data/non-backported.txt:4: Track new Dash-specific benchmark in non-backported.txt — This PR addssrc/bench/dkg_deserialize.cpp, a Dash-specific benchmark that is not matched by the existingsrc/bench/bls*.cppglob on line 4.test/lint/lint-cppcheck-dash.pybuilds its Dash-specific file list exclusively from the patterns in this file viagit ls-files, so the new benchmark will be silently excluded from the extra cppcheck-dash coverage until it is listed here. Carried forward from the prior automated review at c7e3cad4 — still unresolved at head 029fcc228b (the tracked file was not touched by the latest push).
🤖 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.
New finding in the latest delta:
In `src/llmq/dkgsessionhandler.h`:
- [SUGGESTION] src/llmq/dkgsessionhandler.h:82-100: Quota-exceeded drop still poisons the message hash in seenMessages
The reordered `PushPendingMessage` inserts `hash` into `seenMessages` at line 86 via `emplace(hash).second` before the quota check at line 92. If a sender is already at `maxMessagesPerNode` distinct-message quota, the function returns after having already committed the hash to `seenMessages`. Because `seenMessages` is keyed only by hash (not by sender), the same message subsequently delivered from any other peer — or via `AlreadyHave`/`HasSeen` paths — is now treated as "already seen" for the rest of the round and can never reach `pendingMessages`.
This is a narrower variant of the bug fixed in e8d9b6d803: that commit correctly stopped duplicate resends from silently consuming a peer's quota, but the current ordering trades it for a case where a quota-exceeded drop poisons the hash for other, legitimate delivery paths. DKG already tolerates a single peer's messages being lost, so the operational impact is small, but the current shape unnecessarily couples the two intake failure modes. Splitting the seen-check (read-only) from the seen-insert (commit) so the hash is only recorded once the message is actually accepted preserves both invariants.
Carried-forward prior finding:
In `test/util/data/non-backported.txt`:
- [SUGGESTION] test/util/data/non-backported.txt:4: Track new Dash-specific benchmark in non-backported.txt
This PR adds `src/bench/dkg_deserialize.cpp`, a Dash-specific benchmark that is not matched by the existing `src/bench/bls*.cpp` glob on line 4. `test/lint/lint-cppcheck-dash.py` builds its Dash-specific file list exclusively from the patterns in this file via `git ls-files`, so the new benchmark will be silently excluded from the extra cppcheck-dash coverage until it is listed here. Carried forward from the prior automated review at c7e3cad4 — still unresolved at head 029fcc228b (the tracked file was not touched by the latest push).
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Both prior findings are cleanly resolved by the latest delta: PushPendingMessage now only marks a hash as seen after the per-node quota check passes (no more quota-drop poisoning), and src/bench/dkg_deserialize.cpp is now tracked in non-backported.txt. No new issues in the delta; the full PR stack (single-deserialize refactor, benchmark, dedupe/quota fix) preserves the documented intake invariants. Commit-history agents suggested squashing fixup commits, but Dash merges PRs without squashing and these are low-value stylistic notes that don't warrant blocking.
Source: reviewers: opus (general), claude-sonnet-5 (general), gpt-5.5[high] (general, failed), opus (dash-core-commit-history), claude-sonnet-5 (dash-core-commit-history), gpt-5.5[high] (dash-core-commit-history, failed); verifier: opus; specialists: dash-core-commit-history
|
CI note: the current Build source failures are the same depends-cache infrastructure issue tracked in #7403, not a failure in this PR's DKG changes. The source jobs are exiting before compilation at actions/cache/restore with fail-on-cache-miss for freshly built depends cache keys, for example run 28630838726 / linux64-build job 84908995980. No branch change is appropriate for this PR; the workflow/cache fix belongs in #7403. |
506a5e4 to
55e1d5e
Compare
|
This pull request has conflicts, please rebase. |
55e1d5e to
98f20b1
Compare
|
Rebased this PR onto current Preserved the single-deserialize typed queue path, kept the upstream contribution Validation run locally on macOS arm64:
@coderabbitai review |
|
✅ Action performedReview finished.
|
98f20b1 to
51cf424
Compare
|
Reworked this PR to remove the temporary old-vs-new benchmark from the codebase. Current head
The benchmark evidence is preserved in the PR description and in this gist: https://gist.github.com/thepastaclaw/b612f49db3d2870d45c6a6ace5c4b0ad Validation run locally on macOS arm64: ./autogen.sh
./configure --without-gui --disable-tests --disable-bench \
--disable-wallet --without-miniupnpc --without-natpmp
make -C src llmq/libbitcoin_node_a-net_dkg.o -j4
make -C src llmq/libbitcoin_node_a-dkgsessionhandler.o -j4
python3 test/lint/lint-includes.py
python3 test/lint/lint-circular-dependencies.py
git diff --check upstream/develop
COMMIT_RANGE=upstream/develop..HEAD test/lint/lint-whitespace.py
npx markdownlint-cli2 /tmp/dash-7401-body-updated.md |
|
✅ Action performedReview finished.
|
PastaPastaPasta
left a comment
PastaPastaPasta
left a comment
There was a problem hiding this comment.
utACK 51cf424a2167d106c7677d4d7b8b444989396bfe
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 51cf424a21
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
I verified the current head 51cf424a2167d106c7677d4d7b8b444989396bfe against the active reviewer outputs and the older-SHA findings. The current PR diff is confined to the DKG typed pending-queue refactor; the quota poisoning issue is fixed, and the benchmark/non-backported findings are obsolete because src/bench/dkg_deserialize.cpp and test/util/data/non-backported.txt are no longer in the PR diff. No in-scope findings remain.
Source: reviewers: opus (claude general, failed), gpt-5.5 (codex general), opus (claude dash-core-commit-history, failed), gpt-5.5 (codex dash-core-commit-history); verifier: gpt-5.5 (codex; fallback: claude verifier unavailable); specialists: dash-core-commit-history
Prior finding reconciliation
- Quota-exceeded drop still poisons the message hash in seenMessages — Fixed in the current code. src/llmq/dkgsessionhandler.h now performs a read-only duplicate check with seenMessages.count(hash) before the quota gate, returns on quota overflow without touching seenMessages, and only calls seenMessages.emplace(hash) after the quota check passes immediately before enqueue.
- Track new Dash-specific benchmark in non-backported.txt — Obsolete at the assigned head. src/bench/dkg_deserialize.cpp is no longer present or tracked, and test/util/data/non-backported.txt is not part of the current PR diff.
- Add the new Dash-specific benchmark to non-backported tracking — Obsolete at the assigned head for the same reason: the benchmark-only files from the older review are no longer in the PR diff.
Note: Claude/Opus lanes were unavailable due quota during this run; Codex reviewer and verifier lanes completed.
51cf424 to
ce4180d
Compare
|
Updated
Local validation:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce4180d18e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
ce4180d to
48de1d0
Compare
|
Updated What changed:
Local validation:
|
d5b4334 to
5d42157
Compare
|
Addressed the round-start stale-BLS workload in amended commit 5d42157166e: each raw message-type queue is now capped globally across remote NodeIds, reserves one local-message slot, avoids rejected-ID metadata growth, and clears bounded stale raw bytes without typed/BLS deserialization. Added queue-level churn/cap/reset coverage and an end-to-end reconnect-overflow test. Targeted build, llmq_dkg_tests, feature_llmq_dkg_intake.py, Python/include/circular/whitespace lints, clang-format diff, and diff checks pass. Fresh exact-head CI and automated review follow-through are running. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5d42157166
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
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 `@src/llmq/dkgsessionhandler.cpp`:
- Around line 46-50: Update the pending-queue capacity logic in the DKG session
handler to count remote entries independently of the local entry, so remote
messages can always use the full maxMessagesPerNode allowance regardless of
enqueue order while still enforcing the total maxPendingMessages limit. Preserve
the existing local enqueue behavior and Dash-specific DKG handling, and add a
focused unit test covering local-first enqueue followed by the maximum remote
messages.
🪄 Autofix (Beta)
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
Run ID: b3d3c15b-d49b-4699-a70b-0dc569245709
📥 Commits
Reviewing files that changed from the base of the PR and between d5b433419fd45a3211db0598ec3d30f857fa3350 and 5d42157166e77017adf4a6eca484b9e17d6f89a5.
📒 Files selected for processing (5)
src/llmq/dkgsessionhandler.cppsrc/llmq/dkgsessionhandler.hsrc/llmq/net_dkg.cppsrc/test/llmq_dkg_tests.cpptest/functional/feature_llmq_dkg_intake.py
5d42157 to
45821d0
Compare
|
Addressed both queue-cap review findings in the amended single commit 45821d0:
Validation: focused unit tests passed, feature_llmq_dkg_intake.py passed, diff/py_compile checks passed, and the clean code-review gate returned ship with zero findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 45821d078a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
Exact-head CI is complete for Two functional-test failures are unrelated to this DKG-only diff:
A later rerun of the failed multiprocess job did not reach tests: All build, lint, format, unit-test, and remaining functional-test jobs passed or were intentionally skipped. No rerun or branch mutation was performed. |
|
@PastaPastaPasta The current head |
Keep incoming DKG messages as exact raw wire bytes after cheap framing validation. The DKG worker remains the sole normal typed and BLS deserialization point, avoiding repeated point decoding on the message-handler thread. Preserve authenticated-only intake, parameter-derived size and framing checks, exact-wire inventory hashes, duplicate-before-quota ordering, worker preverification, malformed-message scoring in the matching phase, and own-message processing. Bound each raw message-type queue across all remote NodeIds to four times the quorum size, allowing two peers to use their full per-node quotas, with one reserved slot for the local phase message. New-round cleanup discards bounded stale raw bytes without deserializing them, so reconnect churn cannot create an unbounded synchronous BLS workload before quorum initialization. Add unit coverage for per-node and queue-wide caps, duplicate ordering, local-message capacity, and round clearing. Update the focused functional test to cover worker-deferred BLS rejection plus bounded late-message retention across reconnect-generated NodeIds and prompt next-round initialization. Co-Authored-By: Claude <noreply@anthropic.com>
45821d0 to
5580082
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
Independently verified against the exact checked-out head (5580082): the DKG message-intake redesign is correct and matches both agents' claims. Wire-structure validation (CheckContributionWireStructure/CheckComplaintWireStructure/CheckJustificationWireStructure/CheckPrematureCommitmentWireStructure) skips fixed-size BLS blobs via SkipBytes without ever constructing/decompressing a BLS object, the DKG worker's DeserializeAndCheckDKGMessage remains the sole typed+BLS deserialization point, per-node/queue-wide caps (size2 / size4+1) are enforced in PushPendingMessage, round-start CDKGPendingMessages::Clear() is an O(1) container clear with no deserialization, and NetDKG::ProcessMessage rejects oversized/malformed payloads before the CDataStream is ever constructed or pushed into the pending queue. All five carried-forward prior findings are confirmed FIXED with concrete code evidence at this exact head, and no new blocking or suggestion-level issues were identified in the range.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
claude-sonnet-5— final-verifier - Sonnet reviewers:
claude-sonnet-5— general (failed),claude-sonnet-5— dash-core-commit-history (failed),claude-sonnet-5— dash-core-commit-history (completed),claude-sonnet-5— general (failed),claude-sonnet-5— general (completed)
Note: the verifier’s canonical action is APPROVE, but GitHub does not allow an author to approve their own PR. This exact-SHA final review is posted as COMMENT without changing the canonical result.
|
@PastaPastaPasta Current head |
Potential PR merge conflictsThis 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 firstThese open PRs will likely need a rebase:
If these PRs merge firstThis PR will likely need a rebase:
|
|
pls consider commits from https://github.com/UdjinM6/dash/commits/pr7401-review/ |
ReadAndCheckDynBitset materialized a byte vector per dynamic bitset just to inspect the final padding byte. Skip to the last byte instead -- CDataStream already bounds-checks ignore() and throws on overrun -- and return the bit count as an optional rather than a bool plus an out-parameter. Turn the typed CheckDKGMessageStructure explicit specializations into plain overloads and lift them, together with CheckDKGMessageWireStructure, out of the anonymous namespace so a fuzz target can reach them. Overloads fail at compile time rather than link time when a new message type is added. The header comment records that the walk mirrors the (Un)serialize implementations in llmq/dkgmessages.h by hand and must be kept in step with them. The walk still validates a copy of the payload rather than a view of it. The copy is a memcpy bounded by the size cap applied just before the call, which is noise next to the BLS point decompression this code path exists to avoid; eliminating it would need SpanReader::ignore(), which Dash does not have yet (bitcoin#28721 / bitcoin#34483) and which belongs in its own backport rather than here. No behaviour change: the walk accepts and rejects exactly the same payloads.
DKG intake validates framing without materializing BLS objects, then the worker deserializes the retained bytes exactly once. Those are two hand-maintained parsers over one wire format, so they can drift apart. The safety-critical direction is that framing must never reject a payload the worker would accept: honest DKG messages would then be dropped at intake and quorum formation would degrade. Assert exactly that, over all four message types, every entry in available_llmqs, and both BLS schemes. The converse is deliberately not asserted -- framing accepts payloads whose BLS points fail to decode so the worker can score the sender. Also build a well-formed message from fuzzer-chosen field values, serialize it, and require framing to accept our own serializer's output. That catches drift from an empty corpus, without needing the fuzzer to synthesize a valid BLS encoding by chance. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The worker logged "failed to deserialize message" for two different outcomes: a payload whose BLS points or canonical encoding failed to decode, and one that decoded fine but violated a param-derived bound. Both score the sender 100, but they say different things about the peer, and the framing walk at intake has already ruled out plain malformed framing by the time either fires. Return an explicit result from DeserializeAndCheckDKGMessage and log the two cases separately. The out-parameter stays because the result now carries information the pointer cannot. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
_start_fresh_dkg_cycle generated up to a full cycle of blocks with plain generate(), leaving mocktime behind while the DKG phase clock advanced. Every other block move in this test uses move_blocks, which bumps mocktime first; use it here too so phase timing cannot drift. Hoist the hardcoded cycle length into a named module constant while touching both of its users. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
CDKGPendingMessages charged its per-sender retention quota against the NodeId. A peer that disconnects and reconnects gets a fresh NodeId, so it started each connection with a full budget and could hold far more than size*2 messages per round by cycling connections. Key the quota by the sender's MNAuth-verified proTxHash instead. Every pusher on this path is already MNAuth-gated -- CMNAuth::ProcessMessage requires the proTxHash to resolve in the deterministic MN list and to carry a valid operator-key signature, and allows one MNAUTH per connection -- so the identity survives reconnects and is not attacker-inflatable. Own messages pass a null hash and stay exempt from the quota while remaining subject to the queue-wide caps. The map is deliberately not pruned when the worker drains the queue: the quota is cumulative for the round, so that draining does not refund retention slots. Its size is therefore bounded by the registered masternode set rather than by the queue caps, which the declaration now records. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@UdjinM6 Thanks — I reviewed and took all five commits unchanged via cherry-pick, preserving your authorship:
Validation passed: focused builds, I also updated the PR description to reflect the proTxHash-keyed quota. Current head is |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 981e8196d0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const auto decode_result = DeserializeAndCheckDKGMessage(*p.second, params, msg); | ||
| if (decode_result != DKGMessageDecodeResult::Ok) { |
There was a problem hiding this comment.
When an MNAuth peer sends framing-valid but BLS-invalid messages before the matching phase and disconnects before this deferred decode, the queue retains only its NodeId; PeerManagerImpl::PeerMisbehaving later calls GetPeerRef(nodeId) and silently does nothing once that peer has been removed. The sender can therefore force the allowed BLS-decompression workload every round without ever being discouraged. Fresh evidence relative to the earlier late-payload thread is the disconnect window: before this change, intake deserialized the BLS objects and scored the still-connected peer synchronously. Retain an identity against which deferred failures can be enforced, or validate before losing the connection context.
AGENTS.md reference: AGENTS.md:L160-L166
Useful? React with 👍 / 👎.
Capture the MNAuth-verified proTxHash once at intake and reuse it for the per-sender quota, use the codebase's salted uint256 hash map for sender accounting, and retain the existing malformed-message assertion alongside the new worker failure classes. Also describe the exposed framing helpers as test-facing rather than fuzz-only. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Thanks — I reviewed all five commits against the current head and adopted all of them. The resulting stack keeps your authorship and includes a small follow-up tightening:
Local validation at
I also mutation-checked the reconnect regression: changing the quota back to |
✅ Action performedReviews resumed. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0c255ab7a1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| maxMessagesPerSender(_maxMessagesPerSender), | ||
| // Belt-and-braces bound on live queue occupancy. The per-sender quota is | ||
| // the primary limit; this only caps the total across distinct senders. | ||
| maxPendingRemoteMessages(_maxMessagesPerSender * 2), |
There was a problem hiding this comment.
Fresh evidence after the one-sender fix is that the final constructor still makes the global remote cap exactly two full per-sender quotas: _maxMessagesPerSender is 2 * params.size, and this multiplier limits the queue to 4 * params.size. Two MNAuth-verified masternodes—including registered nodes outside the target quorum—can therefore fill a future-phase queue with unique, framing-valid messages before its worker starts; subsequent valid member messages hit pending queue full and are dropped, allowing two identities to disrupt that DKG round. Size the cap for the intended Byzantine population or cheaply account messages by their claimed quorum member before reserving slots.
AGENTS.md reference: AGENTS.md:L160-L160
Useful? React with 👍 / 👎.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
Carried-forward: all five prior findings (round-start unbounded BLS deserialization, double deserialization, opposite-scheme rejection, stale commit-message complaint, malformed-before-queue) remain FIXED at current head 0c255ab; none are STILL_VALID, so none re-enter canonical findings. New latest-delta findings: both agent checkpoints converge on one suggestion — the new Dash-specific fuzz target src/test/fuzz/dkg_message_framing.cpp is not covered by test/util/data/non-backported.txt (whose only relevant pattern, src/test/llmq*.cpp, does not match the src/test/fuzz/ subdirectory), so it is silently skipped by the Dash-specific cppcheck lint pass. The commit-history follow-up-squash suggestion was raised by both agents but is dropped as not actionable given documented intentional cherry-pick-preserving integration practice on this repo.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
claude-sonnet-5— final-verifier - Sonnet reviewers:
claude-sonnet-5— general (completed),claude-sonnet-5— dash-core-commit-history (failed),claude-sonnet-5— dash-core-commit-history (completed)
🟡 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 `test/util/data/non-backported.txt`:
- [SUGGESTION] test/util/data/non-backported.txt:66: Register the new Dash-specific fuzz target as non-backported
This PR adds src/test/fuzz/dkg_message_framing.cpp, a Dash-specific fuzz target that did not originate upstream (it fuzzes llmq/net_dkg.h and llmq/dkgmessages.h). The existing manifest entry `src/test/llmq*.cpp` only matches files directly under src/test/ — it does not match the src/test/fuzz/ subdirectory, and grepping the file confirms no pattern currently covers it. Because test/lint/lint-cppcheck-dash.py builds its Dash-specific cppcheck input list from this manifest, the new target is silently excluded from that additional lint pass. Add `src/test/fuzz/dkg_message_framing.cpp` (or a `src/test/fuzz/dkg_*.cpp` pattern) to the manifest.
3 participants
Issue being fixed or feature implemented
DKG network message intake currently 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. Because these messages contain BLS objects, the redundant intake pass repeats point decompression on a hot network path during DKG rounds.
Tracker: https://github.com/thepastaclaw/tracker/issues/1726
What was done?
Benchmark evidence
A temporary local benchmark compared representative old double-deserialize and new single-deserialize costs for
QCONTRIBandQPCOMMITMENTpayloads. The benchmark was removed from the codebase because it modeled the deleted legacy path and would be maintenance noise as a permanentbench_dashtarget. The benchmark notes are preserved at https://gist.github.com/thepastaclaw/b612f49db3d2870d45c6a6ace5c4b0ad.Representative results:
This showed roughly a 2x reduction in the deserialization/decompression portion of the DKG message path for representative payloads.
How Has This Been Tested?
Validated on macOS arm64 with:
Breaking Changes
None.
Checklist: