p2p: Don't process mutated blocks

dergoegge commented at 5:18 pm on February 8, 2024: member

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).

DrahtBot commented at 5:18 pm on February 8, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

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.

Conflicts

No conflicts as of last run.

DrahtBot added the label P2P on Feb 8, 2024

dergoegge force-pushed on Feb 8, 2024

DrahtBot added the label CI failed on Feb 8, 2024

DrahtBot commented at 6:22 pm on February 8, 2024: contributor

🚧 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.

Debug: https://github.com/bitcoin/bitcoin/runs/21374346056

DrahtBot removed the label CI failed on Feb 8, 2024

epiccurious commented at 3:38 am on February 9, 2024: none

How do you define a mutated block? What are the known forms of mutated blocks?

dergoegge commented at 10:05 am on February 9, 2024: member

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:

When we receive a block it contains a header and a list of transactions. The header contains a merkle root hash (CBlock::hashMerkleRoot) which should commit to the list of transactions but if it doesn’t the block is considered mutated.
The coinbase transaction (post segwit) contains a commitment (very similar to the merkle root in the header) to the witness data in a block. If that commitment does not actually commit to the witnesses that were received, the block is considered mutated.
Our merkle root algorithm has several flaws which allow for further malleation, see here for a great overview.

TheCharlatan commented at 1:36 pm on February 9, 2024: contributor

Concept ACK

epiccurious commented at 2:28 pm on February 9, 2024: none

Concept ACK.

instagibbs commented at 2:37 pm on February 9, 2024: member

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.

dergoegge commented at 4:12 pm on February 9, 2024: member

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.

in src/net_processing.cpp:4724 in d028bb12ab outdated

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,

instagibbs commented at 4:31 pm on February 9, 2024:

should we be checking PoW first before doing potentially a lot of hashing?

sr-gi commented at 7:52 pm on February 9, 2024:

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)

instagibbs commented at 8:47 pm on February 9, 2024:

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.

Sjors commented at 12:12 pm on February 29, 2024:

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.

in src/validation.cpp:3842 in 30bdbcd561 outdated

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()) {

sr-gi commented at 7:31 pm on February 9, 2024:

Same as for ContextualCheckBlock, this can be simplified to:

0if (valid_opt.value_or(true)) {

dergoegge commented at 10:49 am on February 12, 2024:

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.

sr-gi commented at 1:44 pm on February 12, 2024:

I may be used to the pattern due to rustlang, but I have no strong opinions about it

in src/validation.cpp:3963 in a74c3972bf outdated

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;

sr-gi commented at 7:35 pm on February 9, 2024:

We should be able to simplify this as:

0if (!valid_opt.value_or(true)) return false;

in test/functional/p2p_mutated_blocks.py:78 in 5f27c9be7a outdated

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))

sr-gi commented at 8:27 pm on February 9, 2024:

It may be good to assert the debug log for the specific disconnection reason:

0with self.nodes[0].assert_debug_log(expected_msgs=["bad-txnmrklroot, hashMerkleRoot mismatch"]):
1    attacker.send_message(msg_block(mutated_block))

sr-gi commented at 5:17 pm on February 22, 2024:

@dergoegge have you considered this?

dergoegge commented at 6:39 pm on February 23, 2024:

Added the assertion, even though I really don’t like asserting logs

sr-gi commented at 8:28 pm on February 9, 2024: member

Concept ACK

glozow commented at 10:01 pm on February 9, 2024: member

concept ACK, I agree with the approach of catching and dropping these earlier rather than later

dergoegge commented at 11:32 am on February 20, 2024: member

I would like to see this in v27, can we add it to the milestone?

maflcko added this to the milestone 27.0 on Feb 20, 2024

in src/validation.cpp:3841 in 30bdbcd561 outdated

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()) {

sipa commented at 2:08 pm on February 20, 2024:

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).

instagibbs commented at 2:50 pm on February 20, 2024:

I also had the same misconception the first time I reviewed.

fjahr commented at 7:16 pm on February 20, 2024:

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.

