fix: bound bitset allocation and stop deserializing DKG messages at intake - #7523
fix: bound bitset allocation and stop deserializing DKG messages at intake#7523PastaPastaPasta wants to merge 2 commits into
Conversation
WalkthroughThe change replaces unbounded DKG collection deserialization with Estimated code review effort: 3 (Moderate) | ~25 minutes 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 |
|
⛔ Blockers found — Sonnet deferred (commit 6651a6c) |
![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: d9d8f12131
ℹ️ 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".
| const bool ok = CheckDKGMessageStructure(NetMsgType::QCONTRIB, payload, params); | ||
| const auto elapsed = std::chrono::steady_clock::now() - t0; | ||
| BOOST_CHECK(ok); | ||
| BOOST_CHECK_MESSAGE(elapsed < kBudget, |
There was a problem hiding this comment.
On loaded or oversubscribed CI hosts, the test process can be descheduled for more than 2 ms between these timestamps, causing this assertion to fail even though the structural scan performs no BLS deserialization. Use a deterministic hook/counter to prove that BLS objects are not constructed, rather than enforcing a scheduler-dependent latency threshold for this DKG invariant.
AGENTS.md reference: AGENTS.md:L164-L175
Useful? React with 👍 / 👎.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
The production changes correctly replace eager DKG deserialization with a bounded structural scan and reject undersized DYNBITSET payloads before allocation. Serializer-driven tests provide strong coverage of the duplicated wire layouts, but the new 2 ms wall-clock assertion can fail when the test process is descheduled on a loaded CI host.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— final-verifier (fallback) - Sonnet reviewers:
claude-sonnet-5— general (failed),claude-sonnet-5— dash-core-commit-history (completed),claude-sonnet-5— general (completed)
🟡 1 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/test/llmq_dkg_intake_tests.cpp`:
- [SUGGESTION] src/test/llmq_dkg_intake_tests.cpp:80-85: Replace the 2 ms wall-clock assertion with a deterministic check
The measured interval includes arbitrary time while the test process is descheduled, so an oversubscribed CI worker can exceed 2 ms even when `CheckDKGMessageStructure` performs only the intended byte scan. This creates a scheduler-dependent unit-test failure unrelated to the production regression. Verify deterministically that BLS deserialization is not invoked, such as with a test hook or counter, or move the latency measurement to a benchmark rather than enforcing it in the unit suite.
| constexpr auto kBudget = std::chrono::milliseconds(2); | ||
| const auto t0 = std::chrono::steady_clock::now(); | ||
| const bool ok = CheckDKGMessageStructure(NetMsgType::QCONTRIB, payload, params); | ||
| const auto elapsed = std::chrono::steady_clock::now() - t0; | ||
| BOOST_CHECK(ok); | ||
| BOOST_CHECK_MESSAGE(elapsed < kBudget, |
There was a problem hiding this comment.
🟡 Suggestion: Replace the 2 ms wall-clock assertion with a deterministic check
The measured interval includes arbitrary time while the test process is descheduled, so an oversubscribed CI worker can exceed 2 ms even when CheckDKGMessageStructure performs only the intended byte scan. This creates a scheduler-dependent unit-test failure unrelated to the production regression. Verify deterministically that BLS deserialization is not invoked, such as with a test hook or counter, or move the latency measurement to a benchmark rather than enforcing it in the unit suite.
source: ['codex']
There was a problem hiding this comment.
Resolved in this update — Replace the 2 ms wall-clock assertion with a deterministic check no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
d9d8f12 to
1287130
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1287130735
ℹ️ 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".
| const CDataStream payload = BuildQContribPayload(params); | ||
| BOOST_CHECK(CheckDKGMessageStructure(NetMsgType::QCONTRIB, payload, params)); |
There was a problem hiding this comment.
After the earlier wall-clock assertion was removed, the fresh evidence is that every BLS value in this test is still created by CreateRandomBLS*, while the wrong-count case returns before reaching any BLS bytes. Reverting CheckDKGMessageStructure to the eager s >> CDKGContribution implementation would therefore leave the entire new suite passing, so the performance/DoS invariant this change is meant to protect has no regression coverage; use an invalid-but-structurally-sized BLS encoding or a construction counter/hook that distinguishes the byte scan from deserialization.
AGENTS.md reference: AGENTS.md:L164-L175
Useful? React with 👍 / 👎.
| bool CheckDKGMessageStructure(std::string_view msg_type, const CDataStream& vRecv, | ||
| const Consensus::LLMQParams& params); |
There was a problem hiding this comment.
we usually call this something like IsTriviallyValid
|
This pull request has conflicts, please rebase. |
DynamicBitSetFormatter read a CompactSize bit count straight from the wire and handed it to ReadFixedBitSet, which resized the bit vector and allocated a packed byte buffer for the full declared count before the short read threw. Five bytes could declare MAX_SIZE bits and drive an ~8 MiB allocation from a ~70-byte message, reachable from any unauthenticated peer through CFinalCommitment's signers/validMembers via QFCOMMITMENT. Replace it with LimitedBitSetFormatter<Limit>, mirroring the existing LimitedVectorFormatter, and bound every call site by Consensus::MAX_LLMQ_SIZE. Chainparams already rejects a quorum type larger than MAX_LLMQ_SIZE at startup, so the bound cannot reject honest data. CSigSharesInv applies the same limit to its AUTOBITSET; DYNBITSET simply never got it. The unbounded DYNBITSET macro is removed so a bitset cannot be deserialized without a limit. Serialization is unchanged, so the wire and disk formats are bit-identical.
952794e to
583e7aa
Compare
PastaPastaPasta
changed the title
CheckDKGMessageStructure deserialized the whole payload at intake to apply param bounds. CBLSWrapper::Unserialize is eager: it decompresses each G1/G2 point with a subgroup check and re-serializes for the malleability check, retrying under the opposite BLS scheme on mismatch. An llmq_400_85 QCONTRIB carries 340 pubkeys, so ~70 KB of wire bought hundreds of curve operations on the single net-processing thread, and an unsolicited sender could trigger it. Delete the check outright rather than reimplementing the wire walk. The pending queue retains raw bytes and CDKGPendingMessages::PushPendingMessage already bounds messages per peer, so retention is capped by that quota and the existing MaxDKGMessageSize cap. Deserialization then happens on the DKG worker in PopAndDeserializeMessages, which bans on failure, and PreVerifyMessage already applies bounds at least as strict as the ones the check duplicated. The net thread now parses nothing at all. The trade is that a malformed message occupies one of its sender's queue slots until the worker dequeues and bans it, rather than being rejected before retention; the per-peer quota bounds that. Bound the remaining wire-driven counts with LIMITED_VECTOR at Consensus::MAX_LLMQ_SIZE so the worker's deserialization cannot be made to allocate from a declared count either.
583e7aa to
6651a6c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/llmq/dkgmessages.h (1)
32-40: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAdd a count bound for
CDKGContribution::contributions::blobs.
CBLSIESMultiRecipientBlobs::blobsis astd::vector<std::vector<unsigned char>>deserialized through the generic vector path, so it is not constrained byConsensus::MAX_LLMQ_SIZE. Use a bounded deserialization path for this field and exercise it inoversized_vectors_rejected_through_message_serializers.🤖 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/llmq/dkgmessages.h` around lines 32 - 40, Update CDKGContribution serialization around SerializeWithoutSig to deserialize contributions->blobs through a bounded vector path using Consensus::MAX_LLMQ_SIZE, rather than the generic vector deserializer. Ensure the corresponding message-serializer test oversized_vectors_rejected_through_message_serializers covers this field and confirms oversized blob counts are rejected.
🤖 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.
Outside diff comments:
In `@src/llmq/dkgmessages.h`:
- Around line 32-40: Update CDKGContribution serialization around
SerializeWithoutSig to deserialize contributions->blobs through a bounded vector
path using Consensus::MAX_LLMQ_SIZE, rather than the generic vector
deserializer. Ensure the corresponding message-serializer test
oversized_vectors_rejected_through_message_serializers covers this field and
confirms oversized blob counts are rejected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 225dcdc3-f27e-4306-a016-b0d7071bf7a6
📒 Files selected for processing (8)
src/Makefile.test.includesrc/llmq/commitment.cppsrc/llmq/commitment.hsrc/llmq/dkgmessages.hsrc/llmq/net_dkg.cppsrc/serialize.hsrc/test/llmq_commitment_tests.cppsrc/test/llmq_dkg_intake_tests.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Makefile.test.include
|
latest LGTM |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The current-head production changes appear sound; carried-forward prior findings: none, and prior finding 5f125c5404fc is FIXED because the timing assertion and the intake-side DKG payload scan were removed. New latest-delta findings: one blocking commit-stack defect because f5238ab3715 removes DYNBITSET while three DKG serializers in that revision still use it, leaving the commit not independently buildable.
Source: reviewers gpt-5.6-sol (general and dash-core-commit-history); verifier gpt-5.6-sol; the openclaw-agent coordinator is orchestration-only.
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
🤖 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/serialize.h`:
- [BLOCKING] src/serialize.h:588: Move all DYNBITSET migrations into the commit that removes the macro
Commit `f5238ab3715` replaces the `DYNBITSET` macro at this line, but its tree still contains `DYNBITSET(obj.badMembers)`, `DYNBITSET(obj.complainForMembers)`, and `DYNBITSET(obj.validMembers)` in `src/llmq/dkgmessages.h`. That same revision instantiates these serializers from `CheckDKGMessageStructure` in `src/llmq/net_dkg.cpp`, so the undefined identifier causes compilation to fail. The conversions and required `llmq/params.h` include only arrive in child commit `6651a6c85cd`, contrary to `CONTRIBUTING.md` lines 81-82 requiring every individual commit to build successfully. Move the three DKG bitset conversions and required include into `f5238ab3715` while retaining the vector-bound and intake changes in the child commit, or squash the two commits.
| static inline Wrapper<Formatter, T&> Using(T&& t) { return Wrapper<Formatter, T&>(t); } | ||
|
|
||
| #define DYNBITSET(obj) Using<DynamicBitSetFormatter>(obj) | ||
| #define LIMITED_BITSET(obj,n) Using<LimitedBitSetFormatter<n>>(obj) |
There was a problem hiding this comment.
🔴 Blocking: Move all DYNBITSET migrations into the commit that removes the macro
Commit f5238ab3715 replaces the DYNBITSET macro at this line, but its tree still contains DYNBITSET(obj.badMembers), DYNBITSET(obj.complainForMembers), and DYNBITSET(obj.validMembers) in src/llmq/dkgmessages.h. That same revision instantiates these serializers from CheckDKGMessageStructure in src/llmq/net_dkg.cpp, so the undefined identifier causes compilation to fail. The conversions and required llmq/params.h include only arrive in child commit 6651a6c85cd, contrary to CONTRIBUTING.md lines 81-82 requiring every individual commit to build successfully. Move the three DKG bitset conversions and required include into f5238ab3715 while retaining the vector-bound and intake changes in the child commit, or squash the two commits.
source: ['codex']
|
Closing in favor of #7532 and #7401.
*(Note: This comment was generated with AI assistance.) |
2 participants
Issue being fixed or feature implemented
Two related intake problems in the DKG message path.
1. Eager BLS deserialisation on the message-handler thread.
llmq::CheckDKGMessageStructure()deserialised the whole pushed payload in order to apply a few quorum-param bounds.CBLSWrapper::Unserializeis eager: it performs G1 decompression with a subgroup check plus a re-serialise for the malleability check, and on mismatch retries with the opposite BLS scheme — up to two decompress/serialise pairs per element. Forllmq_400_85a QCONTRIB carries 340 G1 points, so roughly 70 KB of wire buys hundreds of curve operations, tens of milliseconds, on the single net-processing thread. Because this ran ahead of the unsolicited-payload check, an unsolicited sender could trigger it.2. Unbounded bitset allocation.
DynamicBitSetFormatterread a CompactSize bit count straight off the wire and handed it toReadFixedBitSet, which resized the bit vector and allocated a packed byte buffer for the full declared count before the short read threw. Five bytes on the wire declareMAX_SIZEbits and produce roughly 8 MiB peak for a ~70-byte message. This one is not DKG-specific: the same formatter carriesCFinalCommitment'ssigners/validMembers, reachable viaQFCOMMITMENTfrom any unauthenticated peer, and it is also on the disk-load path.Neither is bounded by
MAX_PROTOCOL_MESSAGE_LENGTHor the DKG message size cap, because both attacks are undersized messages.What was done?
Both are fixed with mechanisms the codebase already has, rather than with new intake-specific machinery.
Bound the bitset length before allocating.
DynamicBitSetFormatterbecomesLimitedBitSetFormatter<Limit>, mirroring the existingLimitedVectorFormatter, and every call site passesConsensus::MAX_LLMQ_SIZE.CSigSharesInvalready applies exactly this limit to itsAUTOBITSET;DYNBITSETsimply never got it. Chainparams rejects a quorum type larger thanMAX_LLMQ_SIZEat startup, so the bound cannot reject honest data. The unboundedDYNBITSETmacro is removed so a bitset cannot be deserialised without a limit. Serialisation is untouched — the wire and disk formats are bit-identical.Delete
CheckDKGMessageStructurerather than making it cheaper. It turned out to be redundant. The pending queue retains raw bytes; deserialisation already happens on the DKG worker inPopAndDeserializeMessages, which bans on failure, andPreVerifyMessagealready applies bounds at least as strict as the ones the intake check duplicated. Retention was already capped by the existingMaxDKGMessageSizecap and by the per-peer quota inCDKGPendingMessages::PushPendingMessage. Removing it means the message-handler thread now parses nothing at all, which is strictly stronger than parsing cheaply, and it removes a hand-maintained copy of each wire layout that could have drifted from the serialisers and rejected honest messages.The remaining wire-driven counts (
CDKGJustification::contributions,CDKGContribution'svvec) are bounded withLIMITED_VECTORatConsensus::MAX_LLMQ_SIZE, so worker-side deserialisation cannot be driven to allocate from a declared count either.Behaviour change worth noting: a structurally malformed DKG message is now retained until the worker dequeues it and bans, instead of being rejected before retention. The per-peer quota bounds that, and the sender is still banned.
Known limitation:
CBLSIESMultiRecipientBlobs::blobsis not given an explicitLIMITED_VECTORbound, because that type lives insrc/bls/bls_ies.hand bounding it there would make a BLS header depend on LLMQ params. It stays bounded by theMaxDKGMessageSizeintake cap, by the generic 5 MiB batched-allocation path every unguarded vector relies on, and byPreVerifyMessage's exactblobs.size() == members.size()check.How Has This Been Tested?
llmq_dkg_intake_testscovers the bitset limit at and past its boundary, the limit as reached through the realCDKGComplaint/CDKGPrematureCommitment/CFinalCommitmentserialisers, theLIMITED_VECTORbounds on the DKG messages, and round-trips of every DKG message type at both the smallest and largest quorum size — including a full 340-entryvveccontribution. The negative cases assert the limit's own failure message rather than justios_base::failure, since a short read throws the same type and would otherwise mask a removed bound.Full build and test validation is delegated to CI on this PR; locally the touched translation units were only syntax-checked with clang.
Breaking Changes
None. Serialisation is unchanged, so wire and disk formats are bit-identical.
Checklist: