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.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR changes DKG pending-message handling to deduplicate before per-node quota accounting, removes header-side queued-message deserialization, and rewires Estimated code review effort: 4 (Complex) | ~50 minutes Sequence Diagram(s)sequenceDiagram
participant Peer
participant NetDKG
participant CDKGPendingMessages
participant PendingBatch
participant PreVerifyMessage
Peer->>NetDKG: ProcessMessage(vRecv)
NetDKG->>NetDKG: check wire size and wire structure
NetDKG->>CDKGPendingMessages: store serialized pending message
PendingBatch->>PendingBatch: deserialize queued CDataStream
PendingBatch->>PendingBatch: validate typed message
PendingBatch->>PreVerifyMessage: verify accepted message
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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
📒 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 029fcc2. 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 c7e3cad — still unresolved at head 029fcc2 (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. |
thepastaclaw
force-pushed
the
perf/dkg-single-deserialize
branch
from
506a5e4 to
55e1d5e
Compare
|
This pull request has conflicts, please rebase. |
thepastaclaw
force-pushed
the
perf/dkg-single-deserialize
branch
from
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.
|
thepastaclaw
force-pushed
the
perf/dkg-single-deserialize
branch
from
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.
|
![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 51cf424 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.
thepastaclaw
force-pushed
the
perf/dkg-single-deserialize
branch
from
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".
thepastaclaw
force-pushed
the
perf/dkg-single-deserialize
branch
from
ce4180d to
48de1d0
Compare
|
Updated What changed:
Local validation:
|
thepastaclaw
force-pushed
the
perf/dkg-single-deserialize
branch
from
c39cfca to
99ef16e
Compare
|
CI note for current head The failed job reports CodeRabbit is still paused on this branch and the latest command review covered an older head, so triggering one current-head review now. @coderabbitai review |
|
I'll trigger a fresh review of the current head ( ✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
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/net_dkg.cpp`:
- Around line 270-286: DeserializeAndCheckDKGMessage currently accepts
serialized DKG payloads with extra trailing bytes after ds >> *msg, which can
let the same logical message be treated as distinct. Update the
DeserializeAndCheckDKGMessage template in net_dkg.cpp to verify the stream is
fully consumed after deserialization, and reject the message if any unread bytes
remain; keep the existing structure check and reset msg on failure as it does
today.
🪄 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: 8c5ab992-4b94-448f-ae2f-030acfc930bf
📒 Files selected for processing (3)
src/llmq/dkgsessionhandler.cppsrc/llmq/dkgsessionhandler.hsrc/llmq/net_dkg.cpp
💤 Files with no reviewable changes (1)
- src/llmq/dkgsessionhandler.cpp
thepastaclaw
force-pushed
the
perf/dkg-single-deserialize
branch
from
99ef16e to
ea155ab
Compare
|
Updated What changed:
Local validation:
Replied to and resolved the review thread: #7401 (comment) @coderabbitai review |
|
✅ Action performedReview finished.
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Source: reviewers: opus/Claude general failed quota; gpt-5.5/Codex general completed; opus/Claude dash-core-commit-history failed quota; gpt-5.5/Codex dash-core-commit-history completed. Verifier: gpt-5.5/Codex fallback after opus verifier quota failure. Specialist: dash-core-commit-history.
Prior reconciliation: no carried-forward prior findings remain active. The quota-exceeded seenMessages poisoning finding remains fixed at ea155abd, and the benchmark/non-backported findings remain obsolete because the cumulative PR diff contains no benchmark, Makefile, or non-backported tracking changes.
New findings in latest delta: none. Additional cumulative findings: none. I checked the latest ce4180d1..ea155abd delta and the full DKG typed pending-queue/message-intake refactor and found no in-scope defects.
Note: GitHub does not allow me to submit APPROVE on my own PR, so this clean review is posted as a COMMENT.
|
@coderabbitai review |
✅ Action performedReview finished.
|
thepastaclaw
force-pushed
the
perf/dkg-single-deserialize
branch
from
ea155ab to
d1111fd
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d1111fdc9b
ℹ️ 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".
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Source: reviewers: opus/Claude general failed quota; gpt-5.5/Codex general completed; opus/Claude dash-core-commit-history failed quota; gpt-5.5/Codex dash-core-commit-history completed. Verifier: gpt-5.5/Codex fallback after opus verifier quota failure. Specialist: dash-core-commit-history.
Carried-forward prior findings: none; the prior review found no active issues. New findings in latest delta: one blocking regression in DKG message intake, where malformed verified-peer payloads are queued before structural validation and no longer trigger the existing immediate "malformed DKG message" rejection path. Additional cumulative findings: none.
🔴 1 blocking
Prior Reconciliation
Carried-forward prior findings: none. The prior exact-SHA review at ea155abd found no active issues, and the older quota/benchmark findings remained resolved or obsolete.
New Findings In Latest Delta
- [BLOCKING]
src/llmq/net_dkg.cpp:505-525: Malformed DKG payloads are queued before structural validation — The previous code deserialized and structurally checked each DKG payload in ProcessMessage before inserting it into a pending queue, and rejected failures with PeerMisbehaving(..., "malformed DKG message"). The current code moves the raw CDataStream into the pending queue first and only calls DeserializeAndCheckDKGMessage later from ProcessPendingMessageBatch. That regresses the existing intake hardening contract and functional tests: feature_llmq_dkg_intake.py asserts malformed and under-min QCONTRIB payloads are rejected during receipt with the "malformed DKG message" reason, before retention. The later worker path logs a different message and only penalizes when the queued batch is processed, so the current tree no longer satisfies the tested behavior.
Additional Cumulative Findings
None.
🤖 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:505-525: Malformed DKG payloads are queued before structural validation
The previous code deserialized and structurally checked each DKG payload in ProcessMessage before inserting it into a pending queue, and rejected failures with PeerMisbehaving(..., "malformed DKG message"). The current code moves the raw CDataStream into the pending queue first and only calls DeserializeAndCheckDKGMessage later from ProcessPendingMessageBatch. That regresses the existing intake hardening contract and functional tests: feature_llmq_dkg_intake.py asserts malformed and under-min QCONTRIB payloads are rejected during receipt with the "malformed DKG message" reason, before retention. The later worker path logs a different message and only penalizes when the queued batch is processed, so the current tree no longer satisfies the tested behavior.
Note: GitHub does not allow me to submit REQUEST_CHANGES on my own PR, so this is posted as a COMMENT while preserving the verified blocking finding.
thepastaclaw
force-pushed
the
perf/dkg-single-deserialize
branch
from
d1111fd to
e2265ef
Compare
|
Updated What changed:
Local validation:
Replied to and resolved the review thread: #7401 (comment) @coderabbitai review |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e2265ef810
ℹ️ 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".
CheckDKGMessageStructure previously deserialized a copy of the pushed DKG payload on the network thread to enforce param-derived structural bounds, then the original bytes were retained on the pending queue and deserialized again on the DKG worker thread. For QCONTRIB the BLS point decompression is the dominant cost, so paying it twice per message is meaningful. Intake now deserializes the payload once, runs the same param-derived bound check on the typed object, and enqueues an already-typed std::shared_ptr<Message>. The DKG worker no longer deserializes on pop. CDKGPendingMessages is turned into a class template parameterised on the message type so each queue stores its typed message directly. Preserved: - Oversize rejection with peer misbehavior score 100 before any deserialization or retention. - Malformed-payload rejection with score 100: a deserialize throw or a bound violation triggers the same ban path as before. - Inventory hash: still computed over the raw wire bytes at intake, matching what peers compute for the same payload; AlreadyHave / HasSeen / PeerEraseObjectRequest semantics unchanged. - EnqueueOwn still serializes with SER_NETWORK/PROTOCOL_VERSION and hashes those bytes, matching the old serialize-then-hash form. Benchmark notes for the removed old-vs-new simulation are preserved at https://gist.github.com/thepastaclaw/b612f49db3d2870d45c6a6ace5c4b0ad. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
thepastaclaw
force-pushed
the
perf/dkg-single-deserialize
branch
from
e2265ef to
d291993
Compare
|
Updated the branch for the latest Codex review.
@coderabbitai review |
|
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
2 participants
perf: deserialize DKG messages once
Issue being fixed or feature implemented
DKG network message intake currently deserializes each accepted payload twice:
message structure and bounds
queued message
These messages contain BLS objects, so the redundant pass repeats non-trivial
decompression work on a hot network path during DKG rounds.
What was done?
justifications, and premature commitments.
structural checks on the typed object, and queue that validated object for
the worker.
wire bytes.
Benchmark evidence
A temporary local benchmark compared representative old double-deserialize and
new single-deserialize costs for
QCONTRIBandQPCOMMITMENTpayloads. Thebenchmark was removed from the codebase because it modeled the deleted legacy
path and would be maintenance noise as a permanent
bench_dashtarget.Benchmark notes are preserved here:
https://gist.github.com/thepastaclaw/b612f49db3d2870d45c6a6ace5c4b0ad
Representative results:
This showed roughly a 2x reduction in the deserialization/decompression portion
of the DKG message intake path for representative payloads.
How Has This Been Tested?
Built on macOS arm64 with:
Additional validation:
Breaking Changes
None.
Checklist
code-owners and collaborators only)