dergoegge commented at 11:55 am on February 22, 2024:

Split into separate commit and expanded on the comment.

Sjors commented at 11:49 am on February 29, 2024:

I got confused by this returning false in 66abce1d98115e41f394bc4f4f52341960ddc839, but the next commit 2d8495e0800f5332748cd50eaad801ff77671bba goes and implements an actual check.

in src/validation.cpp:3662 in a74c3972bf outdated

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");

maflcko commented at 3:41 pm on February 20, 2024:

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.

dergoegge commented at 11:55 am on February 22, 2024:

Split the refactoring into a separate commit.

in src/validation.cpp:3821 in 30bdbcd561 outdated

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).

fjahr commented at 6:27 pm on February 20, 2024:

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…

dergoegge commented at 11:55 am on February 22, 2024:

Done

fjahr commented at 10:26 pm on February 20, 2024: contributor

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.

naumenkogs commented at 8:05 am on February 22, 2024: member

Concept ACK. Looking forward to addressing already pending comments, then i review.

dergoegge force-pushed on Feb 22, 2024

in src/test/validation_tests.cpp:162 in 0f356b0b2f outdated

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));

instagibbs commented at 2:38 pm on February 22, 2024:

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?

dergoegge commented at 6:39 pm on February 23, 2024:

Done

in src/validation.cpp:3833 in 0f356b0b2f outdated

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)

instagibbs commented at 2:40 pm on February 22, 2024:

have you thought about having this function return an optional error string so unit tests can check expected failure reason?

maflcko commented at 1:25 pm on February 23, 2024:

(reply to #29412 (review))

note: unit tests may use the ASSERT_DEBUG_LOG, as an alternative.

dergoegge commented at 6:38 pm on February 23, 2024:

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…

in src/validation.cpp:3670 in c84db8d60e outdated

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};

glozow commented at 5:12 pm on February 22, 2024:

nit c84db8d60e87eabc1e4f88ef05bf06fbf4db6f02

0    const bool has_witness_commitment{commitpos != NO_WITNESS_COMMITMENT};

in src/validation.cpp:3824 in c62eea853e outdated

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);

glozow commented at 5:14 pm on February 22, 2024:

nit c62eea853e5c5f390d58c5ad9c0ac9a688465abc

0        const auto valid_opt{CheckWitnessCommitment(block, state)};

in src/validation.cpp:3658 in cb2437307e outdated

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)

glozow commented at 5:17 pm on February 22, 2024:

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”

dergoegge commented at 6:37 pm on February 23, 2024:

The tristate no longer exists, see #29412 (review)

in test/functional/p2p_mutated_blocks.py:46 in e2d1eb2e20 outdated

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"])

glozow commented at 5:25 pm on February 22, 2024:

e2d1eb2e2001c31b43154df47ab5be215a26774f

this is the same thing as

0        tx = self.wallet.create_self_transfer()["tx"]

in test/functional/p2p_mutated_blocks.py:68 in e2d1eb2e20 outdated

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)

glozow commented at 5:28 pm on February 22, 2024:

in e2d1eb2e2001c31b43154df47ab5be215a26774f does this need a with p2p_lock?

dergoegge commented at 1:33 pm on February 23, 2024:

Does it? None of the other wait_for_* helpers require it.

0xB10C commented at 9:58 am on February 23, 2024: contributor

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).

in src/validation.cpp:3815 in c62eea853e outdated

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());

maflcko commented at 12:59 pm on February 23, 2024:

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());.

in src/validation.cpp:3826 in c62eea853e outdated

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());

maflcko commented at 12:59 pm on February 23, 2024:

Same

