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. #27608 for which a regression test is included in this PR).
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For detailed information about the code coverage, see the test coverage report.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | maflcko, fjahr, sr-gi, achow101 |
| Concept ACK | TheCharlatan, epiccurious, instagibbs, glozow, naumenkogs |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
<!--174a7506f384e20aa4161008e828411d-->
No conflicts as of last run.
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.
Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
Leave a comment here, if you need help tracking down a confusing failure.
<sub>Debug: https://github.com/bitcoin/bitcoin/runs/21374346056</sub>
How do you define a mutated block? What are the known forms of mutated blocks?
How do you define a mutated block? What are the known forms of mutated blocks?
Looking at IsBlockMutated in this PR should provide the answers for these questions, but to recap:
CBlock::hashMerkleRoot) which should commit to the list of transactions but if it doesn't the block is considered mutated.
Concept ACK
Concept ACK.
concept ACK
might be good to recap why it was only added in BLOCK processing but not other ProcessBlocks: In every other case we already don't punish the sender of compact blocks for failing these higher level checks, while full blocks allow for punishment.
might be good to recap why it was only added in BLOCK processing but not other ProcessBlocks: In every other case we already don't punish the sender of compact blocks for failing these higher level checks, while full blocks allow for punishment.
We call ProcessBlock when we receive a block message (handled in this PR) or as the result of a compact block reconstruction. Compact blocks are relayed before full validation occurs, therefore we don't punish peers for sending us invalid blocks through compact block relay. Block mutation might also occur randomly during compact bock relay due to short-id collisions, which is another reason not to punish.
4718 | @@ -4719,6 +4719,16 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, 4719 | 4720 | LogPrint(BCLog::NET, "received block %s peer=%d\n", pblock->GetHash().ToString(), pfrom.GetId()); 4721 | 4722 | + const CBlockIndex* prev_block{WITH_LOCK(m_chainman.GetMutex(), return m_chainman.m_blockman.LookupBlockIndex(pblock->hashPrevBlock))}; 4723 | + 4724 | + if (IsBlockMutated(/*block=*/*pblock,
should we be checking PoW first before doing potentially a lot of hashing?
Basically performing CheckBlockHeader here too?
That sounds reasonable to me, and the outcome is the same IIRC: Misbehaving(*peer, 100, ...
Also, this is cheap enough that it may not matter if we do it again later (?), so we may not even need to cache it (correct me if I'm wrong)
Ok this doesn't seem to matter: we do this hashing even for already-known blocks we get sent unilaterally, and MAX_PROTOCOL_MESSAGE_LENGTH bounds the size of hashing to something reasonable, and we ban the peer anyways.
49257c0304828a185c273fcb99742c54bbef0c8e I had the same question, specifically why we're not doing this after the CalculateHeadersWork() check below, and right before ProcessBlock.
A code comment would be useful here.
3827 | + // here as it requires at least 224 bits of work. 3828 | + } 3829 | + 3830 | + if (check_witness_root) { 3831 | + const auto valid_opt = CheckWitnessCommitment(block, state); 3832 | + if (valid_opt.has_value() && !valid_opt.value()) {
Same as for ContextualCheckBlock, this can be simplified to:
if (valid_opt.value_or(true)) {
I'm not sure if that is simpler though. Yes it's less code but harder to read/understand imo (e.g. you forgot the ! in your suggestion).
I'm actually not a fan of my std::optional<bool> choice, so I might try to change that.
I may be used to the pattern due to rustlang, but I have no strong opinions about it
3934 | - } 3935 | - fHaveWitness = true; 3936 | - } 3937 | + const auto valid_opt = CheckWitnessCommitment(block, state); 3938 | + fHaveWitness = valid_opt.has_value(); 3939 | + if (fHaveWitness && !valid_opt.value()) return false;
We should be able to simplify this as:
if (!valid_opt.value_or(true)) return false;
73 | + assert_equal(peer_info_prior_to_attack[0]['id'], 0) 74 | + assert_equal([101], peer_info_prior_to_attack[0]["inflight"]) 75 | + 76 | + # Attempt to clear the honest relayer's download request by sending the 77 | + # mutated block (as the attacker). 78 | + attacker.send_message(msg_block(mutated_block))
It may be good to assert the debug log for the specific disconnection reason:
with self.nodes[0].assert_debug_log(expected_msgs=["bad-txnmrklroot, hashMerkleRoot mismatch"]):
attacker.send_message(msg_block(mutated_block))
@dergoegge have you considered this?
Added the assertion, even though I really don't like asserting logs
Concept ACK
concept ACK, I agree with the approach of catching and dropping these earlier rather than later
I would like to see this in v27, can we add it to the milestone?
3814 | + if (!CheckMerkleRoot(block, state)) { 3815 | + LogDebug(BCLog::VALIDATION, "%s: %s\n", __func__, state.ToString()); 3816 | + return true; 3817 | + } 3818 | + 3819 | + if (block.vtx.empty() || !block.vtx[0]->IsCoinBase()) {
Since this "in case of non-coinbase first transaction, treat existence of 64-byte transaction as malleation" is new behavior, would it make sense to move it to a separate commit at the end of the PR? (A clearer comment would also be helpful, because I briefly thought this was a consensus change before it was pointed out to be it only triggers in case the block would already be invalid).
I also had the same misconception the first time I reviewed.
It would also be a little bit easier to reason about if when block.vtx.empty() is true then we just return false; because in that case std::any_of will always return false.
Split into separate commit and expanded on the comment.
I got confused by this returning false in 66abce1d98115e41f394bc4f4f52341960ddc839, but the next commit 2d8495e0800f5332748cd50eaad801ff77671bba goes and implements an actual check.
3655 | + // while still invalidating it. 3656 | + if (mutated) { 3657 | + return state.Invalid( 3658 | + /*result=*/BlockValidationResult::BLOCK_MUTATED, 3659 | + /*reject_reason=*/"bad-txns-duplicate", 3660 | + /*debug_message=*/"duplicate transaction");
a74c3972bf8d0fa51e8f9672422bc34945b66ca7: It would be easier to review if style changes were separate from move-only changes? Otherwise, options such as --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space can not be used.
Also, renaming tokens in a block of code will break the parity check used to find the introduction of the code block (https://git-scm.com/docs/git-log#Documentation/git-log.txt--Sltstringgt). So it may be easier to track the history, if the renames were done in a separate commit, at least for me.
Split the refactoring into a separate commit.
3816 | + return true; 3817 | + } 3818 | + 3819 | + if (block.vtx.empty() || !block.vtx[0]->IsCoinBase()) { 3820 | + // Consider the block mutated if any transaction is 64 bytes in size 3821 | + // (see 3.1: https://lists.linuxfoundation.org/pipermail/bitcoin-dev/attachments/20190225/a27d8837/attachment-0001.pdf).
Given the recent changes around the mailing list could also name what's behind this link? So in case it disappears people know what to search for. I know they promised to keep the links but who knows...
Done
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: https://github.com/fjahr/bitcoin/commit/8e11e9c9ec9d52b2c5c04a33f33da0b22ae5e28b If it sounds valuable I will open a PR.
Concept ACK. Looking forward to addressing already pending comments, then i review.
157 | + CMutableTransaction tx1; 158 | + BOOST_CHECK(DecodeHexTx(tx1, "ff204bd0000000000000", /*try_no_witness=*/true, /*try_witness=*/false)); 159 | + CMutableTransaction tx2; 160 | + BOOST_CHECK(DecodeHexTx(tx2, "8ae53c92000000000000", /*try_no_witness=*/true, /*try_witness=*/false)); 161 | + CMutableTransaction tx3; 162 | + BOOST_CHECK(DecodeHexTx(tx3, "cdaf22d00002c6a7f848f8ae4d30054e61dcf3303d6fe01d282163341f06feecc10032b3160fcab87bdfe3ecfb769206ef2d991b92f8a268e423a6ef4d485f06", /*try_no_witness=*/true, /*try_witness=*/false));
Asserting the stripped serialized size right after is nice for self-documentation/correctness of test, and maybe another case of a witness tx which witness size is not 64?
Done
3813 | @@ -3758,6 +3814,40 @@ bool HasValidProofOfWork(const std::vector<CBlockHeader>& headers, const Consens 3814 | [&](const auto& header) { return CheckProofOfWork(header.GetHash(), header.nBits, consensusParams);}); 3815 | } 3816 | 3817 | +bool IsBlockMutated(const CBlock& block, bool check_witness_root)
have you thought about having this function return an optional error string so unit tests can check expected failure reason?
(reply to #29412 (review))
note: unit tests may use the ASSERT_DEBUG_LOG, as an alternative.
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...
3673 | int commitpos = GetWitnessCommitmentIndex(block); 3674 | - if (commitpos != NO_WITNESS_COMMITMENT) { 3675 | - bool malleated = false; 3676 | - uint256 hashWitness = BlockWitnessMerkleRoot(block, &malleated); 3677 | + 3678 | + bool has_witness_commitment{commitpos != NO_WITNESS_COMMITMENT};
nit c84db8d60e87eabc1e4f88ef05bf06fbf4db6f02
const bool has_witness_commitment{commitpos != NO_WITNESS_COMMITMENT};
3819 | + if (block.vtx.empty() || !block.vtx[0]->IsCoinBase()) { 3820 | + return false; 3821 | + } 3822 | + 3823 | + if (check_witness_root) { 3824 | + const auto valid_opt = CheckWitnessCommitment(block, state);
nit c62eea853e5c5f390d58c5ad9c0ac9a688465abc
const auto valid_opt{CheckWitnessCommitment(block, state)};
3653 | + return state.Invalid(BlockValidationResult::BLOCK_MUTATED, "bad-txns-duplicate", "duplicate transaction"); 3654 | + 3655 | + return true; 3656 | +} 3657 | + 3658 | +static std::optional<bool> CheckWitnessCommitment(const CBlock& block, BlockValidationState& state)
cb2437307e6861e0957899345b6e9b93716c383a Maybe add comment explaining what the tristate return result means? e.g. "bool indicating the witness commitment is correct (state is populated if any error occurs), or std::nullopt if there is no witness commitment to be checked"
The tristate no longer exists, see #29412 (review)
41 | + attacker = self.nodes[0].add_p2p_connection(P2PInterface()) 42 | + 43 | + # Create new block with two transactions (coinbase + 1 self-transfer). 44 | + # The self-transfer transaction is needed to trigger a compact block 45 | + # `getblocktxn` roundtrip. 46 | + tx = tx_from_hex(self.wallet.create_self_transfer()["hex"])
e2d1eb2e2001c31b43154df47ab5be215a26774f
this is the same thing as
tx = self.wallet.create_self_transfer()["tx"]
64 | + return False 65 | + 66 | + get_block_txn = honest_relayer.last_message['getblocktxn'] 67 | + return get_block_txn.block_txn_request.blockhash == block.sha256 and \ 68 | + get_block_txn.block_txn_request.indexes == [1] 69 | + honest_relayer.wait_until(self_transfer_requested, timeout=5)
in e2d1eb2e2001c31b43154df47ab5be215a26774f
does this need a with p2p_lock?
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 (bad-witness-nonce-size) in detail anyway. Can report back in a few days (currently, I receive only 1 or 2 per week).
3807 | @@ -3808,6 +3808,29 @@ bool HasValidProofOfWork(const std::vector<CBlockHeader>& headers, const Consens 3808 | [&](const auto& header) { return CheckProofOfWork(header.GetHash(), header.nBits, consensusParams);}); 3809 | } 3810 | 3811 | +bool IsBlockMutated(const CBlock& block, bool check_witness_root) 3812 | +{ 3813 | + BlockValidationState state; 3814 | + if (!CheckMerkleRoot(block, state)) { 3815 | + LogDebug(BCLog::VALIDATION, "%s: %s\n", __func__, state.ToString());
nit: (Same below) c62eea853e5c5f390d58c5ad9c0ac9a688465abc:
Instead of logging IsBlockMutated twice when function logging is enabled, it would be better to just use your own words to write something meaningful. For example, "Block mutated: %s\n", state.ToString());.
3821 | + } 3822 | + 3823 | + if (check_witness_root) { 3824 | + const auto valid_opt = CheckWitnessCommitment(block, state); 3825 | + if (valid_opt.has_value() && !valid_opt.value()) { 3826 | + LogDebug(BCLog::VALIDATION, "%s: %s\n", __func__, state.ToString());
Same
Received two bad-witness-nonce-size blocks from a mining pools custom client shortly after my last comment. In both cases I my node had reconstructed a cmpctblock two seconds before the mutated block arrived. In this case, the block is "mutated" because the coinbase witness is empty. That's probably a bug on the mining pool side. I have the IPs of the pool clients as noban-peers, so I didn't actually see them getting disconnected.
2024-02-23T10:33:55Z [msghand] [blockencodings.cpp:217] [FillBlock] [cmpctblock] Successfully reconstructed block 000000000000000000017f8957106aae020ce65f69f3ee189559d97fc0f974bb with 1 txn prefilled, 2936 txn from mempool (incl at least 5 from extra pool) and 45 txn requested
2024-02-23T10:33:55Z [msghand] [validationinterface.cpp:273] [NewPoWValidBlock] [validation] NewPoWValidBlock: block hash=000000000000000000017f8957106aae020ce65f69f3ee189559d97fc0f974bb
2024-02-23T10:33:55Z [msghand] [validationinterface.cpp:267] [BlockChecked] [validation] BlockChecked: block hash=000000000000000000017f8957106aae020ce65f69f3ee189559d97fc0f974bb state=Valid
2024-02-23T10:33:55Z [msghand] [validationinterface.cpp:243] [MempoolTransactionsRemovedForBlock] [validation] Enqueuing MempoolTransactionsRemovedForBlock: block height=831673 txs removed=2931
2024-02-23T10:33:55Z [msghand] [validation.cpp:2738] [UpdateTipLog] UpdateTip: new best=000000000000000000017f8957106aae020ce65f69f3ee189559d97fc0f974bb height=831673 version=0x2bc70000 log2_work=94.750438 tx=968653267 date='2024-02-23T10:33:34Z' progress=1.000000 cache=87.0MiB(713889txo)
...
2024-02-23T10:33:57Z [msghand] [net_processing.cpp:3396] [ProcessMessage] [net] received: block (1519889 bytes) peer=153
2024-02-23T10:33:57Z [msghand] [net_processing.cpp:4728] [ProcessMessage] [net] received block 000000000000000000017f8957106aae020ce65f69f3ee189559d97fc0f974bb peer=153
2024-02-23T10:33:57Z [msghand] [validation.cpp:3862] [IsBlockMutated] [validation] IsBlockMutated: bad-witness-nonce-size, CheckWitnessCommitment : invalid witness reserved value size
2024-02-23T10:33:57Z [msghand] [net_processing.cpp:4734] [ProcessMessage] [net] Received mutated block from peer=153
2024-02-23T10:33:57Z [msghand] [net_processing.cpp:1791] [Misbehaving] [net] Misbehaving: peer=153 (0 -> 100) DISCOURAGE THRESHOLD EXCEEDED: mutated block
2024-02-23T10:33:57Z [msghand] [net_processing.cpp:5037] [MaybeDiscourageAndDisconnect] Warning: not punishing noban peer 153!
2024-02-23T11:17:32Z [msghand] [blockencodings.cpp:217] [FillBlock] [cmpctblock] Successfully reconstructed block 0000000000000000000345e15958b691f5079b84a6d5a3f959a8d84ee7989659 with 1 txn prefilled, 1024 txn from mempool (incl at least 0 from extra pool) and 5 txn requested
2024-02-23T11:17:32Z [msghand] [validationinterface.cpp:273] [NewPoWValidBlock] [validation] NewPoWValidBlock: block hash=0000000000000000000345e15958b691f5079b84a6d5a3f959a8d84ee7989659
2024-02-23T11:17:32Z [msghand] [validationinterface.cpp:267] [BlockChecked] [validation] BlockChecked: block hash=0000000000000000000345e15958b691f5079b84a6d5a3f959a8d84ee7989659 state=Valid
2024-02-23T11:17:32Z [msghand] [net.cpp:3784] [PushMessage] [net] sending sendcmpct (9 bytes) peer=117
2024-02-23T11:17:32Z [msghand] [validationinterface.cpp:243] [MempoolTransactionsRemovedForBlock] [validation] Enqueuing MempoolTransactionsRemovedForBlock: block height=831677 txs removed=1024
2024-02-23T11:17:32Z [msghand] [validation.cpp:2738] [UpdateTipLog] UpdateTip: new best=0000000000000000000345e15958b691f5079b84a6d5a3f959a8d84ee7989659 height=831677 version=0x2fffe000 log2_work=94.750499 tx=968663963 date='2024-02-23T11:17:27Z' progress=1.000000 cache=78.7MiB(689008txo)
...
2024-02-23T11:17:34Z [msghand] [net_processing.cpp:3396] [ProcessMessage] [net] received: block (1509042 bytes) peer=20
2024-02-23T11:17:34Z [msghand] [net_processing.cpp:4728] [ProcessMessage] [net] received block 0000000000000000000345e15958b691f5079b84a6d5a3f959a8d84ee7989659 peer=20
2024-02-23T11:17:34Z [msghand] [validation.cpp:3862] [IsBlockMutated] [validation] IsBlockMutated: bad-witness-nonce-size, CheckWitnessCommitment : invalid witness reserved value size
2024-02-23T11:17:34Z [msghand] [net_processing.cpp:4734] [ProcessMessage] [net] Received mutated block from peer=20
2024-02-23T11:17:34Z [msghand] [net_processing.cpp:1791] [Misbehaving] [net] Misbehaving: peer=20 (0 -> 100) DISCOURAGE THRESHOLD EXCEEDED: mutated block
2024-02-23T11:17:34Z [msghand] [net_processing.cpp:5037] [MaybeDiscourageAndDisconnect] Warning: not punishing noban peer 20!
2024-02-23T11:17:34Z [msghand] [net_processing.cpp:3396] [ProcessMessage] [net] received: getdata (37 bytes) peer=20
2024-02-23T11:17:34Z [msghand] [net_processing.cpp:3998] [ProcessMessage] [net] received getdata (1 invsz) peer=20
2024-02-23T11:17:34Z [msghand] [net_processing.cpp:4001] [ProcessMessage] [net] received getdata for: witness-block 0000000000000000000345e15958b691f5079b84a6d5a3f959a8d84ee7989659 peer=20
2024-02-23T11:17:34Z [msghand] [net.cpp:3784] [PushMessage] [net] sending block (1509078 bytes) peer=20
3917 | + const auto valid_opt = CheckWitnessCommitment(block, state); 3918 | + fHaveWitness = valid_opt.has_value(); 3919 | + if (fHaveWitness && !valid_opt.value()) return false; 3920 | } 3921 | 3922 | // No witness data is allowed in blocks that don't commit to witness data, as this would otherwise leave room for spam
nit in cb2437307e6861e0957899345b6e9b93716c383a: Why not move this check into CheckWitnessCommitment as well? This would also fix the "tristate" std::optional<bool> complexity. It seems on point to consider added witness to a block that has no witness commitment as a violation of the commitment and thus a mutation?
Great catch! The "no witnesses allowed for blocks that don't commit to witnesses" rule needs to at the very least also be in IsBlockMutated.
I'm not sure if moving it to CheckWitnessCommitment is the right place because it needs to be checked even if segwit has not yet activated.
I'm not sure if moving it to
CheckWitnessCommitmentis the right place because it needs to be checked even if segwit has not yet activated.
I was thinking that CheckWitnessCommitment would wrap all of it, and you'd pass the segwit bool to it for the code block that would be run conditionally?
Done
left some initial nits/questions
0f356b0b2fb23aef96ed7396890aa36410aa1d59 💬
<details><summary>Show signature</summary>
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==
</details>
273 | + 274 | + // Mutate the block by replacing the two transactions with one 64-byte 275 | + // transaction that serializes into the concatenation of the txids of 276 | + // the transactions in the unmutated block. 277 | + block.vtx.clear(); 278 | + block.vtx.push_back(MakeTransactionRef(tx3));
block.vtx.push_back(MakeTransactionRef(tx3));
BOOST_CHECK(!block.vtx.back()->IsCoinBase());
218 | + BOOST_CHECK(is_mutated(block, /*check_witness_root=*/false)); 219 | + block.hashMerkleRoot = block.vtx[0]->GetHash(); 220 | + BOOST_CHECK(is_not_mutated(block, /*check_witness_root=*/false)); 221 | + 222 | + // Block with two transactions is mutated if the merkle root does not 223 | + // match the double sha256 of the concatenation of the two transactions.
// match the double sha256 of the concatenation of the two transaction hashes.
215 | + // equal to the coinbase tx's hash. 216 | + block.vtx.push_back(create_coinbase_tx()); 217 | + BOOST_CHECK(block.vtx[0]->GetHash() != block.hashMerkleRoot); 218 | + BOOST_CHECK(is_mutated(block, /*check_witness_root=*/false)); 219 | + block.hashMerkleRoot = block.vtx[0]->GetHash(); 220 | + BOOST_CHECK(is_not_mutated(block, /*check_witness_root=*/false));
Would be good to check we don't flag valid 64-byte coinbases as mutated in the function
Done
3686 | + /*debug_message=*/strprintf("%s : invalid witness reserved value size", __func__)); 3687 | + } 3688 | + 3689 | + // The malleation check is ignored; as the transaction tree itself 3690 | + // already does not permit it, it is impossible to trigger in the 3691 | + // witness tree.
future work nit: the mutated arg is never non-nullptr and has no test coverage it seems.
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.
3674 | + CHash256().Write(hashWitness).Write(block.vtx[0]->vin[0].scriptWitness.stack[0]).Finalize(hashWitness); 3675 | + if (memcmp(hashWitness.begin(), &block.vtx[0]->vout[commitpos].scriptPubKey[6], 32)) { 3676 | + return state.Invalid(BlockValidationResult::BLOCK_MUTATED, "bad-witness-merkle-match", strprintf("%s : witness merkle commitment mismatch", __func__)); 3677 | + } 3678 | + 3679 | + return true;
nit: 6cae77ae78bcd9d651b6eaf174982ffdcbc165cb. Not sure about the early-return in case the bool is set to true, and below the check of the bool. May be better to remove either the early return, or remove the bool (and keep the if check as if (commitpos != NO_WITNESS_COMMITMENT) {?
Removed the bool
3679 | - if (fHaveWitness) { 3680 | - bool malleated = false; 3681 | - uint256 hashWitness = BlockWitnessMerkleRoot(block, &malleated); 3682 | + has_witness_commitment = commitpos != NO_WITNESS_COMMITMENT; 3683 | + if (has_witness_commitment) { 3684 | + assert(!block.vtx.empty() && !block.vtx[0]->vin.empty());
aa08aa523d02ee89f2223381de78b35cf31fa6b6: Maybe document the precondition? Otherwise, this could result in a remote triggered crash, if a caller forgets to check the vin size?
Added a comment
I believe I addressed all comments, this is ready for further review
Left two nits, feel free to ignore.
79e62b01d13b300f629574f1349c3b63a36f2b7 🔄
<details><summary>Show signature</summary>
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: 79e62b01d13b300f629574f1349c3b63a36f2b7 🔄
ZzIHVQLTyxW9SBChtAlHaJQkND07+MygNA3HHp0nOTcsKGrdGqDY4WOTeUOQGUXO9AojSXrkpgGVRnV2aTfiDw==
</details>
224 | + // hashes. 225 | + block.vtx.push_back(MakeTransactionRef(CMutableTransaction{})); 226 | + BOOST_CHECK(is_mutated(block, /*check_witness_root=*/false)); 227 | + HashWriter hasher; 228 | + hasher.write({block.vtx[0]->GetHash().data(), 32}); 229 | + hasher.write({block.vtx[1]->GetHash().data(), 32});
follow-up, or separate 1-line commit:
diff --git a/src/test/validation_tests.cpp b/src/test/validation_tests.cpp
index fd2ef47b92..dbe2815991 100644
--- a/src/test/validation_tests.cpp
+++ b/src/test/validation_tests.cpp
@@ -223,8 +223,8 @@ BOOST_AUTO_TEST_CASE(block_malleation)
block.vtx.push_back(MakeTransactionRef(CMutableTransaction{}));
BOOST_CHECK(is_mutated(block, /*check_witness_root=*/false));
HashWriter hasher;
- hasher.write({block.vtx[0]->GetHash().data(), 32});
- hasher.write({block.vtx[1]->GetHash().data(), 32});
+ hasher.write(block.vtx[0]->GetHash());
+ hasher.write(block.vtx[1]->GetHash());
block.hashMerkleRoot = hasher.GetHash();
BOOST_CHECK(is_not_mutated(block, /*check_witness_root=*/false));
diff --git a/src/util/transaction_identifier.h b/src/util/transaction_identifier.h
index 89e10dee01..d4a0ede25a 100644
--- a/src/util/transaction_identifier.h
+++ b/src/util/transaction_identifier.h
@@ -44,6 +44,7 @@ public:
constexpr void SetNull() { m_wrapped.SetNull(); }
std::string GetHex() const { return m_wrapped.GetHex(); }
std::string ToString() const { return m_wrapped.ToString(); }
+ static constexpr auto size() { return decltype(m_wrapped)::size(); }
constexpr const std::byte* data() const { return reinterpret_cast<const std::byte*>(m_wrapped.data()); }
constexpr const std::byte* begin() const { return reinterpret_cast<const std::byte*>(m_wrapped.begin()); }
constexpr const std::byte* end() const { return reinterpret_cast<const std::byte*>(m_wrapped.end()); }
Added an extra commit and patched the tests
272 | + BOOST_CHECK(DecodeHexTx(tx3, "cdaf22d00002c6a7f848f8ae4d30054e61dcf3303d6fe01d282163341f06feecc10032b3160fcab87bdfe3ecfb769206ef2d991b92f8a268e423a6ef4d485f06", /*try_no_witness=*/true, /*try_witness=*/false)); 273 | + { 274 | + // Verify that doubla_sha256(txid1||txid2) == txid3 275 | + HashWriter hasher; 276 | + hasher.write({tx1.GetHash().data(), 32}); 277 | + hasher.write({tx2.GetHash().data(), 32});
same
274 | + // Verify that doubla_sha256(txid1||txid2) == txid3 275 | + HashWriter hasher; 276 | + hasher.write({tx1.GetHash().data(), 32}); 277 | + hasher.write({tx2.GetHash().data(), 32}); 278 | + assert(hasher.GetHash() == tx3.GetHash()); 279 | + // Verify that tx3 is 64 bytes in size (with out witness).
nit: s/with out/without/g
227 | + HashWriter hasher; 228 | + hasher.write({block.vtx[0]->GetHash().data(), 32}); 229 | + hasher.write({block.vtx[1]->GetHash().data(), 32}); 230 | + block.hashMerkleRoot = hasher.GetHash(); 231 | + BOOST_CHECK(is_not_mutated(block, /*check_witness_root=*/false)); 232 | +
Is the test covering is_mutated for duplicate tree nodes? I think not. Suggestion:
diff --git a/src/test/validation_tests.cpp b/src/test/validation_tests.cpp
index dbe2815991..8ad32eb195 100644
--- a/src/test/validation_tests.cpp
+++ b/src/test/validation_tests.cpp
@@ -228,6 +228,17 @@ BOOST_AUTO_TEST_CASE(block_malleation)
block.hashMerkleRoot = hasher.GetHash();
BOOST_CHECK(is_not_mutated(block, /*check_witness_root=*/false));
+ // Block with two transactions is mutated if any node is duplicate.
+ {
+ block.vtx[1] = block.vtx[0];
+ BOOST_CHECK(is_mutated(block, /*check_witness_root=*/false));
+ HashWriter hasher;
+ hasher.write(block.vtx[0]->GetHash());
+ hasher.write(block.vtx[1]->GetHash());
+ block.hashMerkleRoot = hasher.GetHash();
+ BOOST_CHECK(is_mutated(block, /*check_witness_root=*/false));
+ }
+
// Blocks with 64-byte coinbase transactions are not considered mutated
block.vtx.clear();
{
Added
left some more feedback
ACK 79e62b01d13b300f629574f1349c3b63a36f2b 🍚
<details><summary>Show signature</summary>
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: ACK 79e62b01d13b300f629574f1349c3b63a36f2b 🍚
2j503nYgXHtf8lxG+JzbLQsNuYaMcWAIenTx9ArAO/hqVxN41C+nl8EYMiHfteDJZX7JGr2ebPMczOKonNLPDg==
</details>
269 | + CMutableTransaction tx2; 270 | + BOOST_CHECK(DecodeHexTx(tx2, "8ae53c92000000000000", /*try_no_witness=*/true, /*try_witness=*/false)); 271 | + CMutableTransaction tx3; 272 | + BOOST_CHECK(DecodeHexTx(tx3, "cdaf22d00002c6a7f848f8ae4d30054e61dcf3303d6fe01d282163341f06feecc10032b3160fcab87bdfe3ecfb769206ef2d991b92f8a268e423a6ef4d485f06", /*try_no_witness=*/true, /*try_witness=*/false)); 273 | + { 274 | + // Verify that doubla_sha256(txid1||txid2) == txid3
doubla
202 | + }; 203 | + 204 | + { 205 | + CBlock block; 206 | + 207 | + // Emtpy block is expected to have merkle root of 0x0.
Emtpy
3698 | @@ -3694,6 +3699,7 @@ static bool CheckWitnessMalleation(const CBlock& block, bool expect_witness_comm 3699 | /*debug_message=*/strprintf("%s : witness merkle commitment mismatch", __func__)); 3700 | } 3701 | 3702 | + block.m_checked_witness_commitment = true;
There is another return true; case at the bottom of the function where you would need to set this as well.
There is another return true; case at the bottom of the function where you would need to set this as well.
No. This would allow a race where an block, not attached to the chain, is submitted, thus leading to the segwit deployment being assumed disabled, due to the previous block being nullptr. However, cs_main isn't taken for the whole time, so the previous block could be supplied in the meantime, leading the code to incorrectly think this check was already done, when it was not.
This case should be documented then.
I wasn't even thinking of any race but what marco says seems plausible. The cache is only supposed to cache the potentially expensive witness commitment check, which is why I limited its scope to the expect_witness_commitment branch.
I think the name is documentation enough but feel free to suggest a comment I can paste in.
If that race didn't exist I would have argued that, if we are caching, why not cache everything even if the unexpected-witness case is cheaper. But it's ok for now, I will think about it some more and might suggest a comment in a follow-up.
I agree that we should document why the cache is only used inside if (expect_witness_commitment).
We preemptively perform a block mutation check before further processing
a block message (similar to early sanity checks on other messsage
types). The main reasons for this change are as follows:
- `CBlock::GetHash()` is a foot-gun without a prior mutation check, as
the hash returned only commits to the header but not to the actual
transactions (`CBlock::vtx`) contained in the block.
- We have observed attacks that abused mutated blocks in the past, which
could have been prevented by simply not processing mutated blocks
(e.g. https://github.com/bitcoin/bitcoin/pull/27608).
Slight performance improvement by avoiding duplicate work.
ACK d8087adc7ebd4ea05dd3843e5a92e8115fd7bbcc 🏄
<details><summary>Show signature</summary>
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: ACK d8087adc7ebd4ea05dd3843e5a92e8115fd7bbcc 🏄
esXLEJ35qEFyBqeZMSV1JGBnrcJMG5Ami/zV9HPyRQ4Mt5cCWqd8NEkCTySeEOUSf2ngcBWvQdPLp33JU8IaDQ==
</details>
Code review ACK d8087adc7ebd4ea05dd3843e5a92e8115fd7bbcc
231 | + BOOST_CHECK(is_not_mutated(block, /*check_witness_root=*/false)); 232 | + 233 | + // Block with two transactions is mutated if any node is duplicate. 234 | + { 235 | + block.vtx[1] = block.vtx[0]; 236 | + BOOST_CHECK(is_mutated(block, /*check_witness_root=*/false));
I don't get the point of this first check. block.hashMerkleRoot still contains the previous value (where tx0 and tx1 were different), so this is failing because the root computed by IsBlockMutated and the value of block.hashMerkleRoot differ, which is unrelated.
Correct. That's my typo and this line can be removed. See #29412 (review)
Code review ACK https://github.com/bitcoin/bitcoin/commit/d8087adc7ebd4ea05dd3843e5a92e8115fd7bbcc
Just left a single comment on the newly added unit test
ACK d8087adc7ebd4ea05dd3843e5a92e8115fd7bbcc
3671 | +/** CheckWitnessMalleation performs checks for block malleation with regard to 3672 | + * its witnesses. 3673 | + * 3674 | + * Note: If the witness commitment is expected (i.e. `expect_witness_commitment 3675 | + * = true`), then the block is required to have at least one transaction and the 3676 | + * first transaction needs to have at least one input. */
e7669e1343aec4298fd30d539847963e6fa5619c: This note seems incorrect. I think it should say:
If the witness commitment is present (i.e. `expect_witness_commitment
= true` and `commitpos != NO_WITNESS_COMMITMENT`)
4718 | @@ -4719,6 +4719,16 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, 4719 | 4720 | LogPrint(BCLog::NET, "received block %s peer=%d\n", pblock->GetHash().ToString(), pfrom.GetId()); 4721 | 4722 | + const CBlockIndex* prev_block{WITH_LOCK(m_chainman.GetMutex(), return m_chainman.m_blockman.LookupBlockIndex(pblock->hashPrevBlock))}; 4723 | + 4724 | + if (IsBlockMutated(/*block=*/*pblock, 4725 | + /*check_witness_root=*/DeploymentActiveAfter(prev_block, m_chainman, Consensus::DEPLOYMENT_SEGWIT))) {
49257c0304828a185c273fcb99742c54bbef0c8e: DeploymentActiveAfter is going to be false for a block that's disconnected from anything we have. Assuming the block has witness data, the "No witness data is allowed in blocks that don't commit to witness data" check in CheckWitnessMalleation will then get triggered.
So IIUC this could reject a valid block, for the wrong reason. Before this change, the CheckWitnessMalleation() was called from ContextualCheckBlock(), which in turn is guarded by prev-blk-not-found.
I guess you have to repeat the prev-blk-not-found (and bad-prevblk?) checks here.
Related, although caching is not an issue in the scenario I described, because we don't store the block if it's rejected at this point: #29412 (review)
So IIUC this could reject a valid block, for the wrong reason.
I assume you mean invalid?
This behavior change would manifest itself if someone is sending a full unconnected(or built on invalid) segwit block unilaterally. And instead of adding 10 DoS points for missing, we outright ban for "mutation"?
I guess it would have been useful to enumerate the behavior changes in the commit, such as DoS points.
No, I meant valid but disconnected. Although not normal behavior, you might have the chain up to block N, and some peer could send N + 2 without first sending (headers for) N + 1. Such a block should be rejected with prev-blk-not-found, but with this patch it'll be rejected for being "mutated".
I mean, our node calls that invalid :)
opened #29524 as what I think a clean fix is, with test coverage
Fix
Post merge review... I still have to look at the last 4 commits, but want to make sure I'm not misunderstanding something.
Hint for anyone trying to test 5bf4f5ba32da4627f152b54d266df9b2aa930457: uncomment this bit in RemoveBlockRequest.
if (from_peer && *from_peer != node_id) {
range.first++;
continue;
}
81 | + 82 | + # Block at height 101 should *still* be the only block in-flight from 83 | + # peer 0 84 | + peer_info_after_attack = self.nodes[0].getpeerinfo() 85 | + assert_equal(peer_info_after_attack[0]['id'], 0) 86 | + assert_equal([101], peer_info_after_attack[0]["inflight"])
5bf4f5ba32da4627f152b54d266df9b2aa930457: you might need a slight delay here. I noticed several times that it was still inflight after sabotaging net_processing: #29412 (comment)
(I'm a bit surprised by that, given the wait_for_disconnect above)
nit: previously, we were quite loud logging an ERROR when receiving a valid-PoW mutated block we didn't know about. Now, it's only visible in the debug log when net debug-logging is turned on. As it's quite an expensive mistake to mine and broadcast a mutated block with valid PoW on mainnet, it could make sense to continue logging new, valid-PoW blocks as ERRORs to make these cases easier to debug.
@0xB10C I'd consider that a bit of a regression if not fixed since the new-valid-pow logging has been quite helpful to diagnose issues. Want to open a PR for that?
As it's quite an expensive mistake to mine and broadcast a mutated block with valid PoW on mainnet
Yes it is a expensive mistake to mine a block and then only broadcast a mutated version of it. But it is actually really cheap to produce mutated blocks if you have a fully valid one (e.g. by adding one transaction to the block), so logging under the category is better.
I'd consider that a bit of a regression if not fixed since the new-valid-pow logging has been quite helpful to diagnose issues. Want to open a PR for that?
I think you misunderstood Timo's comment. Timo was talking about error logging from mutation checks and I'm not quite sure what you are referring to with "new-valid-pow" logging.
I think you misunderstood Timo's comment. Timo was talking about error logging from mutation checks and I'm not quite sure what you are referring to with "new-valid-pow" logging.
Attempted rephrasing of "when receiving a valid-PoW mutated block we didn't know about" as a subset of cases we're interested in? We may be talking past eachother.
edit: let me investigate if what I'm saying makes sense and get back with a concrete suggestion if so
As it's quite an expensive mistake to mine and broadcast a mutated block with valid PoW on mainnet
Yes it is a expensive mistake to mine a block and then only broadcast a mutated version of it. But it is actually really cheap to produce mutated blocks if you have a fully valid one (e.g. by adding one transaction to the block), so logging under the category is better.
I agree that we don't want to log all mutated blocks we receive. I'm specifically talking about logging valid-PoW, but mutated blocks with header hashes that we haven't seen before. These are expensive to produce. I think @instagibbs's interpretation of my comment is correct.
As an example: Currently, there is a mining pool that broadcasts newly found, mutated blocks due to a bug in their mining pool software. Their (Bitcoin Core) nodes also relay these blocks as compact blocks. Before this PR, we'd log the following to the debug.log when we received the mutated block directly from the pool's broken P2P clients before we received it as a compact block. When we received the compact block first, we didn't process the mutated blocks from the pool broken P2P client as we were already aware of the non-mutated block. With this PR, we don't log anything about mutated blocks by default. To be able to detect cases like these, it might be worth having something like this (untested): https://github.com/0xB10C/bitcoin/commit/54922dacb4aa066bf9bcbfbbad4e5d16420d5b6e
# debug.log on receiving a mutated block before this PR
ERROR: AcceptBlock: bad-witness-nonce-size, ContextualCheckBlock : invalid witness reserved value size
ERROR: ProcessNewBlock: AcceptBlock FAILED (bad-witness-nonce-size, ContextualCheckBlock : invalid witness reserved value size)
it might be worth having something like this (untested): 0xB10C@54922da
Seems like accepting the header would be required? Otherwise someone could use a past stale block to fill a peer's storage via the log in a loop?
it might be worth having something like this (untested): 0xB10C@54922da
Seems like accepting the header would be required? Otherwise someone could use a past stale block to fill a peer's storage via the log in a loop?
Agree that we should do something like this. Since we disconnect after each mutated block, the attack frequency is somewhat limited. This attack was also possible before this PR (but is now prevented).
Milestone
27.0