test: p2p_blockfilters tests for BIP157 config args

robot-dreams commented at 7:01 pm on October 3, 2020: contributor

Based on #19720

Add tests for BIP157 configuration options in p2p_blockfilters as mentioned in #19070 (review). 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.

- add tests for BIP157 configuration options
- test with tighter extra args
- fixup the message names
- clean up log messages

1a3f92da41

DrahtBot commented at 8:27 pm on October 3, 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:

#19967 (test: Replace (dis)?connect_nodes globals with TestFramework methods by robot-dreams)

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.

DrahtBot added the label Tests on Oct 3, 2020

in test/functional/p2p_blockfilters.py:67 in 1a3f92da41

64+        err_msg = 'Error: Unknown -blockfilterindex value foo.'
65+        self.nodes[0].assert_start_raises_init_error(['-blockfilterindex=foo', '-peerblockfilters'], err_msg)
66+
67+        # Restart and restore default "chain" topology
68+        self.start_node(0, extra_args=self.extra_args[0])
69+        self.connect_nodes(1, 0)

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

ISTM this stopping, restarting, and connecting can be avoided per https://github.com/bitcoin/bitcoin/pull/19720/files#r499226085.

in test/functional/p2p_blockfilters.py:61 in 1a3f92da41

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

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

nit: here, above line 57, and below line 212, I prefer the less-verbose “Test…” over “Check that…” and it’s also probably the most frequently used term in the functional test logging. It’s subjective though, apart from being less long.

in test/functional/p2p_blockfilters.py:95 in 1a3f92da41

91@@ -78,7 +92,7 @@ def run_test(self):
92         assert int(self.nodes[0].getnetworkinfo()['localservices'], 16) & NODE_COMPACT_FILTERS != 0
93         assert int(self.nodes[1].getnetworkinfo()['localservices'], 16) & NODE_COMPACT_FILTERS == 0
94 
95-        self.log.info("get cfcheckpt on chain to be re-orged out.")
96+        self.log.debug("get cfcheckpt on chain to be re-orged out.")

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

pico-nit, if you retouch maybe (a) s/get/Get/ and (b) can remove the EOL full stop per most frequent practice in the functional test logging

in test/functional/p2p_blockfilters.py:212 in 1a3f92da41

208@@ -195,7 +209,7 @@ def run_test(self):
209         computed_cfhash = uint256_from_str(hash256(cfilter.filter_data))
210         assert_equal(computed_cfhash, stale_cfhashes[999])
211 
212-        self.log.info("Requests to node 1 without NODE_COMPACT_FILTERS results in disconnection.")
213+        self.log.info("Check that requests without NODE_COMPACT_FILTERS results in disconnection.")

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

nit, here and line 106, can remove the full stop while changing this

jonatack commented at 9:52 am on October 4, 2020: member

Concept ACK, thanks for picking this up.

jonatack commented at 9:59 am on October 4, 2020: member

Just found an odd edge case in manual testing when the node is already up:

0$ ./src/bitcoind -blockfilterindex=1
1Error: Cannot obtain a lock on data directory /###/.bitcoin. Bitcoin Core is probably already running.
2
3$ ./src/bitcoind -blockfilterindex=basic
4Error: Unknown -blockfilterindex value 1.

Edit: the cause was setting blockfilterindex=1 in bitcoin.conf, removing that line then yields:

0$ ./src/bitcoind -blockfilterindex=basic
1Error: Cannot obtain a lock on data directory /###/.bitcoin. Bitcoin Core is probably already running.

jonatack commented at 11:15 am on October 4, 2020: member

Seems reproducible:

0self.log.info('Test passing two valid -blockfilter types raises init error')
1err_msg = 'Error: Unknown -blockfilterindex value basic -blockfilterindex=1.'
2self.nodes[0].assert_start_raises_init_error(['-blockfilterindex=basic -blockfilterindex=1', '-peerblockfilters'], err_msg)
3err_msg = 'Error: Unknown -blockfilterindex value basic -blockfilterindex=basic.'
4self.nodes[0].assert_start_raises_init_error(['-blockfilterindex=basic -blockfilterindex=basic', '-peerblockfilters'], err_msg)
5err_msg = 'Error: Unknown -blockfilterindex value 1 -blockfilterindex=basic.'
6self.nodes[0].assert_start_raises_init_error(['-blockfilterindex=1 -blockfilterindex=basic', '-peerblockfilters'], err_msg)
7err_msg = 'Error: Unknown -blockfilterindex value 1 -blockfilterindex=1.'
8self.nodes[0].assert_start_raises_init_error(['-blockfilterindex=1 -blockfilterindex=1', '-peerblockfilters'], err_msg)
9eerblockfilters'], err_msg)

theStack commented at 1:12 pm on October 4, 2020: member

May I ask what is the point of closing a PR with “Up for grabs” without even addressing its review comments? From my understanding the “Up for grabs” label is mainly used if a PR has been abandoned for a long time and the original author is not actively working on it anymore (that was not the case for #19720, the PR was not even 2 months old, had one ACK and unaddressed review comments).

jonatack commented at 1:43 pm on October 4, 2020: member

A combination of it not being on my priority list right now, not being merged despite favorable review (thank you), and my preference to not encumber the review stack with more than a few open PRs.

DrahtBot commented at 1:31 pm on October 21, 2020: member

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

DrahtBot added the label Needs rebase on Oct 21, 2020

theStack commented at 6:24 pm on January 22, 2021: member

@robot-dreams: Are you still available/interested to work on this PR? Happy to review as soon as the necessary rebase happens.

adamjonas commented at 4:24 pm on June 1, 2021: member

Checked-in with @robot-dreams and he does not intend on moving forward with this. Closing.

adamjonas closed this on Jun 1, 2021

adamjonas added the label Up for grabs on Jul 20, 2021

MarcoFalke removed the label Up for grabs on Mar 31, 2022

MarcoFalke commented at 2:37 pm on March 31, 2022: member

Picked up in https://github.com/bitcoin/bitcoin/pull/24698

DrahtBot locked this on Mar 31, 2023

test: p2p_blockfilters tests for BIP157 config args #20074

Conflicts