0xB10C commented at 1:15 pm on February 23, 2024: contributor

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.

 02024-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
 12024-02-23T10:33:55Z [msghand] [validationinterface.cpp:273] [NewPoWValidBlock] [validation] NewPoWValidBlock: block hash=000000000000000000017f8957106aae020ce65f69f3ee189559d97fc0f974bb
 22024-02-23T10:33:55Z [msghand] [validationinterface.cpp:267] [BlockChecked] [validation] BlockChecked: block hash=000000000000000000017f8957106aae020ce65f69f3ee189559d97fc0f974bb state=Valid
 32024-02-23T10:33:55Z [msghand] [validationinterface.cpp:243] [MempoolTransactionsRemovedForBlock] [validation] Enqueuing MempoolTransactionsRemovedForBlock: block height=831673 txs removed=2931
 42024-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)
 5
 6...
 7
 82024-02-23T10:33:57Z [msghand] [net_processing.cpp:3396] [ProcessMessage] [net] received: block (1519889 bytes) peer=153
 92024-02-23T10:33:57Z [msghand] [net_processing.cpp:4728] [ProcessMessage] [net] received block 000000000000000000017f8957106aae020ce65f69f3ee189559d97fc0f974bb peer=153
102024-02-23T10:33:57Z [msghand] [validation.cpp:3862] [IsBlockMutated] [validation] IsBlockMutated: bad-witness-nonce-size, CheckWitnessCommitment : invalid witness reserved value size
112024-02-23T10:33:57Z [msghand] [net_processing.cpp:4734] [ProcessMessage] [net] Received mutated block from peer=153
122024-02-23T10:33:57Z [msghand] [net_processing.cpp:1791] [Misbehaving] [net] Misbehaving: peer=153 (0 -> 100) DISCOURAGE THRESHOLD EXCEEDED: mutated block
132024-02-23T10:33:57Z [msghand] [net_processing.cpp:5037] [MaybeDiscourageAndDisconnect] Warning: not punishing noban peer 153!

 02024-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
 12024-02-23T11:17:32Z [msghand] [validationinterface.cpp:273] [NewPoWValidBlock] [validation] NewPoWValidBlock: block hash=0000000000000000000345e15958b691f5079b84a6d5a3f959a8d84ee7989659
 22024-02-23T11:17:32Z [msghand] [validationinterface.cpp:267] [BlockChecked] [validation] BlockChecked: block hash=0000000000000000000345e15958b691f5079b84a6d5a3f959a8d84ee7989659 state=Valid
 32024-02-23T11:17:32Z [msghand] [net.cpp:3784] [PushMessage] [net] sending sendcmpct (9 bytes) peer=117
 42024-02-23T11:17:32Z [msghand] [validationinterface.cpp:243] [MempoolTransactionsRemovedForBlock] [validation] Enqueuing MempoolTransactionsRemovedForBlock: block height=831677 txs removed=1024
 52024-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)
 6
 7...
 8
 92024-02-23T11:17:34Z [msghand] [net_processing.cpp:3396] [ProcessMessage] [net] received: block (1509042 bytes) peer=20
102024-02-23T11:17:34Z [msghand] [net_processing.cpp:4728] [ProcessMessage] [net] received block 0000000000000000000345e15958b691f5079b84a6d5a3f959a8d84ee7989659 peer=20
112024-02-23T11:17:34Z [msghand] [validation.cpp:3862] [IsBlockMutated] [validation] IsBlockMutated: bad-witness-nonce-size, CheckWitnessCommitment : invalid witness reserved value size
122024-02-23T11:17:34Z [msghand] [net_processing.cpp:4734] [ProcessMessage] [net] Received mutated block from peer=20
132024-02-23T11:17:34Z [msghand] [net_processing.cpp:1791] [Misbehaving] [net] Misbehaving: peer=20 (0 -> 100) DISCOURAGE THRESHOLD EXCEEDED: mutated block
142024-02-23T11:17:34Z [msghand] [net_processing.cpp:5037] [MaybeDiscourageAndDisconnect] Warning: not punishing noban peer 20!
152024-02-23T11:17:34Z [msghand] [net_processing.cpp:3396] [ProcessMessage] [net] received: getdata (37 bytes) peer=20
162024-02-23T11:17:34Z [msghand] [net_processing.cpp:3998] [ProcessMessage] [net] received getdata (1 invsz) peer=20
172024-02-23T11:17:34Z [msghand] [net_processing.cpp:4001] [ProcessMessage] [net] received getdata for: witness-block 0000000000000000000345e15958b691f5079b84a6d5a3f959a8d84ee7989659 peer=20
182024-02-23T11:17:34Z [msghand] [net.cpp:3784] [PushMessage] [net] sending block (1509078 bytes) peer=20

