fix: bound DYNBITSET allocation against remaining stream size by PastaPastaPasta · Pull Request #7532 · dashpay/dash

PastaPastaPasta · 2026-08-03T14:54:15Z

Issue being fixed or feature implemented

ReadFixedBitSet allocated from a wire-declared CompactSize with no bound beyond ReadCompactSize's 33,554,432 cap. Roughly five bytes on the wire — a CompactSize claiming millions of bits, with no payload following — therefore forced a std::vector<bool> resize plus a byte buffer totalling several MiB, all of which was only abandoned when the subsequent short read threw.

MAX_PROTOCOL_MESSAGE_LENGTH does not help here, because the attack uses an undersized message.

The primitive is reachable from an unauthenticated QFCOMMITMENT via CFinalCommitment's signers and validMembers bitsets, and applies to every other DYNBITSET caller as well.

This was split out of #7523, where it was bundled with a much larger and more contentious DKG-intake change. The bound stands on its own, so it is offered separately for independent review.

What was done?

Bound the declared length against the bytes actually remaining in the stream, before allocating anything. A well-formed message always carries exactly the required bytes, so this rejects only claims that could never have been satisfied.

The guard needs the stream to report how many bytes are still available, so ReadFixedBitSet is constrained on a SizedStream concept. That constraint is deliberately a hard requirement rather than a fallback: a stream without size() — CHashVerifier, for instance — now fails to compile here instead of silently deserializing an unbounded bitset. An earlier revision duck-typed the check with if constexpr, which would have let a future caller quietly lose the bound with no diagnostic.

Nothing in tree is affected, because every stream that reaches this path already reports bytes remaining: DataStream (vch.size() - m_read_pos), SpanReader, and OverrideStream, which forwards to its wrapped stream. That "remaining, not total" requirement is what the bound rests on, so it is documented on the concept itself — a stream whose size() meant something else would weaken the bound silently rather than fail to compile.

Note on what is deliberately not included: an earlier revision also checked nbytes > MAX_SIZE. That check is dead code — size comes from ReadCompactSize, which already caps at 33,554,432, so (size + 7) / 8 can never exceed 4 MiB and the condition is always false.

How Has This Been Tested?

src/test/serialize_tests.cpp covers the amplification case (a declared length far exceeding the remaining stream must be rejected before allocating, not by a short read afterwards) and a maximum-size bitset (MAX_SIZE, 33,554,432 bits) round-tripping unchanged.

Built locally and test_dash --run_test=serialize_tests passes. Full validation is delegated to CI.

Breaking Changes

None. Only messages declaring a bit count that the message could not possibly contain are affected, and those were never valid.

Checklist:

I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have added or updated relevant unit/integration/functional/e2e tests
I have made corresponding changes to the documentation
I have assigned this pull request to a milestone

thepastaclaw · 2026-08-03T14:57:08Z

✅ Final review complete — no blockers (commit aa1447f)

CheckDKGMessageStructure runs on the message-handler thread before a pushed DKG message is retained, and it validated by deserializing a copy of the payload. CBLSWrapper::Unserialize is eager: it decompresses each point, re-serializes it for the malleability check, and retries with the opposite scheme on mismatch. For llmq_400_85 a QCONTRIB carries 340 G1 points, so roughly 70 KB of wire bought hundreds of curve operations before the per-peer pending-message quota could drop the message. Replace the deserialization with a byte walk that reads compact-sizes and skips fixed-width fields, checking only bounds derived from quorum params. BLS decompression, member-list lookup and signature verification all stay on the DKG worker thread, behind the quota. The scan mirrors each message's wire layout with no type-level link to the serializers, so the acceptance tests build every message by serializing a real object with operator<< rather than hand-assembling bytes. A serializer change that the scan does not track will fail them, which matters because a message honest peers accept but this node rejects would partition it from the DKG. The DYNBITSET allocation bound that was previously bundled here is now in dashpay#7532.

coderabbitai · 2026-08-03T14:59:54Z

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

▶️ Resume reviews
🔍 Trigger review

Walkthrough

ReadFixedBitSet now requires streams with a size() method. It checks the required byte count against remaining stream data before resizing or allocating. Tests cover oversized DYNBITSET declarations and maximum-size round trips.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

dashpay/dash#7401: Both changes modify deserialization safety for dynamic bitsets.
dashpay/dash#7523: Both changes add pre-allocation validation for bitset deserialization in src/serialize.h.

Suggested reviewers: udjinm6

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly and concisely describes the main change: limiting DYNBITSET allocation by the remaining stream size.
Description check	✅ Passed	The description directly explains the allocation issue, implementation, affected paths, tests, and validation results.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

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

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/test/serialize_bitset_tests.cpp`:
- Around line 21-26: Strengthen RejectedBeforeAllocating to accept only the
guard-specific “exceeds remaining” error, not the generic ReadFixedBitSet
function name. In the affected regression assertions, compare each destination’s
size exactly against its pre-deserialization size using equality checks, rather
than merely asserting it differs from the claimed size.

🪄 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 Plus

Run ID: 93ef6fcc-11e9-4716-bb44-6ad96fdea032

📥 Commits

Reviewing files that changed from the base of the PR and between f1dde51 and 4b7ae62.

📒 Files selected for processing (3)

src/Makefile.test.include
src/serialize.h
src/test/serialize_bitset_tests.cpp

coderabbitai · 2026-08-03T14:59:58Z

+//! The bound must reject before allocating, so a short read after the fact is not good enough.
+bool RejectedBeforeAllocating(const std::string& what)
+{
+    return what.find("exceeds remaining") != std::string::npos ||
+           what.find("ReadFixedBitSet") != std::string::npos;
+}


🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Make the regression assert the guarded failure path.

RejectedBeforeAllocating accepts any exception containing ReadFixedBitSet. A later short-read path can contain that function name and still pass. BOOST_CHECK_NE also accepts any partial resize except the exact claimed size. Match the guard-specific error and compare each destination with its size before deserialization.

Proposed test assertion fix

- return what.find("exceeds remaining") != std::string::npos || - what.find("ReadFixedBitSet") != std::string::npos; + return what.find("declared size exceeds remaining bytes") != std::string::npos; ... + const auto initial_bits_size = bits.size(); ... - BOOST_CHECK_NE(bits.size(), kClaimedBits); + BOOST_CHECK_EQUAL(bits.size(), initial_bits_size); ... + const auto initial_signers_size = qc.signers.size(); ... - BOOST_CHECK_NE(qc.signers.size(), 1'000'000u); + BOOST_CHECK_EQUAL(qc.signers.size(), initial_signers_size);

Also applies to: 44-55, 88-100

🤖 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/test/serialize_bitset_tests.cpp` around lines 21 - 26, Strengthen RejectedBeforeAllocating to accept only the guard-specific “exceeds remaining” error, not the generic ReadFixedBitSet function name. In the affected regression assertions, compare each destination’s size exactly against its pre-deserialization size using equality checks, rather than merely asserting it differs from the claimed size.

thepastaclaw

Final validation — Codex + Sonnet

The core fix in ReadFixedBitSet correctly bounds the wire-declared bit count against bytes actually remaining in the stream before any vector resize or byte-buffer allocation, closing a real memory-amplification vector reachable via unauthenticated QFCOMMITMENT and every other DYNBITSET/AUTOBITSET caller. The requires{s.size();} guard properly scopes the check to network-deserialization streams without touching file-backed AutoFile/CBufferedFile paths, and the new tests demonstrate the rejection for both a raw DYNBITSET and a full CFinalCommitment. Two non-blocking test/lint-coverage issues remain: the regression test's failure-matching helper and size assertions are looser than the property being proven, and the new test file is missing from non-backported.txt.

