Merge bitcoin/bitcoin#25156: refactor: Introduce PeerManagerImpl::RejectIncomingTxs

fafddafc2c refactor: Introduce PeerManagerImpl::RejectIncomingTxs (MacroFake)

Pull request description:

  Currently there are some confusions in net_processing:

  * There is confusion between `-blocksonly mode` and `block-relay-only`, so adjust all comments to use the same nomenclature.
  * Whether to disconnect peers for providing invs/txs is implemented differently. For example, it seems a bit confusing to disconnect `block-relay-only` peers with `relay` permission when they send a tx message, but not when they send an inv message. Also, keeping track of their inv announcements seems both wasteful and confusing, as it does nothing. This isn't possible in practice, as outbound connections do not have permissions assigned, but sees fragile to rely on. Especially in light of proposed changes to make that possible: https://github.com/bitcoin/bitcoin/pull/17167

ACKs for top commit:
  MarcoFalke:
    Should be trivial to re-ACK with `git range-diff bitcoin-core/master  fa2b5fe0c1 fafddafc2c`.
  jnewbery:
    Code review ACK fafddafc2c
  mzumsande:
    ACK fafddafc2c

Tree-SHA512: 73bf91afe93be619169cfbf3bf80cb08a5e6f73df4e0318b86817bd4d45f67408ea85998855992281d2decc9d24f7d75cffb83a0518d670090907309df8a3490

src/net_processing.cpp

@@ -603,9 +603,11 @@ private:
     /** Next time to check for stale tip */
     std::chrono::seconds m_stale_tip_check_time{0s};
 
-    /** Whether this node is running in blocks only mode */
+    /** Whether this node is running in -blocksonly mode */
     const bool m_ignore_incoming_txs;
 
+    bool RejectIncomingTxs(const CNode& peer) const;
+
     /** Whether we've completed initial sync yet, for determining when to turn
       * on extra block-relay-only peers. */
     bool m_initial_sync_finished{false};
@@ -1009,7 +1011,7 @@ void PeerManagerImpl::MaybeSetPeerAsAnnouncingHeaderAndIDs(NodeId nodeid)
 {
     AssertLockHeld(cs_main);
 
-    // Never request high-bandwidth mode from peers if we're blocks-only. Our
+    // When in -blocksonly mode, never request high-bandwidth mode from peers. Our
     // mempool will not contain the transactions necessary to reconstruct the
     // compact block.
     if (m_ignore_incoming_txs) return;
@@ -3084,14 +3086,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
             return;
         }
 
-        // Reject tx INVs when the -blocksonly setting is enabled, or this is a
-        // block-relay-only peer
-        bool reject_tx_invs{m_ignore_incoming_txs || pfrom.IsBlockOnlyConn()};
-
-        // Allow peers with relay permission to send data other than blocks in blocks only mode
-        if (pfrom.HasPermission(NetPermissionFlags::Relay)) {
-            reject_tx_invs = false;
-        }
+        const bool reject_tx_invs{RejectIncomingTxs(pfrom)};
 
         LOCK(cs_main);
 
@@ -3372,10 +3367,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
     }
 
     if (msg_type == NetMsgType::TX) {
-        // Stop processing the transaction early if
-        // 1) We are in blocks only mode and peer has no relay permission; OR
-        // 2) This peer is a block-relay-only peer
-        if ((m_ignore_incoming_txs && !pfrom.HasPermission(NetPermissionFlags::Relay)) || pfrom.IsBlockOnlyConn()) {
+        if (RejectIncomingTxs(pfrom)) {
             LogPrint(BCLog::NET, "transaction sent in violation of protocol peer=%d\n", pfrom.GetId());
             pfrom.fDisconnect = true;
             return;
@@ -4659,6 +4651,15 @@ public:
         return mp->CompareDepthAndScore(*b, *a, m_wtxid_relay);
     }
 };
+} // namespace
+
+bool PeerManagerImpl::RejectIncomingTxs(const CNode& peer) const
+{
+    // block-relay-only peers may never send txs to us
+    if (peer.IsBlockOnlyConn()) return true;
+    // In -blocksonly mode, peers need the 'relay' permission to send txs to us
+    if (m_ignore_incoming_txs && !peer.HasPermission(NetPermissionFlags::Relay)) return true;
+    return false;
 }
 
 bool PeerManagerImpl::SetupAddressRelay(const CNode& node, Peer& peer)

test/functional/p2p_blocksonly.py

@@ -89,6 +89,11 @@ class P2PBlocksOnly(BitcoinTestFramework):
         assert_equal(self.nodes[0].getpeerinfo()[0]['relaytxes'], False)
         _, txid, _, tx_hex = self.check_p2p_tx_violation()
 
+        self.log.info("Tests with node in normal mode with block-relay-only connection, sending an inv")
+        conn = self.nodes[0].add_outbound_p2p_connection(P2PInterface(), p2p_idx=0, connection_type="block-relay-only")
+        assert_equal(self.nodes[0].getpeerinfo()[0]['relaytxes'], False)
+        self.check_p2p_inv_violation(conn)
+
         self.log.info("Check that txs from RPC are not sent to blockrelay connection")
         conn = self.nodes[0].add_outbound_p2p_connection(P2PTxInvStore(), p2p_idx=1, connection_type="block-relay-only")
 
@@ -100,6 +105,13 @@ class P2PBlocksOnly(BitcoinTestFramework):
         conn.sync_send_with_ping()
         assert(int(txid, 16) not in conn.get_invs())
 
+    def check_p2p_inv_violation(self, peer):
+        self.log.info("Check that tx-invs from P2P are rejected and result in disconnect")
+        with self.nodes[0].assert_debug_log(["inv sent in violation of protocol, disconnecting peer"]):
+            peer.send_message(msg_inv([CInv(t=MSG_WTX, h=0x12345)]))
+            peer.wait_for_disconnect()
+        self.nodes[0].disconnect_p2ps()
+
     def check_p2p_tx_violation(self):
         self.log.info('Check that txs from P2P are rejected and result in disconnect')
         spendtx = self.miniwallet.create_self_transfer()
@@ -108,9 +120,7 @@ class P2PBlocksOnly(BitcoinTestFramework):
             self.nodes[0].p2ps[0].send_message(msg_tx(spendtx['tx']))
             self.nodes[0].p2ps[0].wait_for_disconnect()
             assert_equal(self.nodes[0].getmempoolinfo()['size'], 0)
-
-        # Remove the disconnected peer
-        del self.nodes[0].p2ps[0]
+        self.nodes[0].disconnect_p2ps()
 
         return spendtx['tx'], spendtx['txid'], spendtx['wtxid'], spendtx['hex']