This PR adds a functional test for -discover. It tests different scenarios where localaddresses should be empty or may contain the addresses. Obs: localaddresses is not always accurate, so it's not possible to ensure (100%) it will contain any addresses.
https://github.com/bitcoin/bitcoin/blob/515200298b555845696a07ae2bc0a84a5dd02ae4/src/init.cpp#L449
Obs: See #24258 - It adds test coverage for this field but for nodes with proxy.
cirrus task: https://cirrus-ci.com/build/5675560427847680 commit: https://github.com/RandyMcMillan/bitcoin/commit/b41f962f349ab3d3f8a4d8e761ac5592120f8576
We cover an array of possible command line combinations:
Concept ACK
37 | + self.log.info(f"Validating {address}") 38 | + valid = (is_valid_ipv4_address(address) 39 | + or is_valid_ipv6_address(address)) 40 | + assert_equal(valid, True) 41 | + 42 | + def test_local_addresses(self, test_case, should_be_empty=False):
nit, maybe use named args (for all, or after the first one), maybe better to omit a default value if you're passing values in for clarity, and naming suggestion
- def test_local_addresses(self, test_case, should_be_empty=False):
+ def test_local_addresses(self, test_case, *, expect_empty=False):
...
for test_case in test_cases:
- self.test_local_addresses(test_case, False)
+ self.test_local_addresses(test_case, expect_empty=False)
for test_case in test_cases_empty:
- self.test_local_addresses(test_case, True)
+ self.test_local_addresses(test_case, expect_empty=True)
Great, done!
58 | + ["-listen=1", "-discover=1"], 59 | + ["-discover"]] 60 | + 61 | + test_cases_empty = [["-discover=0"], 62 | + ["-listen", "-discover=0"], 63 | + []]
formatting nit
def run_test(self):
- test_cases = [["-listen", "-discover"],
- ["-listen=1", "-discover"],
- ["-listen", "-discover=1"],
- ["-listen=1", "-discover=1"],
- ["-discover"]]
-
- test_cases_empty = [["-discover=0"],
- ["-listen", "-discover=0"],
- []]
+ test_cases = [
+ ["-listen", "-discover"],
+ ["-listen=1", "-discover"],
+ ["-listen", "-discover=1"],
+ ["-listen=1", "-discover=1"],
+ ["-discover"],
+ ]
+
+ test_cases_empty = [
+ ["-discover=0"],
+ ["-listen", "-discover=0"],
+ [],
+ ]
Cool, done!
47 | + if n['reachable'] and n['name'] in ['ipv4', 'ipv6']] 48 | + local_addrs = list(network_info["localaddresses"]) 49 | + if should_be_empty or not network_enabled: 50 | + assert_equal(local_addrs, []) 51 | + elif len(local_addrs) > 0: 52 | + self.validate_addresses(local_addrs)
On my system (Debian 5.16.14-1 (2022-03-15) x86_64 GNU/Linux) this is never hit and so validate_addresses(), is_valid_ipv4_address() and is_valid_ipv6_address() are all unused. Is it ever hit for you?
(maybe have a look at test/functional/feature_bind_port_discover.py and add the tests there, if helpful)
Yes, it hits here. Example:
2022-03-24T17:37:48.362000Z TestFramework (INFO): Restart node with ['-listen', '-discover']
2022-03-24T17:37:49.181000Z TestFramework (INFO): Validating 2804:14d:5281:8ae2:c84:4731:10d6:7311
2022-03-24T17:37:49.181000Z TestFramework (INFO): Validating 2804:14d:5281:8ae2:1c3e:c153:e75f:59c3
2022-03-24T17:37:49.181000Z TestFramework (INFO): Validating 2804:14d:5281:8ae2:3132:b83c:1915:c06e
2022-03-24T17:37:49.181000Z TestFramework (INFO): Validating 2804:14d:5281:8ae2:4152:dcc5:5862:734
2022-03-24T17:37:49.181000Z TestFramework (INFO): Validating 2804:14d:5281:8ae2:4d52:b4d3:d93d:8f8e
2022-03-24T17:37:49.181000Z TestFramework (INFO): Validating 2804:14d:5281:8ae2:5594:b545:6081:a40b
2022-03-24T17:37:49.181000Z TestFramework (INFO): Validating 2804:14d:5281:8ae2:f97b:bd76:25aa:a0d3
I researched about localaddresses and noticed it's not accurate, this is a reason I don't ensure in the test that localaddresses MUST be filled. See: https://bitcoin.stackexchange.com/questions/74991/when-ipv6-is-enabled-my-ipv4-address-does-not-appear-in-getnetworkinfo-localaddr
On which platform/OS?
MacOS 12.0.1 Monterey
I edited the last comment because the link was wrong haha.
Please, try again with the new changes.
Concept ACK, thanks for working to improve the network test coverage.
56 | + ["-listen", "-discover"], 57 | + ["-listen=1", "-discover"], 58 | + ["-listen", "-discover=1"], 59 | + ["-listen=1", "-discover=1"], 60 | + ["-discover"], 61 | + ]
Are there any other occasions where we test the same behavior for different variations of an argument? Testing with combinations of -discover, -discover=1 or -listen, -listen=1 seems redundant. Taking that into consideration, I think that this test could be replaced with just a test case for discover=0 at test/functional/feature_bind_port_discover.py.
It was suggested by other reviewer, but I agree with you, there is no need to test redundant cases.
Done!
27 | + 28 | + 29 | +class DiscoverTest(BitcoinTestFramework): 30 | + def set_test_params(self): 31 | + self.setup_clean_chain = True 32 | + self.num_nodes = 1
On my system (MacOS 10.15.7) localaddresses is always empty. Which is a different behavior from what I see at test/functional/feature_bind_port_discover.py where localaddresses is accurate.
What I think is the problem is that without explicitly set bind_to_localhost_only = False the node binds to localhost which is NET_UNROUTABLE thus never added to mapLocalHost which is what is returned for localaddresses by the getnetworkinfo RPC call. With this change, localaddresses always contains my addresses as expected.
def set_test_params(self):
self.setup_clean_chain = True
self.bind_to_localhost_only = False
self.num_nodes = 1
But I still don't understand how this works for others, so there must be a flaw in my thinking.
Make sense, bind_to_localhost_onlyis True by default. thanks!
ACK c80df01cf19d702b94a3e7a883fcf5f400289586 🚀
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
No conflicts as of last run.
60 | + 61 | + test_cases_empty = [ 62 | + ["-discover=0"], 63 | + ["-listen", "-discover=0"], 64 | + [], 65 | + ]
It seems for both these cases local address list is empty. Is that expected behavior?
I get a list of addresses only for ipv6. Testing on macOS.
It depends on the specifics of your network. In some cases, the -discover functionality is able to find local addresses, in other cases it isn't. I think that's why this test doesn't assert that anything is found, just that if something is found, it is valid (validate_addresses).
Perfect, @mzumsande
It seems that this is a common point of confusion between reviewers of this PR. Further explanation has been provided in multiple places in this PR but maybe you should consider adding additional comments for people reading the code in the future?
A comment saying "the resultant network list is dependent on local configurations" or something similar can be helpful..
tACK c80df01cf19d702b94a3e7a883fcf5f400289586
Just one question..
66 | + 67 | + for test_case in test_cases: 68 | + self.test_local_addresses(test_case, expect_empty=False) 69 | + 70 | + for test_case in test_cases_empty: 71 | + self.test_local_addresses(test_case, expect_empty=False)
Shouldn't expect_empty be True for the test_cases_empty cases? If it's False for all calls to test_local_addresses, it doesn't really make sense to split up the test cases test_cases and test_cases_empty or have a expect_empty argument in the first place.
+1 Have the same question..
My mistake, it should be expect_empty=True
Concept ACK
Force-pushed addressing #24269 (review), thanks @mzumsande and @rajarshimaitra
Code review ACK bff05bd7456d3634b0c83539293a753db6d76376
43 | + def test_local_addresses(self, test_case, *, expect_empty=False): 44 | + self.log.info(f"Restart node with {test_case}") 45 | + self.restart_node(0, test_case) 46 | + network_info = self.nodes[0].getnetworkinfo() 47 | + network_enabled = [n for n in network_info['networks'] 48 | + if n['reachable'] and n['name'] in ['ipv4', 'ipv6']]
What are the scenarios where the network is not available? It has to do with the configuration of the machine doing the test?
Yes, it has to do with the configuration of the machine doing it. That's the reason I validate the addresses only whether it's available.
tACK bff05bd7456d3634b0c83539293a753db6d76376
ACK bff05bd7456d3634b0c83539293a753db6d76376
