Merge #19198: test: Check that peers with forcerelay permission are not asked to feefilter

fac63eb5ea doc: Remove -whitelistforcerelay from comment (MarcoFalke)
faabd1514f test: Check that peers with forcerelay permission do not get a feefilter message (MarcoFalke)
fad676b8d2 test: Add connect_nodes method (MarcoFalke)
fac6ef4fb2 test: Add test for no net permission (MarcoFalke)
ffff3fe50a test: Replace self.nodes[0].p2p with conn (MarcoFalke)
faccdc8a31 test: remove redundant generate (MarcoFalke)
fab83b934a test: pep-8 p2p_feefilter.py (MarcoFalke)

Pull request description:

ACKs for top commit:
  jonatack:
    re-ACK fac63eb move-only change of two class member functions in test_framework.py and rebases since my review @ faccf0a per `git range-diff 4b5c919 faccf0a fac63eb`. Verified p2p_feefilter and p2p_permissions functional tests are running 🟢 locally.

Tree-SHA512: 30a1c83baee15a4236d127d199c4f264852045372918d5aa5c09ef3d48041762ce3920ff86ef2466d4b2c792ddf56943d12b16c6dce34c6c5aea2a4af2eb4d49

src/net_processing.cpp

@@ -4387,9 +4387,9 @@ bool PeerLogicValidation::SendMessages(CNode* pto)
         //
         // Message: feefilter
         //
-        // We don't want white listed peers to filter txs to us if we have -whitelistforcerelay
         if (pto->m_tx_relay != nullptr && pto->nVersion >= FEEFILTER_VERSION && gArgs.GetBoolArg("-feefilter", DEFAULT_FEEFILTER) &&
-            !pto->HasPermission(PF_FORCERELAY)) {
+            !pto->HasPermission(PF_FORCERELAY) // peers with the forcerelay permission should not filter txs to us
+        ) {
             CAmount currentFilter = m_mempool.GetMinFee(gArgs.GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000).GetFeePerK();
             int64_t timeNow = GetTimeMicros();
             if (timeNow > pto->m_tx_relay->nextSendTimeFeeFilter) {

test/functional/p2p_feefilter.py

@@ -10,11 +10,13 @@ import time
 from test_framework.messages import MSG_TX, msg_feefilter
 from test_framework.mininode import mininode_lock, P2PInterface
 from test_framework.test_framework import BitcoinTestFramework
+from test_framework.util import assert_equal
 
 
 def hashToHex(hash):
     return format(hash, '064x')
 
+
 # Wait up to 60 secs to see if the testnode has received all the expected invs
 def allInvsMatch(invsExpected, testnode):
     for x in range(60):
@@ -24,6 +26,18 @@ def allInvsMatch(invsExpected, testnode):
         time.sleep(1)
     return False
 
+
+class FeefilterConn(P2PInterface):
+    feefilter_received = False
+
+    def on_feefilter(self, message):
+        self.feefilter_received = True
+
+    def assert_feefilter_received(self, recv: bool):
+        with mininode_lock:
+            assert_equal(self.feefilter_received, recv)
+
+
 class TestP2PConn(P2PInterface):
     def __init__(self):
         super().__init__()
@@ -38,6 +52,7 @@ class TestP2PConn(P2PInterface):
         with mininode_lock:
             self.txinvs = []
 
+
 class FeeFilterTest(BitcoinTestFramework):
     def set_test_params(self):
         self.num_nodes = 2
@@ -46,41 +61,54 @@ class FeeFilterTest(BitcoinTestFramework):
         # mempool and wallet feerate calculation based on GetFee
         # rounding down 3 places, leading to stranded transactions.
         # See issue #16499
-        self.extra_args = [["-minrelaytxfee=0.00000100", "-mintxfee=0.00000100"]]*self.num_nodes
+        self.extra_args = [["-minrelaytxfee=0.00000100", "-mintxfee=0.00000100"]] * self.num_nodes
 
     def skip_test_if_missing_module(self):
         self.skip_if_no_wallet()
 
     def run_test(self):
+        self.test_feefilter_forcerelay()
+        self.test_feefilter()
+
+    def test_feefilter_forcerelay(self):
+        self.log.info('Check that peers without forcerelay permission (default) get a feefilter message')
+        self.nodes[0].add_p2p_connection(FeefilterConn()).assert_feefilter_received(True)
+
+        self.log.info('Check that peers with forcerelay permission do not get a feefilter message')
+        self.restart_node(0, extra_args=['-whitelist=forcerelay@127.0.0.1'])
+        self.nodes[0].add_p2p_connection(FeefilterConn()).assert_feefilter_received(False)
+
+        # Restart to disconnect peers and load default extra_args
+        self.restart_node(0)
+        self.connect_nodes(1, 0)
+
+    def test_feefilter(self):
         node1 = self.nodes[1]
         node0 = self.nodes[0]
-        # Get out of IBD
-        node1.generate(1)
-        self.sync_blocks()
 
-        self.nodes[0].add_p2p_connection(TestP2PConn())
+        conn = self.nodes[0].add_p2p_connection(TestP2PConn())
 
         # Test that invs are received by test connection for all txs at
         # feerate of .2 sat/byte
         node1.settxfee(Decimal("0.00000200"))
         txids = [node1.sendtoaddress(node1.getnewaddress(), 1) for x in range(3)]
-        assert allInvsMatch(txids, self.nodes[0].p2p)
-        self.nodes[0].p2p.clear_invs()
+        assert allInvsMatch(txids, conn)
+        conn.clear_invs()
 
         # Set a filter of .15 sat/byte on test connection
-        self.nodes[0].p2p.send_and_ping(msg_feefilter(150))
+        conn.send_and_ping(msg_feefilter(150))
 
         # Test that txs are still being received by test connection (paying .15 sat/byte)
         node1.settxfee(Decimal("0.00000150"))
         txids = [node1.sendtoaddress(node1.getnewaddress(), 1) for x in range(3)]
-        assert allInvsMatch(txids, self.nodes[0].p2p)
-        self.nodes[0].p2p.clear_invs()
+        assert allInvsMatch(txids, conn)
+        conn.clear_invs()
 
         # Change tx fee rate to .1 sat/byte and test they are no longer received
         # by the test connection
         node1.settxfee(Decimal("0.00000100"))
         [node1.sendtoaddress(node1.getnewaddress(), 1) for x in range(3)]
-        self.sync_mempools() # must be sure node 0 has received all txs
+        self.sync_mempools()  # must be sure node 0 has received all txs
 
         # Send one transaction from node0 that should be received, so that we
         # we can sync the test on receipt (if node1's txs were relayed, they'd
@@ -91,14 +119,15 @@ class FeeFilterTest(BitcoinTestFramework):
         # as well.
         node0.settxfee(Decimal("0.00020000"))
         txids = [node0.sendtoaddress(node0.getnewaddress(), 1)]
-        assert allInvsMatch(txids, self.nodes[0].p2p)
-        self.nodes[0].p2p.clear_invs()
+        assert allInvsMatch(txids, conn)
+        conn.clear_invs()
 
         # Remove fee filter and check that txs are received again
-        self.nodes[0].p2p.send_and_ping(msg_feefilter(0))
+        conn.send_and_ping(msg_feefilter(0))
         txids = [node1.sendtoaddress(node1.getnewaddress(), 1) for x in range(3)]
-        assert allInvsMatch(txids, self.nodes[0].p2p)
-        self.nodes[0].p2p.clear_invs()
+        assert allInvsMatch(txids, conn)
+        conn.clear_invs()
+
 
 if __name__ == '__main__':
     FeeFilterTest().main()

test/functional/p2p_permissions.py

@@ -42,6 +42,12 @@ class P2PPermissionsTests(BitcoinTestFramework):
             ["relay", "noban", "mempool"],
             True)
 
+        self.checkpermission(
+            # no permission (even with forcerelay)
+            ["-whitelist=@127.0.0.1", "-whitelistforcerelay=1"],
+            [],
+            False)
+
         self.checkpermission(
             # relay permission removed (no specific permissions)
             ["-whitelist=127.0.0.1", "-whitelistrelay=0"],

test/functional/test_framework/test_framework.py

@@ -353,9 +353,9 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
         # See fPreferredDownload in net_processing.
         #
         # If further outbound connections are needed, they can be added at the beginning of the test with e.g.
-        # connect_nodes(self.nodes[1], 2)
+        # self.connect_nodes(1, 2)
         for i in range(self.num_nodes - 1):
-            connect_nodes(self.nodes[i + 1], i)
+            self.connect_nodes(i + 1, i)
         self.sync_all()
 
     def setup_nodes(self):
@@ -532,11 +532,17 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
     def wait_for_node_exit(self, i, timeout):
         self.nodes[i].process.wait(timeout)
 
+    def connect_nodes(self, a, b):
+        connect_nodes(self.nodes[a], b)
+
+    def disconnect_nodes(self, a, b):
+        disconnect_nodes(self.nodes[a], b)
+
     def split_network(self):
         """
         Split the network of four nodes into nodes 0/1 and 2/3.
         """
-        disconnect_nodes(self.nodes[1], 2)
+        self.disconnect_nodes(1, 2)
         self.sync_all(self.nodes[:2])
         self.sync_all(self.nodes[2:])
 
@@ -544,7 +550,7 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
         """
         Join the (previously split) network halves together.
         """
-        connect_nodes(self.nodes[1], 2)
+        self.connect_nodes(1, 2)
         self.sync_all()
 
     def sync_blocks(self, nodes=None, wait=1, timeout=60):