in src/validation.cpp:3906 in cb2437307e outdated

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

maflcko commented at 1:34 pm on February 23, 2024:

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?

dergoegge commented at 4:46 pm on February 23, 2024:

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.

maflcko commented at 4:50 pm on February 23, 2024:

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 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?

dergoegge commented at 6:36 pm on February 23, 2024:

Done

maflcko commented at 1:49 pm on February 23, 2024: member

left some initial nits/questions

0f356b0b2fb23aef96ed7396890aa36410aa1d59 💬

Signature:

0untrusted 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}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: 0f356b0b2fb23aef96ed7396890aa36410aa1d59 💬
3kBfK9UKECARIswfAVfqnH6nAuO9mA7Tcad0J1J6aCTeTkHT4VK7z3GpY/0Wz6H0SeVaxH2Thgu3Qvz87+QF6CA==

dergoegge force-pushed on Feb 23, 2024

in src/test/validation_tests.cpp:304 in 8c18b708b9 outdated

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));

instagibbs commented at 6:51 pm on February 23, 2024:

0        block.vtx.push_back(MakeTransactionRef(tx3));
1        BOOST_CHECK(!block.vtx.back()->IsCoinBase());

in src/test/validation_tests.cpp:223 in 8c18b708b9 outdated

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.

instagibbs commented at 6:57 pm on February 23, 2024:

0        // match the double sha256 of the concatenation of the two transaction hashes.

in src/test/validation_tests.cpp:220 in 8c18b708b9 outdated

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));

instagibbs commented at 7:09 pm on February 23, 2024:

Would be good to check we don’t flag valid 64-byte coinbases as mutated in the function

dergoegge commented at 7:59 pm on February 23, 2024:

Done

in src/validation.cpp:3694 in 8c18b708b9 outdated

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.

instagibbs commented at 7:14 pm on February 23, 2024:

future work nit: the mutated arg is never non-nullptr and has no test coverage it seems.

maflcko commented at 9:45 am on February 26, 2024:

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.

dergoegge force-pushed on Feb 23, 2024

in src/validation.cpp:3706 in 6cae77ae78 outdated

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;

maflcko commented at 10:40 am on February 26, 2024:

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) {?

dergoegge commented at 2:29 pm on February 27, 2024:

Removed the bool

in src/validation.cpp:3682 in aa08aa523d outdated

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());

maflcko commented at 10:57 am on February 26, 2024:

aa08aa523d02ee89f2223381de78b35cf31fa6b6: Maybe document the precondition? Otherwise, this could result in a remote triggered crash, if a caller forgets to check the vin size?

dergoegge commented at 2:29 pm on February 27, 2024:

Added a comment

dergoegge commented at 11:00 am on February 26, 2024: member

I believe I addressed all comments, this is ready for further review

maflcko commented at 11:21 am on February 26, 2024: member

Left two nits, feel free to ignore.

79e62b01d13b300f629574f1349c3b63a36f2b7 🔄

Signature:

0untrusted 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}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: 79e62b01d13b300f629574f1349c3b63a36f2b7 🔄
3ZzIHVQLTyxW9SBChtAlHaJQkND07+MygNA3HHp0nOTcsKGrdGqDY4WOTeUOQGUXO9AojSXrkpgGVRnV2aTfiDw==

in src/test/validation_tests.cpp:229 in 79e62b01d1 outdated

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});

maflcko commented at 3:37 pm on February 26, 2024:

