init, test: improve network reachability test coverage and safety

jonatack commented at 10:03 pm on January 30, 2022: member

Adds missing network reachability test coverage and an assertion during init, noticed while reviewing #22834:

assert during init that each network reachability is true by default
add CJDNS to the LimitedAndReachable_Network unit tests
hoist proxy out of two network loops in feature_proxy.py
test that passing invalid -proxy raises expected init error
test that passing invalid -onion raises expected init error
test that passing -onlynet=onion without -proxy and -onion raises expected init error
test that passing -onlynet=onion with -onion=0 and with -noonion raises expected init error

DrahtBot added the label Tests on Jan 30, 2022

DrahtBot commented at 8:44 am on January 31, 2022: 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:

#23531 (Add Yggdrasil support by prusnak)

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.

vasild approved

vasild commented at 12:53 pm on January 31, 2022: member

ACK d1d04469a96d15b4d0a30881cbe6760d916e0762

jonatack commented at 1:30 pm on January 31, 2022: member

Note, this pull has a trivial one-line test conflict with #22834 that IMO should be reviewed/merged first so this pull doesn’t invalidate the review there.

in src/init.cpp:1303 in d1d04469a9 outdated

1316@@ -1317,6 +1317,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1317         }
1318         for (int n = 0; n < NET_MAX; n++) {
1319             enum Network net = (enum Network)n;

PastaPastaPasta commented at 12:41 pm on February 1, 2022:

please convert this c-style cast to a static_cast, since you are touching the line below

0            enum Network net = static_cast<enum Network>(n);

jonatack commented at 10:14 pm on March 1, 2022:

Thanks, but this is unrelated to the changes here and I prefer to not touch it.

DrahtBot added the label Needs rebase on Mar 1, 2022

net, init: assert each network reachability is true by default

The default network reachability values are implicitly set
by this line in net.cpp:

static bool vfLimited[NET_MAX] GUARDED_BY(g_maplocalhost_mutex) = {};

This commit asserts that each network is reachable during
the first loop through them during bitcoind init.

2b7a8180a9

test: add CJDNS to LimitedAndReachable_Network unit tests afdf2de282

test: hoist proxy out of 2 network loops in feature_proxy.py bd57dcbaf2

jonatack force-pushed on Mar 1, 2022

jonatack commented at 10:09 pm on March 1, 2022: member

Rebased following the merge of #22834 and added further test coverage and doc follow-ups to that pull.

DrahtBot removed the label Needs rebase on Mar 1, 2022

in src/init.cpp:465 in 2dc734f3cf outdated

461@@ -462,7 +462,7 @@ void SetupServerArgs(ArgsManager& argsman)
462     argsman.AddArg("-onion=<ip:port>", "Use separate SOCKS5 proxy to reach peers via Tor onion services, set -noonion to disable (default: -proxy)", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
463     argsman.AddArg("-i2psam=<ip:port>", "I2P SAM proxy to reach I2P peers and accept I2P connections (default: none)", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
464     argsman.AddArg("-i2pacceptincoming", "If set and -i2psam is also set then incoming I2P connections are accepted via the SAM proxy. If this is not set but -i2psam is set then only outgoing connections will be made to the I2P network. Ignored if -i2psam is not set. Listening for incoming I2P connections is done through the SAM proxy, not by binding to a local address and port (default: 1)", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
465-    argsman.AddArg("-onlynet=<net>", "Make automatic outgoing connections only through network <net> (" + Join(GetNetworkNames(), ", ") + "). Incoming connections are not affected by this option. This option can be specified multiple times to allow multiple networks.", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
466+    argsman.AddArg("-onlynet=<net>", "Make automatic outbound connections only through network <net> (" + Join(GetNetworkNames(), ", ") + "). Inbound and manual connections are not affected by this option. This option can be specified multiple times to allow multiple networks.", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);

vasild commented at 12:56 pm on March 3, 2022:

nit: “only through network”… hmm, what about making connections to IPv4 addresses through the Tor network? Could this be misinterpreted as “onlynet=onion will also make connections to IPv4 as long as it is through Tor”?

Is “only to network” better wording?

jonatack commented at 2:13 pm on March 3, 2022:

I’m going to propose this documentation in another pull for potential backport to v23 if it doesn’t make the branch-off (and drop it here); will look at adding your suggestion there.

jonatack commented at 2:47 pm on March 3, 2022:

nit: “only through network”… hmm, what about making connections to IPv4 addresses through the Tor network? Could this be misinterpreted as “onlynet=onion will also make connections to IPv4 as long as it is through Tor”?

Is “only to network” better wording?

Agreed.

jonatack commented at 2:48 pm on March 3, 2022:

and we already use “to” here in doc/tor.md and doc/i2p.md, so good call.

jonatack commented at 3:21 pm on March 3, 2022:

Done in #24468

in test/functional/feature_proxy.py:320 in 2dc734f3cf outdated

315+        msg = "Error: Invalid -proxy address or hostname: 'abc:def'"
316+        self.nodes[1].assert_start_raises_init_error(expected_msg=msg)
317+
318+        self.log.info("Test passing invalid -onion raises expected init error")
319+        self.nodes[1].extra_args = ["-onion=xyz:123"]
320+        msg = "Error: Invalid -onion address or hostname: 'xyz:123'"

vasild commented at 1:03 pm on March 3, 2022:

xyz:123 could be valid if xyz resolves. For example, echo "127.0.0.1 xyz" >> /etc/hosts bricks the test. Better use non-numeric port like in the -proxy test.

jonatack commented at 2:11 pm on March 3, 2022:

Done

jonatack commented at 2:21 pm on March 3, 2022:

(Good point!)

in test/functional/feature_proxy.py:277 in 2dc734f3cf outdated

272             if net == NET_I2P:
273                 expected_proxy = ''
274                 expected_randomize = False
275             else:
276-                expected_proxy = f'{self.conf2.addr[0]}:{self.conf2.addr[1]}'
277+                expected_proxy = proxy

vasild commented at 1:07 pm on March 3, 2022:

nit: this change looks unnecessary because the expression is used just once, so putting it in a variable does not make it more maintainable. It maybe makes it less readable because now one has to visually go up a few lines in order to find what expected_proxy ends up with.

jonatack commented at 2:14 pm on March 3, 2022:

This line is invoked 4 times in the loop, so it seems better to do the more complex lookups and string interpolation before entering it (I would be surprised if Python is auto-optimising this as it’s an interpreted language).

vasild commented at 2:37 pm on March 3, 2022:

Oh, performance :eyes: :space_invader:

in test/functional/feature_proxy.py:290 in 2dc734f3cf outdated

288             for net in NETWORKS:
289-                if net == NET_I2P or net == NET_ONION:
290-                    expected_proxy = ''
291-                else:
292-                    expected_proxy = f'[{self.conf3.addr[0]}]:{self.conf3.addr[1]}'
293+                expected_proxy = '' if net == NET_I2P or net == NET_ONION else proxy

vasild commented at 1:09 pm on March 3, 2022:

nit: this looks unnecessary coz the expression is used just once. Also, personally, I find the perl-style harder to read: x=1 if some_condition else 2.

I am fine as it is, but I will be happier if commit test: hoist proxy out of 2 network loops in feature_proxy.py is removed :)

jonatack commented at 2:17 pm on March 3, 2022:

The more complex lookups and string interpolation are currently invoked three times in the loop, so it seems better to do them once before entering it. The ternary is used frequently in our Python tests and I think it’s more succinct here to use the ternary form for an assignment instead of if - else with two assignments.

vasild commented at 1:12 pm on March 3, 2022: member

Approach ACK

test: passing invalid -proxy raises expected init error d5edb08708

test: passing invalid -onion raises expected init error 8332e6e4cf

test: passing -onlynet=onion without -proxy/-onion raises expected init error 7000f66d36

test: passing -onlynet=onion with -onion=0/-noonion raises expected init error 58a14795b8

jonatack force-pushed on Mar 3, 2022

vasild approved

vasild commented at 2:36 pm on March 3, 2022: member

ACK 58a14795b89a6bd812e0b71cb8b3088b8ab55c11

laanwj referenced this in commit f6d335e828 on Mar 7, 2022

sidhujag referenced this in commit c561f58099 on Mar 7, 2022

jonatack renamed this:
~~net, init, test: network reachability testing and safety improvements~~
init, test: improve network reachability test coverage and safety
on Mar 17, 2022

brunoerg approved

brunoerg commented at 4:56 pm on March 21, 2022: member

ACK 58a14795b89a6bd812e0b71cb8b3088b8ab55c11

Checked the following init errors are now covered in functional tests: https://github.com/bitcoin/bitcoin/blob/91d12344b1e51809c1ef6b630b631a6da00267c3/src/init.cpp#L1333 https://github.com/bitcoin/bitcoin/blob/91d12344b1e51809c1ef6b630b631a6da00267c3/src/init.cpp#L1352 https://github.com/bitcoin/bitcoin/blob/91d12344b1e51809c1ef6b630b631a6da00267c3/src/init.cpp#L1363

Code looks great for both tests. Just ran them several times and got no error.

jonatack commented at 1:17 pm on March 24, 2022: member

I think this has been ready for a few weeks now, if anyone would like to do a final review or merge.

dongcarl approved

dongcarl commented at 7:58 pm on March 24, 2022: member

Code Review ACK 58a14795b89a6bd812e0b71cb8b3088b8ab55c11

Change seems straightforward. Had to convince myself of the assert in src/init.cpp, but it seems right that we don’t want users to specify -onlynet=foo and foo be somehow default disabled, and now bitcoind is silently operating without foo.

MarcoFalke merged this on Mar 24, 2022

MarcoFalke closed this on Mar 24, 2022

michaelfolkson commented at 1:19 pm on March 26, 2022: contributor

Post-merge ACK 58a14795b89a6bd812e0b71cb8b3088b8ab55c11

Built on MacOS Big Sur, reviewed code changes and checked the functional test was covering the errors in the PR description.

jonatack deleted the branch on Mar 26, 2022

sidhujag referenced this in commit 94a2f75119 on Apr 2, 2022

DrahtBot locked this on Mar 26, 2023

init, test: improve network reachability test coverage and safety #24205

Conflicts