Add test for -blocksonly and -whitelistforcerelay param interaction #18530

glowang commented at 3:19 AM on April 5, 2020: contributor

Related to: #18428

When -blocksonly is turned on, a node would still relay transactions from whitelisted peers. This funcitonality has not been tested.

fanquake added the label Tests on Apr 5, 2020

in test/functional/p2p_blocksonly.py:6 in d86dba18b0 outdated

   2 | @@ -3,7 +3,7 @@
   3 |  # Distributed under the MIT software license, see the accompanying
   4 |  # file COPYING or http://www.opensource.org/licenses/mit-license.php.
   5 |  """Test p2p blocksonly"""
   6 | -
   7 | +from pprint import pprint

glowang commented at 3:21 AM on April 5, 2020:

(will remove after debugging)

in test/functional/p2p_blocksonly.py:99 in d86dba18b0 outdated

  94 | +        pprint(self.nodes[0].p2p.last_message.get('tx'))
  95 | +
  96 | +        with self.nodes[0].assert_debug_log(['received getdata for: tx {} peer=0'.format(txid1)]):
  97 | +            self.nodes[0].sendrawtransaction(sigtx1)
  98 | +            self.nodes[0].p2p.wait_for_tx(txid1)
  99 | +            pprint(self.nodes[0].p2p.wait_for_tx(txid1))

glowang commented at 3:23 AM on April 5, 2020:

I am getting this error on this line: File "/Users/gwang/Documents/bitcoin/test/functional/test_framework/util.py", line 234, in wait_until raise AssertionError("Predicate {} not true after {} seconds".format(predicate_source, timeout)) AssertionError: Predicate '''' def test_function(): assert self.is_connected if not self.last_message.get('tx'): return False return self.last_message['tx'].tx.rehash() == txid ''' not true after 60 seconds

I'm a bit confused about why this transaction isn't working, since I constructed it very similarly to the first transaction in this test file. I am still learning the way that this test setup works, and I might have been missing something

in test/functional/p2p_blocksonly.py:63 in d86dba18b0 outdated

  54 | @@ -55,8 +55,51 @@ def run_test(self):
  55 |          with self.nodes[0].assert_debug_log(['received getdata for: tx {} peer=1'.format(txid)]):
  56 |              self.nodes[0].sendrawtransaction(sigtx)
  57 |              self.nodes[0].p2p.wait_for_tx(txid)
  58 | +            pprint(self.nodes[0].p2p.wait_for_tx(txid))
  59 |              assert_equal(self.nodes[0].getmempoolinfo()['size'], 1)
  60 |  
  61 | +        self.log.info("Restarting node 0 with whitelist permission and blocksonly")
  62 | +        self.stop_node(0)
  63 | +        self.start_node(0, ["-whitelist=127.0.0.1", "-whitelistforcerelay", "-blocksonly"])

glowang commented at 3:26 AM on April 5, 2020:

There seem to exist many variations of the "whitelist" params and different tests set these params differently. For example, ["-whitelist=127.0.0.1", "-whitelistrelay=0"], or ["-whitelist=127.0.0.1", "-whitelistrelay=0"],, ['-whitelist=noban@127.0.0.1'], etc. Understanding the nuances between them is a bit challenging after I have done quite a bit of digging. Could someone point me to some resource for this?

RANDALL-hub commented at 3:30 AM on April 5, 2020:

REVISA LA BASE Y CONTINUA CON LO DEMÁS.

MarcoFalke commented at 1:10 PM on April 5, 2020:

I think if you add nopersistmempool, it will drop the tx from the mempool and you can reuse it for this test without having to construct it again. In fact the below construction looks similar to the one above, so the resulting tx might be the same, thus leading to test failures.

        self.start_node(0, ["-whitelist=127.0.0.1", "-whitelistforcerelay", "-blocksonly", "-nopersistmempool"])

glowang commented at 5:18 PM on April 7, 2020:

Thanks Marco, going to try again with the suggestion!

in test/functional/p2p_blocksonly.py:65 in d86dba18b0 outdated

  63 | +        self.start_node(0, ["-whitelist=127.0.0.1", "-whitelistforcerelay", "-blocksonly"])
  64 | +        self.nodes[0].add_p2p_connection(P2PInterface())
  65 | +        self.log.info('Check that txs from whitelisted peers are not rejected and relayed to others')
  66 | +        peerInfo = self.nodes[0].getpeerinfo()[0]
  67 | +        assert_equal(peerInfo['whitelisted'], True)
  68 | +        assert_equal(peerInfo['permissions'], ['noban', 'forcerelay', 'relay', 'mempool']) #checkpermission use check permission from p2p_permission

