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

c157438 qa: test that we do disconnect upon a second invalid compact block being announced (Antoine Poinsot)
fb2dcbb qa: test cached failure for compact block (Antoine Poinsot)
f12d8b1 qa: test a compact block with an invalid transaction (Antoine Poinsot)
d6c37b2 qa: remove unnecessary tx removal from compact block (Antoine Poinsot)

Pull request description:

  In thinking about bitcoin/bitcoin#33050 and bitcoin/bitcoin#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
  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
  diff --git a/src/net_processing.cpp b/src/net_processing.cpp
  index 0c4a89c..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`.

ACKs for top commit:
  kevkevinpal:
    ACK [c157438](bitcoin/bitcoin@c157438)
  nervana21:
    tACK [c157438](bitcoin/bitcoin@c157438)
  instagibbs:
    crACK c157438
  stratospher:
    ACK c157438.

Tree-SHA512: a77d5a9768c0d73f122b06db2e416e80d0b3c3fd261dae8e340ecec2ae92d947d31988894bc732cb6dad2e338b3c82f33e75eb3280f8b0933b285657cf3b212c

Author: fanquake

test/functional/p2p_compactblocks.py

@@ -54,6 +54,7 @@
     CScript,
     OP_DROP,
     OP_TRUE,
+    OP_RETURN,
 )
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import (
@@ -709,7 +710,6 @@ def test_invalid_tx_in_compactblock(self, test_node):
         utxo = self.utxos[0]
 
         block = self.build_block_with_transactions(node, utxo, 5)
-        del block.vtx[3]
         block.hashMerkleRoot = block.calc_merkle_root()
         # Drop the coinbase witness but include the witness commitment.
         add_witness_commitment(block)
@@ -727,6 +727,37 @@ def test_invalid_tx_in_compactblock(self, test_node):
         assert_not_equal(node.getbestblockhash(), block.hash_hex)
         test_node.sync_with_ping()
 
+        # Re-establish a proper witness commitment with the coinbase witness, but
+        # invalidate the last tx in the block.
+        block.vtx[4].vin[0].scriptSig = CScript([OP_RETURN])
+        block.hashMerkleRoot = block.calc_merkle_root()
+        add_witness_commitment(block)
+        block.solve()
+
+        # This will lead to a consensus failure for which we also won't be disconnected but which
+        # will be cached.
+        comp_block.initialize_from_block(block, prefill_list=list(range(len(block.vtx))), use_witness=True)
+        msg = msg_cmpctblock(comp_block.to_p2p())
+        test_node.send_and_ping(msg)
+
+        # The tip still didn't advance.
+        assert_not_equal(node.getbestblockhash(), block.hash_hex)
+        test_node.sync_with_ping()
+
+        # The failure above was cached. Submitting the compact block again will returned a cached
+        # consensus error (the code path is different) and still not get us disconnected (nor
+        # advance the tip).
+        test_node.send_and_ping(msg)
+        assert_not_equal(node.getbestblockhash(), block.hash_hex)
+        test_node.sync_with_ping()
+
+        # Now, announcing a second block building on top of the invalid one will get us disconnected.
+        block.hashPrevBlock = block.hash_int
+        block.solve()
+        comp_block.initialize_from_block(block, prefill_list=list(range(len(block.vtx))), use_witness=True)
+        msg = msg_cmpctblock(comp_block.to_p2p())
+        test_node.send_await_disconnect(msg)
+
     # Helper for enabling cb announcements
     # Send the sendcmpct request and sync headers
     def request_cb_announcements(self, peer):
@@ -943,6 +974,9 @@ def run_test(self):
         self.log.info("Testing handling of invalid compact blocks...")
         self.test_invalid_tx_in_compactblock(self.segwit_node)
 
+        # The previous test will lead to a disconnection. Reconnect before continuing.
+        self.segwit_node = self.nodes[0].add_p2p_connection(TestP2PConn())
+
         self.log.info("Testing invalid index in cmpctblock message...")
         self.test_invalid_cmpctblock_message()