follow-up, or separate 1-line commit:

 0diff --git a/src/test/validation_tests.cpp b/src/test/validation_tests.cpp
 1index fd2ef47b92..dbe2815991 100644
 2--- a/src/test/validation_tests.cpp
 3+++ b/src/test/validation_tests.cpp
 4@@ -223,8 +223,8 @@ BOOST_AUTO_TEST_CASE(block_malleation)
 5         block.vtx.push_back(MakeTransactionRef(CMutableTransaction{}));
 6         BOOST_CHECK(is_mutated(block, /*check_witness_root=*/false));
 7         HashWriter hasher;
 8-        hasher.write({block.vtx[0]->GetHash().data(), 32});
 9-        hasher.write({block.vtx[1]->GetHash().data(), 32});
10+        hasher.write(block.vtx[0]->GetHash());
11+        hasher.write(block.vtx[1]->GetHash());
12         block.hashMerkleRoot = hasher.GetHash();
13         BOOST_CHECK(is_not_mutated(block, /*check_witness_root=*/false));
14 
15diff --git a/src/util/transaction_identifier.h b/src/util/transaction_identifier.h
16index 89e10dee01..d4a0ede25a 100644
17--- a/src/util/transaction_identifier.h
18+++ b/src/util/transaction_identifier.h
19@@ -44,6 +44,7 @@ public:
20     constexpr void SetNull() { m_wrapped.SetNull(); }
21     std::string GetHex() const { return m_wrapped.GetHex(); }
22     std::string ToString() const { return m_wrapped.ToString(); }
23+    static constexpr auto size() { return decltype(m_wrapped)::size(); }
24     constexpr const std::byte* data() const { return reinterpret_cast<const std::byte*>(m_wrapped.data()); }
25     constexpr const std::byte* begin() const { return reinterpret_cast<const std::byte*>(m_wrapped.begin()); }
26     constexpr const std::byte* end() const { return reinterpret_cast<const std::byte*>(m_wrapped.end()); }

dergoegge commented at 2:25 pm on February 27, 2024:

Added an extra commit and patched the tests

in src/test/validation_tests.cpp:277 in 79e62b01d1 outdated

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});

maflcko commented at 3:42 pm on February 26, 2024:

same

in src/test/validation_tests.cpp:279 in 79e62b01d1 outdated

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).

maflcko commented at 3:43 pm on February 26, 2024:

nit: s/with out/without/g

in src/test/validation_tests.cpp:232 in 79e62b01d1 outdated

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+

maflcko commented at 4:28 pm on February 26, 2024:

Is the test covering is_mutated for duplicate tree nodes? I think not. Suggestion:

 0diff --git a/src/test/validation_tests.cpp b/src/test/validation_tests.cpp
 1index dbe2815991..8ad32eb195 100644
 2--- a/src/test/validation_tests.cpp
 3+++ b/src/test/validation_tests.cpp
 4@@ -228,6 +228,17 @@ BOOST_AUTO_TEST_CASE(block_malleation)
 5         block.hashMerkleRoot = hasher.GetHash();
 6         BOOST_CHECK(is_not_mutated(block, /*check_witness_root=*/false));
 7 
 8+        // Block with two transactions is mutated if any node is duplicate.
 9+        {
10+            block.vtx[1] = block.vtx[0];
11+            BOOST_CHECK(is_mutated(block, /*check_witness_root=*/false));
12+            HashWriter hasher;
13+            hasher.write(block.vtx[0]->GetHash());
14+            hasher.write(block.vtx[1]->GetHash());
15+            block.hashMerkleRoot = hasher.GetHash();
16+            BOOST_CHECK(is_mutated(block, /*check_witness_root=*/false));
17+        }
18+
19         // Blocks with 64-byte coinbase transactions are not considered mutated
20         block.vtx.clear();
21         {

dergoegge commented at 2:25 pm on February 27, 2024:

Added

maflcko commented at 4:30 pm on February 26, 2024: member

left some more feedback

ACK 79e62b01d13b300f629574f1349c3b63a36f2b 🍚

Signature:

0untrusted 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}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: ACK 79e62b01d13b300f629574f1349c3b63a36f2b 🍚
32j503nYgXHtf8lxG+JzbLQsNuYaMcWAIenTx9ArAO/hqVxN41C+nl8EYMiHfteDJZX7JGr2ebPMczOKonNLPDg==