Source: reviewers codex/general=gpt-5.6-sol(completed); codex/dash-core-commit-history=gpt-5.6-sol(completed); claude/general=claude-sonnet-5(failed); claude/dash-core-commit-history=claude-sonnet-5(failed); claude/general=claude-sonnet-5(failed); claude/dash-core-commit-history=claude-sonnet-5(failed); claude/general=claude-sonnet-5(completed); claude/dash-core-commit-history=claude-sonnet-5(failed); claude/dash-core-commit-history=claude-sonnet-5(failed); claude/dash-core-commit-history=claude-sonnet-5(completed); verifier=claude/final-verifier=claude-sonnet-5(completed); coordinator=openclaw-agent/cliproxy/gpt-5.6-sol(orchestration-only).

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 — general (failed), claude-sonnet-5 — dash-core-commit-history (failed), claude-sonnet-5 — general (completed), claude-sonnet-5 — dash-core-commit-history (failed), claude-sonnet-5 — dash-core-commit-history (failed), claude-sonnet-5 — dash-core-commit-history (completed)

🟡 2 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 `src/test/serialize_bitset_tests.cpp`:
- [SUGGESTION] src/test/serialize_bitset_tests.cpp:21-26: Test assertions don't tightly prove rejection happens before mutation
  RejectedBeforeAllocating() matches on either "exceeds remaining" or the bare substring "ReadFixedBitSet", the latter of which would also match any future exception message from within that function (e.g. a reworded variant of the existing "Out-of-range bits set" check, if it were ever changed to include the function name) even if the new pre-allocation guard didn't fire. Separately, BOOST_CHECK_NE(bits.size(), kClaimedBits) and BOOST_CHECK_NE(qc.signers.size(), 1'000'000u) only prove the destination wasn't resized to the exact attacker-claimed value — they don't prove it wasn't mutated to some other size, which is the actual invariant the PR is trying to assert (rejection before destination mutation/allocation). Matching only the guard-specific message text and comparing against the pre-deserialization size with BOOST_CHECK_EQUAL would make this a tight regression test for the fix rather than one that happens to pass today. This was raised by CodeRabbit on this same head and remains open.

In `test/util/data/non-backported.txt`:
- [SUGGESTION] test/util/data/non-backported.txt:68: New Dash-specific test file missing from non-backported.txt
  src/test/serialize_bitset_tests.cpp is a brand-new file testing Dash-specific serialization (DYNBITSET, llmq::CFinalCommitment) that did not originate upstream. non-backported.txt tracks files that lint-cppcheck-dash.py uses to apply extra Dash-specific static analysis; the file isn't listed, so it's currently skipped by that lint pass. Doesn't break the build, but it's a one-line omission worth fixing before merge.

thepastaclaw · 2026-08-03T16:27:30Z

+//! The bound must reject before allocating, so a short read after the fact is not good enough.
+bool RejectedBeforeAllocating(const std::string& what)
+{
+    return what.find("exceeds remaining") != std::string::npos ||
+           what.find("ReadFixedBitSet") != std::string::npos;
+}


🟡 Suggestion: Test assertions don't tightly prove rejection happens before mutation

RejectedBeforeAllocating() matches on either "exceeds remaining" or the bare substring "ReadFixedBitSet", the latter of which would also match any future exception message from within that function (e.g. a reworded variant of the existing "Out-of-range bits set" check, if it were ever changed to include the function name) even if the new pre-allocation guard didn't fire. Separately, BOOST_CHECK_NE(bits.size(), kClaimedBits) and BOOST_CHECK_NE(qc.signers.size(), 1'000'000u) only prove the destination wasn't resized to the exact attacker-claimed value — they don't prove it wasn't mutated to some other size, which is the actual invariant the PR is trying to assert (rejection before destination mutation/allocation). Matching only the guard-specific message text and comparing against the pre-deserialization size with BOOST_CHECK_EQUAL would make this a tight regression test for the fix rather than one that happens to pass today. This was raised by CodeRabbit on this same head and remains open.

Suggested change

//! The bound must reject before allocating, so a short read after the fact is not good enough.

bool RejectedBeforeAllocating(const std::string& what)

{

return what.find("exceeds remaining") != std::string::npos ||

what.find("ReadFixedBitSet") != std::string::npos;

}

bool RejectedBeforeAllocating(const std::string& what)

{

return what.find("declared size exceeds remaining bytes") != std::string::npos;

}

_{source: ['claude', 'codex']}

Resolved in this update — Test assertions don't tightly prove rejection happens before mutation 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.}

github-actions · 2026-08-03T19:04:48Z

This pull request has conflicts, please rebase.

PastaPastaPasta

LGTM

thepastaclaw

Final validation — Codex + Sonnet

The latest delta correctly replaces the fail-open size() detection with a compile-time SizedStream requirement, and no new correctness issue was introduced at head 8825c6b. Two carried-forward suggestions remain unchanged: the regression tests do not precisely establish guard-specific rejection before destination mutation, and the new Dash-specific test file is still absent from non-backported.txt. The proposed commit squash is not retained because the two commits are coherent, independently useful changes and the project permits review follow-up commits.

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 (completed), claude-sonnet-5 — dash-core-commit-history (completed)

🟡 1 suggestion(s)

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

1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.

🤖 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:68: New Dash-specific test file missing from non-backported.txt
  src/test/serialize_bitset_tests.cpp is a new Dash-originated source file, but no pattern in this list matches it. test/lint/lint-cppcheck-dash.py passes only files selected through these patterns to the additional Dash-specific cppcheck run, so the new test is currently omitted. Add the file alongside the other Dash-specific unit tests.

knst · 2026-08-04T13:48:29Z

+ * std::vector<bool> to ~4 MiB and allocate another ~4 MiB byte buffer before the short read
+ * throws. The claim below is deliberately modest so the pre-fix path also stays safe on CI.
+ */
+BOOST_AUTO_TEST_CASE(dynbitset_rejects_oversized_declared_length)


should be part of test/serialize_tests.cpp ; not a new file

thepastaclaw

Final validation — Codex + Sonnet

The current head preserves the DYNBITSET allocation-bound fix and moves its regression coverage into the existing serialization test suite without introducing a correctness defect. Two non-blocking review suggestions remain: tighten the tests so they directly establish guard-specific rejection before mutation, and fold the relocation-only commit into the original fix to avoid permanent add-then-delete history. The prior non-backported.txt issue is fixed because the standalone Dash-specific test file no longer exists.

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 (failed), claude-sonnet-5 — general (completed), claude-sonnet-5 — dash-core-commit-history (completed)

🟡 2 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 `src/test/serialize_tests.cpp`:
- [SUGGESTION] src/test/serialize_tests.cpp:192-270: Test assertions don't tightly prove rejection happens before mutation
  `RejectedBeforeAllocating()` accepts the generic `ReadFixedBitSet` substring instead of matching the new guard-specific `declared size exceeds remaining bytes` failure. The subsequent checks only establish that `bits` and `qc.signers` did not finish at the exact attacker-declared size; mutation to another size before throwing would still pass. Match the guard-specific error and assert that each destination remains equal to its exact pre-deserialization state, which is empty in both tests, so the coverage directly enforces rejection before destination mutation.

In `<commit:2a1155f>`:
- [SUGGESTION] <commit:2a1155f>:1: Squash the test-relocation follow-up into the original fix
  Commit 2a1155f206 deletes `src/test/serialize_bitset_tests.cpp`, removes its build entry, and moves the same tests into `src/test/serialize_tests.cpp`. The standalone file and build entry were introduced by 4b7ae620272 earlier in this PR, so keeping the follow-up separately leaves an unnecessary add-then-delete sequence in permanent history. Fold 2a1155f206 into 4b7ae620272 so the allocation-bound fix introduces its tests in their intended suite from the outset.

thepastaclaw · 2026-08-04T19:40:07Z

