p2p: Don’t process mutated blocks #29412

pull dergoegge wants to merge 9 commits into bitcoin:master from dergoegge:2024-01-mut-blocks changing 8 files +449 −41
  1. 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).

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

    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.

  3. DrahtBot added the label P2P on Feb 8, 2024
  4. dergoegge force-pushed on Feb 8, 2024
  5. DrahtBot added the label CI failed on Feb 8, 2024
  6. 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

  7. DrahtBot removed the label CI failed on Feb 8, 2024
  8. 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?
  9. 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:

  10. TheCharlatan commented at 1:36 pm on February 9, 2024: contributor
    Concept ACK
  11. epiccurious commented at 2:28 pm on February 9, 2024: none
    Concept ACK.
  12. 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.

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

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

  15. 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
  16. 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;
    
  17. 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
  18. sr-gi commented at 8:28 pm on February 9, 2024: member
    Concept ACK
  19. 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
  20. 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?
  21. maflcko added this to the milestone 27.0 on Feb 20, 2024
  22. 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.
  23. 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.
  24. 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
  25. 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.

  26. naumenkogs commented at 8:05 am on February 22, 2024: member
    Concept ACK. Looking forward to addressing already pending comments, then i review.
  27. dergoegge force-pushed on Feb 22, 2024
  28. 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
  29. 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…

  30. 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};
    
  31. 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)};
    
  32. 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)
  33. 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"]
    
  34. 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.
  35. 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).
  36. 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());.

  37. 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
  38. 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
    
  39. 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
  40. 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==
    
  41. dergoegge force-pushed on Feb 23, 2024
  42. 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());
    
  43. 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.
    
  44. 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
  45. 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.

  46. dergoegge force-pushed on Feb 23, 2024
  47. 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
  48. 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
  49. dergoegge commented at 11:00 am on February 26, 2024: member
    I believe I addressed all comments, this is ready for further review
  50. 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==
    
  51. 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
  52. 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
  53. 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
  54. 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
  55. 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==
    
  56. DrahtBot requested review from fjahr on Feb 26, 2024
  57. DrahtBot requested review from naumenkogs on Feb 26, 2024
  58. DrahtBot requested review from sr-gi on Feb 26, 2024
  59. DrahtBot requested review from glozow on Feb 26, 2024
  60. DrahtBot requested review from TheCharlatan on Feb 26, 2024
  61. DrahtBot requested review from instagibbs on Feb 26, 2024
  62. 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
  63. 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
  64. 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).
  65. DrahtBot requested review from fjahr on Feb 26, 2024
  66. [validation] Isolate merkle root checks 95bddb930a
  67. [refactor] Cleanup merkle root checks e7669e1343
  68. [validation] Introduce IsBlockMutated 66abce1d98
  69. [validation] Merkle root malleation should be caught by IsBlockMutated 2d8495e080
  70. [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
  71. [test] Add regression test for #27608 5bf4f5ba32
  72. [validation] Cache merkle root and witness commitment checks
    Slight performance improvement by avoiding duplicate work.
    1ec6bbeb8d
  73. Add transaction_identifier::size to allow Span conversion 1ed2c98297
  74. [test] IsBlockMutated unit tests d8087adc7e
  75. dergoegge force-pushed on Feb 27, 2024
  76. 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==
    
  77. fjahr commented at 3:58 pm on February 27, 2024: contributor
    Code review ACK d8087adc7ebd4ea05dd3843e5a92e8115fd7bbcc
  78. DrahtBot added the label CI failed on Feb 27, 2024
  79. DrahtBot removed the label CI failed on Feb 27, 2024
  80. 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)
  81. 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

  82. achow101 commented at 10:44 pm on February 28, 2024: member
    ACK d8087adc7ebd4ea05dd3843e5a92e8115fd7bbcc
  83. achow101 merged this on Feb 28, 2024
  84. achow101 closed this on Feb 28, 2024

  85. 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`)
    
  86. 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

  87. 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.
  88. 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}
    
  89. 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)

  90. 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.
  91. sr-gi referenced this in commit 6ee3997d03 on Feb 29, 2024
  92. 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?
  93. 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.

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

  95. fanquake referenced this in commit 9057598605 on Feb 29, 2024
  96. 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)
    
  97. 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?

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

  99. fanquake referenced this in commit e60804f121 on Mar 4, 2024
  100. glozow referenced this in commit 97a1d0a459 on Mar 5, 2024
  101. glozow referenced this in commit 076c67c3aa on Mar 5, 2024
  102. glozow referenced this in commit aff368fa81 on Mar 5, 2024
  103. glozow referenced this in commit 50c0b61a9d on Mar 5, 2024
  104. glozow referenced this in commit 24736350e9 on Mar 5, 2024
  105. glozow referenced this in commit 0c5c5962cb on Mar 5, 2024
  106. glozow referenced this in commit 8141498f3a on Mar 5, 2024
  107. glozow referenced this in commit 0535c253fe on Mar 5, 2024
  108. glozow commented at 10:59 am on March 5, 2024: member
    Backported for 26.x in #29509
  109. 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)
  110. achow101 referenced this in commit 4f5baac6ca on Mar 5, 2024
  111. achow101 referenced this in commit 8804c368f5 on Mar 5, 2024
  112. achow101 referenced this in commit 098f07dc8d on Mar 5, 2024
  113. achow101 referenced this in commit 8cc4b24c74 on Mar 5, 2024
  114. achow101 referenced this in commit de97ecf14f on Mar 5, 2024
  115. achow101 referenced this in commit 0667441a7b on Mar 5, 2024
  116. achow101 referenced this in commit 3eaaafa225 on Mar 5, 2024
  117. achow101 commented at 5:19 pm on March 5, 2024: member
    Backported to 25.x in #29531

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-11 03:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me