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

jonatack · 2022-01-30T22:03:17Z

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 · 2022-01-31T08:44:13Z

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

ACK d1d04469a96d15b4d0a30881cbe6760d916e0762

jonatack · 2022-01-31T13:30:10Z

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.

src/init.cpp

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.

jonatack · 2022-03-01T22:09:46Z

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

vasild

Approach ACK

src/init.cpp

test/functional/feature_proxy.py

…it error

…nit error

vasild

ACK 58a1479

…ated tor/i2p documentation a1db99a init, doc: improve -onlynet help and tor/i2p documentation (Jon Atack) Pull request description: including review feedback from bitcoin/bitcoin#22834 (comment) and bitcoin/bitcoin#24205 (comment) concerning `src/init.cpp`, `doc/tor.md` and `doc/i2p.md` - s/outgoing/automatic outbound/ - s/Incoming/Inbound and manual/ (are not affected by this option.) - s/only through network/only to network/ - s/this option. This option/this option. It/ - s/network types/networks/ and pick up a few nits in `doc/p2p-bad-ports.md` from bitcoin/bitcoin#23542 (review). ACKs for top commit: laanwj: ACK a1db99a w0xlt: ACK a1db99a theStack: ACK a1db99a Tree-SHA512: dd727904b9b3dadb16053e2b0350e6c0814ef68fb0cca7d34880b883123cfe3aa03b15813b40a863f6367d596d17ee4517eab55281cfe35cd00767b8a39593ca

…/i2p documentation a1db99a init, doc: improve -onlynet help and tor/i2p documentation (Jon Atack) Pull request description: including review feedback from bitcoin#22834 (comment) and bitcoin#24205 (comment) concerning `src/init.cpp`, `doc/tor.md` and `doc/i2p.md` - s/outgoing/automatic outbound/ - s/Incoming/Inbound and manual/ (are not affected by this option.) - s/only through network/only to network/ - s/this option. This option/this option. It/ - s/network types/networks/ and pick up a few nits in `doc/p2p-bad-ports.md` from bitcoin#23542 (review). ACKs for top commit: laanwj: ACK a1db99a w0xlt: ACK a1db99a theStack: ACK a1db99a Tree-SHA512: dd727904b9b3dadb16053e2b0350e6c0814ef68fb0cca7d34880b883123cfe3aa03b15813b40a863f6367d596d17ee4517eab55281cfe35cd00767b8a39593ca

brunoerg

ACK 58a1479

Checked the following init errors are now covered in functional tests:

bitcoin/src/init.cpp

Line 1333 in 91d1234

    
           return InitError(strprintf(_("Invalid -proxy address or hostname: '%s'"), proxyArg));

bitcoin/src/init.cpp

Line 1352 in 91d1234

    
           return InitError(strprintf(_("Invalid -onion address or hostname: '%s'"), onionArg));

bitcoin/src/init.cpp

Line 1363 in 91d1234

_("Outbound connections restricted to Tor (-onlynet=onion) but the proxy for "

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

jonatack · 2022-03-24T13:17:22Z

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

dongcarl

Code Review ACK 58a1479

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.

michaelfolkson · 2022-03-26T13:19:55Z

Post-merge ACK 58a1479

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

…verage and safety 58a1479 test: passing -onlynet=onion with -onion=0/-noonion raises expected init error (Jon Atack) 7000f66 test: passing -onlynet=onion without -proxy/-onion raises expected init error (Jon Atack) 8332e6e test: passing invalid -onion raises expected init error (Jon Atack) d5edb08 test: passing invalid -proxy raises expected init error (Jon Atack) bd57dcb test: hoist proxy out of 2 network loops in feature_proxy.py (Jon Atack) afdf2de test: add CJDNS to LimitedAndReachable_Network unit tests (Jon Atack) 2b7a818 net, init: assert each network reachability is true by default (Jon Atack) Pull request description: Adds missing network reachability test coverage and an assertion during init, noticed while reviewing bitcoin#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 ACKs for top commit: vasild: ACK 58a1479 brunoerg: ACK 58a1479 dongcarl: Code Review ACK 58a1479 Tree-SHA512: bdee6dd0c12bb63591ce7c9321fe77b509ab1265123054e774adc38a187746dddafe1627cbe89e990bcc78b45e194bfef8dc782710d5b217e2e2106ab0158827

jonatack mentioned this pull request Jan 30, 2022

net: respect -onlynet= when making outbound connections #22834

Merged

DrahtBot added the Tests label Jan 30, 2022

DrahtBot mentioned this pull request Jan 31, 2022

docs: avoid C-style casts; use modern C++ casts #23810

Merged

vasild approved these changes Jan 31, 2022

View reviewed changes

PastaPastaPasta reviewed Feb 1, 2022

View reviewed changes

src/init.cpp Outdated Show resolved Hide resolved

DrahtBot added the Needs rebase label Mar 1, 2022

jonatack added 3 commits March 1, 2022 21:03

test: add CJDNS to LimitedAndReachable_Network unit tests

afdf2de

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

bd57dcb

jonatack force-pushed the network-reachability-assertion-and-testing branch from d1d0446 to 2dc734f Compare March 1, 2022 22:01

DrahtBot removed the Needs rebase label Mar 1, 2022

DrahtBot mentioned this pull request Mar 2, 2022

Add Yggdrasil support #23531

Closed

vasild reviewed Mar 3, 2022

View reviewed changes

src/init.cpp Outdated Show resolved Hide resolved

test/functional/feature_proxy.py Outdated Show resolved Hide resolved

test/functional/feature_proxy.py Outdated Show resolved Hide resolved

test/functional/feature_proxy.py Outdated Show resolved Hide resolved

jonatack added 4 commits March 3, 2022 15:31

test: passing invalid -proxy raises expected init error

d5edb08

test: passing invalid -onion raises expected init error

8332e6e

test: passing -onlynet=onion without -proxy/-onion raises expected in…

7000f66

…it error

test: passing -onlynet=onion with -onion=0/-noonion raises expected i…

58a1479

…nit error

jonatack force-pushed the network-reachability-assertion-and-testing branch from 2dc734f to 58a1479 Compare March 3, 2022 14:34

vasild approved these changes Mar 3, 2022

View reviewed changes

jonatack mentioned this pull request Mar 3, 2022

init, doc: improve -onlynet help and related tor/i2p documentation #24468

Merged

jonatack changed the title ~~net, init, test: network reachability testing and safety improvements~~ init, test: improve network reachability test coverage and safety Mar 17, 2022

brunoerg approved these changes Mar 21, 2022

View reviewed changes

dongcarl approved these changes Mar 24, 2022

View reviewed changes

maflcko merged commit f0c9ba2 into bitcoin:master Mar 24, 2022

jonatack deleted the network-reachability-assertion-and-testing branch March 26, 2022 13:36

bitcoin locked and limited conversation to collaborators Mar 26, 2023

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

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

Uh oh!

Conversation

jonatack commented Jan 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jan 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack commented Jan 31, 2022

Uh oh!

Uh oh!

jonatack commented Mar 1, 2022

Uh oh!

vasild left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

brunoerg left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack commented Mar 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dongcarl left a comment

Choose a reason for hiding this comment

Uh oh!

michaelfolkson commented Mar 26, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

jonatack commented Jan 30, 2022 •

edited

Loading

DrahtBot commented Jan 31, 2022 •

edited

Loading

vasild left a comment •

edited

Loading

jonatack commented Mar 24, 2022 •

edited

Loading