DrahtBot requested review from fjahr on Feb 26, 2024

DrahtBot requested review from naumenkogs on Feb 26, 2024

DrahtBot requested review from sr-gi on Feb 26, 2024

DrahtBot requested review from glozow on Feb 26, 2024

DrahtBot requested review from TheCharlatan on Feb 26, 2024

DrahtBot requested review from instagibbs on Feb 26, 2024

in src/test/validation_tests.cpp:274 in 79e62b01d1 outdated

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

fjahr commented at 9:12 pm on February 26, 2024:

doubla

in src/test/validation_tests.cpp:207 in 79e62b01d1 outdated

202+    };
203+
204+    {
205+        CBlock block;
206+
207+        // Emtpy block is expected to have merkle root of 0x0.

fjahr commented at 9:16 pm on February 26, 2024:

Emtpy

in src/validation.cpp:3705 in d10e1fd4d8 outdated

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;

fjahr commented at 9:49 pm on February 26, 2024:

There is another return true; case at the bottom of the function where you would need to set this as well.

maflcko commented at 7:42 am on February 27, 2024:

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.

fjahr commented at 1:20 pm on February 27, 2024:

This case should be documented then.

dergoegge commented at 2:29 pm on February 27, 2024:

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.

fjahr commented at 2:47 pm on February 27, 2024:

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.

Sjors commented at 1:25 pm on February 29, 2024:

I agree that we should document why the cache is only used inside if (expect_witness_commitment).

DrahtBot requested review from fjahr on Feb 26, 2024

[validation] Isolate merkle root checks 95bddb930a

[refactor] Cleanup merkle root checks e7669e1343

[validation] Introduce IsBlockMutated 66abce1d98

[validation] Merkle root malleation should be caught by IsBlockMutated 2d8495e080

[net processing] Don't process mutated blocks

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).

49257c0304

[test] Add regression test for #27608 5bf4f5ba32

[validation] Cache merkle root and witness commitment checks

Slight performance improvement by avoiding duplicate work.

1ec6bbeb8d

Add transaction_identifier::size to allow Span conversion 1ed2c98297

[test] IsBlockMutated unit tests d8087adc7e

dergoegge force-pushed on Feb 27, 2024

maflcko commented at 2:40 pm on February 27, 2024: member

ACK d8087adc7ebd4ea05dd3843e5a92e8115fd7bbcc 🏄

Signature:

0untrusted 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}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: ACK d8087adc7ebd4ea05dd3843e5a92e8115fd7bbcc 🏄
3esXLEJ35qEFyBqeZMSV1JGBnrcJMG5Ami/zV9HPyRQ4Mt5cCWqd8NEkCTySeEOUSf2ngcBWvQdPLp33JU8IaDQ==

fjahr commented at 3:58 pm on February 27, 2024: contributor

Code review ACK d8087adc7ebd4ea05dd3843e5a92e8115fd7bbcc

DrahtBot added the label CI failed on Feb 27, 2024

DrahtBot removed the label CI failed on Feb 27, 2024

in src/test/validation_tests.cpp:236 in d8087adc7e

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));

sr-gi commented at 7:40 pm on February 28, 2024:

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.

maflcko commented at 8:26 am on February 29, 2024:

Correct. That’s my typo and this line can be removed. See #29412 (review)

sr-gi commented at 7:59 pm on February 28, 2024: member

Code review ACK https://github.com/bitcoin/bitcoin/commit/d8087adc7ebd4ea05dd3843e5a92e8115fd7bbcc

Just left a single comment on the newly added unit test

achow101 commented at 10:44 pm on February 28, 2024: member

ACK d8087adc7ebd4ea05dd3843e5a92e8115fd7bbcc

achow101 merged this on Feb 28, 2024

achow101 closed this on Feb 28, 2024

in src/validation.cpp:3671 in e7669e1343 outdated

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. */

Sjors commented at 11:22 am on February 29, 2024:

e7669e1343aec4298fd30d539847963e6fa5619c: This note seems incorrect. I think it should say:

0 If the witness commitment is present (i.e. `expect_witness_commitment
1 = true` and `commitpos != NO_WITNESS_COMMITMENT`)

in src/net_processing.cpp:4725 in 49257c0304 outdated

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))) {

Sjors commented at 12:27 pm on February 29, 2024:

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.

Sjors commented at 1:29 pm on February 29, 2024:

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)

instagibbs commented at 8:00 pm on February 29, 2024:

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”?

maflcko commented at 9:35 am on March 1, 2024:

I guess it would have been useful to enumerate the behavior changes in the commit, such as DoS points.

Sjors commented at 10:52 am on March 1, 2024:

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”.

instagibbs commented at 11:51 am on March 1, 2024:

I mean, our node calls that invalid :)

opened #29524 as what I think a clean fix is, with test coverage

Sjors commented at 12:35 pm on February 29, 2024: member

Post merge review… I still have to look at the last 4 commits, but want to make sure I’m not misunderstanding something.

Sjors commented at 1:11 pm on February 29, 2024: member

Hint for anyone trying to test 5bf4f5ba32da4627f152b54d266df9b2aa930457: uncomment this bit in RemoveBlockRequest.

0if (from_peer && *from_peer != node_id) {
1  range.first++;
2  continue;
3}

in test/functional/p2p_mutated_blocks.py:86 in 5bf4f5ba32 outdated

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"])

Sjors commented at 1:14 pm on February 29, 2024:

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)

0xB10C commented at 2:00 pm on February 29, 2024: contributor

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.

sr-gi referenced this in commit 6ee3997d03 on Feb 29, 2024

instagibbs commented at 4:27 pm on February 29, 2024: member

@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?

dergoegge commented at 4:42 pm on February 29, 2024: member

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.

instagibbs commented at 4:45 pm on February 29, 2024: member

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

fanquake referenced this in commit 9057598605 on Feb 29, 2024

0xB10C commented at 10:11 pm on February 29, 2024: contributor

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

0# debug.log on receiving a mutated block before this PR
1ERROR: AcceptBlock: bad-witness-nonce-size, ContextualCheckBlock : invalid witness reserved value size
2ERROR: ProcessNewBlock: AcceptBlock FAILED (bad-witness-nonce-size, ContextualCheckBlock : invalid witness reserved value size)

maflcko commented at 9:34 am on March 1, 2024: member

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?

0xB10C commented at 5:43 pm on March 1, 2024: contributor

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).

fanquake referenced this in commit e60804f121 on Mar 4, 2024

glozow referenced this in commit 97a1d0a459 on Mar 5, 2024

glozow referenced this in commit 076c67c3aa on Mar 5, 2024

glozow referenced this in commit aff368fa81 on Mar 5, 2024

glozow referenced this in commit 50c0b61a9d on Mar 5, 2024

glozow referenced this in commit 24736350e9 on Mar 5, 2024

glozow referenced this in commit 0c5c5962cb on Mar 5, 2024

glozow referenced this in commit 8141498f3a on Mar 5, 2024

glozow referenced this in commit 0535c253fe on Mar 5, 2024

glozow commented at 10:59 am on March 5, 2024: member

Backported for 26.x in #29509

dergoegge commented at 2:06 pm on March 5, 2024: member

@achow101 can you add it to #29531 as well? (i think Gloria’s 26.x version should be cleanly fit on top of 25.x)

achow101 referenced this in commit 4f5baac6ca on Mar 5, 2024

achow101 referenced this in commit 8804c368f5 on Mar 5, 2024

achow101 referenced this in commit 098f07dc8d on Mar 5, 2024

achow101 referenced this in commit 8cc4b24c74 on Mar 5, 2024

achow101 referenced this in commit de97ecf14f on Mar 5, 2024

achow101 referenced this in commit 0667441a7b on Mar 5, 2024

achow101 referenced this in commit 3eaaafa225 on Mar 5, 2024

achow101 commented at 5:19 pm on March 5, 2024: member

Backported to 25.x in #29531

p2p: Don’t process mutated blocks #29412

Code Coverage

Reviews

Conflicts