p2p: Don't process mutated blocks #29412
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. 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. ConflictsNo conflicts as of last run. |
56ed5bd to
d028bb1
Compare
|
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the Possibly this is due to a silent merge conflict (the changes in this pull request being Leave a comment here, if you need help tracking down a confusing failure. |
|
How do you define a mutated block? What are the known forms of mutated blocks? |
Looking at
|
|
Concept ACK |
|
Concept ACK. |
|
concept ACK might be good to recap why it was only added in BLOCK processing but not other |
We call |
|
I would like to see this in v27, can we add it to the milestone? |
There was a problem hiding this comment.
Concept ACK
"CBlock::GetHash() is a foot-gun without a prior mutation check", I hadn't felt this strongly about but I get it. I think it makes sense to rename it. I have drafted it here: fjahr@8e11e9c If it sounds valuable I will open a PR.
|
Concept ACK. Looking forward to addressing already pending comments, then i review. |
d028bb1 to
0f356b0
Compare
| @@ -3758,6 +3814,40 @@ bool HasValidProofOfWork(const std::vector<CBlockHeader>& headers, const Consens | |||
| [&](const auto& header) { return CheckProofOfWork(header.GetHash(), header.nBits, consensusParams);}); | |||
| } | |||
|
|
|||
| bool IsBlockMutated(const CBlock& block, bool check_witness_root) | |||
There was a problem hiding this comment.
have you thought about having this function return an optional error string so unit tests can check expected failure reason?
There was a problem hiding this comment.
(reply to #29412 (comment))
note: unit tests may use the ASSERT_DEBUG_LOG, as an alternative.
There was a problem hiding this comment.
have you thought about having this function return an optional error string so unit tests can check expected failure reason?
I'm considering returning the mutation type but I will not be asserting logs...
| get_block_txn = honest_relayer.last_message['getblocktxn'] | ||
| return get_block_txn.block_txn_request.blockhash == block.sha256 and \ | ||
| get_block_txn.block_txn_request.indexes == [1] | ||
| honest_relayer.wait_until(self_transfer_requested, timeout=5) |
There was a problem hiding this comment.
in e2d1eb2
does this need a with p2p_lock?
There was a problem hiding this comment.
Does it? None of the other wait_for_* helpers require it.
|
I'm running this PR on my mainnet monitoring infrastructure as I was looking at the currently broadcast mutated blocks ( |
|
Received two |
There was a problem hiding this comment.
left some initial nits/questions
0f356b0 💬
Show signature
Signature:
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: 0f356b0b2fb23aef96ed7396890aa36410aa1d59 💬
kBfK9UKECARIswfAVfqnH6nAuO9mA7Tcad0J1J6aCTeTkHT4VK7z3GpY/0Wz6H0SeVaxH2Thgu3Qvz87+QF6CA==
| @@ -3758,6 +3814,40 @@ bool HasValidProofOfWork(const std::vector<CBlockHeader>& headers, const Consens | |||
| [&](const auto& header) { return CheckProofOfWork(header.GetHash(), header.nBits, consensusParams);}); | |||
| } | |||
|
|
|||
| bool IsBlockMutated(const CBlock& block, bool check_witness_root) | |||
There was a problem hiding this comment.
(reply to #29412 (comment))
note: unit tests may use the ASSERT_DEBUG_LOG, as an alternative.
0f356b0 to
8c18b70
Compare
|
|
||
| // The malleation check is ignored; as the transaction tree itself | ||
| // already does not permit it, it is impossible to trigger in the | ||
| // witness tree. |
There was a problem hiding this comment.
future work nit: the mutated arg is never non-nullptr and has no test coverage it seems.
There was a problem hiding this comment.
future work nit: the mutated arg is never non-nullptr and has no test coverage it seems.
I presume the reason is that it can't be mutated and all callers are expected to pass nullptr? Seems fine to remove the arg, but should be fine either way.
d8087ad [test] IsBlockMutated unit tests (dergoegge) 1ed2c98 Add transaction_identifier::size to allow Span conversion (dergoegge) 1ec6bbe [validation] Cache merkle root and witness commitment checks (dergoegge) 5bf4f5b [test] Add regression test for bitcoin#27608 (dergoegge) 49257c0 [net processing] Don't process mutated blocks (dergoegge) 2d8495e [validation] Merkle root malleation should be caught by IsBlockMutated (dergoegge) 66abce1 [validation] Introduce IsBlockMutated (dergoegge) e7669e1 [refactor] Cleanup merkle root checks (dergoegge) 95bddb9 [validation] Isolate merkle root checks (dergoegge) Pull request description: This PR proposes to check for mutated blocks early as a defense-in-depth mitigation against attacks leveraging mutated blocks. We introduce `IsBlockMutated` which catches all known forms of block malleation and use it to do an early mutation check whenever we receive a `block` message. We have observed attacks that abused mutated blocks in the past, which could have been prevented by simply not processing mutated blocks (e.g. bitcoin#27608 for which a regression test is included in this PR). ACKs for top commit: achow101: ACK d8087ad maflcko: ACK d8087ad 🏄 fjahr: Code review ACK d8087ad sr-gi: Code review ACK bitcoin@d8087ad Tree-SHA512: 618ff4ea7f168e10f07504d3651290efbb1bb2ab3b838ffff3527c028caf6c52dedad18d04d3dbc627977479710930e200f2dfae18a08f627efe7e64a57e535f
This PR proposes to check for mutated blocks early as a defense-in-depth mitigation against attacks leveraging mutated blocks.
We introduce
IsBlockMutatedwhich catches all known forms of block malleation and use it to do an early mutation check whenever we receive ablockmessage.We have observed attacks that abused mutated blocks in the past, which could have been prevented by simply not processing mutated blocks (e.g. #27608 for which a regression test is included in this PR).