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.
|
|
🔍 Review in progress — actively reviewing now (commit 45821d0) |
WalkthroughThis PR changes DKG pending-message deduplication and queue capacity handling, validates serialized payload structure before retention, defers typed deserialization until worker processing, and updates round processing. Unit and functional tests cover trailing bytes, 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 |
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
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
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.
|
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
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.
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:
|
|
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".
e2265ef to
d291993
Compare
|
Updated the branch for the latest Codex review.
@coderabbitai review |
|
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
✅ Action performedReview finished.
|
✅ Action performedReview finished.
|
✅ Action performedReview finished.
|
✅ Action performedReview finished.
|
✅ Action performedReview finished.
|
✅ Action performedReview finished.
|
✅ Action performedReviews resumed. |
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The exact head preserves bounded raw-byte queues, quota ordering, pre-queue malformed/trailing-byte rejection, and worker-side structural validation. It does not achieve the PR's stated performance goal: intake decompresses every BLS field and the worker decompresses the same fields again, while the new validity pass also creates a scheme-transition rejection path. The commit and PR descriptions still describe the superseded typed-queue implementation.
Validated blockers were found in the Codex precheck. Sonnet 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)
🤖 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:152-154: BLS wire validation restores the dominant double-deserialization cost
The intake checker calls `ReadAndCheckBLSObject` for every QCONTRIB verification-vector key, its ephemeral key, and its signature; QPCOMMITMENT similarly decodes its public key and both signatures. This invokes normal BLS deserialization and point decompression. Because the queue retains only `CDataStream`, `DeserializeAndCheckDKGMessage` later executes `ds >> *msg` and decompresses the same objects again. BLS decompression is the dominant cost identified by the PR and its benchmark, so the current implementation does not deliver the claimed single-deserialization or approximately 2x improvement. Validate malformed encodings without materializing all BLS objects at intake, or reuse the successfully decoded objects in the worker.
- [SUGGESTION] src/llmq/net_dkg.cpp:102-108: Intake BLS validation can reject a valid opposite-scheme encoding
`ds >> obj` selects the mutable process-wide `bls::bls_legacy_scheme`. `CBLSWrapper::Unserialize` deliberately resets an object when its bytes are canonical only under the opposite scheme, so `obj.IsValid()` fails and the verified sender receives a score of 100. `ProcessMessage` does not hold `cs_main` across this pass, while activation, reorganization, and failed or dry-run block validation can change and subsequently restore the global scheme. A message valid under the stable scheme can therefore be classified as malformed during a transient scheme change; the previous structural intake pass did not require temporary BLS objects to remain valid, allowing the worker to deserialize the queued bytes after the scheme stabilized. Select the scheme from stable quorum/chain context or validate the encoding independently of the mutable global flag.
In `<commit:d291993>`:
- [SUGGESTION] <commit:d291993>:1: Reword the commit to match the raw-queue implementation
Commit d291993e says intake enqueues an already-typed `std::shared_ptr<Message>`, makes `CDKGPendingMessages` a typed template, and removes worker deserialization. The exact head instead stores `std::shared_ptr<CDataStream>`, performs BLS-object deserialization during `CheckDKGMessageWireStructure`, and fully deserializes the message again in the worker. Update the subject, body, and PR description after settling the implementation so permanent history describes the code that will actually merge.
Note: the verifier’s canonical action is REQUEST_CHANGES, but GitHub does not allow an author to request changes on their own PR. This exact-SHA preliminary review is posted as COMMENT while preserving the validated blocker gate.
d291993 to
9561ac9
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9561ac9683
ℹ️ 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".
9561ac9 to
d5b4334
Compare
✅ Action performedReviews resumed. |
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
Exact-head review confirms that seven prior issues are fixed, the benchmark-tracking concern is outdated, and intake-side BLS decoding remains deliberately deferred. The latest delta introduces one blocking issue: stale queues are decoded synchronously and without a global or authenticated-identity bound before the next DKG round initializes. An authenticated masternode can renew the per-NodeId quota through reconnects and accumulate enough BLS-heavy messages to delay or prevent round participation.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (failed),gpt-5.6-sol— dash-core-commit-history (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— dash-core-commit-history (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— dash-core-commit-history (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— dash-core-commit-history (failed),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
🤖 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:520-527: Round-start drain performs an unbounded synchronous BLS workload
The normal phase path limits each batch to eight messages, but this new round-start path drains and deserializes every stale message before `InitNewQuorum`. Queue capacity is enforced per `NodeId`, while `CConnman` assigns every connection a fresh ID and MNAuth supplies no persistent per-ProTx quota; an authenticated masternode can therefore reconnect after the matching phase and add another `2 * params.size` messages. For a size-400 quorum, each connection can retain 800 distinct QCONTRIBs. Each payload can contain the required valid verification-vector public keys followed by an invalid final signature, forcing all preceding BLS point decoding before failure, while varying an unchecked field produces distinct inventory hashes. Messages from disconnected NodeIds are not penalized because `PeerMisbehaving` cannot find their peer state, and the loop continues decoding even when a peer is still connected. Bound stale work globally or by authenticated ProTx identity, or process it under a strict budget that cannot delay new-round initialization.
Note: the verifier’s canonical action is REQUEST_CHANGES, but GitHub does not allow an author to request changes on their own PR. This exact-SHA preliminary review is posted as COMMENT while preserving the validated blocker gate.
d5b4334 to
5d42157
Compare
|
Addressed the round-start stale-BLS workload in amended commit 5d42157: 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
📒 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
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>
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:
All build, lint, format, unit-test, and remaining functional-test jobs passed or were intentionally skipped. No rerun or branch mutation was performed. |
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: