From fab83b934abcd1228ff21afdc9f8b30ad09745fa Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Sun, 7 Jun 2020 09:03:42 -0400 Subject: [PATCH 1/7] test: pep-8 p2p_feefilter.py --- test/functional/p2p_feefilter.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/test/functional/p2p_feefilter.py b/test/functional/p2p_feefilter.py index 805cb1e84f6d8..ed4fcbd6ef3a5 100755 --- a/test/functional/p2p_feefilter.py +++ b/test/functional/p2p_feefilter.py @@ -15,6 +15,7 @@ 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 +25,7 @@ def allInvsMatch(invsExpected, testnode): time.sleep(1) return False + class TestP2PConn(P2PInterface): def __init__(self): super().__init__() @@ -38,6 +40,7 @@ def clear_invs(self): with mininode_lock: self.txinvs = [] + class FeeFilterTest(BitcoinTestFramework): def set_test_params(self): self.num_nodes = 2 @@ -46,7 +49,7 @@ def set_test_params(self): # 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() @@ -80,7 +83,7 @@ def run_test(self): # 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 @@ -100,5 +103,6 @@ def run_test(self): assert allInvsMatch(txids, self.nodes[0].p2p) self.nodes[0].p2p.clear_invs() + if __name__ == '__main__': FeeFilterTest().main() From faccdc8a3143c9849e61312a7f438bc6e8232496 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Sun, 7 Jun 2020 09:07:59 -0400 Subject: [PATCH 2/7] test: remove redundant generate setup_nodes takes care of getting out of ibd --- test/functional/p2p_feefilter.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/test/functional/p2p_feefilter.py b/test/functional/p2p_feefilter.py index ed4fcbd6ef3a5..8cc62fbe12885 100755 --- a/test/functional/p2p_feefilter.py +++ b/test/functional/p2p_feefilter.py @@ -57,9 +57,6 @@ def skip_test_if_missing_module(self): def run_test(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()) From ffff3fe50a16bd7dde3d2d206bbe7bc41c483bb8 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Sun, 7 Jun 2020 09:14:33 -0400 Subject: [PATCH 3/7] test: Replace self.nodes[0].p2p with conn --- test/functional/p2p_feefilter.py | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/test/functional/p2p_feefilter.py b/test/functional/p2p_feefilter.py index 8cc62fbe12885..0dfbb694688e4 100755 --- a/test/functional/p2p_feefilter.py +++ b/test/functional/p2p_feefilter.py @@ -58,23 +58,23 @@ def run_test(self): node1 = self.nodes[1] node0 = self.nodes[0] - 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 @@ -91,14 +91,14 @@ def run_test(self): # 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__': From fac6ef4fb2bbe6187a52d716eab734d0b1e9a221 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Sun, 7 Jun 2020 09:24:18 -0400 Subject: [PATCH 4/7] test: Add test for no net permission --- test/functional/p2p_permissions.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/test/functional/p2p_permissions.py b/test/functional/p2p_permissions.py index 2c200fccadf5b..bea202855d104 100755 --- a/test/functional/p2p_permissions.py +++ b/test/functional/p2p_permissions.py @@ -42,6 +42,12 @@ def run_test(self): ["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"], From fad676b8d2dfc3a8a62db3d3395d36d3e3076a5b Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Sun, 7 Jun 2020 09:54:56 -0400 Subject: [PATCH 5/7] test: Add connect_nodes method --- test/functional/test_framework/test_framework.py | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py index 1761aa496512b..9d9e065158a4f 100755 --- a/test/functional/test_framework/test_framework.py +++ b/test/functional/test_framework/test_framework.py @@ -353,9 +353,9 @@ def setup_network(self): # 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 @@ def restart_node(self, i, extra_args=None): 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 @@ def join_network(self): """ 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): From faabd1514fecd828451387b025c1cc74a37bc854 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Sun, 7 Jun 2020 10:00:41 -0400 Subject: [PATCH 6/7] test: Check that peers with forcerelay permission do not get a feefilter message --- test/functional/p2p_feefilter.py | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/test/functional/p2p_feefilter.py b/test/functional/p2p_feefilter.py index 0dfbb694688e4..f939ea965cf62 100755 --- a/test/functional/p2p_feefilter.py +++ b/test/functional/p2p_feefilter.py @@ -10,6 +10,7 @@ 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): @@ -26,6 +27,17 @@ def allInvsMatch(invsExpected, testnode): 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__() @@ -55,6 +67,22 @@ 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] From fac63eb5eabcbbc2e51d414b9cf76f0e897dba1a Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Sun, 7 Jun 2020 10:03:13 -0400 Subject: [PATCH 7/7] doc: Remove -whitelistforcerelay from comment Instead, permission flags should be used. For example -whitelist=forcerelay@127.0.0.1 --- src/net_processing.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index d7fbf6941d306..270e415e4205c 100644 --- a/src/net_processing.cpp +++ b/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) {