backport: compact block relay hardening (bitcoin#26898, #27626, #27743, #26969, #29412, #32646, #33296) - #7398
Merged
PastaPastaPasta merged 7 commits intoJul 7, 2026
Conversation
|
This pull request has conflicts, please rebase. |
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
|
✅ Review complete (commit 9d947f3) |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📥 CommitsReviewing files that changed from the base of the PR and between 2487b9a224f621e03e347e7ad12cbc09bb01698d and 9d947f3. 📒 Files selected for processing (15)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (10)
WalkthroughThis PR adds merkle-root mutation detection through Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant HonestRelayer
participant Attacker
participant Node
participant PeerManagerImpl
participant PartiallyDownloadedBlock
participant Validation
HonestRelayer->>Node: msg_cmpctblock
Node->>PartiallyDownloadedBlock: InitData / FillBlock
Node->>PeerManagerImpl: track in-flight block
Attacker->>Node: msg_block (mutated)
Node->>Validation: IsBlockMutated
Validation-->>Node: true
Node->>PeerManagerImpl: RemoveBlockRequest(hash, attacker)
Node->>Attacker: disconnect
HonestRelayer->>Node: msg_blocktxn
Node->>PartiallyDownloadedBlock: ProcessCompactBlockTxns
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@test/functional/p2p_compactblocks.py`:
- Around line 594-596: The disconnect wait in the compact block
duplicate-blocktxn test is happening outside the `assert_debug_log` context,
which can race with async processing. Update the `test_p2p_compactblocks` flow
so `test_node.wait_for_disconnect()` stays inside the same
`node.assert_debug_log(['previous compact block reconstruction attempt
failed'])` block used around `send_message()`, matching the safer pattern
already used in `p2p_mutated_blocks.py`.
In `@test/functional/p2p_mutated_blocks.py`:
- Around line 66-67: The continuation in the blockhash/indexes assertion is
using a hanging indent that triggers Flake8 E127. Update the multiline return
expression in the affected test helper so the continued line aligns with the
surrounding expression style used in the file, keeping the condition in the same
predicate intact while fixing the indentation in the block that compares
get_block_txn.block_txn_request.blockhash and
get_block_txn.block_txn_request.indexes.
🪄 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: b8befd59-6620-4def-bddf-24b27557b289
📥 Commits
Reviewing files that changed from the base of the PR and between 547bf5e and 786a47b05bf7712fdb9f9e161b86a1366cee0e36.
📒 Files selected for processing (14)
src/Makefile.test.includesrc/blockencodings.cppsrc/blockencodings.hsrc/net.hsrc/net_processing.cppsrc/net_processing.hsrc/primitives/block.hsrc/rpc/blockchain.cppsrc/test/fuzz/partially_downloaded_block.cppsrc/validation.cppsrc/validation.htest/functional/p2p_compactblocks.pytest/functional/p2p_mutated_blocks.pytest/functional/test_runner.py
thepastaclaw
suggested changes
Jul 1, 2026
There was a problem hiding this comment.
Code Review
Policy gate: an agent-reported missing upstream prerequisite was restored as a blocking finding. This PR is a full Bitcoin backport, and omitted upstream hunks require their prerequisite PRs unless the finding is explicitly allowlisted (intentional_exclusion / policy_override). The agent's original evidence is preserved in the finding(s) below.
Prior verifier summary (overridden by policy gate): Clean 8-commit compact-block hardening backport stack with faithful non-segwit adaptations. One notable Dash-specific deviation from upstream persists in the BLOCK success-path RemoveBlockRequest call (should mirror the fix already applied to the BLOCKTXN path in bitcoin#26969) and one worthwhile test-coverage suggestion for the new IsBlockMutated predicate. No blockers.
🔴 1 blocking | 🟡 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/net_processing.cpp`:
- [BLOCKING] src/net_processing.cpp:5263: BLOCK success-path RemoveBlockRequest deviates from bitcoin#27608/#27626 (should be pfrom.GetId(), not std::nullopt)
In the BLOCK message handler success path (inside `if (msg_type == NetMsgType::BLOCK)`, after `forceProcessing = IsBlockRequested(hash);`), the current code is:
RemoveBlockRequest(hash, std::nullopt);
Upstream bitcoin#27608 introduced this call as `RemoveBlockRequest(hash, pfrom.GetId())` specifically so that a peer's BLOCK message cannot clear another peer's in-flight state (that is the entire point of #27608's added `from_peer` parameter). Upstream bitcoin#27626 does not modify this line — verified against `51c050787f:src/net_processing.cpp` where the equivalent call is `RemoveBlockRequest(hash, pfrom.GetId());`. In the post-#29412 upstream tree (`2649e655b9:src/net_processing.cpp:4740`), it is likewise `RemoveBlockRequest(hash, pfrom.GetId());`.
Evidence of the regression inside this PR stack:
- Dash's #27608 cherry-pick `dcc6e58f5c` correctly landed `RemoveBlockRequest(hash, pfrom.GetId())` in the BLOCK success path.
- Dash's #27626 cherry-pick `2e31db8d8e` (auto-backported from `7b8553f630`) reverted it to `RemoveBlockRequest(hash, std::nullopt)`; the same auto-backport also flipped the BLOCKTXN success-path call to `std::nullopt`.
- The #26969 cherry-pick `b1798d239a` explicitly re-aligned the BLOCKTXN success-path RemoveBlockRequest back to `pfrom.GetId()` (see its commit message: "aligns the success-path RemoveBlockRequest to upstream's pfrom.GetId() (the #27626 auto-backport had used std::nullopt)"). The identical slip in the BLOCK success path was not corrected.
Behavioral impact is mild but real and matches the scenario #27608 was written to prevent: with the current `std::nullopt`, a BLOCK message from peer B for a hash we are also fetching from peer A will clear peer A's in-flight entry, which is exactly the interference #27608 fixed. The regression is masked by the p2p_mutated_blocks.py functional test because that test goes through the mutation branch (which correctly uses `RemoveBlockRequest(pblock->GetHash(), pfrom.GetId())` at line 5252), not the success branch.
Suggest changing line 5263 to `RemoveBlockRequest(hash, pfrom.GetId());` so this cherry-pick fully matches upstream, mirroring the fix already applied to the BLOCKTXN success path.
---
**Policy gate (backport-prereq-restore):** For full upstream backport PRs, a missing prerequisite is blocking unless the finding is explicitly allowlisted (e.g. `intentional_exclusion: true` or a matching entry in `policy_overrides`). The agent's original evidence above is the basis for this block; either backport the prerequisite or annotate the intentional exclusion in the PR description.
In `src/validation.cpp`:
- [SUGGESTION] src/validation.cpp:4073-4081: Add direct unit coverage for the Dash-specific IsBlockMutated predicate
This stack introduces Dash's slimmed `IsBlockMutated()` (no witness commitment path), but the added coverage exercises it only indirectly: the new functional test (`p2p_mutated_blocks.py`) hits the merkle-root mismatch path via P2P, and the fuzz harness in `test/fuzz/partially_downloaded_block.cpp` mocks `IsBlockMutated()` rather than exercising the real predicate. Since this predicate is the load-bearing defence-in-depth check that decides whether a reconstructed compact block is treated as a mutation/collision failure, a small unit test covering (a) merkle-root mismatch, (b) duplicate-merkle-branch mutation, and (c) the no-coinbase / 64-byte-transaction case would make the Dash-specific adaptation materially less fragile against future refactors.
|
CodeRabbit's two current comments look worth folding into the next update:
These are in addition to the blocking backport divergence I left in the review ( |
PastaPastaPasta
force-pushed
the
backport-compact-block-hardening
branch
from
July 1, 2026 19:27
786a47b to
0a072b2
Compare
PastaPastaPasta
changed the title
thepastaclaw
suggested changes
Jul 1, 2026
There was a problem hiding this comment.
Code Review
Incremental+cumulative review at 0a072b27e9. The latest delta from 786a47b0 is mostly a rebase onto newer develop; I found no new in-scope findings in that delta. Both prior findings were re-validated against the current head and are still valid: the RemoveBlockRequest success-path blocker moved to src/net_processing.cpp:5216, and the direct IsBlockMutated() unit-coverage suggestion moved to src/validation.cpp:3886.
🔴 1 blocking | 🟡 1 suggestion(s)
Prior Finding Reconciliation
- STILL VALID:
src/net_processing.cpp:5216still callsRemoveBlockRequest(hash, std::nullopt)in theBLOCKsuccess path; it should remain peer-scoped withpfrom.GetId(). - STILL VALID:
src/validation.cpp:3886-3910still lacks direct unit coverage for Dash's slimmedIsBlockMutated()predicate; current coverage remains indirect or mocked.
🤖 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/net_processing.cpp`:
- [BLOCKING] src/net_processing.cpp:5216: BLOCK success-path RemoveBlockRequest passes std::nullopt instead of pfrom.GetId() (regression vs bitcoin#27608)
Carried forward from prior review. In the BLOCK message success path, after `forceProcessing = IsBlockRequested(hash);`, the current code is `RemoveBlockRequest(hash, std::nullopt);`. Upstream bitcoin#27608 introduced the `from_peer` parameter specifically so that one peer's BLOCK message cannot clear another peer's in-flight state for the same hash; upstream (both #27608 and post-#27626 / post-#29412) uses `RemoveBlockRequest(hash, pfrom.GetId())` here. Two other call sites in this same file — the mutated-block branch at line 5205 and the BLOCKTXN success path inside ProcessCompactBlockTxns (fixed by the #26969 cherry-pick, and explicitly called out in ae4bbabf's commit message) — already use `pfrom.GetId()`, confirming the BLOCK success path is the sole outlier and mirrors the same slip #26969 had already corrected for BLOCKTXN. With `std::nullopt`, in the parallel-compact-block regime introduced by #27626 in this same stack (MAX_CMPCTBLOCKS_INFLIGHT_PER_BLOCK = 3), a BLOCK from peer B for a hash we are also fetching from peer A will clear every sibling in-flight entry, defeating the fallback resiliency #27626 intended to provide — exactly the cross-peer interference #27608 was written to prevent. Neither p2p_mutated_blocks.py (mutation branch only) nor p2p_compactblocks.py (does not race a full BLOCK against a parallel compact-block fetch on the same hash) catches this.
In `src/validation.cpp`:
- [SUGGESTION] src/validation.cpp:3886-3910: Add direct unit coverage for the Dash-specific IsBlockMutated predicate
Carried forward from prior review. This stack introduces Dash's slimmed IsBlockMutated() (no witness-commitment half; retains CheckMerkleRoot plus the CVE-2012-2459 / 64-byte-transaction malleation guard using `::GetSerializeSize(*tx, PROTOCOL_VERSION)`). This predicate is the load-bearing defence-in-depth check for (a) rejecting mutated BLOCK messages in net_processing.cpp:5202 with a 100-point Misbehaving score and (b) validating reconstructed compact blocks inside PartiallyDownloadedBlock::FillBlock (bitcoin#32646 backport). Current coverage is indirect only: p2p_mutated_blocks.py exercises the merkle-root mismatch path over P2P, and src/test/fuzz/partially_downloaded_block.cpp installs `pdb.m_check_block_mutated_mock` rather than the real predicate. Upstream bitcoin#29412 added direct block_malleation unit coverage; a Dash-adapted blockencodings_tests / validation_tests entry covering (i) merkle-root mismatch, (ii) duplicate-merkle-branch mutation, and (iii) the no-coinbase / 64-byte-transaction branch would materially reduce the risk of a silent regression during future refactors of CBlock or transaction serialization.
PastaPastaPasta
force-pushed
the
backport-compact-block-hardening
branch
from
July 1, 2026 19:51
0a072b2 to
422e69f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/test/validation_tests.cpp (1)
42-98: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the 64-byte non-coinbase mutation branch.
IsBlockMutated()has a dedicated 64-byte transaction heuristic for blocks without a coinbase, but this test only covers merkle mismatch and duplicated-tx mutation cases.Proposed coverage addition
block.hashMerkleRoot = BlockMerkleRoot(block); BOOST_CHECK(is_not_mutated(block)); + // A non-coinbase block containing a 64-byte transaction is treated as mutated. + CBlock no_coinbase_block; + CMutableTransaction tx_64; + tx_64.vin.resize(1); + tx_64.vin[0].prevout = COutPoint{uint256::ONE, 0}; + tx_64.vin[0].scriptSig.resize(4); + tx_64.vout.resize(1); + no_coinbase_block.vtx.push_back(MakeTransactionRef(tx_64)); + BOOST_CHECK_EQUAL(::GetSerializeSize(*no_coinbase_block.vtx[0], PROTOCOL_VERSION), 64); + no_coinbase_block.hashMerkleRoot = BlockMerkleRoot(no_coinbase_block); + BOOST_CHECK(is_mutated(no_coinbase_block)); + // A block with a duplicated transaction (CVE-2012-2459) is mutated even🤖 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/validation_tests.cpp` around lines 42 - 98, Add a test case in block_malleation to exercise the 64-byte non-coinbase mutation path in IsBlockMutated(): create a block without a coinbase transaction whose transaction data hashes to the special 64-byte heuristic condition, then verify the block is reported as mutated even when the merkle root matches. Reuse the existing helpers is_mutated and is_not_mutated, and place the new coverage alongside the current CBlock/BlockMerkleRoot mutation checks so the dedicated branch is explicitly covered.
🤖 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/net_processing.cpp`:
- Around line 2069-2073: The GETDATA path in the block request flow leaves stale
in-flight state when the push to the peer fails. Update the request handling
around BlockRequested() and ForNode() so that if sending the GETDATA message
returns false, you roll back the entries added to vBlocksInFlight,
mapBlocksInFlight, and m_peers_downloading_from before returning the error. Keep
the fix localized to the block-download logic in net_processing.cpp, including
the duplicate path referenced by the same BlockRequested()/ForNode() sequence.
---
Nitpick comments:
In `@src/test/validation_tests.cpp`:
- Around line 42-98: Add a test case in block_malleation to exercise the 64-byte
non-coinbase mutation path in IsBlockMutated(): create a block without a
coinbase transaction whose transaction data hashes to the special 64-byte
heuristic condition, then verify the block is reported as mutated even when the
merkle root matches. Reuse the existing helpers is_mutated and is_not_mutated,
and place the new coverage alongside the current CBlock/BlockMerkleRoot mutation
checks so the dedicated branch is explicitly covered.
🪄 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: d9b1c6d4-2f0d-49a0-8f3f-273d97f2cb40
📥 Commits
Reviewing files that changed from the base of the PR and between 0a072b27e9e081cf4982df2bcff6dcfee5bb790b and 422e69fffdfed13954fad923bfcecdff3d9ff2ff.
📒 Files selected for processing (15)
src/Makefile.test.includesrc/blockencodings.cppsrc/blockencodings.hsrc/net.hsrc/net_processing.cppsrc/net_processing.hsrc/primitives/block.hsrc/rpc/blockchain.cppsrc/test/fuzz/partially_downloaded_block.cppsrc/test/validation_tests.cppsrc/validation.cppsrc/validation.htest/functional/p2p_compactblocks.pytest/functional/p2p_mutated_blocks.pytest/functional/test_runner.py
✅ Files skipped from review due to trivial changes (2)
- src/rpc/blockchain.cpp
- src/net.h
🚧 Files skipped from review as they are similar to previous changes (9)
- src/Makefile.test.include
- test/functional/test_runner.py
- src/primitives/block.h
- src/net_processing.h
- test/functional/p2p_mutated_blocks.py
- src/validation.h
- src/blockencodings.h
- src/validation.cpp
- src/blockencodings.cpp
Comment on lines
+2069
to
2073
| // Forget about all prior requests | ||
| RemoveBlockRequest(block_index.GetBlockHash(), std::nullopt); | ||
|
|
||
| // Mark block as in-flight | ||
| if (!BlockRequested(peer_id, block_index)) return "Already requested from this peer"; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Roll back the in-flight entry if the GETDATA push fails.
BlockRequested() updates vBlocksInFlight, mapBlocksInFlight, and m_peers_downloading_from before ForNode() can return false. On that failure path, the RPC returns an error but leaves a download recorded that was never requested.
Proposed fix
bool success = m_connman.ForNode(peer_id, [this, &invs](CNode* node) {
const CNetMsgMaker msgMaker(node->GetCommonVersion());
this->m_connman.PushMessage(node, msgMaker.Make(NetMsgType::GETDATA, invs));
return true;
});
- if (!success) return "Peer not fully connected";
+ if (!success) {
+ RemoveBlockRequest(hash, peer_id);
+ return "Peer not fully connected";
+ }Also applies to: 2080-2086
🤖 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/net_processing.cpp` around lines 2069 - 2073, The GETDATA path in the
block request flow leaves stale in-flight state when the push to the peer fails.
Update the request handling around BlockRequested() and ForNode() so that if
sending the GETDATA message returns false, you roll back the entries added to
vBlocksInFlight, mapBlocksInFlight, and m_peers_downloading_from before
returning the error. Keep the fix localized to the block-download logic in
net_processing.cpp, including the duplicate path referenced by the same
BlockRequested()/ForNode() sequence.
There was a problem hiding this comment.
Verified this one at current head 422e69ff: BlockRequested(peer_id, block_index) updates the in-flight tracking before ForNode(peer_id, ...) runs, and the !success path currently returns without rolling that state back. So a disconnected/not-fully-connected peer can leave a stale request recorded even though no GETDATA was sent.
This should be fixed in the next update, ideally with a localized rollback such as RemoveBlockRequest(hash, peer_id) before returning Peer not fully connected. The separate 64-byte non-coinbase IsBlockMutated() coverage gap is already covered by my current blocking review.
There was a problem hiding this comment.
@thepastaclaw, agreed — thanks for confirming this at 422e69ff. The rollback via RemoveBlockRequest(hash, peer_id) before returning "Peer not fully connected" is the right localized fix, since BlockRequested() already commits the peer to vBlocksInFlight/mapBlocksInFlight/m_peers_downloading_from before ForNode() even attempts the push. I'll keep this flagged as a blocking item pending that update, and I've noted that the 64-byte non-coinbase IsBlockMutated() gap is being tracked separately under your blocking review rather than here.
(っ◔‿◔)っ 🥕
🧠 Learnings used
Learnt from: knst
Repo: dashpay/dash PR: 7113
File: src/net_processing.cpp:2369-2371
Timestamp: 2026-01-22T08:03:52.099Z
Learning: In src/net_processing.cpp, justify and preserve the design that CLSIG handling uses ChainlockHandler::AlreadyHave (backed by a seenChainLocks cache) to avoid re-requesting older chainlocks, while GetData(... MSG_CLSIG ...) serves only the current best via chainlock::Chainlocks::GetChainLockByHash. This mismatch is intentional because the node propagates only the best CLSIG and discards older ones; NOTFOUND for old CLSIG requests is acceptable. When reviewing related changes, confirm that the code continues to avoid re-fetching older CLSIGs, continues to only propagate the latest CLSIG, and that tests cover that NOTFOUND for outdated CLSIGs is expected behavior.
Learnt from: UdjinM6
Repo: dashpay/dash PR: 7265
File: src/wallet/wallet.cpp:3381-3382
Timestamp: 2026-04-03T14:17:17.656Z
Learning: In Dash Core, `strprintf()` uses `tinyformat` (type-safe printf). Do not flag format-specifier mismatches for integer types when using `strprintf()`: `%d` is correct for any integer type, including `CAmount`/`int64_t` arguments. Only treat `%d`/integer combinations as a formatting issue if they occur in a non-`strprintf()` (e.g., libc `printf`) context or there is evidence the format string is not being handled by `tinyformat`.
Learnt from: knst
Repo: dashpay/dash PR: 7275
File: src/wallet/scriptpubkeyman.cpp:2152-2157
Timestamp: 2026-04-21T07:05:38.615Z
Learning: In dashpay/dash, the CMake configuration strips `-DNDEBUG`/`/DNDEBUG` from release-mode compiler flags (per `src/secp256k1/CMakeLists.txt`), so `assert()` remains enabled in all build types. Therefore, in this repository, do not treat bare `assert()` usage as “unsafe-in-release-builds” or a release-only issue; only flag `assert()` concerns when there is a different, concrete problem (e.g., side effects, incorrect assumptions, or thread-safety), not based solely on release/build-type behavior.
Learnt from: knst
Repo: dashpay/dash PR: 7256
File: src/support/allocators/secure.h:63-66
Timestamp: 2026-06-02T11:23:47.755Z
Learning: In the dashpay/dash repository, if the code under review is a direct backport from upstream Bitcoin Core (e.g., the changes match an upstream Bitcoin Core PR/commit such as bitcoin#28500), avoid raising or insisting on review comments that would introduce differences from upstream. If a maintainer confirms a comment is irrelevant because it concerns backported/upstream-matching code, acknowledge that rationale and resolve the comment without requesting changes.
thepastaclaw
suggested changes
Jul 1, 2026
There was a problem hiding this comment.
Code Review
Incremental review of 0a072b27..422e69ff: prior-1 is fixed because src/net_processing.cpp:5216 now passes pfrom.GetId(). Prior-2 is mostly addressed by the new direct block_malleation unit test, but the retained 64-byte non-coinbase mutation branch remains untested. No separate new latest-delta issues were found; the deterministic backport policy gate keeps that residual omitted upstream coverage as blocking until covered or explicitly allowlisted.
🔴 1 blocking
Prior Finding Reconciliation
- FIXED:
src/net_processing.cpp:5216now callsRemoveBlockRequest(hash, pfrom.GetId()), so the prior BLOCK success-path blocker is resolved. - STILL VALID (narrowed):
src/validation.cpp:3894-3902still lacks direct coverage for Dash's retained 64-byte non-coinbaseIsBlockMutated()branch.
🤖 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/validation.cpp`:
- [BLOCKING] src/validation.cpp:3894-3902: Missing upstream coverage for the retained 64-byte transaction mutation branch
Carried forward from prior-2, narrowed after the latest delta. The new `validation_tests::block_malleation` case directly covers empty/single-coinbase blocks, merkle-root mismatch, and CVE-2012-2459 duplicate-transaction mutation, but it still omits the retained branch at `src/validation.cpp:3894-3902`: a block with no coinbase and a transaction whose serialized size is exactly 64 bytes. That branch is not witness-specific; Dash kept it from bitcoin#29412 using `::GetSerializeSize(*tx, PROTOCOL_VERSION)`, and upstream also pins it with a direct `block_malleation` assertion. Add the Dash-adapted 64-byte non-coinbase case so the retained malleation defense is covered alongside the merkle-root and duplicate-merkle cases.
---
**Policy gate (backport-prereq-restore):** For full upstream backport PRs, a missing upstream hunk/coverage item reported as a backport prerequisite is blocking unless it is explicitly allowlisted (for example `intentional_exclusion: true` or a matching `policy_overrides` entry). Add the adapted upstream assertion or document the intentional exclusion in the PR description.
PastaPastaPasta
force-pushed
the
backport-compact-block-hardening
branch
from
July 2, 2026 04:47
422e69f to
2487b9a
Compare
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 `@test/functional/p2p_compactblocks.py`:
- Around line 838-907: The compact block reconstruction test leaves
stalling_peer’s cached getblocktxn state uncleared between num_missing
iterations, so the next announce_cmpct_block() call can pass against stale
last_message data instead of fresh behavior. Update
test_compactblock_reconstruction_parallel_reconstruction() to clear
stalling_peer’s getblocktxn state alongside delivery_peer, inbound_peer, and
outbound_peer at the end of each loop iteration, so the next round’s assertions
only see newly generated requests.
🪄 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: c97e46f8-5162-4939-8532-2a8c89463109
📥 Commits
Reviewing files that changed from the base of the PR and between 422e69fffdfed13954fad923bfcecdff3d9ff2ff and 2487b9a224f621e03e347e7ad12cbc09bb01698d.
📒 Files selected for processing (15)
src/Makefile.test.includesrc/blockencodings.cppsrc/blockencodings.hsrc/net.hsrc/net_processing.cppsrc/net_processing.hsrc/primitives/block.hsrc/rpc/blockchain.cppsrc/test/fuzz/partially_downloaded_block.cppsrc/test/validation_tests.cppsrc/validation.cppsrc/validation.htest/functional/p2p_compactblocks.pytest/functional/p2p_mutated_blocks.pytest/functional/test_runner.py
✅ Files skipped from review due to trivial changes (4)
- src/Makefile.test.include
- src/net.h
- src/rpc/blockchain.cpp
- src/net_processing.h
🚧 Files skipped from review as they are similar to previous changes (8)
- src/primitives/block.h
- src/validation.h
- test/functional/test_runner.py
- src/blockencodings.cpp
- src/blockencodings.h
- src/validation.cpp
- test/functional/p2p_mutated_blocks.py
- src/net_processing.cpp
thepastaclaw
approved these changes
Jul 2, 2026
There was a problem hiding this comment.
Code Review
Prior finding prior-1 (missing coverage for retained 64-byte non-coinbase transaction mutation branch in IsBlockMutated) is FIXED at head 2487b9a2: src/test/validation_tests.cpp:100-133 now adds both a 64-byte coinbase (asserts not mutated, verifying the coinbase-gated guard) and a 64-byte non-coinbase (asserts mutated, exercising src/validation.cpp:3894-3902) case, both using ::GetSerializeSize(*tx, PROTOCOL_VERSION) matching Dash's non-witness production predicate. Verified directly against the working tree. No new latest-delta findings and no still-valid carried-forward findings across all reviewer legs (Opus/Sonnet 5/Codex x general/commit-history/backport-reviewer).
5 tasks
knst
reviewed
Jul 2, 2026
| BlockValidationResult::BLOCK_INVALID_PREV, | ||
| BlockValidationResult::BLOCK_TIME_FUTURE, | ||
| BlockValidationResult::BLOCK_CHECKPOINT, | ||
| BlockValidationResult::BLOCK_CHAINLOCK}); |
Collaborator
There was a problem hiding this comment.
26898: mark as partial; due to missing BLOCK_HEADER_LOW_WORK
btw, it's added by https://github.com/dashpay/dash/pull/7318/files which is not merged yet.
nevermind, it's removed by 32646 anyway
| @@ -190,7 +190,8 @@ class CBlock : public CBlockHeader | |||
| std::vector<CTransactionRef> vtx; | |||
|
|
|||
| // memory only | |||
| mutable bool fChecked; | |||
| mutable bool fChecked; // CheckBlock() | |||
Collaborator
There was a problem hiding this comment.
29412: missing comment:
// Memory-only flags for caching expensive checks
|
|
||
| if (partialBlock.header.IsNull()) { | ||
| // It is possible for the header to be empty if a previous call to FillBlock wiped the header but | ||
| // left the PartiallyDownloadedBlock pointer around (i.e. a prior reconstruction attempt returned |
Collaborator
There was a problem hiding this comment.
33296:
text of comment is changed
(i.e. did not call RemoveBlockRequest)
->
i.e. a prior reconstruction attempt returned
| LogPrint(BCLog::NET, "Peer %d sent compact block transactions multiple times\n", pfrom.GetId()); | ||
| return; | ||
| } | ||
|
|
Collaborator
There was a problem hiding this comment.
33296 is partial due to missing changes relevant to first_in_flight
This code is added by bitcoin#27626 which is not backported yet.
UdjinM6
previously approved these changes
Jul 3, 2026
UdjinM6
left a comment
UdjinM6
left a comment
There was a problem hiding this comment.
utACK 2487b9a224f621e03e347e7ad12cbc09bb01698d
Backport of bitcoin#26898 (fuzz harness + assert->READ_STATUS_INVALID hardening in PartiallyDownloadedBlock). Dash adaptations to the fuzz harness: use tx->GetHash() (no segwit/GetWitnessHash), FUZZ_TARGET(..., .init=) instead of FUZZ_TARGET_INIT, drop nonexistent test/fuzz/util/mempool.h include (symbols come from test/util/txmempool.h + test/fuzz/util.h), and swap the absent BLOCK_HEADER_LOW_WORK for Dash's BLOCK_CHAINLOCK. (cherry picked from knst commit 2ea984f — bitcoin#26898)
This is a backport of bitcoin#27626 This PR attempts to mitigate a single case, where high bandwidth peers can bail us out of a flakey peer not completing blocks for us. We allow up to 2 additional getblocktxns requests per unique block. This would hopefully allow the chance for an honest high bandwidth peer to hand us the transactions even if the first in flight peer stalls out. Changes: - Convert mapBlocksInFlight from std::map to std::multimap to allow multiple peers to be downloading the same block - Add IsBlockRequestedFromOutbound() to check if block is being fetched from an outbound peer - Update RemoveBlockRequest() to take optional from_peer parameter to selectively remove only that peers request - Add MAX_CMPCTBLOCKS_INFLIGHT_PER_BLOCK = 3 constant - Remove nBlocksInFlight counter, use vBlocksInFlight.size() instead - Add parallel download test in p2p_compactblocks.py Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com> (cherry picked from commit 7b8553f)
… == READ_STATUS_FAILED Backport of bitcoin#27743 (commit d972695). Applied manually onto the post-bitcoin#27626 CMPCTBLOCK handler; Dash already uses plain CInv(MSG_BLOCK, ...) (no GetFetchFlags).
…tBlockTxns Backport of bitcoin#26969 (commit 77d6d89). Applied manually onto Dash's post-bitcoin#27626/bitcoin#27743 tree because the auto-merge duplicated class members; the extraction uses Dash's own BLOCKTXN handler body. Dash adaptations: Misbehaving(pfrom.GetId(), ...) (NodeId API), plain CInv(MSG_BLOCK, ...) (no GetFetchFlags), ProcessBlock(..., /*force_processing=*/true) (no min_pow_checked), and the method's lock annotation adds !m_peer_mutex (required by Dash's Misbehaving). Also aligns the success-path RemoveBlockRequest to upstream's pfrom.GetId() (the bitcoin#27626 auto-backport had used std::nullopt).
Backport of bitcoin#29412, slimmed for Dash. Introduces IsBlockMutated() and isolates the merkle-root check into CheckMerkleRoot() (with an m_checked_merkle_root cache on CBlock), and rejects/discourages mutated blocks early in the BLOCK message handler as a defence-in-depth DoS mitigation. Dash adaptations: Dash is not a segwit chain and has none of the witness-commitment primitives (GetWitnessCommitmentIndex, BlockWitnessMerkleRoot, HasWitness, m_checked_witness_commitment). IsBlockMutated therefore drops the witness/check_witness_root machinery entirely and reduces to the merkle-root and CVE-2012-2459 / 64-byte-transaction malleation checks; it uses ::GetSerializeSize(*tx, PROTOCOL_VERSION) instead of TX_NO_WITNESS. The early net_processing check is unconditional (no prev_block/DEPLOYMENT_SEGWIT gating) and uses Misbehaving(pfrom.GetId(), 100, ...).
Backport of bitcoin#32646 (commits bac9ee4, 28299ce), slimmed for Dash. PartiallyDownloadedBlock::FillBlock now calls IsBlockMutated() instead of CheckBlock() as a belt-and-suspenders mutation check on reconstructed compact blocks, and the vestigial READ_STATUS_CHECKBLOCK_FAILED status is removed. Dash adaptations: no segwit, so FillBlock keeps its 2-arg signature (no segwit_active parameter) and the mutation mock hook becomes IsBlockMutatedFn = std::function<bool(const CBlock&)> (m_check_block_mutated_mock). The fuzz harness is updated to mock IsBlockMutated instead of CheckBlock.
…lock Backport of bitcoin#33296 (commits 5e585a0, 8b62647), slimmed for Dash. Guards ProcessCompactBlockTxns against re-entering PartiallyDownloadedBlock::FillBlock when a prior reconstruction attempt returned READ_STATUS_FAILED and left the (now header-less) partial block in flight; such a second blocktxn now discourages and disconnects the peer with a clear reason. Adds the test_multiple_blocktxn_response regression test to p2p_compactblocks.py. Dash adaptations: no LookupBlockIndex/DEPLOYMENT_SEGWIT (Dash's FillBlock takes no segwit_active parameter), and Misbehaving(pfrom.GetId(), 100, ...). Note on the double-blocktxn SIGABRT: on this branch that crash is already eliminated by bitcoin#26898 (FillBlock returns READ_STATUS_INVALID instead of asserting on a null header, which the caller then handles by discouraging and removing the block request). Upstream's guard exists to avoid a LookupBlockIndex/Assume() on the wiped header, which Dash does not perform. This guard is therefore a faithful belt-and-suspenders port -- it rejects the duplicate earlier with a clearer message and skips a wasted reconstruction pass -- rather than the fix for a still-reachable crash.
PastaPastaPasta
force-pushed
the
backport-compact-block-hardening
branch
from
July 3, 2026 16:22
2487b9a to
9d947f3
Compare
thepastaclaw
approved these changes
Jul 3, 2026
There was a problem hiding this comment.
Code Review
Clean 7-commit Bitcoin Core compact-block relay hardening backport (bitcoin#26898, bitcoin#27626, bitcoin#27743, bitcoin#26969, bitcoin#29412, bitcoin#32646, bitcoin#33296). All agent reviewers (opus/sonnet5/codex across general, commit-history, and backport-reviewer specializations) converge on zero in-scope findings. Prior finding prior-1 (64-byte non-coinbase IsBlockMutated coverage) remains FIXED at head. The one codex-general finding on src/llmq/net_dkg.cpp is out of scope — that file is not part of this PR's 15-file backport stack and entered the branch via rebase onto develop.
Source: reviewers opus/opus, sonnet5/claude-sonnet-5, codex/gpt-5.5 for roles general, dash-core-commit-history, backport-reviewer; verifier opus/opus; specialists dash-core-commit-history, backport-reviewer.
5 tasks
thepastaclaw
added a commit
to thepastaclaw/dash
that referenced
this pull request
Jul 22, 2026
…ate for other peers Backport prerequisite for dashpay#7398: this is the isolated bitcoin#27608 commit from merged Dash PR dashpay#7237, required by the compact-block hardening stack's peer-specific RemoveBlockRequest calls. dashpay#7237 is intentionally not cherry-picked in full: only this narrow p2p prerequisite is in the v23.1.8 release tree.
thepastaclaw
pushed a commit
to thepastaclaw/dash
that referenced
this pull request
Jul 22, 2026
…26898, bitcoin#27626, bitcoin#27743, bitcoin#26969, bitcoin#29412, bitcoin#32646, bitcoin#33296) 9d947f3 Merge bitcoin#33296: net: check for empty header before calling FillBlock (pasta) fe1e05d Merge bitcoin#32646: p2p: Add witness mutation check inside FillBlock (pasta) c9b3d19 Merge bitcoin#29412: p2p: Don't process mutated blocks (pasta) 5306dfa Merge bitcoin#26969: net, refactor: net_processing, add ProcessCompactBlockTxns (pasta) 6668c2a Merge bitcoin#27743: Unconditionally return when compact block status == READ_STATUS_FAILED (pasta) cc87ed2 Merge bitcoin#27626: Parallel compact block downloads, take 3 (Claude Code) 622299e Merge bitcoin#26898: fuzz: Add PartiallyDownloadedBlock target (MarcoFalke) Pull request description: ## What Backports a stack of Bitcoin Core compact-block relay hardening PRs, including the fix for a remotely-triggerable crash in compact-block reconstruction. | PR | Title | |----|-------| | bitcoin#26898 | fuzz: Add `PartiallyDownloadedBlock` target (+ `FillBlock` returns `READ_STATUS_INVALID` instead of asserting on a null header) | | bitcoin#27626 | Parallel compact block downloads, take 3 | | bitcoin#27743 | Unconditionally return when compact block status == `READ_STATUS_FAILED` | | bitcoin#26969 | net, refactor: add `ProcessCompactBlockTxns` | | bitcoin#29412 | p2p: Don't process mutated blocks | | bitcoin#32646 | p2p: Add witness mutation check inside `FillBlock` | | bitcoin#33296 | net: check for empty header before calling `FillBlock` | > bitcoin#27608 (the `from_peer` parameter for `RemoveBlockRequest`), which the stack builds on, is already in `develop` via dashpay#7237, so it is not re-included here. ## Why The primary motivation is closing a remotely-triggerable abort. A malicious peer acting as the in-flight compact-block source could send a `cmpctblock` followed by two `blocktxn` messages: the first `blocktxn` fails reconstruction (merkle mismatch) and leaves the now header-less `PartiallyDownloadedBlock` in flight (the `READ_STATUS_FAILED` branch does not call `RemoveBlockRequest`), and the second `blocktxn` re-entered `PartiallyDownloadedBlock::FillBlock`, hitting `assert(!header.IsNull())` → `SIGABRT`. On this branch that crash is eliminated by bitcoin#26898 (`FillBlock` now returns `READ_STATUS_INVALID` on a null header, which the caller handles by discouraging + removing the block request), with bitcoin#33296's explicit guard as belt-and-suspenders. The stack also brings parallel compact-block downloads (fetch the same block from up to `MAX_CMPCTBLOCKS_INFLIGHT_PER_BLOCK` peers) and a defence-in-depth "don't process mutated blocks" check. ## Dash-specific adaptations (non-segwit) Dash is not a segwit chain and has none of the witness-commitment machinery (`GetWitnessCommitmentIndex`, `BlockWitnessMerkleRoot`, `HasWitness`, `m_checked_witness_commitment`, `GetWitnessHash`, `DEPLOYMENT_SEGWIT`). The witness-dependent PRs were slimmed accordingly: - **bitcoin#29412 `IsBlockMutated`** drops the witness-malleation branch entirely; it reduces to the merkle-root check (`CheckMerkleRoot`, with an `m_checked_merkle_root` cache on `CBlock`) plus the CVE-2012-2459 / 64-byte-transaction check (using `::GetSerializeSize(*tx, PROTOCOL_VERSION)` instead of `TX_NO_WITNESS`). The early `BLOCK`-message check is unconditional (no `prev_block`/`DEPLOYMENT_SEGWIT` gating, which upstream only needs for the witness half). - **bitcoin#32646** keeps `FillBlock`'s 2-arg signature (no `segwit_active`); the mutation mock hook becomes `IsBlockMutatedFn`. - **bitcoin#33296** drops the `LookupBlockIndex`/`DEPLOYMENT_SEGWIT` lookup. - `Misbehaving(Peer&, score, msg)` and `ProcessBlock(..., force_processing)` (no `min_pow_checked`) throughout. - Functional tests adapted for the non-witness model: `p2p_compactblocks.py` (the stalling-peer corruption uses a coinbase-content mutation rather than witness data) and a slimmed `p2p_mutated_blocks.py`. ## Testing - Full build, all 7 commits; every changed TU compiles clean. - Unit: full `test_dash` suite passes; `blockencodings_tests` and `validation_tests` specifically exercised. - Functional: `p2p_compactblocks.py` (incl. new `test_multiple_blocktxn_response`), `p2p_mutated_blocks.py` (new), `feature_block.py`, `p2p_invalid_block.py` (v1+v2), `p2p_sendheaders.py`, `p2p_unrequested_blocks.py`, `mining_basic.py` — all pass. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Top commit has no ACKs. Tree-SHA512: 66e910053e3576559e687986256a51c07fa7a1da9f3abb01091107e7b98d84d11a28f8a48d9ecf409e0779ab61d3cc77d3436df85d3c960c801b98894731799f
thepastaclaw
added a commit
to thepastaclaw/dash
that referenced
this pull request
Jul 22, 2026
…ate for other peers Narrow exception from Dash dashpay#7237: include only the Bitcoin Core bitcoin#27608 prerequisite required by later compact-block relay hardening (dashpay#7398). The full dashpay#7237 v0.26 batch is intentionally not included on v23.1.x. Co-Authored-By: Claude <noreply@anthropic.com>
thepastaclaw
pushed a commit
to thepastaclaw/dash
that referenced
this pull request
Jul 22, 2026
…26898, bitcoin#27626, bitcoin#27743, bitcoin#26969, bitcoin#29412, bitcoin#32646, bitcoin#33296) 9d947f3 Merge bitcoin#33296: net: check for empty header before calling FillBlock (pasta) fe1e05d Merge bitcoin#32646: p2p: Add witness mutation check inside FillBlock (pasta) c9b3d19 Merge bitcoin#29412: p2p: Don't process mutated blocks (pasta) 5306dfa Merge bitcoin#26969: net, refactor: net_processing, add ProcessCompactBlockTxns (pasta) 6668c2a Merge bitcoin#27743: Unconditionally return when compact block status == READ_STATUS_FAILED (pasta) cc87ed2 Merge bitcoin#27626: Parallel compact block downloads, take 3 (Claude Code) 622299e Merge bitcoin#26898: fuzz: Add PartiallyDownloadedBlock target (MarcoFalke) Pull request description: ## What Backports a stack of Bitcoin Core compact-block relay hardening PRs, including the fix for a remotely-triggerable crash in compact-block reconstruction. | PR | Title | |----|-------| | bitcoin#26898 | fuzz: Add `PartiallyDownloadedBlock` target (+ `FillBlock` returns `READ_STATUS_INVALID` instead of asserting on a null header) | | bitcoin#27626 | Parallel compact block downloads, take 3 | | bitcoin#27743 | Unconditionally return when compact block status == `READ_STATUS_FAILED` | | bitcoin#26969 | net, refactor: add `ProcessCompactBlockTxns` | | bitcoin#29412 | p2p: Don't process mutated blocks | | bitcoin#32646 | p2p: Add witness mutation check inside `FillBlock` | | bitcoin#33296 | net: check for empty header before calling `FillBlock` | > bitcoin#27608 (the `from_peer` parameter for `RemoveBlockRequest`), which the stack builds on, is already in `develop` via dashpay#7237, so it is not re-included here. ## Why The primary motivation is closing a remotely-triggerable abort. A malicious peer acting as the in-flight compact-block source could send a `cmpctblock` followed by two `blocktxn` messages: the first `blocktxn` fails reconstruction (merkle mismatch) and leaves the now header-less `PartiallyDownloadedBlock` in flight (the `READ_STATUS_FAILED` branch does not call `RemoveBlockRequest`), and the second `blocktxn` re-entered `PartiallyDownloadedBlock::FillBlock`, hitting `assert(!header.IsNull())` → `SIGABRT`. On this branch that crash is eliminated by bitcoin#26898 (`FillBlock` now returns `READ_STATUS_INVALID` on a null header, which the caller handles by discouraging + removing the block request), with bitcoin#33296's explicit guard as belt-and-suspenders. The stack also brings parallel compact-block downloads (fetch the same block from up to `MAX_CMPCTBLOCKS_INFLIGHT_PER_BLOCK` peers) and a defence-in-depth "don't process mutated blocks" check. ## Dash-specific adaptations (non-segwit) Dash is not a segwit chain and has none of the witness-commitment machinery (`GetWitnessCommitmentIndex`, `BlockWitnessMerkleRoot`, `HasWitness`, `m_checked_witness_commitment`, `GetWitnessHash`, `DEPLOYMENT_SEGWIT`). The witness-dependent PRs were slimmed accordingly: - **bitcoin#29412 `IsBlockMutated`** drops the witness-malleation branch entirely; it reduces to the merkle-root check (`CheckMerkleRoot`, with an `m_checked_merkle_root` cache on `CBlock`) plus the CVE-2012-2459 / 64-byte-transaction check (using `::GetSerializeSize(*tx, PROTOCOL_VERSION)` instead of `TX_NO_WITNESS`). The early `BLOCK`-message check is unconditional (no `prev_block`/`DEPLOYMENT_SEGWIT` gating, which upstream only needs for the witness half). - **bitcoin#32646** keeps `FillBlock`'s 2-arg signature (no `segwit_active`); the mutation mock hook becomes `IsBlockMutatedFn`. - **bitcoin#33296** drops the `LookupBlockIndex`/`DEPLOYMENT_SEGWIT` lookup. - `Misbehaving(Peer&, score, msg)` and `ProcessBlock(..., force_processing)` (no `min_pow_checked`) throughout. - Functional tests adapted for the non-witness model: `p2p_compactblocks.py` (the stalling-peer corruption uses a coinbase-content mutation rather than witness data) and a slimmed `p2p_mutated_blocks.py`. ## Testing - Full build, all 7 commits; every changed TU compiles clean. - Unit: full `test_dash` suite passes; `blockencodings_tests` and `validation_tests` specifically exercised. - Functional: `p2p_compactblocks.py` (incl. new `test_multiple_blocktxn_response`), `p2p_mutated_blocks.py` (new), `feature_block.py`, `p2p_invalid_block.py` (v1+v2), `p2p_sendheaders.py`, `p2p_unrequested_blocks.py`, `mining_basic.py` — all pass. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Top commit has no ACKs. Tree-SHA512: 66e910053e3576559e687986256a51c07fa7a1da9f3abb01091107e7b98d84d11a28f8a48d9ecf409e0779ab61d3cc77d3436df85d3c960c801b98894731799f
PastaPastaPasta
pushed a commit
to PastaPastaPasta/dash
that referenced
this pull request
Jul 29, 2026
…ate for other peers Cherry-pick of commit b89e9a6 from merged Dash PR dashpay#7237 (Bitcoin Core v0.26 backports, batch 1). Only this single commit from dashpay#7237 is taken: it is the piece the compact block relay hardening in dashpay#7398 depends on; the rest of the v0.26 batch is deliberately excluded from v23.1.x. Verified line-for-line identical (content) to the genuine upstream Bitcoin commit: bitcoin/bitcoin@dbfc748d3df (merge of bitcoin#27608, author Suhas Daftuar). NOTE FOR REVIEWERS: there is no reviewed Dash PR for this exact slice against v23.1.x - please validate taking this single commit rather than all of dashpay#7237. (cherry picked from commit b89e9a6)
PastaPastaPasta
added a commit
to PastaPastaPasta/dash
that referenced
this pull request
Jul 29, 2026
…26898, bitcoin#27626, bitcoin#27743, bitcoin#26969, bitcoin#29412, bitcoin#32646, bitcoin#33296) Backport of dashpay#7398 (upstream merge cfd18f1, cherry-picked with -m1). v23.1.x adaptations: (1) Misbehaving() on this branch only has the NodeId overload (develop's Peer& overload does not exist here), so the three new call sites use pfrom.GetId() instead of peer/*peer. (2) CheckBlock() keeps this branch's signature without the develop-only known_hash parameter, which predates this PR on develop and is not part of it. All other hunks are unchanged from upstream. (cherry picked from commit cfd18f13f5896c1b2a851764d99d38e2e21071c1)
PastaPastaPasta
added a commit
that referenced
this pull request
Jul 30, 2026
24920a0 chore: prepare v23.1.8 release (pasta) 2194248 Merge #7348: fix: penalize oversized notfound messages (pasta) f5c72c3 Merge #7347: fix: punish invalid dstx messages (pasta) 550caf7 Merge #7465: fix(qt): handle pixel-sized fonts when scaling widgets (pasta) e203710 Merge #7419: fix(net): bound CoinJoin message vector intake (pasta) 5f5b960 Merge #7418: fix(net): bound signing message vector intake (Pasta) 7cc2cca Merge #7450: test: make governance vote fixtures wire-valid (Pasta) f011c80 Merge #7440: fix(net): bound governance vote signature deserialization (Pasta) 4b4d96a Merge #7442: fix(net): authorize governance inv responses via the net-layer per-peer request tracker (Pasta) f855b13 Merge #7444: fix(net): bound bloom message vectors before allocation (Pasta) 5b5c6fb Merge #7415: fix: bound pending sig share queue (Pasta) 9bbe808 Merge #7416: fix(net): bound quorum data response vectors (Pasta) da42f50 Merge #7424: fix: bound ChainLock seen cache (Pasta) 5b310df Merge #7438: fix: bound SPORK signature deserialization (Pasta) e118d0c Merge #7259: fix: dangling point to cj client (Pasta) 9921621 Merge #7439: refactor: add bounded vector deserialization (Pasta) 89bdf7c Merge #7414: fix(net): throttle per-object governance vote sync requests (Pasta) 44c396d Merge #7402: fix: bound pending recovered sig queue to prevent remote OOM (Pasta) 05cfe27 Merge #7351: fix: limit signing share sessions per peer (pasta) 3ef3a5b Merge #7408: fix: bound DKG contribution blob intake (pasta) 0ea6532 Merge #7387: test: migrate governance inv cache coverage to unit tests (Pasta) 8ffdf7f Merge #7398: backport: compact block relay hardening (bitcoin#26898, bitcoin#27626, bitcoin#27743, bitcoin#26969, bitcoin#29412, bitcoin#32646, bitcoin#33296) (Pasta) 2915142 backport: bitcoin#27608 - p2p: Avoid prematurely clearing download state for other peers (PastaClaw) 90b5473 Merge #7396: fix: run of circular-dependencies with python3.15 (Pasta) b003cdc Merge #7395: ci: update GitHub Actions pins for Node 24 (pasta) 97c3dd1 Merge #7394: fix: stabilize par help text in manpages (pasta) 8f8616b Merge #7372: backport: bitcoin#32693: depends: fix cmake compatibility error for freetype (pasta) 48f72be Merge #7360: fix: empty platformP2PPort deprecated field in protx listdiff results (pasta) a8cccff Merge #7298: fix(qt): keep PoSe score visible when hiding banned masternodes (pasta) Pull request description: Release PR for Dash Core v23.1.8, a patch release on top of v23.1.7. Fast-forwards from `v23.1.x` (currently at `chore: prepare v23.1.7 release`), 29 commits, no merge commits, no conflicts. ## Contents Backports of PRs already reviewed and merged on `develop`: `#7259` `#7347` `#7348` `#7351` `#7298` `#7360` `#7372` `#7387` `#7394` `#7395` `#7396` `#7398` `#7402` `#7408` `#7414` `#7415` `#7416` `#7418` `#7419` `#7424` `#7438` `#7439` `#7440` `#7442` `#7444` `#7450` `#7465` Plus `backport: bitcoin#27608`, a single commit taken from Dash #7237 because #7398's compact-block hardening depends on it. The rest of that v0.26 batch is intentionally not included on v23.1.x. The commit is byte-identical to its reviewed counterpart inside #7237. And release preparation: version bump, regenerated man pages, release notes, archived 23.1.7 notes. ## Note for reviewers: this branch was rebuilt An earlier revision of this PR was discarded and the branch rebuilt from scratch. Review comments on the previous revision point at commits that no longer exist, though the feedback itself was carried over (see below). The reason: several commits titled `Merge #NNNN` in the earlier revision contained substantial code that exists nowhere upstream — apparently written from a description of each PR rather than ported from its diff. For example, `feature_llmq_simplepose.py` is byte-identical between v23.1.7 and `develop`, yet the earlier `Merge #7408` rewrote 66 lines of it; `test/functional/p2p_governance_invs.py` does not exist on `develop` at all, yet had grown from 62 to 148 lines. That mislabeling matters because a commit titled `Merge #NNNN` invites less scrutiny, not more. It also had consequences: the earlier revision was **missing #7440 entirely**, and contained eleven consecutive commits that did not compile (code written against newer upstream APIs this branch does not have — `Misbehaving(Peer&)`, and `PeerIsBanned` used five commits before it was declared). Every commit on this branch has now been diffed against its upstream merge commit. Where a backport differs, it is because v23.1.x predates an upstream refactor and the change had to be applied to the pre-refactor file — for example #7418 and #7438 patch `signing_shares.cpp` / `spork.cpp` where upstream patches `net_signing.cpp` / `net_processing.cpp`. ## Dropped from this branch - **#7350** (`net: don't lock cs_main while reading blocks`) — dropped on review feedback. It is a 110-line lock-structure refactor of `ProcessGetBlockData` with no measured benefit, and it would add avoidable churn to the eventual master→develop merge-back. Nothing on this branch depends on it: #7398's compact-block work precedes it, and the remaining 14 commits replay with zero conflicts once it is removed. Thanks @knst. ## Added after the initial review pass - **#7351** (`fix: limit signing share sessions per peer`) — cherry-picked as a single commit and placed before #7402, matching upstream's merge order. The include block additionally carries `<ranges>`: upstream's diff adds only `<algorithm>` because develop already had it, whereas v23.1.x did not and the backported `GetSessionCount()` / `GetAnnouncementSessionCount()` use `std::ranges::count_if`. - **#7465** (`fix(qt): handle pixel-sized fonts when scaling widgets`) — cherry-picked from the five upstream commits. `optiontests.cpp` additionally includes `qt/guiutil_font.h`, because `fontsLoaded()` and `updateFonts()` are declared there on v23.1.x while develop declares them in `qt/guiutil.h`, which is all the upstream test includes. Two further backports were added later and applied without any adaptation -- their diffs are byte-for-byte identical to upstream: - **#7347** (`fix: punish invalid dstx messages`) - **#7348** (`fix: penalize oversized notfound messages`) ## Adaptations worth flagging - **#7360** — upstream gates `platformP2PPort` / `platformHTTPPort` in `protx listdiff` behind `IsServiceDeprecatedRPCEnabled()`. On 23.x those deprecated fields are deliberately not enforced through gating (see `bbcd9d543e6`), so shipping the gate as-is would silently drop two fields that v23.1.7 always returned. Changed to `if (true)` with a comment, per review feedback, keeping the block aligned with `develop`. The substantive fix from #7360 — reading the live port from `netInfo` instead of the always-zero scalar — is retained. - **#7415** — the pending-map caps (`MAX_PENDING_SIG_SHARES_PER_NODE`, `MAX_PENDING_SIG_SHARES_TOTAL`) are backported. The additional bound upstream places on batches awaiting verification is not, because it guards a condition that does not exist here: upstream's dispatcher pushes one task per batch inside an inner loop, whereas v23.1.x pushes a single looping worker per 10 ms tick. There is no unbounded task queue to bound. - **Man pages** — regenerated without the `lock` debug category, which only exists under `DEBUG_LOCKCONTENTION` and so is absent from release binaries. Thanks @UdjinM6 for catching this. ## Known CI failure macOS jobs are expected to fail. `actions/upload-artifact@v6` rejects filenames containing `:`, and the Xcode SDK ships Perl man pages with `::` in the name. A release-branch-only workaround existed on the earlier revision but was dropped as it corresponds to no upstream PR. This is accepted for this release. ## Testing - Every commit through #7465 compiles individually (verified for 27 of the 29; the three additions below were verified at the tip) — verified individually, not just at the tip. - Full build clean; no new warnings. - Unit tests pass. - Functional tests pass: `feature_llmq_signing` (both variants), `feature_llmq_chainlocks`, `feature_llmq_dkgerrors`, `feature_llmq_is_cl_conflicts`, `p2p_instantsend`, `feature_dip3_deterministicmns` (both wallet types), `rpc_coinjoin`. - Qt unit tests pass (32 cases, run under the `cocoa` platform plugin so the pixel-sized font regression from #7465 actually executes rather than self-skipping). - Lint: one pre-existing `lint-cppcheck-dash` failure, identical on v23.1.7, in files this branch does not touch. Top commit has no ACKs. Tree-SHA512: 0fa469c9a33820aa85fbb8b90c5877409d09490f746f1300b05ceda470a600765f900bec42e5aad5d90a2d46b44e28c20e44dc4a8fe719553a072298069eaec4
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Backports a stack of Bitcoin Core compact-block relay hardening PRs, including the fix for a remotely-triggerable crash in compact-block reconstruction.
PartiallyDownloadedBlocktarget (+FillBlockreturnsREAD_STATUS_INVALIDinstead of asserting on a null header)READ_STATUS_FAILEDProcessCompactBlockTxnsFillBlockFillBlockWhy
The primary motivation is closing a remotely-triggerable abort. A malicious peer acting as the in-flight compact-block source could send a
cmpctblockfollowed by twoblocktxnmessages: the firstblocktxnfails reconstruction (merkle mismatch) and leaves the now header-lessPartiallyDownloadedBlockin flight (theREAD_STATUS_FAILEDbranch does not callRemoveBlockRequest), and the secondblocktxnre-enteredPartiallyDownloadedBlock::FillBlock, hittingassert(!header.IsNull())→SIGABRT.On this branch that crash is eliminated by bitcoin#26898 (
FillBlocknow returnsREAD_STATUS_INVALIDon a null header, which the caller handles by discouraging + removing the block request), with bitcoin#33296's explicit guard as belt-and-suspenders. The stack also brings parallel compact-block downloads (fetch the same block from up toMAX_CMPCTBLOCKS_INFLIGHT_PER_BLOCKpeers) and a defence-in-depth "don't process mutated blocks" check.Dash-specific adaptations (non-segwit)
Dash is not a segwit chain and has none of the witness-commitment machinery (
GetWitnessCommitmentIndex,BlockWitnessMerkleRoot,HasWitness,m_checked_witness_commitment,GetWitnessHash,DEPLOYMENT_SEGWIT). The witness-dependent PRs were slimmed accordingly:IsBlockMutateddrops the witness-malleation branch entirely; it reduces to the merkle-root check (CheckMerkleRoot, with anm_checked_merkle_rootcache onCBlock) plus the CVE-2012-2459 / 64-byte-transaction check (using::GetSerializeSize(*tx, PROTOCOL_VERSION)instead ofTX_NO_WITNESS). The earlyBLOCK-message check is unconditional (noprev_block/DEPLOYMENT_SEGWITgating, which upstream only needs for the witness half).FillBlock's 2-arg signature (nosegwit_active); the mutation mock hook becomesIsBlockMutatedFn.LookupBlockIndex/DEPLOYMENT_SEGWITlookup.Misbehaving(Peer&, score, msg)andProcessBlock(..., force_processing)(nomin_pow_checked) throughout.p2p_compactblocks.py(the stalling-peer corruption uses a coinbase-content mutation rather than witness data) and a slimmedp2p_mutated_blocks.py.Testing
test_dashsuite passes;blockencodings_testsandvalidation_testsspecifically exercised.p2p_compactblocks.py(incl. newtest_multiple_blocktxn_response),p2p_mutated_blocks.py(new),feature_block.py,p2p_invalid_block.py(v1+v2),p2p_sendheaders.py,p2p_unrequested_blocks.py,mining_basic.py— all pass.🤖 Generated with Claude Code