backport: compact block relay hardening (bitcoin#26898, #27626, #27743, #26969, #29412, #32646, #33296)#7398
Conversation
|
This pull request has conflicts, please rebase. |
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
|
✅ Review complete (commit 422e69f) |
WalkthroughThis PR adds merkle-root mutation detection and exposes it 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
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 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
📒 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
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.
| // need it even when it's not a candidate for a new best tip. | ||
| forceProcessing = IsBlockRequested(hash); | ||
| RemoveBlockRequest(hash); | ||
| RemoveBlockRequest(hash, std::nullopt); |
There was a problem hiding this comment.
🟡 Suggestion: BLOCK success-path RemoveBlockRequest should pass pfrom.GetId(), not std::nullopt
In the BLOCK message handler success path (after forceProcessing = IsBlockRequested(hash);), the call is:
RemoveBlockRequest(hash, std::nullopt);
This is a Dash-specific deviation from upstream. Upstream bitcoin#27608 introduced the from_peer argument here specifically so a BLOCK message from peer B cannot clear peer A's in-flight state — the entire premise of bitcoin#27608. Upstream keeps this as RemoveBlockRequest(hash, pfrom.GetId()) through bitcoin#27626, bitcoin#26969, bitcoin#29412, and later.
Provenance inside this stack:
dcc6e58f5c(Dash's p2p: Avoid prematurely clearing download state for other peers bitcoin/bitcoin#27608 cherry-pick) correctly landedpfrom.GetId()here.2e31db8d8e(auto-backport of Parallel compact block downloads, take 3 bitcoin/bitcoin#27626) flipped it tostd::nullopt, along with the analogous BLOCKTXN success-path call.b1798d239a(Dash's net, refactor: net_processing, addProcessCompactBlockTxnsbitcoin/bitcoin#26969 backport) explicitly re-aligned the BLOCKTXN success-path call back topfrom.GetId(), per its commit message. The identical slip in the BLOCK success path was not corrected here.
Behavioral impact matches exactly what bitcoin#27608 was written to prevent: with std::nullopt, a BLOCK from peer B for a hash also being fetched from peer A will clear peer A's in-flight entry. The regression is masked by p2p_mutated_blocks.py because that test goes through the mutation branch (line 5252 correctly uses pfrom.GetId()), not the success branch. Aligning this line with upstream restores the intended parallel-download isolation.
| RemoveBlockRequest(hash, std::nullopt); | |
| RemoveBlockRequest(hash, pfrom.GetId()); |
source: ['claude']
There was a problem hiding this comment.
Resolved in this update — BLOCK success-path RemoveBlockRequest should pass pfrom.GetId(), not std::nullopt 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.
| if (block.vtx.empty() || !block.vtx[0]->IsCoinBase()) { | ||
| // Consider the block mutated if any transaction is 64 bytes in size (see 3.1 | ||
| // in "Weaknesses in Bitcoin's Merkle Root Construction": | ||
| // https://lists.linuxfoundation.org/pipermail/bitcoin-dev/attachments/20190225/a27d8837/attachment-0001.pdf). | ||
| // | ||
| // Note: This is not a consensus change as this only applies to blocks that | ||
| // don't have a coinbase transaction and would therefore already be invalid. | ||
| return std::any_of(block.vtx.begin(), block.vtx.end(), | ||
| [](auto& tx) { return ::GetSerializeSize(*tx, PROTOCOL_VERSION) == 64; }); |
There was a problem hiding this comment.
🟡 Suggestion: 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.
source: ['codex']
| // need it even when it's not a candidate for a new best tip. | ||
| forceProcessing = IsBlockRequested(hash); | ||
| RemoveBlockRequest(hash); | ||
| RemoveBlockRequest(hash, std::nullopt); |
There was a problem hiding this comment.
🔴 Blocking: BLOCK success-path RemoveBlockRequest deviates from bitcoin#27608/bitcoin#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 bitcoin#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-bitcoin#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 p2p: Avoid prematurely clearing download state for other peers bitcoin/bitcoin#27608 cherry-pick
dcc6e58f5ccorrectly landedRemoveBlockRequest(hash, pfrom.GetId())in the BLOCK success path. - Dash's Parallel compact block downloads, take 3 bitcoin/bitcoin#27626 cherry-pick
2e31db8d8e(auto-backported from7b8553f630) reverted it toRemoveBlockRequest(hash, std::nullopt); the same auto-backport also flipped the BLOCKTXN success-path call tostd::nullopt. - The net, refactor: net_processing, add
ProcessCompactBlockTxnsbitcoin/bitcoin#26969 cherry-pickb1798d239aexplicitly re-aligned the BLOCKTXN success-path RemoveBlockRequest back topfrom.GetId()(see its commit message: "aligns the success-path RemoveBlockRequest to upstream's pfrom.GetId() (the Parallel compact block downloads, take 3 bitcoin/bitcoin#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 bitcoin#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 bitcoin#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.
| RemoveBlockRequest(hash, std::nullopt); | |
| RemoveBlockRequest(hash, pfrom.GetId()); |
source: ['claude-backport-reviewer']
There was a problem hiding this comment.
Resolved in this update — BLOCK success-path RemoveBlockRequest deviates from bitcoin#27608/bitcoin#27626 (should be pfrom.GetId(), not std::nullopt) 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.
|
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
786a47b to
0a072b2
Compare
PastaPastaPasta
changed the title
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.
| // need it even when it's not a candidate for a new best tip. | ||
| forceProcessing = IsBlockRequested(hash); | ||
| RemoveBlockRequest(hash, pfrom.GetId()); | ||
| RemoveBlockRequest(hash, std::nullopt); |
There was a problem hiding this comment.
🔴 Blocking: BLOCK success-path RemoveBlockRequest still clears all in-flight peers
Carried forward from prior-1. In the current head 0a072b2, the BLOCK message success path still calls RemoveBlockRequest(hash, std::nullopt);. The prior review established that upstream bitcoin#27608/bitcoin#27626 use RemoveBlockRequest(hash, pfrom.GetId()) here so one peer's BLOCK message cannot clear another peer's in-flight request. Since this is still present after the latest push, keep the prior cleanup request: change this call to use pfrom.GetId() so the backport matches the upstream peer-specific removal behavior.
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.
| RemoveBlockRequest(hash, std::nullopt); | |
| RemoveBlockRequest(hash, pfrom.GetId()); |
source: ['codex-dash-core-commit-history']
There was a problem hiding this comment.
Resolved in this update — BLOCK success-path RemoveBlockRequest still clears all in-flight peers 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.
| // need it even when it's not a candidate for a new best tip. | ||
| forceProcessing = IsBlockRequested(hash); | ||
| RemoveBlockRequest(hash, pfrom.GetId()); | ||
| RemoveBlockRequest(hash, std::nullopt); |
There was a problem hiding this comment.
🔴 Blocking: Backport regression against prerequisite bitcoin#27608/bitcoin#27626 in BLOCK success path
The current BLOCK message success path still clears all in-flight requests for the received block with RemoveBlockRequest(hash, std::nullopt). This diverges from the prerequisite change in upstream bitcoin#27608, which introduced the from_peer parameter specifically so one peer's BLOCK cannot clear another peer's download state; upstream changed this exact path to RemoveBlockRequest(hash, pfrom.GetId()), and bitcoin#27626 preserved that call. Dash already has the prerequisite API and uses pfrom.GetId() in the compact-block mutation and BLOCKTXN paths, so this is not a Dash adaptation. It is a missed conflict-resolution/backport alignment issue that can let peer B's successful BLOCK message clear peer A's in-flight state for the same hash.
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.
| RemoveBlockRequest(hash, std::nullopt); | |
| RemoveBlockRequest(hash, pfrom.GetId()); |
source: ['codex-backport-reviewer']
There was a problem hiding this comment.
Resolved in this update — Backport regression against prerequisite bitcoin#27608/bitcoin#27626 in BLOCK success path 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.
| bool IsBlockMutated(const CBlock& block) | ||
| { | ||
| BlockValidationState state; | ||
| if (!CheckMerkleRoot(block, state)) { | ||
| LogPrint(BCLog::VALIDATION, "Block mutated: %s\n", state.ToString()); | ||
| return true; | ||
| } | ||
|
|
||
| if (block.vtx.empty() || !block.vtx[0]->IsCoinBase()) { | ||
| // Consider the block mutated if any transaction is 64 bytes in size (see 3.1 | ||
| // in "Weaknesses in Bitcoin's Merkle Root Construction": | ||
| // https://lists.linuxfoundation.org/pipermail/bitcoin-dev/attachments/20190225/a27d8837/attachment-0001.pdf). | ||
| // | ||
| // Note: This is not a consensus change as this only applies to blocks that | ||
| // don't have a coinbase transaction and would therefore already be invalid. | ||
| return std::any_of(block.vtx.begin(), block.vtx.end(), | ||
| [](auto& tx) { return ::GetSerializeSize(*tx, PROTOCOL_VERSION) == 64; }); |
There was a problem hiding this comment.
🔴 Blocking: Soft omitted upstream coverage for Dash's IsBlockMutated adaptation
Upstream bitcoin#29412 added direct block_malleation unit coverage for IsBlockMutated, including merkle-root mismatch, duplicate-merkle mutation, and the 64-byte-transaction malleation case. Dash correctly slimmed the predicate for non-witness blocks, but the PR does not add an equivalent direct unit test for the non-witness branches. The existing P2P test covers the merkle-root mismatch path indirectly, and the PartiallyDownloadedBlock fuzz target mocks IsBlockMutated rather than exercising the real predicate. This is a soft completeness gap rather than a functional prerequisite blocker.
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.
source: ['codex-backport-reviewer']
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
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
📒 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
| // 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.
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.
| if (block.vtx.empty() || !block.vtx[0]->IsCoinBase()) { | ||
| // Consider the block mutated if any transaction is 64 bytes in size (see 3.1 | ||
| // in "Weaknesses in Bitcoin's Merkle Root Construction": | ||
| // https://lists.linuxfoundation.org/pipermail/bitcoin-dev/attachments/20190225/a27d8837/attachment-0001.pdf). | ||
| // | ||
| // Note: This is not a consensus change as this only applies to blocks that | ||
| // don't have a coinbase transaction and would therefore already be invalid. | ||
| return std::any_of(block.vtx.begin(), block.vtx.end(), | ||
| [](auto& tx) { return ::GetSerializeSize(*tx, PROTOCOL_VERSION) == 64; }); |
There was a problem hiding this comment.
🟡 Suggestion: block_malleation still doesn't exercise the retained 64-byte non-coinbase branch
The new validation_tests::block_malleation case is a solid direct check of IsBlockMutated for empty-block, single-coinbase, two-tx merkle-root-mismatch, and CVE-2012-2459 duplicate-transaction scenarios, but it stops short of the branch at src/validation.cpp:3894-3902. That branch — mutation via any transaction whose serialized size equals 64 bytes when the block has no coinbase — is the Dash-retained, non-witness half of upstream bitcoin#29412's 64-byte malleation defence, and upstream's block_malleation test does cover it. Because the branch is Dash-owned (segwit-slimmed) code rather than a mirror of upstream, it is the piece most likely to silently regress in a future serialization or block-primitive refactor without an assertion pinning it down. Add an assertion that constructs a non-coinbase transaction with ::GetSerializeSize(*tx, PROTOCOL_VERSION) == 64 and confirms IsBlockMutated returns true, to close out the coverage gap flagged in prior-2.
source: ['codex']
| if (block.vtx.empty() || !block.vtx[0]->IsCoinBase()) { | ||
| // Consider the block mutated if any transaction is 64 bytes in size (see 3.1 | ||
| // in "Weaknesses in Bitcoin's Merkle Root Construction": | ||
| // https://lists.linuxfoundation.org/pipermail/bitcoin-dev/attachments/20190225/a27d8837/attachment-0001.pdf). | ||
| // | ||
| // Note: This is not a consensus change as this only applies to blocks that | ||
| // don't have a coinbase transaction and would therefore already be invalid. | ||
| return std::any_of(block.vtx.begin(), block.vtx.end(), | ||
| [](auto& tx) { return ::GetSerializeSize(*tx, PROTOCOL_VERSION) == 64; }); |
There was a problem hiding this comment.
🔴 Blocking: Missing upstream coverage for the retained 64-byte transaction mutation branch
Carried forward from prior-2, narrowed after the latest delta. The new block_malleation unit test directly covers merkle-root mismatch and duplicate-merkle mutation, but it still omits the non-coinbase 64-byte transaction case that upstream bitcoin#29412 added to its direct IsBlockMutated coverage. This is not a witness-only case: Dash retained the same non-witness defense here, using ::GetSerializeSize(*tx, PROTOCOL_VERSION) when the block is empty or the first transaction is not coinbase. Upstream's test constructs a 64-byte non-coinbase transaction whose txid equals the merkle parent of two other transactions and asserts IsBlockMutated returns true. Dash has no segwit/witness reason to omit an equivalent adapted assertion for this retained branch. This is a soft completeness gap because the production branch is present and the rest of the adaptation is clean.
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.
source: ['codex-backport-reviewer']
3 participants
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