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 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: 📒 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
📒 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.
|
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.
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.
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
📒 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
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 2487b9a: 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).
| BlockValidationResult::BLOCK_INVALID_PREV, | ||
| BlockValidationResult::BLOCK_TIME_FUTURE, | ||
| BlockValidationResult::BLOCK_CHECKPOINT, | ||
| BlockValidationResult::BLOCK_CHAINLOCK}); |
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
|
|
||
| // memory only | ||
| mutable bool fChecked; | ||
| mutable bool fChecked; // CheckBlock() |
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 |
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; | ||
| } | ||
|
|
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.
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
2487b9a to
9d947f3
Compare
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 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