This PR adds test coverage for the field localaddresses for getnetworkinfo. In this case, it verifies if this field is empty for all nodes since they are using proxy.
Reference: https://github.com/bitcoin/bitcoin/blob/515200298b555845696a07ae2bc0a84a5dd02ae4/src/init.cpp#L449
It’s an interesting PR and deserves a concept ACK if the claim in the description is accurate. However, I could not find any credible source to verify that a node using proxy would not (or cannot) have local addresses. It would be pretty helpful to share some readings regarding this topic.
.
tACK 458edba68b872d528ece63c6835c8997e6f1fdbf.
I verified that nodes do not have local addresses by default if using -proxy: you must explicitly specify -discover for this. To verify that, I compared the logs of bitcoind -discover -proxy=127.0.0.1:10080 and bitcoind -proxy=127.0.0.1:10080.
Also stated here as brunoerg mentioned: https://github.com/bitcoin/bitcoin/blob/515200298b555845696a07ae2bc0a84a5dd02ae4/src/init.cpp#L449
Thus, adding -discover to a node using a proxy makes the test fail.
233 | @@ -234,7 +234,13 @@ def networks_dict(d): 234 | return r 235 | 236 | self.log.info("Test RPC getnetworkinfo") 237 | - n0 = networks_dict(self.nodes[0].getnetworkinfo()) 238 | + nodes_network_info = [] 239 | + for node in self.nodes:
I think adding documentation as follows would be helpful for future readers of this test.
self.log.info("Test RPC getnetworkinfo")
nodes_network_info = []
+
+ self.log.debug("Test that setting -proxy disables local address discovery, i.e. -discover=0")
for node in self.nodes:
network_info = node.getnetworkinfo()
assert_equal(network_info["localaddresses"], [])
I agree, going to add it!
done!
Thus, adding
-discoverto a node using a proxy makes the test fail.
Perhaps worth adding a test case for this.
Thus, adding
-discoverto a node using a proxy makes the test fail.Perhaps worth adding a test case for this.
I am not sure if it fails, are you? I didn't find anything pointing it should fail.
Force-pushed addressing #24258 (review)
Thus, adding
-discoverto a node using a proxy makes the test fail.Perhaps worth adding a test case for this.
I am not sure if it fails, are you? I didn't find anything pointing it should fail.
Yes adding -discover to the following args makes the test fail because the field localaddresses of the node now gets filled, at least on my machine.
https://github.com/bitcoin/bitcoin/blob/94039022de2fe47e418b2819eed48c43f4fb205b/test/functional/feature_proxy.py#L103
Yes adding
-discoverto the following args makes the test fail because the fieldlocaladdressesof the node now gets filled, at least on my machine.
Make sense, but I don't think we should cover it in this PR because we can't ensure localaddresses is filled even if we set -discover (and it could cause intermittent failures), I can only 100% ensure that not setting -discover and setting a proxy, it must be empty.
Yes adding
-discoverto the following args makes the test fail because the fieldlocaladdressesof the node now gets filled, at least on my machine.Make sense, but I don't think we should cover it in this PR because we can't ensure
localaddressesis filled even if we set-discover(and it could cause intermittent failures), I can only 100% ensure that not setting-discoverand setting a proxy, it must be empty.
Yes I agree this was my thought as well.
268 | @@ -261,7 +269,7 @@ def networks_dict(d): 269 | assert_equal(n1['i2p']['proxy_randomize_credentials'], False) 270 | assert_equal(n1['i2p']['reachable'], True) 271 | 272 | - n2 = networks_dict(self.nodes[2].getnetworkinfo()) 273 | + n2 = networks_dict(nodes_network_info[2])
Approach ACK, is there a reason to not use nodes_network_info for n3 and n4?
@ -285,7 +285,7 @@ class ProxyTest(BitcoinTestFramework):
assert_equal(n2['cjdns']['reachable'], False)
if self.have_ipv6:
- n3 = networks_dict(self.nodes[3].getnetworkinfo())
+ n3 = networks_dict(nodes_network_info[3])
assert_equal(NETWORKS, n3.keys())
for net in NETWORKS:
if net == NET_I2P or net == NET_ONION:
@@ -298,7 +298,7 @@ class ProxyTest(BitcoinTestFramework):
assert_equal(n3['i2p']['reachable'], False)
assert_equal(n3['cjdns']['reachable'], False)
- n4 = networks_dict(self.nodes[4].getnetworkinfo())
+ n4 = networks_dict(nodes_network_info[4])
assert_equal(NETWORKS, n4.keys())
for net in NETWORKS:
if net == NET_I2P:
No, I forgot to change them. Force-pushed addressing it. Thank you so much!
If you're interested to add test coverage of -discover in another PR, it might be useful to look at test/functional/feature_bind_port_discover.py.
If you're interested to add test coverage of -discover in another PR, it might be useful to look at test/functional/feature_bind_port_discover.py.
I opened a PR that covers it. See #24269. I didn't know about feature_bind_port_discover.py, I will take a look. Thanks
ACK 89bb25d22a0e1c700dba4e3b754984c9b2b14836
