p2p: Add witness mutation check inside FillBlock #32646
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32646. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
7224acf to
0d4f232
Compare
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
0d4f232 to
2b15ec7
Compare
|
Concept ACK |
src/blockencodings.cpp
Outdated
| @@ -218,6 +218,13 @@ ReadStatus PartiallyDownloadedBlock::FillBlock(CBlock& block, const std::vector< | |||
| return READ_STATUS_CHECKBLOCK_FAILED; | |||
| } | |||
|
|
|||
| // Belt-and-suspenders check for other mutations now that we have a seemingly good block | |||
| if (IsBlockMutated(/*block=*/block, | |||
There was a problem hiding this comment.
Could this be an opportunity to resolve the TODO comment above after check_block by not calling this function anymore? If I understand the comment correctly it only calls CheckBlock() to allow the caching of the check result and avoid repeated Merkle checks, which IsBlockMutated() also would do today?
There was a problem hiding this comment.
Would we be able to distinguish shorttxid collision from an intentionally mutated block without the check?
There was a problem hiding this comment.
For the sake of disconnecting the peer? I think we could use the flags set or not set in the block to learn which mutation check (CheckMerkleRoot() or CheckWitnessMalleation()) has failed.
There was a problem hiding this comment.
@mzumsande pushed as a follow-up commit. lmk what you think.
There was a problem hiding this comment.
Actually, this strategy is unneeded. Both merkle checks can fail due to collisions, and either may need full blocks fetched if they're from the first peer. Pushed a change that is simpler and cleaned up READ_STATUS_CHECKBLOCK_FAILED which is no longer necessary.
There was a problem hiding this comment.
Both merkle checks can fail due to collisions
Interesting, I did not know this. Since the header merkle check is first... if the witness merkle check fails due to collision, does that mean that the transaction that collided had the same txid but a different wtxid? Or is there another way for short ID collisions to affect the witness merkle check?
There was a problem hiding this comment.
if the witness merkle check fails due to collision, does that mean that the transaction that collided had the same txid but a different wtxid?
That would be my understanding as well, i.e. the txs are only different in their witness. Pretty sure that is the only scenario, as otherwise the prior merkle root check would fail.
a585820 to
a7cbaca
Compare
src/blockencodings.cpp
Outdated
| @@ -218,6 +218,13 @@ ReadStatus PartiallyDownloadedBlock::FillBlock(CBlock& block, const std::vector< | |||
| return READ_STATUS_CHECKBLOCK_FAILED; | |||
| } | |||
|
|
|||
| // Belt-and-suspenders check for other mutations now that we have a seemingly good block | |||
| if (IsBlockMutated(/*block=*/block, | |||
There was a problem hiding this comment.
Both merkle checks can fail due to collisions
Interesting, I did not know this. Since the header merkle check is first... if the witness merkle check fails due to collision, does that mean that the transaction that collided had the same txid but a different wtxid? Or is there another way for short ID collisions to affect the witness merkle check?
a7cbaca to
8ce8e93
Compare
Since bitcoin#29412, we have not allowed mutated blocks to continue being processed immediately the block is received, but this is only done for the legacy BLOCK message. Extend these checks as belt-and-suspenders to not allow similar mutation strategies to affect relay by honest peers by applying the check inside PartiallyDownloadedBlock::FillBlock, immediately before returning READ_STATUS_OK. This also removes the extraneous CheckBlock call.
8ce8e93 to
28299ce
Compare
|
ACK 28299ce |
|
ACK 28299ce |
There was a problem hiding this comment.
ACK 28299ce.
makes sense to just do mutation check at this stage, CheckBlock checks are done later in ProcessBlock anyways.
compared the logs in the compact block functional tests on master and PR.
- on master (when witness mutation happens -
CheckBlockpasses, later checks inProcessNewBlockleads to failure)
node0 2025-06-19T07:17:31.494150Z [msghand] [blockencodings.cpp:260] [FillBlock] ### FillBlock: CheckBlock success
node0 2025-06-19T07:17:31.494249Z [msghand] [blockencodings.cpp:263] [FillBlock] ### Successfully reconstructed block 342e2a5c1ffe8bdf9df52480eae1d8f8cb0f6b2fa12839e0e688c4e3c0ac08f7 with 1 txn prefilled, 5 txn from mempool (incl at least 0 from extra pool) and 0 txn (0 bytes) requested
node0 2025-06-19T07:17:31.495905Z [msghand] [validation.cpp:4608] [ProcessNewBlock] [error] ProcessNewBlock: AcceptBlock FAILED (unexpected-witness, CheckWitnessMalleation : unexpected witness data found)
- in PR (when witness mutation happens -
IsBlockMutatedfails and we don't need to wait tillProcessNewBlockto detect failure)
node0 2025-06-19T07:09:06.235430Z [msghand] [validation.cpp:4030] [CheckWitnessMalleation] ### CheckWitnessMalleation: BLOCK_MUTATED unexpected-witness
node0 2025-06-19T07:09:06.235860Z [msghand] [blockencodings.cpp:236] [FillBlock] ### FillBlock: CheckBlock success
node0 2025-06-19T07:09:06.236283Z [msghand] [net_processing.cpp:3385] [ProcessCompactBlockTxns] [net] Peer 1 sent us a compact block but it failed to reconstruct, waiting on first download to complete
|
could also remove the forward declaration of now unused classes in |
Since #29412, we have not allowed mutated blocks to continue being processed immediately the block is received, but this is only done for the legacy BLOCK message.
Extend these checks as belt-and-suspenders to not allow similar mutation strategies to affect relay by honest peers by applying the check inside
PartiallyDownloadedBlock::FillBlock, immediately before returningREAD_STATUS_OK.