glowang commented at 3:26 AM on April 5, 2020:

not sure if noban is necessary to check here

glowang renamed this:
~~WIP: add test for -blocksonly and -whitelistforcerelay~~
Help needed: add test for -blocksonly and -whitelistforcerelay
on Apr 5, 2020

DrahtBot commented at 7:24 AM on April 5, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#16890 (rpc: Don't allow to 'estimatesmartfee' in blocksonly mode by darosior)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

MarcoFalke commented at 1:12 PM on April 5, 2020: member

Concept ACK

in test/functional/test_framework/test_node.py:215 in 037f2d3d33 outdated

 211 | @@ -212,6 +212,7 @@ def wait_for_rpc_connection(self):
 212 |          poll_per_s = 4
 213 |          for _ in range(poll_per_s * self.rpc_timeout):
 214 |              if self.process.poll() is not None:
 215 | +                print('process poll is not NONE', self.process.poll()) #this outputs me 1

MarcoFalke commented at 12:13 AM on April 8, 2020:

On startup failures or general test failures, you can combine the logs to see the issue:

$ ./test/functional/combine_logs.py -c /tmp/bitcoin_func_test_wnpgbmh_  | tail 
                                     File "/home/marco/workspace/btc_bitcoin_core/test/functional/test_framework/test_framework.py", line 419, in start_node
                                       node.wait_for_rpc_connection()
                                     File "/home/marco/workspace/btc_bitcoin_core/test/functional/test_framework/test_node.py", line 217, in wait_for_rpc_connection
                                       'bitcoind exited with status {} during initialization'.format(self.process.returncode)))
                                   test_framework.test_node.FailedToStartError: [node 0] bitcoind exited with status 1 during initialization
 test  2020-04-08T00:11:49.629000Z TestFramework (DEBUG): Closing down network thread 
 test  2020-04-08T00:11:49.680000Z TestFramework (INFO): Stopping nodes 
 test  2020-04-08T00:11:49.681000Z TestFramework.node0 (DEBUG): Stopping node 

 node0 stderr Error: Command line contains unexpected token 'persistmempool=0', see bitcoind -h for a list of options.

MarcoFalke commented at 12:14 AM on April 8, 2020:

I just ran the test with

$ ./test/functional/p2p_blocksonly.py --tracerpc -l DEBUG

in test/functional/p2p_blocksonly.py:62 in 037f2d3d33 outdated

  54 | @@ -55,8 +55,28 @@ def run_test(self):
  55 |          with self.nodes[0].assert_debug_log(['received getdata for: tx {} peer=1'.format(txid)]):
  56 |              self.nodes[0].sendrawtransaction(sigtx)
  57 |              self.nodes[0].p2p.wait_for_tx(txid)
  58 | +            pprint(self.nodes[0].p2p.wait_for_tx(txid))
  59 |              assert_equal(self.nodes[0].getmempoolinfo()['size'], 1)
  60 |  
  61 | +        self.log.info("Restarting node 0 with whitelist permission and blocksonly")
  62 | +        self.restart_node(0, ["persistmempool=0", "-whitelist=127.0.0.1", "-whitelistforcerelay", "-blocksonly"])

MarcoFalke commented at 12:13 AM on April 8, 2020:

        self.restart_node(0, ["-persistmempool=0", "-whitelist=127.0.0.1", "-whitelistforcerelay", "-blocksonly"])

glowang force-pushed on Apr 12, 2020

in test/functional/p2p_blocksonly.py:76 in bc432ee0b3 outdated

  71 | +        pprint(self.nodes[0].p2p.last_message)
  72 | +        pprint(self.nodes[0].p2p.last_message.get('tx'))
  73 | +
  74 | +        assert_equal(self.nodes[0].p2p.is_connected, True)
  75 | +        with self.nodes[0].assert_debug_log(['received getdata for: tx {} peer=0'.format(txid)]):
  76 | +            self.nodes[0].sendrawtransaction(sigtx)

MarcoFalke commented at 1:04 AM on April 12, 2020:

You will need at least three (mini)nodes if you want to check what you claim to be wanting to check. "Check that txs from whitelisted peers are not rejected and relayed to others"

whitlisted peer --- tx ---> Bitcoin Core ---- tx -----> another peer

            another_peer = self.nodes[0].add_p2p_connection(P2PInterface())
            ... etc

glowang commented at 2:36 AM on April 12, 2020:

Achso! I thnk I know why I am erroring...I couldn't figure out why self.nodes[0].p2p.wait_for_tx(txid) kept timing out(line 77)...but little did I realized that this tx was originated by self.nodes[0].p2p hence it wouldn't hear this transaction again?? And instead, another_peer.wait_for_tx(txid) would be the correct thing to check for..?

glowang commented at 2:44 AM on April 12, 2020:

Also, this has been unclear to me: line 69 suggests to me that it is nodes[0]'s peer who sends out the transaction, which should be received by node[0]. We want to test whether node[0] would accept and relay this tx. This is aligned with purpose of the test.

However, on line 24, 33, and line 76, it seems to be that node[0] is the one who initiates the transaction, zB self.nodes[0].sendrawtransaction(sigtx). As a relay node (of the peer node[0].p2p), why is node[0] creating, signing, and transaction then?

glowang commented at 2:45 AM on April 12, 2020:

@MarcoFalke It would be really great if you could clarify this too

MarcoFalke commented at 10:46 PM on April 12, 2020:

And instead, another_peer.wait_for_tx(txid) would be the correct thing to check for..?

Correct

MarcoFalke commented at 10:50 PM on April 12, 2020:

self.nodes[0] is the Bitcoin Core node "in the middle". I suggest not using self.nodes[0].p2p at all and instead assigning the peers proper names. You did that with first_peer, second_peer.

first_peer and second_peer are not Bitcoin Core nodes, but python mininodes (mock nodes).

When you say self.nodes[0].sendrawtransacttion, I believe you really meant first_peer.send_message(msg_tx(FromHex(CTransaction(), sigtx)))?

glowang commented at 5:39 PM on April 18, 2020:

I think what confuses me is the existing code line 56 uses this construct too: self.nodes[0].sendrawtransaction(sigtx) so I kind of mimicked that line 😂 Does this mean that line 56 should be fixed too?

MarcoFalke commented at 5:52 PM on April 18, 2020:

That is a different test "Check that txs from rpc are not rejected", here in this line you are testing "Check that the tx from whitelisted peers ..."

So those two lines should differ between the test cases

glowang force-pushed on Apr 12, 2020

glowang renamed this:
~~Help needed: add test for -blocksonly and -whitelistforcerelay~~
Add test for -blocksonly and -whitelistforcerelay
on Apr 12, 2020

glowang marked this as ready for review on Apr 12, 2020

glowang force-pushed on Apr 12, 2020

glowang renamed this:
~~Add test for -blocksonly and -whitelistforcerelay~~
Add test for -blocksonly and -whitelistforcerelay param interaction
on Apr 12, 2020

glowang force-pushed on Apr 18, 2020

in test/functional/p2p_blocksonly.py:76 in f348b9bda9 outdated

  71 | +
  72 | +        self.log.info('Check that the whitelisted peer is still connected after sending the transaction')
  73 | +        assert_equal(first_peer.is_connected, True)
  74 | +       
  75 | +        self.log.info('Check that the tx from whitelisted first_peer is relayed to others (ie.second_peer)')
  76 | +        second_peer.wait_for_tx(txid)

glowang commented at 9:57 PM on April 18, 2020:

I think the fact that wait_for_tx does not throw indicate my second_peer has received this tx. Not sure if there is a way to check this more explicitly

in test/functional/p2p_blocksonly.py:82 in f348b9bda9 outdated

  73 | +        assert_equal(first_peer.is_connected, True)
  74 | +       
  75 | +        self.log.info('Check that the tx from whitelisted first_peer is relayed to others (ie.second_peer)')
  76 | +        second_peer.wait_for_tx(txid)
  77 | +        assert_equal(self.nodes[0].getmempoolinfo()['size'], 1)
  78 | +        self.log.info("Whitelisted peer's transaction is accepted and relayed")

glowang commented at 10:00 PM on April 18, 2020:

I noticed a pattern of using "with" statement, or context manager, for checks like this. For example, line (42 and 54). But I am not quite sure whether that is necessary. wait_for_tx will timeout after 60 seconds anyways. What resources are we trying to release with the "with" anyways?

MarcoFalke commented at 10:31 PM on April 18, 2020:

It is optional. If you want you can check that an exact code path was hit that also contained a debug statement.

In this case it would be:

