refactor: Introduce PeerManagerImpl::RejectIncomingTxs

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:
  bitcoin/bitcoin#17167

Author: MacroFake

src/net_processing.cpp

@@ -603,9 +603,11 @@ class PeerManagerImpl final : public PeerManager
     /** 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 @@ class CompareInvMempoolOrder
         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 @@ def blocks_relay_conn_tests(self):
         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 @@ def blocks_relay_conn_tests(self):
         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 @@ def check_p2p_tx_violation(self):
             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']