qa: test that we do not disconnect a peer for submitting an invalid compact block #33083

darosior commented at 9:52 PM on July 28, 2025: member

In thinking about #33050 and #33012 (comment), i went through the code paths for peer disconnection upon submitting an invalid block. It turns out that the fact we exempt a peer from disconnection upon submitting an invalid compact block was not properly tested, as can be checked with these diffs:

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index https://github.com/bitcoin/bitcoin/commit/0c4a89c44cb6e9a61fdd486d576781e5b66c618c..d243fb88d4b 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -1805,10 +1805,10 @@ void PeerManagerImpl::MaybePunishNodeForBlock(NodeId nodeid, const BlockValidati
     // The node is providing invalid data:
     case BlockValidationResult::BLOCK_CONSENSUS:
     case BlockValidationResult::BLOCK_MUTATED:
-        if (!via_compact_block) {
+        //if (!via_compact_block) {
             if (peer) Misbehaving(*peer, message);
             return;
-        }
+        //}
         break;
     case BlockValidationResult::BLOCK_CACHED_INVALID:
         {

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 0c4a89c44cb..e8e0c805367 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -1814,10 +1814,10 @@ void PeerManagerImpl::MaybePunishNodeForBlock(NodeId nodeid, const BlockValidati
         {
             // Discourage outbound (but not inbound) peers if on an invalid chain.
             // Exempt HB compact block peers. Manual connections are always protected from discouragement.
-            if (peer && !via_compact_block && !peer->m_is_inbound) {
+            //if (peer && !via_compact_block && !peer->m_is_inbound) {
                 if (peer) Misbehaving(*peer, message);
                 return;
-            }
+            //}
             break;
         }
     case BlockValidationResult::BLOCK_INVALID_HEADER:

We do have a test for this, but it actually uses a coinbase witness commitment error, which is checked much earlier in FillBlock. This PR adds coverage for the two exemptions in MaybePunishNodeForBlock.

qa: remove unnecessary tx removal from compact block

The error being checked here is BLOCK_MUTATED, as returned by IsBlockMutated()
in FillBlock(). Dropping the fourth transaction from the block is unnecessary
and would make testing of other block validation failures in following commits
more verbose.

d6c37b28a7

qa: test a compact block with an invalid transaction

The current test to exercise a block with an invalid transaction actually
creates a block with an invalid coinbase witness, which is checked early and
for which MaybePunishNodeForBlock() is not called.

Add a test case with an invalid regular transaction, which will lead
CheckInputScripts to return a CONSENSUS error and MaybePunishNodeForBlock() to
be called, appropriately not disconnecting upon an invalid compact block. This
was until now untested as can be checked with the following diff:
```diff
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 0c4a89c44cb..d243fb88d4b 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -1805,10 +1805,10 @@ void PeerManagerImpl::MaybePunishNodeForBlock(NodeId nodeid, const BlockValidati
     // The node is providing invalid data:
     case BlockValidationResult::BLOCK_CONSENSUS:
     case BlockValidationResult::BLOCK_MUTATED:
-        if (!via_compact_block) {
+        //if (!via_compact_block) {
             if (peer) Misbehaving(*peer, message);
             return;
-        }
+        //}
         break;
     case BlockValidationResult::BLOCK_CACHED_INVALID:
         {
```

Finally, note this failure is cached (unlike the malleated witness failure),
which will be used in the following commits.

f12d8b104e

DrahtBot added the label Tests on Jul 28, 2025

DrahtBot commented at 9:52 PM on July 28, 2025: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33083.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	kevkevinpal, nervana21, instagibbs, stratospher

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#32606 (p2p: Drop unsolicited CMPCTBLOCK from non-HB peer by davidgumberg)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

“will returned” -> “will return” [wrong verb form makes sentence ungrammatical]

<sup>drahtbot_id_4_m</sup>

qa: test cached failure for compact block

Submit the block with an invalid transaction Script again, leading to
CACHED_INVALID being returned by AcceptBlockHeader(). Ensure that also this
code path does not lead to a disconnection.

This was previously untested, as can be checked with the following diff:
```diff
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 0c4a89c44cb..e8e0c805367 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -1814,10 +1814,10 @@ void PeerManagerImpl::MaybePunishNodeForBlock(NodeId nodeid, const BlockValidati
         {
             // Discourage outbound (but not inbound) peers if on an invalid chain.
             // Exempt HB compact block peers. Manual connections are always protected from discouragement.
-            if (peer && !via_compact_block && !peer->m_is_inbound) {
+            //if (peer && !via_compact_block && !peer->m_is_inbound) {
                 if (peer) Misbehaving(*peer, message);
                 return;
-            }
+            //}
             break;
         }
     case BlockValidationResult::BLOCK_INVALID_HEADER:
```

fb2dcbb160

darosior force-pushed on Jul 28, 2025

darosior commented at 1:29 PM on July 29, 2025: member

CI failure is random:

Downloading https://github.com/boostorg/optional/archive/boost-1.87.0.tar.gz -> boostorg-optional-boost-1.87.0.tar.gz error: https://github.com/boostorg/optional/archive/boost-1.87.0.tar.gz: failed: status code 503

qa: test that we do disconnect upon a second invalid compact block being announced

This was in fact untested until now. This can be checked with the following diff.

```diff
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 0c4a89c44cb..f8b9adf910a 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -1822,7 +1822,7 @@ void PeerManagerImpl::MaybePunishNodeForBlock(NodeId nodeid, const BlockValidati
         }
     case BlockValidationResult::BLOCK_INVALID_HEADER:
     case BlockValidationResult::BLOCK_INVALID_PREV:
-        if (peer) Misbehaving(*peer, message);
+        if (!via_compact_block && peer) Misbehaving(*peer, message);
         return;
     // Conflicting (but not necessarily invalid) data or different policy:
     case BlockValidationResult::BLOCK_MISSING_PREV:
```

c157438116

darosior commented at 1:59 PM on July 29, 2025: member

Turns out we also didn't test the disconnection logic after a second block building on top of an invalid block is submitted to us (nor the disconnection logic for when an invalid compact block header is submitted to us). This can be checked with:

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 0c4a89c44cb..f8b9adf910a 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -1822,7 +1822,7 @@ void PeerManagerImpl::MaybePunishNodeForBlock(NodeId nodeid, const BlockValidati
         }
     case BlockValidationResult::BLOCK_INVALID_HEADER:
     case BlockValidationResult::BLOCK_INVALID_PREV:
-        if (peer) Misbehaving(*peer, message);
+        if (!via_compact_block && peer) Misbehaving(*peer, message);
         return;
     // Conflicting (but not necessarily invalid) data or different policy:
     case BlockValidationResult::BLOCK_MISSING_PREV:

I added a commit which exercises the case where AcceptBlockHeader will return BLOCK_INVALID_PREV.

fanquake requested review from instagibbs on Jul 29, 2025

kevkevinpal commented at 12:52 AM on July 30, 2025: contributor

ACK c157438

I was able to reproduce the errors with the diffs provided above, and was also able to validate that the master branch tests pass when the diffs are applied as well

in test/functional/p2p_compactblocks.py:747 in fb2dcbb160 outdated

 743 | @@ -744,6 +744,13 @@ def test_invalid_tx_in_compactblock(self, test_node):
 744 |          assert_not_equal(node.getbestblockhash(), block.hash_hex)
 745 |          test_node.sync_with_ping()
 746 |  
 747 | +        # The failure above was cached. Submitting the compact block again will returned a cached

nervana21 commented at 2:41 AM on July 30, 2025:

Small nit

        # The failure above was cached. Submitting the compact block again will return a cached

nervana21 commented at 5:39 AM on July 31, 2025: contributor

tACK c157438

I reproduced the errors as expected with the provided diffs. I validated that the master branch tests pass when the proposed commits are applied.

instagibbs commented at 7:05 AM on July 31, 2025: member

crACK c1574381168573c561ebddf1945d2debefb340f7

stratospher commented at 7:13 PM on July 31, 2025: contributor

ACK c1574381168573c561ebddf1945d2debefb340f7.

nice coverage! I guess we had some coverage for it before #32646 since we used to call a different check in FillBlock().

fanquake merged this on Aug 1, 2025

fanquake closed this on Aug 1, 2025

darosior deleted the branch on Aug 1, 2025