test: bip157 p2p_blockfilters configuration options #19720

1. 
   jonatack commented at 1:39 pm on August 14, 2020: member

   Add tests for BIP157 configuration options in p2p_blockfilters as mentioned in #19070#pullrequestreview-424449204. While here, optionally test with tighter extra_args, change an info log to debug as it doesn’t describe a test, and fix up the message names in the header docstring.
2. test: p2p_blockfilters tests for BIP157 config args

   - add tests for BIP157 configuration options
   - test with tighter extra args
   - fixup the message names
   - change a log message from info to debug

   7cdd5ce7b4
3. DrahtBot added the label Tests on Aug 14, 2020
4. 
   practicalswift commented at 3:09 pm on August 14, 2020: contributor

   Concept ACK: thanks for adding tests!
5. theStack approved
6. 
   theStack commented at 2:51 pm on August 15, 2020: member

   ACK https://github.com/bitcoin/bitcoin/pull/19720/commits/7cdd5ce7b440d07154a08a90ac1b6bb79f15c579 Verified that message types in docstring are named according to BIP157, checked that the extra arg changes are just more explicit and tighter, reasoned that the added tests make sense and ran the functional test locally with success. 🍺
7. in test/functional/p2p_blockfilters.py:90 in 7cdd5ce7b4

   86@@ -79,7 +87,7 @@ def run_test(self):
   87         assert int(self.nodes[0].getnetworkinfo()['localservices'], 16) & NODE_COMPACT_FILTERS != 0
   88         assert int(self.nodes[1].getnetworkinfo()['localservices'], 16) & NODE_COMPACT_FILTERS == 0
   89 
   90-        self.log.info("get cfcheckpt on chain to be re-orged out.")
   91+        self.log.debug("get cfcheckpt on chain to be re-orged out.")
   

   ---

   ---

   

   robot-dreams commented at 5:54 pm on August 29, 2020:

   Nit: Would it also make sense to make the following changes below?

   • Change “Reorg node 0 to a new chain” from info to debug
   • Reword “Requests to node 1 without NODE_COMPACT_FILTERS results in disconnection.” as “Check that requests without NODE_COMPACT_FiLTERS result in disconnection.”

   ---

   

   jonatack commented at 9:44 am on October 4, 2020:

   Sure.
8. in test/functional/p2p_blockfilters.py:62 in 7cdd5ce7b4

   59+        err_msg = 'Error: Cannot set -peerblockfilters without -blockfilterindex.'
   60+        self.nodes[0].assert_start_raises_init_error(['-blockfilterindex=0', '-peerblockfilters'], err_msg)
   61+
   62+        self.log.info('Test passing unknown -blockfilterindex type raises init error')
   63+        err_msg = 'Error: Unknown -blockfilterindex value foo.'
   64+        self.nodes[0].assert_start_raises_init_error(['-blockfilterindex=foo', '-peerblockfilters'], err_msg)
   

   ---

   ---

   

   robot-dreams commented at 6:15 pm on August 29, 2020:

   Just confirming, is it OK to call assert_start_raises_init_error when the node is already started?

   For example, if I pass a valid blockfilterindex instead,

   self.nodes[0].assert_start_raises_init_error(['-blockfilterindex=basic', '-peerblockfilters'], err_msg)
   

   I thought I would get “test failed because node started successfully”.

   But instead, I get “test failed because expected error didn’t match”:

   AssertionError: [node 0] Expected message "Error: Cannot set -peerblockfilters without -blockfilterindex." does not fully match stderr:
   "Error: Cannot obtain a lock on data directory /var/folders/2b/58wl974967710d36y5sb2_440000gn/T/bitcoin_func_test_zz7jcluz/node0/regtest. Bitcoin Core is probably already running."

   ---

   

   jonatack commented at 9:39 am on October 4, 2020:

   I think it’s ok because (a) it’s the same as what I see when testing these manually, (b) this also tests that the args are checked first, and (c) it avoids needlessly stopping and restarting the node. Per test/functional/README.md, Avoid stop-starting the nodes multiple times during the test if possible. A stop-start takes several seconds, so doing it several times blows up the runtime of the test.
9. 
   robot-dreams commented at 6:23 pm on August 29, 2020: contributor

   Concept ACK
10. 
    jonatack commented at 12:09 pm on October 2, 2020: member

    Up for grabs.
11. jonatack closed this on Oct 2, 2020

    ---

12. amitiuttarwar added the label Up for grabs on Oct 2, 2020
13. 
    adamjonas commented at 4:25 pm on June 1, 2021: member

    See #20074 for a follow-up attempt.
14. MarcoFalke removed the label Up for grabs on Mar 31, 2022
15. DrahtBot locked this on Mar 31, 2023

Contributors
jonatack practicalswift theStack robot-dreams adamjonas

Labels
Tests