with self.nodes[0].assert_debug_log(["Force relaying tx {} from whitelisted peer=0".format(txid)]):
  first_peer.send(...
  ...

To check you are hitting the right branch in src/net_processing.cpp, where it say "Force relaying tx %s ..."

glowang requested review from MarcoFalke on Apr 18, 2020

in test/functional/p2p_blocksonly.py:6 in f348b9bda9 outdated

   2 | @@ -3,7 +3,6 @@
   3 |  # Distributed under the MIT software license, see the accompanying
   4 |  # file COPYING or http://www.opensource.org/licenses/mit-license.php.
   5 |  """Test p2p blocksonly"""
   6 | -

MarcoFalke commented at 10:47 PM on April 18, 2020:

Travis seems to be passing, but there is a linter error. Potentially pep8. Maybe it is this missing line?

You can try to format the python code with some kind of formatter or run the ./test/lint/lint-python.sh and fix the issues up manually.

MarcoFalke commented at 11:11 PM on April 18, 2020: member

From travis:

test/functional/p2p_blocksonly.py:68:83: W291 trailing whitespace

test/functional/p2p_blocksonly.py:74:1: W293 blank line contains whitespace

^---- failure generated from test/lint/lint-python.sh


This diff appears to have added new lines with trailing whitespace.

The following changes were suspected:

diff --git a/test/functional/p2p_blocksonly.py b/test/functional/p2p_blocksonly.py

@@ -59,0 +59,20 @@ class P2PBlocksOnly(BitcoinTestFramework):

+        assert_equal(self.nodes[0].testmempoolaccept([sigtx])[0]['allowed'], True)

+

^---- failure generated from test/lint/lint-whitespace.sh

glowang force-pushed on Apr 25, 2020

in test/functional/p2p_blocksonly.py:76 in 13cff3a8f7 outdated

  75 | +
  76 | +        self.log.info('Check that the whitelisted peer is still connected after sending the transaction')
  77 | +        assert_equal(first_peer.is_connected, True)
  78 | +
  79 | +        self.log.info('Check that the tx from whitelisted first_peer is relayed to others (ie.second_peer)')
  80 | +        with self.nodes[0].assert_debug_log(["received getdata"]):

glowang commented at 11:22 PM on April 25, 2020:

I wasn't able assert that if (pfrom->HasPermission(PF_FORCERELAY)) {...} is hit, even though my peer's permissions list includes "forcerelay", which should correspond to PF_FORCERELAY...

From here: "The getdata message should usually only be used to request data from a node which previously advertised it had that data by sending an inv message." "It [inv] can be sent unsolicited to announce new transactions or blocks" ...this makes me think that perhaps self.nodes[0] announces the transaction once it receives it from first_peer, sending out inv to second_peer, and second_peer sends getdata to self.nodes[0]

glowang commented at 3:52 PM on April 28, 2020:

@MarcoFalke could you take a look at this?

MarcoFalke commented at 5:03 PM on April 28, 2020:

        with self.nodes[0].assert_debug_log(["received getdata", "Force relaying tx {} from whitelisted peer={}".format(txid, 0)]):

You can assert that the if-branch is hit by reading the log statement that is written to the debug log when this if-branch is executed.

glowang commented at 5:41 AM on April 29, 2020:

I am still not sure why I am getting error trying to assert that this if branch is hit.

From else if (permission == "forcerelay") NetPermissions::AddFlag(flags, PF_FORCERELAY); I konw that we can assert that PF_FORCERELAY flag is set with "forcerelay". However, when I tried to assert this, I got this following error that suggest that this branch isn't:

AssertionError: [node 0] Expected messages "['received getdata', 'Force relaying tx af34fc5ff9ea8babbd4083fbb79ffd2ad5aff1d6def803c07ca5aeed880bd60f from whitelisted peer=0']" does not partially match log:

2020-04-29T05:33:05.256801Z [msghand] Enqueuing TransactionAddedToMempool: txid=af34fc5ff9ea8babbd4083fbb79ffd2ad5aff1d6def803c07ca5aeed880bd60f wtxid=af34fc5ff9ea8babbd4083fbb79ffd2ad5aff1d6def803c07ca5aeed880bd60f
2020-04-29T05:33:05.257550Z [msghand] Checking mempool with 1 transactions and 1 inputs
2020-04-29T05:33:05.257628Z [msghand] AcceptToMemoryPool: peer=0: accepted af34fc5ff9ea8babbd4083fbb79ffd2ad5aff1d6def803c07ca5aeed880bd60f (poolsz 1 txn, 1 kB)
2020-04-29T05:33:05.257685Z [msghand] sending inv (37 bytes) peer=1
2020-04-29T05:33:05.257745Z [scheduler] TransactionAddedToMempool: txid=af34fc5ff9ea8babbd4083fbb79ffd2ad5aff1d6def803c07ca5aeed880bd60f wtxid=af34fc5ff9ea8babbd4083fbb79ffd2ad5aff1d6def803c07ca5aeed880bd60f
2020-04-29T05:33:05.257955Z [scheduler] [default wallet] AddToWallet af34fc5ff9ea8babbd4083fbb79ffd2ad5aff1d6def803c07ca5aeed880bd60f
2020-04-29T05:33:05.258554Z [msghand] received: getdata (37 bytes) peer=1
2020-04

glowang commented at 5:46 AM on April 29, 2020:

And the other place where we tested whether this if-branch is hit is in p2p_permissions.py: and the major difference between the two setups is this one uses P2PDataStore, mine uses P2PInterface: p2p_rebroadcast_wallet = self.nodes[1].add_p2p_connection(P2PDataStore())

Also, this test was able to assert Force relaying ... beacuse this broadcast happened between two full-nodes, not between full node and mini-node (like the case here). However, I did try substituing one full node out with a mini node, and this didn't change anything and test still passed.

MarcoFalke commented at 1:15 PM on April 29, 2020:

You will need to send the message (tx) inside the assert debug log context mangager. Otherwise the log is not captured.

glowang force-pushed on Apr 28, 2020

glowang requested review from MarcoFalke on Apr 29, 2020

glowang force-pushed on May 4, 2020

Add test for param interaction b/w -blocksonly and -whitelistforcerelay be01449cc8

glowang force-pushed on May 5, 2020

glowang commented at 12:23 AM on May 10, 2020: contributor

All comments have been addressed. This PR is ready for review.

in src/net_processing.cpp:2538 in 2b462be152 outdated

2534 | @@ -2535,8 +2535,9 @@ bool ProcessMessage(CNode* pfrom, const std::string& msg_type, CDataStream& vRec
2535 |  
2536 |      if (msg_type == NetMsgType::TX) {
2537 |          // Stop processing the transaction early if
2538 | -        // We are in blocks only mode and peer is either not whitelisted or whitelistrelay is off
2539 | -        // or if this peer is supposed to be a block-relay-only peer
2540 | +        // 1) We are in blocks only mode and peer is either not whitelisted or the peer is whitelisted

MarcoFalke commented at 1:30 PM on May 10, 2020:

        // 1) We are in blocks only mode and the peer has no relay permission

I think this can be said slightly more concise, but still correct

MarcoFalke approved

MarcoFalke commented at 1:30 PM on May 10, 2020: member

Concept ACK. Thanks for working on the documentation and adding tests.

glowang requested review from MarcoFalke on May 15, 2020

Updated comment for the condition where a transaction relay is denied 0ea5d70b47

in src/net_processing.cpp:2539 in 7e21dbb787 outdated

2534 | @@ -2535,8 +2535,9 @@ bool ProcessMessage(CNode* pfrom, const std::string& msg_type, CDataStream& vRec
2535 |  
2536 |      if (msg_type == NetMsgType::TX) {
2537 |          // Stop processing the transaction early if
2538 | -        // We are in blocks only mode and peer is either not whitelisted or whitelistrelay is off
2539 | -        // or if this peer is supposed to be a block-relay-only peer
2540 | +        // 1) We are in blocks only mode and the peer has no relay permission
2541 | +        // but "-whitelistrelay" is off, or

MarcoFalke commented at 10:18 AM on May 15, 2020:

this needs to be removed as well, ofc

glowang commented at 3:18 PM on May 17, 2020:

Ach ja, natürlich 😂

glowang force-pushed on May 17, 2020

glowang requested review from MarcoFalke on May 17, 2020

glowang commented at 4:02 PM on May 17, 2020: contributor

net_processing.cpp file comment has been updated. ready for re-review.

MarcoFalke commented at 4:56 PM on May 17, 2020: member

ACK 0ea5d70b4756f376342417e0019490233cb4a918

MarcoFalke commented at 1:01 PM on May 21, 2020: member

Thanks again for improving documentation and adding tests. This should be ready for merge now.

MarcoFalke merged this on May 21, 2020

MarcoFalke closed this on May 21, 2020

sidhujag referenced this in commit 8c3da42ad5 on May 21, 2020

deadalnix referenced this in commit 5abc69901b on Feb 8, 2021

DrahtBot locked this on Feb 15, 2022