+//! The bound must reject before allocating, so a short read after the fact is not good enough.
+static bool RejectedBeforeAllocating(const std::string& what)
+{
+    return what.find("exceeds remaining") != std::string::npos ||
+           what.find("ReadFixedBitSet") != std::string::npos;
+}
+
+/**
+ * DYNBITSET must not allocate from an attacker-declared CompactSize when the remaining stream
+ * is far too small to hold the claimed bit payload. A handful of bytes claiming ~1e6 bits is
+ * the amplification primitive: ReadCompactSize permits up to 33,554,432, which would resize a
+ * std::vector<bool> to ~4 MiB and allocate another ~4 MiB byte buffer before the short read
+ * throws. The claim below is deliberately modest so the pre-fix path also stays safe on CI.
+ */
+BOOST_AUTO_TEST_CASE(dynbitset_rejects_oversized_declared_length)
+{
+    constexpr uint64_t kClaimedBits = 1'000'000;
+
+    CDataStream s(SER_NETWORK, PROTOCOL_VERSION);
+    WriteCompactSize(s, kClaimedBits);
+    // No bit payload follows, so the remaining size is zero.
+
+    std::vector<bool> bits;
+    std::string what;
+    bool threw = false;
+    try {
+        s >> DYNBITSET(bits);
+    } catch (const std::ios_base::failure& e) {
+        threw = true;
+        what = e.what();
+    }
+    BOOST_CHECK_MESSAGE(threw, "DYNBITSET must reject a declared length that exceeds remaining bytes");
+    BOOST_CHECK_NE(bits.size(), kClaimedBits);
+    BOOST_CHECK_MESSAGE(RejectedBeforeAllocating(what), "Expected a pre-allocation rejection, got: " + what);
+}
+
+/** A legitimately sized DYNBITSET (LLMQ max 400) must still round-trip unchanged. */
+BOOST_AUTO_TEST_CASE(dynbitset_accepts_legitimate_llmq_size)
+{
+    constexpr size_t kSize = Consensus::MAX_LLMQ_SIZE;
+    std::vector<bool> original(kSize, false);
+    for (size_t i = 0; i < kSize; i += 3) {
+        original[i] = true;
+    }
+
+    CDataStream s(SER_NETWORK, PROTOCOL_VERSION);
+    s << DYNBITSET(original);
+
+    std::vector<bool> decoded;
+    s >> DYNBITSET(decoded);
+    BOOST_CHECK(decoded == original);
+}
+
+/**
+ * The same primitive is reachable from an unauthenticated QFCOMMITMENT via CFinalCommitment's
+ * signers bitset, so cover the real message type too.
+ */
+BOOST_AUTO_TEST_CASE(qfinalcommitment_rejects_oversized_signers_bitset)
+{
+    CDataStream s(SER_NETWORK, PROTOCOL_VERSION);
+    // nVersion (u16) | llmqType (u8) | quorumHash (32) | signers DYNBITSET | ...
+    s << static_cast<uint16_t>(llmq::CFinalCommitment::BASIC_BLS_NON_INDEXED_QUORUM_VERSION);
+    s << Consensus::LLMQType::LLMQ_400_85;
+    s << uint256::ONE;
+    WriteCompactSize(s, 1'000'000);
+
+    llmq::CFinalCommitment qc;
+    std::string what;
+    bool threw = false;
+    try {
+        s >> qc;
+    } catch (const std::ios_base::failure& e) {
+        threw = true;
+        what = e.what();
+    }
+    BOOST_CHECK(threw);
+    BOOST_CHECK_NE(qc.signers.size(), 1'000'000u);
+    BOOST_CHECK_MESSAGE(RejectedBeforeAllocating(what),
+                        "Expected a pre-allocation rejection for CFinalCommitment, got: " + what);


🟡 Suggestion: Test assertions don't tightly prove rejection happens before mutation

RejectedBeforeAllocating() accepts the generic ReadFixedBitSet substring instead of matching the new guard-specific declared size exceeds remaining bytes failure. The subsequent checks only establish that bits and qc.signers did not finish at the exact attacker-declared size; mutation to another size before throwing would still pass. Match the guard-specific error and assert that each destination remains equal to its exact pre-deserialization state, which is empty in both tests, so the coverage directly enforces rejection before destination mutation.

_{source: ['claude', 'codex']}

ReadFixedBitSet allocated from a wire-declared CompactSize with no bound beyond ReadCompactSize's 33,554,432 cap. Roughly five bytes on the wire (a CompactSize claiming millions of bits and no payload) therefore forced a std::vector<bool> resize plus a byte buffer totalling several MiB, all of which was only abandoned when the subsequent short read threw. MAX_PROTOCOL_MESSAGE_LENGTH does not help, because the attack uses an undersized message. Bound the declared length against the bytes actually remaining in the stream before allocating. A well-formed message always carries exactly the required bytes, so this rejects only claims that could never have been satisfied. This covers every DYNBITSET caller, including CFinalCommitment::signers and validMembers, which are reachable from an unauthenticated QFCOMMITMENT.

coderabbitai · 2026-08-04T21:15:32Z

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

PastaPastaPasta · 2026-08-05T18:20:28Z

LGTM

knst

see comments

knst · 2026-08-06T15:53:06Z

+    std::string what;
+    bool threw = false;
+    try {
+        s >> qc;
+    } catch (const std::ios_base::failure& e) {
+        threw = true;
+        what = e.what();
+    }
+    BOOST_CHECK(threw);
+    BOOST_CHECK(qc.signers.empty());
+    BOOST_CHECK_MESSAGE(what.find(BOUND_REJECTION) != std::string::npos,
+                        "Expected a pre-allocation rejection for CFinalCommitment, got: " + what);


firstly, there's BOOST_CHECK_EXCEPTION that already does it, no manual helpers are needed

Secondly, qfinalcommitment_rejects_oversized_signers_bitset has nothing to do with serialize.h and serialize_tests.cpp.

It should not be in this file.

Thirdly; there's no specific implementation for exactly CFinalCommitment; there's nothing actually test.
The test-case qfinalcommitment_rejects_oversized_signers_bitset should be just removed.

knst · 2026-08-06T15:54:49Z

+/** A legitimately sized DYNBITSET (LLMQ max 400) must still round-trip unchanged. */
+BOOST_AUTO_TEST_CASE(dynbitset_accepts_legitimate_llmq_size)


this regression is a bit strange to have MAX_LLMQ_SIZE here ; instead, value 33,554,432 should be tested which is a real cap for dynbitset [I took this value from PR description]; mentions of llmq should be removed from it

knst · 2026-08-06T15:55:35Z

+    std::vector<bool> bits;
+    std::string what;
+    bool threw = false;
+    try {
+        s >> DYNBITSET(bits);
+    } catch (const std::ios_base::failure& e) {
+        threw = true;
+        what = e.what();
+    }
+    BOOST_CHECK_MESSAGE(threw, "DYNBITSET must reject a declared length that exceeds remaining bytes");
+    // Rejection has to precede the resize, so the destination must still hold its exact
+    // pre-deserialization state. Merely falling short of the declared size would also be
+    // satisfied by an allocation that happened and was then abandoned.
+    BOOST_CHECK(bits.empty());
+    BOOST_CHECK_MESSAGE(what.find(BOUND_REJECTION) != std::string::npos,
+                        "Expected a pre-allocation rejection, got: " + what);


use BOOST_CHECK_EXCEPTION helper instead manually written try-catch-boost-check

…ping it The if constexpr guard failed open silently: a stream without size() compiled the bound away with no diagnostic, leaving the unbounded allocation in place. Constraining the template turns that into a build error, so the invariant is checked on every build rather than by inspection.

chatgpt-codex-connector · 2026-08-06T16:03:13Z

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

coderabbitai · 2026-08-06T16:03:16Z

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

thepastaclaw

Final validation — Codex + Sonnet

At exact head aa1447f, the production allocation bound and SizedStream constraint remain correct. Both carried-forward suggestions are fixed by the rewritten history and tightened assertions. The latest delta intentionally revises the test scope following maintainer feedback, but the PR testing description and latest commit message still describe the previous coverage.

Review provenance

Codex reviewers: gpt-5.6-sol — general (completed), gpt-5.6-sol — dash-core-commit-history (completed)
Verifier: gpt-5.6-sol — final-verifier (Sonnet-verifier fallback)
Sonnet reviewers: claude-sonnet-5 — general (completed), claude-sonnet-5 — dash-core-commit-history (completed)

💬 1 nitpick(s)

thepastaclaw · 2026-08-06T16:44:51Z

+/** The largest bit count accepted by ReadCompactSize must still round-trip unchanged. */
+BOOST_AUTO_TEST_CASE(dynbitset_accepts_maximum_size)
+{
+    constexpr size_t kSize = MAX_SIZE;
+    std::vector<bool> original(kSize, false);
+    for (size_t i = 0; i < kSize; i += 3) {
+        original[i] = true;
+    }
+
+    CDataStream s(SER_NETWORK, PROTOCOL_VERSION);
+    s << DYNBITSET(original);
+
+    std::vector<bool> decoded;
+    s >> DYNBITSET(decoded);
+    BOOST_CHECK(decoded == original);
+}


💬 Nitpick: Testing documentation no longer matches the revised cases

The latest delta intentionally replaces the LLMQ-sized round trip with a MAX_SIZE round trip and removes the CFinalCommitment deserialization case following maintainer feedback. The PR's testing section still claims both removed cases, while the current suite contains only the raw DYNBITSET rejection and maximum-size round trip. Commit aa1447f also describes only the SizedStream constraint despite containing these test-scope revisions. Update the PR testing section to match the current cases and, if the commit remains structured this way, note the test cleanup in its body so neither the PR nor the history claims integration coverage that is no longer present.

_{source: ['claude', 'codex']}

knst

utACK aa1447f

… size aa1447f serialize: require SizedStream for ReadFixedBitSet instead of duck-typing it (pasta) 9f026a9 fix: bound DYNBITSET allocation against remaining stream size (pasta) Pull request description: ## Issue being fixed or feature implemented `ReadFixedBitSet` allocated from a wire-declared `CompactSize` with no bound beyond `ReadCompactSize`'s 33,554,432 cap. Roughly five bytes on the wire — a `CompactSize` claiming millions of bits, with no payload following — therefore forced a `std::vector<bool>` resize plus a byte buffer totalling several MiB, all of which was only abandoned when the subsequent short read threw. `MAX_PROTOCOL_MESSAGE_LENGTH` does not help here, because the attack uses an *undersized* message. The primitive is reachable from an unauthenticated `QFCOMMITMENT` via `CFinalCommitment`'s `signers` and `validMembers` bitsets, and applies to every other `DYNBITSET` caller as well. This was split out of [#7523](#7523), where it was bundled with a much larger and more contentious DKG-intake change. The bound stands on its own, so it is offered separately for independent review. ## What was done? Bound the declared length against the bytes actually remaining in the stream, before allocating anything. A well-formed message always carries exactly the required bytes, so this rejects only claims that could never have been satisfied. The guard needs the stream to report how many bytes are still available, so `ReadFixedBitSet` is constrained on a `SizedStream` concept. That constraint is deliberately a hard requirement rather than a fallback: a stream without `size()` — `CHashVerifier`, for instance — now fails to compile here instead of silently deserializing an unbounded bitset. An earlier revision duck-typed the check with `if constexpr`, which would have let a future caller quietly lose the bound with no diagnostic. Nothing in tree is affected, because every stream that reaches this path already reports bytes *remaining*: `DataStream` (`vch.size() - m_read_pos`), `SpanReader`, and `OverrideStream`, which forwards to its wrapped stream. That "remaining, not total" requirement is what the bound rests on, so it is documented on the concept itself — a stream whose `size()` meant something else would weaken the bound silently rather than fail to compile. Note on what is deliberately *not* included: an earlier revision also checked `nbytes > MAX_SIZE`. That check is dead code — `size` comes from `ReadCompactSize`, which already caps at 33,554,432, so `(size + 7) / 8` can never exceed 4 MiB and the condition is always false. ## How Has This Been Tested? `src/test/serialize_tests.cpp` covers the amplification case (a declared length far exceeding the remaining stream must be rejected *before* allocating, not by a short read afterwards) and a maximum-size bitset (`MAX_SIZE`, 33,554,432 bits) round-tripping unchanged. Built locally and `test_dash --run_test=serialize_tests` passes. Full validation is delegated to CI. ## Breaking Changes None. Only messages declaring a bit count that the message could not possibly contain are affected, and those were never valid. ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone ACKs for top commit: knst: utACK aa1447f Tree-SHA512: 0b7da754ac61d1aa21e6c3fcf75c6986f69f6e0f90d51b9061cf45b03909bdf77d6b125f5f3665976aeca8defe361300bc465789347cac927895433c90a035ae (cherry picked from commit 4c33e73)

71b3f0c Merge #7556: fix: gate BLS vote-signature failure log behind gobject category (pasta) 14dddb1 Merge #7838: fix(wallet): authenticate unlock deadline extensions (pasta) 5f73355 Merge #7858: fix: do not InstantSend-lock provider transactions (pasta) 0d2e579 fix(evo): keep -allowprivatenet out of masternode address validation (Konstantin Akimov) b4cd3fa fix(evo): only write credit pool snapshots inside a block-scoped EvoDB transaction (#7549) (pasta) d215280 Merge #7846: fix: treat pending owner keys and Platform node IDs as one conflict namespace (pasta) db9ec2b Merge #7826: fix: skip repeated authenticated Platform bans (pasta) dcc9596 Merge #7867: fix: refuse masternode list RPCs for blocks outside the active chain (pasta) 197b14a Merge #7866: fix: stop `masternode payments` at the genesis block (pasta) 3055a19 Merge #7850: perf: let CHECKDATASIG use the signature cache (pasta) 4da0099 Merge #7821: fix: relock encrypted wallets after HD upgrade failures (pasta) c7393c0 Merge #7833: fix: gate signing intake before quorum retrieval (pasta) fdf40fe Merge #7828: fix: omit recovery words from invalid mnemonic errors (pasta) 8888af8 Merge #7753: fix: bound masternode list caches by size instead of by new block (pasta) 457af6d Merge #7738: refactor: avoid double lookups and default-inserting map access in governance (pasta) be3b55e Merge #7662: fix: isolate missing quorums in InstantSend batches (pasta) 9bc1e77 Merge #7489: fix: conflict ProRegTx collateral reuse with in-mempool masternode updates (pasta) 5278ce8 Merge #7583: fix: bound pending DKG message retention per proTxHash (pasta) 9ebf78f Merge #7522: fix(net): bound verified inbound connections per proRegTxHash (pasta) 9f2946e Merge #7487: fix: bound DomainPort string deserialization and stop swallowing deser failures (pasta) c077d32 Merge #7527: fix: require a valid masternode signature before caching an orphan governance vote (pasta) dfa7db1 Merge #7518: fix: memoize governance vote signature checks to bound govsync cost (pasta) bb09c49 Merge #7532: fix: bound DYNBITSET allocation against remaining stream size (pasta) 9739f35 Merge #7488: fix: validate ProUpServTx nType at mempool acceptance (pasta) 4687550 Merge #7385: fix: reject invalid extended masternode net info (Pasta) Pull request description: ## Issue being fixed or feature implemented Backports for v23.1.9, on top of current `v23.1.x`. The base already carries the negative-nVersion consensus fix (#2), the invalid-DKG-share fix (#4), the per-sender sig-share scoping (#5), the individual DKG contribution verification (#7) and the provider-decode/orphan-cap hardening (#8), so those are not part of this diff. ## What was done? Cherry-picked (`git cherry-pick -m1 -x`) the develop merges below into `v23.1.x`, in develop merge order. Where a develop merge carried develop-only refactors or APIs absent on `v23.1.x`, only the fix was taken and the adaptation is noted in that commit's message. ### Included (cherry-picks of merged dashpay/dash PRs) - #7385 — `fix: reject invalid extended masternode net info` - #7488 — `fix: validate ProUpServTx nType at mempool acceptance` - #7532 — `fix: bound DYNBITSET allocation against remaining stream size` - #7518 — `fix: memoize governance vote signature checks to bound govsync cost` - #7527 — `fix: require a valid masternode signature before caching an orphan governance vote` - #7487 — `fix: bound DomainPort string deserialization and stop swallowing deser failures` - #7522 — `fix(net): bound verified inbound connections per proRegTxHash` - #7583 — `fix: bound pending DKG message retention per proTxHash` - #7489 — `fix: conflict ProRegTx collateral reuse with in-mempool masternode updates` - #7662 — `fix: isolate missing quorums in InstantSend batches` - #7738 — `fix: do not record a masternode vote before it is validated` - #7753 — `fix: bound masternode list caches by size instead of by new block` - #7828 — `fix: omit recovery words from invalid mnemonic errors` - #7833 — `fix: gate signing intake before quorum retrieval` - #7821 — `fix: relock encrypted wallets after HD upgrade failures` - #7850 — `perf: let CHECKDATASIG use the signature cache` - #7866 — `fix: stop 'masternode payments' at the genesis block` - #7867 — `fix: refuse masternode list RPCs for blocks outside the active chain` - #7826 — `fix: skip repeated authenticated Platform bans` - #7846 — `fix: treat pending owner keys and Platform node IDs as one conflict namespace` - #7858 — `fix: do not InstantSend-lock provider transactions` (with its functional test, ported to v23.1.x) - #7838 — `fix(wallet): authenticate unlock deadline extensions` - #7556 — `fix: gate BLS vote-signature failure log behind gobject category` ### Upstream-derived changes with no clean develop merge (own commits) - `fix(evo): only write credit pool snapshots inside a block-scoped EvoDB transaction (#7549)` — the develop fix landed inside #7639, which is not on this branch, so it is hand-ported (EvoDB transaction-depth counter + `HasActiveTransaction()` gate); develop commits `8503c5d211`/`0325195ecb` cited in the commit. - `fix(evo): keep -allowprivatenet out of masternode address validation` — the consensus portion of #7888 only (develop commit `e3e780801b`); the V24-gated ExtNetInfo parts are excluded. ### Adaptation notes - #7556 pairs with #7527 (already here): once orphan governance votes must pass `IsValid` before caching, a peer with a valid masternode outpoint and an invalid BLS signature can reach the BLS vote-signature failure log, so that log (and two sibling `ProcessVote` logs) is gated behind `BCLog::GOBJECT`. - #7662: the unit test calls `SetMinSporkKeys(1)`; v23.1.x defaults the spork signer threshold to `INT_MAX`, so without it `SPORK_2_INSTANTSEND_ENABLED` never activates and the regression check would be vacuous. - #7858 is taken with its functional test ported to v23.1.x: `wait_for_instantlock` is `(txid, node)` here and does not sync/bump mocktime itself, and develop's `DashTestFramework.bury_tx` does not exist, so the waits sync+bump per node and the bury step is inlined. `IsProviderTx` lists only the four v23.1.x provider special-tx types (the v24-only `DISSOLVE`/`UPDATE_SHARE`/`UPDATE_SHARED_REGISTRAR` are omitted). - #7385/#7488/#7489/#7846: kept v23.1.x `ProTxVersion::ExtAddr` equality and the atomic-pointer `dmnman`; dropped develop-only test setups and the shared-masternode owner-key branch. - #7527/#7738: v23.1.x governance lookup/`ProcessVote` shapes; develop-only refactors excluded. - #7583: re-threaded through `active/dkgsession.*`; #7867: `insert_or_assign`; #7821: `interfaces::MakeHandler`; #7753: dropped the `<atomic>` include/`DoMaintenance` test calls. ### Not included - #7822 (`fix: reject malformed detached quorum share batches`) — its one-line change (record the node in `badSources` rather than `BanNode`) is already present in the base via #5, so the pick is redundant and was dropped. - #7895 (`test: cover retroactive provider tx refusal`) — its functional test needs the single-member-quorum harness absent on `v23.1.x`. - Compact-block extra-tx fix (bitcoin bitcoin#35670) — dropped from this release as a latent, very-hard-to-trigger crash; opened as its own PR against `develop`. - #7861 (pruned-node historical-block reject) — deferred; also missing from `v24.0.0-rc.4` (should land in v24.0.0 final). - Develop-only regressions and V24-gated changes that do not affect `v23.1.x`. ## How Has This Been Tested? Rebased onto current `v23.1.x`; the only merge conflicts were the additive test appends in `src/test/evo_netinfo_tests.cpp` and `src/test/governance_inv_tests.cpp` (kept both the base's and the picks' cases). Assembled branch builds with `--enable-werror` (`dashd` + `test_dash`). The full `test_dash` unit suite passes, and `p2p_instantsend.py` (including the restored `test_provider_tx_not_locked`) passes. Each pick's changed lines were compared with its develop merge; differences are the adaptations above. ## Breaking Changes - #7858: provider transactions no longer receive InstantSend locks. Software that waited for an islock on a ProTx should wait for a ChainLock or confirmations instead. - #7828: invalid-mnemonic RPC errors no longer echo the mnemonic. #7821: encrypted wallets relock after a failed HD upgrade. #7867: `protx info/listdiff` and `masternode payments` error for blocks off the active chain. #7489/#7488: provider transactions with reused collateral or an invalid type are rejected at mempool acceptance. #7888: the RFC1918 masternode-address check applies even with `-allowprivatenet`. #7838: a wallet unlock deadline can no longer be extended without re-authenticating. ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone 🤖 Generated with [Claude Code](https://claude.com/claude-code)   ## Summary by CodeRabbit * **Behavior Changes** * Provider transactions no longer receive InstantSend locks. They are mined after 10 minutes in the mempool and then protected by a ChainLock; transactions spending their outputs can be locked once mined. Use ChainLocks or confirmations instead of waiting for an InstantSend lock on a provider transaction. * **Bug Fixes** * Masternode RPCs now reject blocks outside the active chain. * Wallet unlock attempts with an incorrect passphrase no longer alter an existing unlock period. * Improved validation and handling of governance votes, network addresses, and masternode connections.  Top commit has no ACKs. Tree-SHA512: d48eaae945bcbad443cf3280c08d2ed00f06816bb09ef70f85fca497a864b89e502d09fb3c9c977f8d0a74d761dc3c29f3b11e217cc7e29c43d1ee691d4df572

4722ad9 chore: prepare v23.1.9 release (pasta) 71b3f0c Merge #7556: fix: gate BLS vote-signature failure log behind gobject category (pasta) 14dddb1 Merge #7838: fix(wallet): authenticate unlock deadline extensions (pasta) 5f73355 Merge #7858: fix: do not InstantSend-lock provider transactions (pasta) 0d2e579 fix(evo): keep -allowprivatenet out of masternode address validation (Konstantin Akimov) b4cd3fa fix(evo): only write credit pool snapshots inside a block-scoped EvoDB transaction (#7549) (pasta) d215280 Merge #7846: fix: treat pending owner keys and Platform node IDs as one conflict namespace (pasta) db9ec2b Merge #7826: fix: skip repeated authenticated Platform bans (pasta) dcc9596 Merge #7867: fix: refuse masternode list RPCs for blocks outside the active chain (pasta) 197b14a Merge #7866: fix: stop `masternode payments` at the genesis block (pasta) 3055a19 Merge #7850: perf: let CHECKDATASIG use the signature cache (pasta) 4da0099 Merge #7821: fix: relock encrypted wallets after HD upgrade failures (pasta) c7393c0 Merge #7833: fix: gate signing intake before quorum retrieval (pasta) fdf40fe Merge #7828: fix: omit recovery words from invalid mnemonic errors (pasta) 8888af8 Merge #7753: fix: bound masternode list caches by size instead of by new block (pasta) 457af6d Merge #7738: refactor: avoid double lookups and default-inserting map access in governance (pasta) be3b55e Merge #7662: fix: isolate missing quorums in InstantSend batches (pasta) 9bc1e77 Merge #7489: fix: conflict ProRegTx collateral reuse with in-mempool masternode updates (pasta) 5278ce8 Merge #7583: fix: bound pending DKG message retention per proTxHash (pasta) 9ebf78f Merge #7522: fix(net): bound verified inbound connections per proRegTxHash (pasta) 9f2946e Merge #7487: fix: bound DomainPort string deserialization and stop swallowing deser failures (pasta) c077d32 Merge #7527: fix: require a valid masternode signature before caching an orphan governance vote (pasta) dfa7db1 Merge #7518: fix: memoize governance vote signature checks to bound govsync cost (pasta) bb09c49 Merge #7532: fix: bound DYNBITSET allocation against remaining stream size (pasta) 9739f35 Merge #7488: fix: validate ProUpServTx nType at mempool acceptance (pasta) 4687550 Merge #7385: fix: reject invalid extended masternode net info (Pasta) 58b9bce fix: keep flat-file positions 64-bit clean at the remaining raw seek/tell sites (Konstantin Akimov) 7a21752 Merge bitcoin#36667: net: enforce timeout interval for paused send peers (Konstantin Akimov) 58abb28 Merge bitcoin#36665: sock: align exception monitoring between poll() and select() backends (Konstantin Akimov) 8030deb Merge bitcoin#36669: refactor: store positions as a 64 bits integer in flatfiles (Konstantin Akimov) b1178f9 fix(llmq): judge every sig-share sender by its own verified shares (pasta) 33ec74e fix(governance): cap the orphan parent objects re-requested per tick (pasta) 7fc0edb doc(rpc): mark provider payload network fields optional in the help (pasta) 2aa3a87 fix(rpc): don't dereference a missing netInfo when decoding provider payloads (pasta) 120b5db fix(llmq): verify DKG contributions individually when a batch vvec check fails (pasta) 808b105 Merge #7486: fix: guard VerifyContributionShares against an empty verification-vector set (pasta) 8351db9 fix(llmq): reject invalid decrypted DKG shares before aggregation (pasta) 07494dd fix(consensus): reject negative transaction nVersion after DIP3 (pasta) Pull request description: ## Issue being fixed or feature implemented Merges the v23.1.9 release back into develop. v23.1.9 ships five security fixes and three Bitcoin Core network/storage backports that develop doesn't have yet. Without this merge, develop-based nodes (v24) stay exposed to all of them. Note on the base: `master` is still at 728f505 (v23.1.8 + crash fixes) and has not been fast-forwarded to the release yet. It is an ancestor of the signed `v23.1.9` tag, so this PR merges the tag commit (67c106f). That gives exactly the result of merging `master` once it is fast-forwarded. ## What was done? One merge commit. Most of v23.1.9 is backports of develop PRs that develop already has (#7385, #7486, #7487, #7488, #7489, #7518, #7522, #7527, #7532, #7556, #7583, #7662, #7738, #7753, #7821, #7826, #7828, #7833, #7838, #7846, #7850, #7858, #7866, #7867, and the `-allowprivatenet` fix from #7888). Those conflicts resolve to develop's code. The new payload, adapted to develop's layout: - **Sig-share sender aliasing** (`fix(llmq): judge every sig-share sender by its own verified shares`). The batch verifier is now keyed by `(NodeId, SigShareKey)`, so two peers pushing the same key can't be judged by each other's signature. Ported into develop's `NetSigning::ProcessPendingSigShares` (`net_signing.cpp`). The second half of the v23 fix (malformed share → `badSources`) was already on develop. - **Negative tx `nVersion`** (`fix(consensus): reject negative transaction nVersion after DIP3`). Applied as-is, plus `PackVersionAndType()` at all three pack sites. - **Invalid decrypted DKG shares** (`fix(llmq): reject invalid decrypted DKG shares before aggregation`). The QDATA check lived in `src/active/quorums.cpp`, which develop deleted; it is ported into `NetQuorum::ProcessContribQDATA` (`llmq/net_quorum.cpp`). The new `feature_llmq_dkgerrors.py` case passes numeric `dkgsimerror` rates, which develop's RPC requires. - **Batch vvec fallback** (`fix(llmq): verify DKG contributions individually when a batch vvec check fails`). The new `bls_tests` case calls develop's `CBLSWorker::Start(4)`, which takes an explicit worker count. - **Orphan governance re-request cap** (`fix(governance): cap the orphan parent objects re-requested per tick`). At most 100 orphan parents are re-requested per tick. The test helper uses develop's `governance::OrphanVote`. - **bitcoin#36665, bitcoin#36667**. Applied cleanly. - **bitcoin#36669**. Develop moved `TruncateFile`/`AllocateFileRange` into `util/fs_helpers.{h,cpp}`, so those hunks, `FileSeek64` and `FileTell64` land there. - **64-bit flat-file positions** (`fix: keep flat-file positions 64-bit clean at the remaining raw seek/tell sites`). Covers `txindex.cpp` and `blockstorage.cpp`, which now include `util/fs_helpers.h`. It also covers develop-only `BlockFilterIndex::LookupFilterRange` (from #7574's file-handle reuse), which seeks on a `FlatFilePos` the same way. - **Release bookkeeping**: v23.1.9 release notes, archived 23.1.8 notes, flatpak release entry and regenerated man pages. `configure.ac` keeps develop's 24.1.0 / `IS_RELEASE=false`. Deliberately resolved to develop's side: - `fix(rpc): don't dereference a missing netInfo when decoding provider payloads` and its help-text follow-up. On develop an unknown-version ProTx payload reaches `netInfo` only through `CHECK_NONFATAL`, so `decoderawtransaction` returns an RPC error instead of dereferencing null; the v23 commit message says this is the develop equivalent. Reporting the payload without network fields (as v23.1.9 now does) would be a nicer develop follow-up, but it doesn't belong in this merge. - `fix(evo): only write credit pool snapshots inside a block-scoped EvoDB transaction (#7549)`. This is a v23 hand-port of develop's own 8503c5d / 0325195. - `src/active/quorums.cpp` stays deleted. ## How Has This Been Tested? Linux x86_64, `--enable-debug --enable-werror`, built `dashd` and `test_dash`. - Unit suites, all passing: `llmq_utils_tests`, `transaction_tests`, `txvalidation_tests`, `sighash_tests`, `bls_tests`, `governance_inv_tests`, `llmq_dkg_tests`, `flatfile_tests`, `fs_tests`, `net_tests`, `sock_tests`, `evo_netinfo_tests`, `evo_assetlocks_tests`, `blockfilter_index_tests`, `txindex_tests`, `blockmanager_tests`. - Functional tests, all passing: `feature_llmq_dkgerrors.py` (including the new contribution-invalid case), `feature_llmq_signing.py` (both variants), `p2p_timeouts.py` (v1/v2), `mempool_accept.py`. - `git diff --remerge-diff` is empty on all eight v23.1.9 merge commits, so the release has no hidden merge-only changes. - An independent review compared each port with its v23.1.9 commit and checked develop for second copies of each bug: other `SigShareKey` batch verifiers, other decrypt-then-aggregate sites, other `fPauseSend` setters and other raw seeks on `FlatFilePos`. It also spot-checked that the PRs treated as "already on develop" have equivalent code there. I did not run the full functional suite locally; CI covers it. ## Breaking Changes Consensus: once DIP3 is active, transactions with a negative `nVersion` are rejected (`bad-txns-version`). Every v23.1.9 node already enforces this. Such transactions were already split-inducing, since a re-serialized copy changes `nType`. ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ 🤖 Generated with [Claude Code](https://claude.com/claude-code) ACKs for top commit: UdjinM6: utACK 257af26 Tree-SHA512: d6d19ecbfaa0a891b09887169ec2709a7dfde06af022203ca28e80fdf009ca360f2704ebadca1f830a97663dac09da78d05c979d9c30ee91780065550070b954

…xes, #7947) 4722ad9 chore: prepare v23.1.9 release (pasta) 71b3f0c Merge #7556: fix: gate BLS vote-signature failure log behind gobject category (pasta) 14dddb1 Merge #7838: fix(wallet): authenticate unlock deadline extensions (pasta) 5f73355 Merge #7858: fix: do not InstantSend-lock provider transactions (pasta) 0d2e579 fix(evo): keep -allowprivatenet out of masternode address validation (Konstantin Akimov) b4cd3fa fix(evo): only write credit pool snapshots inside a block-scoped EvoDB transaction (#7549) (pasta) d215280 Merge #7846: fix: treat pending owner keys and Platform node IDs as one conflict namespace (pasta) db9ec2b Merge #7826: fix: skip repeated authenticated Platform bans (pasta) dcc9596 Merge #7867: fix: refuse masternode list RPCs for blocks outside the active chain (pasta) 197b14a Merge #7866: fix: stop `masternode payments` at the genesis block (pasta) 3055a19 Merge #7850: perf: let CHECKDATASIG use the signature cache (pasta) 4da0099 Merge #7821: fix: relock encrypted wallets after HD upgrade failures (pasta) c7393c0 Merge #7833: fix: gate signing intake before quorum retrieval (pasta) fdf40fe Merge #7828: fix: omit recovery words from invalid mnemonic errors (pasta) 8888af8 Merge #7753: fix: bound masternode list caches by size instead of by new block (pasta) 457af6d Merge #7738: refactor: avoid double lookups and default-inserting map access in governance (pasta) be3b55e Merge #7662: fix: isolate missing quorums in InstantSend batches (pasta) 9bc1e77 Merge #7489: fix: conflict ProRegTx collateral reuse with in-mempool masternode updates (pasta) 5278ce8 Merge #7583: fix: bound pending DKG message retention per proTxHash (pasta) 9ebf78f Merge #7522: fix(net): bound verified inbound connections per proRegTxHash (pasta) 9f2946e Merge #7487: fix: bound DomainPort string deserialization and stop swallowing deser failures (pasta) c077d32 Merge #7527: fix: require a valid masternode signature before caching an orphan governance vote (pasta) dfa7db1 Merge #7518: fix: memoize governance vote signature checks to bound govsync cost (pasta) bb09c49 Merge #7532: fix: bound DYNBITSET allocation against remaining stream size (pasta) 9739f35 Merge #7488: fix: validate ProUpServTx nType at mempool acceptance (pasta) 4687550 Merge #7385: fix: reject invalid extended masternode net info (Pasta) 58b9bce fix: keep flat-file positions 64-bit clean at the remaining raw seek/tell sites (Konstantin Akimov) 7a21752 Merge bitcoin#36667: net: enforce timeout interval for paused send peers (Konstantin Akimov) 58abb28 Merge bitcoin#36665: sock: align exception monitoring between poll() and select() backends (Konstantin Akimov) 8030deb Merge bitcoin#36669: refactor: store positions as a 64 bits integer in flatfiles (Konstantin Akimov) b1178f9 fix(llmq): judge every sig-share sender by its own verified shares (pasta) 33ec74e fix(governance): cap the orphan parent objects re-requested per tick (pasta) 7fc0edb doc(rpc): mark provider payload network fields optional in the help (pasta) 2aa3a87 fix(rpc): don't dereference a missing netInfo when decoding provider payloads (pasta) 120b5db fix(llmq): verify DKG contributions individually when a batch vvec check fails (pasta) 808b105 Merge #7486: fix: guard VerifyContributionShares against an empty verification-vector set (pasta) 8351db9 fix(llmq): reject invalid decrypted DKG shares before aggregation (pasta) 07494dd fix(consensus): reject negative transaction nVersion after DIP3 (pasta) 11ea306 Merge #7947: fix: stop quorum key warming from stalling DKG on 2-core masternodes (pasta) 25d7c85 Merge #7826: fix: skip repeated authenticated Platform bans (pasta) 18073dd Merge #7850: perf: let CHECKDATASIG use the signature cache (pasta) 3759397 Merge #7833: fix: gate signing intake before quorum retrieval (pasta) db98ee2 fix: keep flat-file positions 64-bit clean at the remaining raw seek/tell sites (Konstantin Akimov) 54c9738 Merge bitcoin#36667: net: enforce timeout interval for paused send peers (Konstantin Akimov) f1adb2c Merge bitcoin#36665: sock: align exception monitoring between poll() and select() backends (Konstantin Akimov) 8becf6a Merge bitcoin#36669: refactor: store positions as a 64 bits integer in flatfiles (Konstantin Akimov) 4c0cbd6 fix(llmq): judge every sig-share sender by its own verified shares (pasta) 649259f fix(governance): cap the orphan parent objects re-requested per tick (pasta) 418f5ee fix(llmq): verify DKG contributions individually when a batch vvec check fails (pasta) abf2eb0 fix(llmq): reject invalid decrypted DKG shares before aggregation (pasta) 39bda8f fix(consensus): reject negative transaction nVersion after DIP3 (pasta) Pull request description: ## Issue being fixed or feature implemented Backports for v24.0.0-rc.5, on top of `v24.0.x` at the v24.0.0-rc.4 tag (0e7382d, #7917). This PR holds the must-haves for rc.5: the v23.1.9 fixes that v24.0.x does not have yet, and #7947 (DKG stalls that get 2-core masternodes PoSe-banned). Without the v23.1.9 part, rc.5 would be missing fixes that the already published v23.1.9 ships, and it would accept transactions that v23.1.9 nodes reject (see Breaking Changes). The remaining `backport-candidate-24.0.x` work (#7916 and its test prerequisite) will follow in a separate PR so this one stays reviewable. ## What was done? ### v23.1.9 fixes not yet on v24.0.x v23.1.9 (tag 67c106f) contains 46 commits since v23.1.8. Each one was checked by content against `v24.0.x`. Most are backports of develop PRs that `v24.0.x` already has, either from before its branch point or picked for rc.3/rc.4. The rest are picked here with `git cherry-pick -x` from `v23.1.x`, in v23.1.9 merge order: - `fix(consensus): reject negative transaction nVersion after DIP3` (07494dd), **consensus**. Context-only conflict in `primitives/transaction.h`. - `fix(llmq): reject invalid decrypted DKG shares before aggregation` (8351db9). The QDATA path that v23 has in `src/active/quorums.cpp` is `src/llmq/net_quorum.cpp` on this branch. The new `feature_llmq_dkgerrors.py` case passes numeric `dkgsimerror` rates. - `fix(llmq): verify DKG contributions individually when a batch vvec check fails` (120b5db). The new `bls_tests` case calls `CBLSWorker::Start(4)`, since `Start()` takes an explicit worker count here. - `fix(governance): cap the orphan parent objects re-requested per tick` (33ec74e). The test helper uses this branch's `governance::OrphanVote`. - `fix(llmq): judge every sig-share sender by its own verified shares` (b1178f9). Ported into `NetSigning::ProcessPendingSigShares` (`llmq/net_signing.cpp`). The second half of the v23 fix (malformed share → `badSources`) is already on this branch. - bitcoin#36669 (8030deb). The `util/system` hunks land in `util/fs_helpers.{h,cpp}`. Two upstream hunks that the v23 port dropped are restored here, because this branch has their equivalents: `BlockInfo::data_pos` becomes `uint64_t` (in `interfaces/chain.h`), and `AutoFile`'s XOR read/write paths use `FileTell64`. - bitcoin#36665 (58abb28), bitcoin#36667 (7a21752). Clean. - `fix: keep flat-file positions 64-bit clean at the remaining raw seek/tell sites` (58b9bce). Also covers `BlockFilterIndex::LookupFilterRange`, which only this branch has. - #7833 fix: gate signing intake before quorum retrieval - #7850 perf: let CHECKDATASIG use the signature cache - #7826 fix: skip repeated authenticated Platform bans The last three are develop merges, picked with `-m1 -x` from develop; they change exactly the same lines as their develop merges. Every adaptation above is described in its commit message, and each matches how the open v23.1.9 → develop merge (#7961) resolves the same commit. Already on `v24.0.x` in another form, so not picked: - The credit-pool snapshot fix for #7549 (b4cd3fa) is a v23 hand-port of develop's 8503c5d / 0325195 (#7639), which this branch already has. - `fix(evo): keep -allowprivatenet out of masternode address validation` (0d2e579) is already covered by #7888. - `fix(rpc): don't dereference a missing netInfo when decoding provider payloads` (2aa3a87) and its help-text follow-up (7fc0edb). Here those accesses go through `CHECK_NONFATAL`, so an unknown-version ProTx payload gets an RPC error instead of a crash. The v23 commit message names this as the develop equivalent. ### Merge of v23.1.9 The last commit merges the v23.1.9 tag (67c106f, now `master`) with the tree unchanged (`-s ours`). `master` moved to v23.1.9 today, and `check_merge` requires `master` to fast-forward to `v24.0.x` plus the PR. The final v24.0.0 also has to contain every v23.1.9 commit. All of v23.1.9's content is already on the branch through the picks above, so every commit resolves to the `v24.0.x` side, as #7961 did on develop. Release bookkeeping (version, v23.1.9 release notes, man pages, flatpak entry) stays with v24's own release process. A trial real merge confirmed this: its only clean additions were those docs and three duplicate re-insertions of code the branch already has. ### #7947 fix: stop quorum key warming from stalling DKG on 2-core masternodes Picked with `-m1 -x`, before the merge. It changes exactly the same lines as its develop merge, and `v24.0.x` has 756fc4f (#7659), the commit it reverts. No version change is needed. `configure.ac` is already 24.0.0 with `CLIENT_VERSION_IS_RELEASE` false, so the build takes its version string from the `v24.0.0-rc.5` tag. ## How Has This Been Tested? Linux x86_64 against depends, `--enable-debug --enable-werror --disable-hardening --without-gui`: `dashd` and `test_dash` build cleanly. - Unit suites, all passing (149 cases): `transaction_tests`, `txvalidation_tests`, `sighash_tests`, `bls_tests`, `governance_inv_tests`, `llmq_utils_tests`, `llmq_dkg_tests`, `quorum_proofs_tests`, `flatfile_tests`, `fs_tests`, `net_tests`, `sock_tests`, `evo_netinfo_tests`, `evo_assetlocks_tests`, `blockfilter_index_tests`, `txindex_tests`, `blockmanager_tests`, `checkdatasig_tests`. - Functional tests, all passing: `feature_llmq_dkgerrors.py` (including the new contribution-invalid case), `feature_llmq_signing.py` (plain and `--spork21`), `feature_llmq_simplepose.py` (plain and `--disable-spork23`), `feature_llmq_rotation.py`, `feature_llmq_data_recovery.py`, `p2p_timeouts.py` (v1/v2), `mempool_accept.py`, `p2p_platform_ban.py`. In the first parallel run, `feature_llmq_data_recovery.py` (known flaky, see #7947), one `feature_llmq_signing.py --spork21` run and `p2p_platform_ban.py` failed while another build was loading the machine. All three pass when re-run on their own. - Each port was compared with its v23.1.9 commit and with #7961's develop resolution. The per-file changes are identical apart from the branch's own API differences listed above. A separate review checked v24-only call sites for second copies of each bug: - other `SigShareKey` batch verifiers; - other decrypt-then-aggregate paths; - other orphan re-request loops; - other tx version packing sites; - other raw `FlatFilePos` seeks. It also checked that the batch-vvec fallback cannot deadlock on a single-thread BLS pool; it runs on the DKG phase threads, not on a BLS worker. ## Breaking Changes - Consensus: once DIP3 is active, transactions with a negative `nVersion` are rejected (`bad-txns-version`). Every v23.1.9 node already enforces this, so mixed v23.1.9/rc.4 networks could disagree until rc.5 is deployed. Valid transactions and their txids are unchanged. - From #7947: quorum key share warming runs on its own `q-cache` thread again, which is one more thread per node. ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone 🤖 Generated with [Claude Code](https://claude.com/claude-code) Top commit has no ACKs. Tree-SHA512: eaf46d1446c5f6752f2ac5d7aeaa37234cb6da4430db0a1a432320cfe61242dc3bb90c42e0a364238c2817ce0b1b1654dcc0cdd1ed9570f3b35cd2af5330e98e

github-actions Bot mentioned this pull request Aug 3, 2026

fix: bound bitset allocation and stop deserializing DKG messages at intake #7523

Closed

3 of 5 tasks

coderabbitai Bot reviewed Aug 3, 2026

View reviewed changes

thepastaclaw reviewed Aug 3, 2026

View reviewed changes

PastaPastaPasta requested review from UdjinM6 and knst August 3, 2026 18:10

github-actions Bot added the needs rebase label Aug 3, 2026

github-actions Bot removed the needs rebase label Aug 3, 2026

PastaPastaPasta commented Aug 3, 2026

View reviewed changes

thepastaclaw reviewed Aug 3, 2026

View reviewed changes

knst reviewed Aug 4, 2026

View reviewed changes

thepastaclaw reviewed Aug 4, 2026

View reviewed changes

PastaPastaPasta force-pushed the sec/v021a-bitset-bound branch from 2a1155f to b429615 Compare August 4, 2026 21:15

knst added this to the 24 milestone Aug 6, 2026

knst reviewed Aug 6, 2026

View reviewed changes

PastaPastaPasta force-pushed the sec/v021a-bitset-bound branch from b429615 to aa1447f Compare August 6, 2026 16:03

thepastaclaw reviewed Aug 6, 2026

View reviewed changes

knst approved these changes Aug 6, 2026

View reviewed changes

PastaPastaPasta merged commit 4c33e73 into dashpay:develop Aug 7, 2026
33 of 37 checks passed

PastaPastaPasta added the backport-candidate-23.1.x label Aug 13, 2026 — with ChatGPT Codex Connector

sidhujag mentioned this pull request Aug 20, 2026

fix(llmq): bound bitset message decoding syscoin/syscoin#614

Merged

PastaPastaPasta mentioned this pull request Oct 29, 2026

chore: merge master (v23.1.9) back into develop #7961

Merged

4 of 5 tasks

		/** A legitimately sized DYNBITSET (LLMQ max 400) must still round-trip unchanged. */
		BOOST_AUTO_TEST_CASE(dynbitset_accepts_legitimate_llmq_size)

Conversation

PastaPastaPasta commented Aug 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue being fixed or feature implemented

What was done?

How Has This Been Tested?

Breaking Changes

Checklist:

Uh oh!

thepastaclaw commented Aug 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot commented Aug 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews paused

Walkthrough

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Aug 3, 2026

Choose a reason for hiding this comment

Uh oh!

thepastaclaw left a comment

Choose a reason for hiding this comment

Final validation — Codex + Sonnet

Review provenance

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Aug 3, 2026

Uh oh!

PastaPastaPasta left a comment

Choose a reason for hiding this comment

Uh oh!

thepastaclaw left a comment

Choose a reason for hiding this comment

Final validation — Codex + Sonnet

Review provenance

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thepastaclaw left a comment

Choose a reason for hiding this comment

Final validation — Codex + Sonnet

Review provenance

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot commented Aug 4, 2026

Uh oh!

PastaPastaPasta commented Aug 5, 2026

Uh oh!

knst left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot commented Aug 6, 2026

Uh oh!

coderabbitai Bot commented Aug 6, 2026

Uh oh!

thepastaclaw left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Final validation — Codex + Sonnet

Review provenance

Uh oh!

Choose a reason for hiding this comment

Uh oh!

knst left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

PastaPastaPasta commented Aug 3, 2026 •

edited

Loading

thepastaclaw commented Aug 3, 2026 •

edited

Loading

coderabbitai Bot commented Aug 3, 2026 •

edited

Loading

thepastaclaw left a comment •

edited

Loading