tor: respect non-onion -onlynet= for outgoing Tor connections #22651

vasild · 2021-08-06T15:17:12Z

If

-onlynet= is given one or more times but none of them includes
onion and
-proxy is not set and
-onion is not set,

then we should not be making outbound connections to the Tor network.

Fixes #22647

vasild · 2021-08-06T15:17:28Z

See also https://en.wikipedia.org/wiki/Principle_of_least_astonishment

jonatack · 2021-08-06T15:32:22Z

Concept/Approach ACK. It may be good to have functional test coverage (or make it a separate function with unit tests).

Edit: updated the -onlynet documentation in bebcf78.

src/torcontrol.cpp

ghost · 2021-08-06T18:34:05Z

See also https://en.wikipedia.org/wiki/Principle_of_least_astonishment

This link says "The behavior should not astonish or surprise users"

I am surprised by lot of things in Bitcoin Core 😄

Concept ACK. Doesn't make sense to create outbound connections with onion peers if onlynet=i2p is used.

src/torcontrol.cpp

ghost · 2021-08-06T21:27:21Z

It may be good to have functional test coverage (or make it a separate function with unit tests).

Agree. Do you think these steps are correct for test?

Run Node 1:

bitcoind -port=18333 -rpcport=18222 -datadir="/home/user/node1" -regtest=1 -listen=1 -server=1 -debug=net -rpcuser=user1 -rpcpassword=password1 -torcontrol='127.0.0.1:9051' -proxy='127.0.0.1:9050' -onlynet=onion

Run Node 2:

bitcoind -port=18777 -rpcport=18666 -datadir="/home/user/node2" -regtest=1 -listen=1 -server=1 -debug=net -rpcuser=user2 -rpcpassword=password2 -addnode='127.0.0.1:18333' -torcontrol='127.0.0.1:9051' -proxy='127.0.0.1:9050' -onlynet=onion

Stop Node 1 and 2:

bitcoin-cli -rpcport=18222 -rpcuser=user1 -rpcpassword=password1 stop
bitcoin-cli -rpcport=18666 -rpcuser=user2 -rpcpassword=password2 stop

Restart Node 2 with different config (onlynet=i2p and no proxy):

bitcoind -port=18777 -rpcport=18666 -datadir="/home/user/node2" -regtest=1 -listen=1 -server=1 -debug=net -rpcuser=user2 -rpcpassword=password2 -i2psam=127.0.0.1:7656 -onlynet=i2p

Check debug.log:

trying connection 3llpwlxkisrm2iu5oayh2hd6df7q36wdmt6nounenzqcxkasxfk34eid.onion

Master branch should print such connection attempts and PR branch should not

jonatack · 2021-08-06T21:47:58Z

Checking the debug log output is a bit of a last resort when there's nothing else to assert on... getnetworkinfo limited/reachable (and maybe getpeerinfo) would probably be good.

DrahtBot · 2021-08-07T05:05:47Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#19358 (torcontrol : avoid to set wrong outbound proxy and network settings when creating an inbound onion service. by Saibato)
#15423 (torcontrol: Query Tor for correct -onion configuration by luke-jr)

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.

ghost · 2021-08-07T06:37:35Z

Would this fix also #13378 ?

ghost · 2021-08-07T21:20:30Z

Checking the debug log output is a bit of a last resort when there's nothing else to assert on... getnetworkinfo limited/reachable (and maybe getpeerinfo) would probably be good.

Also the steps I mentioned above don't work. They worked on one of my VMs. Maybe it already had some onion address in peers.dat. I was expecting it will add onion peer in peers.dat but it didn't. addpeeraddress with some onion address and port followed by getpeerinfo should work.

ghost · 2021-08-08T08:12:19Z

Wrote a PowerShell script to test this. Tried on Windows 10 and Pop!_OS. Node 2 is connected to Node 1 on Linux and fails on Windows (Master branch).

TL;DR

Run Node 1 and get onion URL for it.
Sleep 10 seconds
Get onion address for Node 1 from getnetworkinfo -> localaddresses -> address ending with .onion and its port
Run Node 2 with onlynet=i2p (No Tor proxy)
Sleep 10 seconds
Add Node 1 in peers.dat for Node 2 using hidden RPC addpeeraddress
Sleep 30 seconds
Check if Node 2 is connected to Node 1 with getpeerinfo

Peer info:

xejoddwvi35krze3546qtx6dfmednjs2ccsociyezkyhzdxtemp5v5ad.onion:18444
outbound-full-relay

vasild · 2021-08-09T09:15:52Z

Would this fix also #13378 ?

To my understanding, yes, but only if none of -proxy or -onion is set.

Providing both -onlynet=ipv4 -onion=127.0.0.1:9050 is kind of contradictory - why provide an outgoing tor proxy if you only want to connect to IPv4? I think Bitcoin Core should print an error and refuse to start in this case, but instead it treats it as "onlynet ipv4 or tor". Changing/fixing that is out of the scope of this PR which only aims to fix the behavior if none of -proxy or -onion is set.

ghost · 2021-08-09T11:22:35Z

Also of course out of this PR, just to mention: I am sure you are already aware that to have multiple "only" nets is paradox, the option should better be called e.g. "net" instead of "onlynet"..

vasild · 2021-08-11T07:20:54Z

4f4c7d8b11..4f3020d524: append a functional test to exercise the modified behavior

vasild · 2021-08-11T07:40:13Z

4f3020d524...00b7ba51eb: hush test/lint/lint-python.sh

vasild · 2021-08-11T07:46:11Z

00b7ba51eb...25f9e113db: change allow_outbound_onion to a function, as per suggestion

ghost · 2021-08-11T08:17:39Z

Introduce a basic TCP server in the functional testing framework and use
it to simulate a Tor Control daemon, which is needed in order for the
code in src/torcontrol.cpp to execute the code that is relevant for
-onlynet.

Interesting. Will try this today.

jonatack

Nice! A few suggestions for the test. Only skimmed the basic server so far.

test/functional/feature_onlynet.py

test/functional/test_framework/basic_server.py

test/functional/feature_onlynet.py

vasild · 2021-08-11T12:22:37Z

test_framework/basic_server.py can also be used to add some functional tests for I2P. We can simulate an I2P router at least as long as the text/line-based SAM is being talked on the socket. Later, when the socket switches from SAM to the binary bitcoin p2p protocol we would have to close the socket.

vasild · 2021-08-19T13:49:51Z

5959ece29e...a2ebcda19a: address some suggestions

@vasild, do you plan to update?

Done! :)

jonatack · 2021-08-19T14:53:08Z

Code review re-ACK a2ebcda per git diff 5959ece a2ebcda, rebase on master, debug build, re-ran test a few times, and previously manually verified the behavior extensively as described in #22651 (comment) and #22651 (comment)

Rspigler · 2021-08-19T22:45:57Z

Concept ACK

ghost · 2021-08-20T16:08:57Z

reACK a2ebcda

Major changes since last review:

…tests 0175977 Add I2P network SetReachable/IsReachable unit test assertions (Jon Atack) b87a9c4 Improve doc/i2p.md regarding I2P router options/versions (Jon Atack) bebcf78 Update i2p.md and tor.md regarding -onlynet config option (Jon Atack) Pull request description: This pull addresses #22634 (comment) and various user feedback/questions, updates the -onlynet documentation in doc/i2p.md and doc/tor.md per #22651 (src/init.cpp is already fine) and fills in some missing I2P unit test coverage. Note: this PR depends in part on whether #22651 is merged in order to propose the correct -onlynet documentation (it is currently aligned with the change in #22651), so that PR should be decided or merged first. ACKs for top commit: Rspigler: Re-ACK 0175977 prayank23: reACK 0175977 vasild: ACK 0175977 Tree-SHA512: ae606437522bfccdfb7508108cddc7dfede2385e30a0561dbd007b784ed2639962c28552eb0e9336412faa323637fe964c26b8d8fc6dcf9fc63734ac00d05736

laanwj · 2021-08-26T11:28:44Z

See also https://en.wikipedia.org/wiki/Principle_of_least_astonishment

To be honest this whole situation around onlynet gives me a headache: #22648 (review)

src/torcontrol.cpp

If * `-onlynet=` is given one or more times but none of them includes `onion` and * `-proxy` is not set and * `-onion` is not set, then we should not be making outbound connections to the Tor network. Fixes bitcoin#22647

Introduce a basic TCP server in the functional testing framework and use it to simulate a Tor Control daemon, which is needed in order for the code in `src/torcontrol.cpp` to execute the code that is relevant for `-onlynet`.

Do not make outbound connections to hosts which belong to a network which is restricted by `-onlynet`. This applies to hosts that are automatically chosen to connect to and to anchors. This does not apply to hosts given to `-connect`, `-addnode`, `addnode` RPC, dns seeds, `-seednodes`. Fixes bitcoin#13378 Fixes bitcoin#22647 Supersedes bitcoin#22651

vasild · 2021-08-30T13:16:01Z

a2ebcda19a...baa6cb12ac: address minor suggestion and fix commit id in a comment.

vasild · 2021-08-30T13:18:04Z

@laanwj, I agree with your comments at #22648 (comment). Opened #22834 which if merged will make this PR unnecessary.

IMO #22834 should be merged and this PR (#22651) closed without merge. But lets see what reviewers think.

jonatack · 2021-08-30T17:00:56Z

Code review re-ACK baa6cb1 per git diff a2ebcda baa6cb1, rebase on master, debug build, ran the new test test/functional/feature_onlynet.py a few times, previously manually verified the behavior extensively as described in #22651 (comment) and #22651 (comment)

Will review #22834 soon. Perhaps the functional test work here can be ~ported over to it if that change is preferred.

Do not make outbound connections to hosts which belong to a network which is restricted by `-onlynet`. This applies to hosts that are automatically chosen to connect to and to anchors. This does not apply to hosts given to `-connect`, `-addnode`, `addnode` RPC, dns seeds, `-seednodes`. Fixes bitcoin#13378 Fixes bitcoin#22647 Supersedes bitcoin#22651

luke-jr · 2021-09-14T16:44:40Z

#22834 looks like a better solution IMO

Do not make outbound connections to hosts which belong to a network which is restricted by `-onlynet`. This applies to hosts that are automatically chosen to connect to and to anchors. This does not apply to hosts given to `-connect`, `-addnode`, `addnode` RPC, dns seeds, `-seednodes`. Fixes bitcoin#13378 Fixes bitcoin#22647 Supersedes bitcoin#22651 Github-Pull: bitcoin#22834 Rebased-From: 0ea0de6

Talkless · 2021-11-06T14:00:47Z

#22834 looks like a better solution IMO

So this PR should be closed?

@vasild I'm confused :)

vasild · 2021-11-08T08:47:46Z

Closing this in favor of #22834 which has more (concept) ACKs.

Thanks, @Talkless!

Do not make outbound connections to hosts which belong to a network which is restricted by `-onlynet`. This applies to hosts that are automatically chosen to connect to and to anchors. This does not apply to hosts given to `-connect`, `-addnode`, `addnode` RPC, dns seeds, `-seednodes`. Fixes bitcoin#13378 Fixes bitcoin#22647 Supersedes bitcoin#22651

Do not make outbound connections to hosts which belong to a network which is restricted by `-onlynet`. This applies to hosts that are automatically chosen to connect to and to anchors. This does not apply to hosts given to `-connect`, `-addnode`, `addnode` RPC, dns seeds, `-seednodes`. Fixes bitcoin#13378 Fixes bitcoin#22647 Supersedes bitcoin#22651 Github-Pull: bitcoin#22834 Rebased-From: 0ea0de6

…und connections 0eea83a scripted-diff: rename `proxyType` to `Proxy` (Vasil Dimov) e53a850 net: respect -onlynet= when making outbound connections (Vasil Dimov) Pull request description: Do not make outbound connections to hosts which belong to a network which is restricted by `-onlynet`. This applies to hosts that are automatically chosen to connect to and to anchors. This does not apply to hosts given to `-connect`, `-addnode`, `addnode` RPC, dns seeds, `-seednode`. Fixes bitcoin/bitcoin#13378 Fixes bitcoin/bitcoin#22647 Supersedes bitcoin/bitcoin#22651 ACKs for top commit: naumenkogs: utACK 0eea83a prayank23: reACK bitcoin/bitcoin@0eea83a jonatack: ACK 0eea83a code review, rebased to master, debug built, and did some manual testing with various config options on signet Tree-SHA512: 37d68b449dd6d2715843fc84d85f48fa2508be40ea105a7f4a28443b318d0b6bd39e3b2ca2a6186f2913836adf08d91038a8b142928e1282130f39ac81aa741b

Do not make outbound connections to hosts which belong to a network which is restricted by `-onlynet`. This applies to hosts that are automatically chosen to connect to and to anchors. This does not apply to hosts given to `-connect`, `-addnode`, `addnode` RPC, dns seeds, `-seednodes`. Fixes bitcoin/bitcoin#13378 Fixes bitcoin/bitcoin#22647 Supersedes bitcoin/bitcoin#22651

vasild mentioned this pull request Aug 6, 2021

If both options "onion" and "proxy" are unset, no outbound Onion connections should be made #22647

Closed

DrahtBot added the P2P label Aug 6, 2021

jonatack reviewed Aug 6, 2021

View reviewed changes

src/torcontrol.cpp Outdated Show resolved Hide resolved

jonatack mentioned this pull request Aug 6, 2021

doc, test: improve i2p/tor docs and i2p reachable unit tests #22648

Merged

ghost reviewed Aug 6, 2021

View reviewed changes

src/torcontrol.cpp Show resolved Hide resolved

jonatack mentioned this pull request Aug 6, 2021

v22.0 testing #22634

Closed

ghost mentioned this pull request Aug 7, 2021

torcontrol: Query Tor for correct -onion configuration #15423

Merged

DrahtBot mentioned this pull request Aug 8, 2021

torcontrol : avoid to set wrong outbound proxy and network settings when creating an inbound onion service. #19358

Closed

ghost mentioned this pull request Aug 9, 2021

P2P: Node with onlynet=ipv6 connects outgoing to Onion #13378

Closed

vasild force-pushed the respect_onlynet_for_tor branch from 4f3020d to 00b7ba5 Compare August 11, 2021 07:39

vasild force-pushed the respect_onlynet_for_tor branch from 00b7ba5 to 25f9e11 Compare August 11, 2021 07:45

jonatack reviewed Aug 11, 2021

View reviewed changes

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

vasild force-pushed the respect_onlynet_for_tor branch from 25f9e11 to 16bf93e Compare August 11, 2021 12:48

vasild force-pushed the respect_onlynet_for_tor branch from 5959ece to a2ebcda Compare August 19, 2021 13:49

Talkless reviewed Aug 26, 2021

View reviewed changes

src/torcontrol.cpp Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Aug 30, 2021

net: remove reinit of the onion proxy #22830

Closed

vasild added 2 commits August 30, 2021 11:39

tor: respect non-onion -onlynet= for outgoing Tor connections

5384c98

If * `-onlynet=` is given one or more times but none of them includes `onion` and * `-proxy` is not set and * `-onion` is not set, then we should not be making outbound connections to the Tor network. Fixes bitcoin#22647

test: ensure -onlynet behaves as expected

baa6cb1

Introduce a basic TCP server in the functional testing framework and use it to simulate a Tor Control daemon, which is needed in order for the code in `src/torcontrol.cpp` to execute the code that is relevant for `-onlynet`.

vasild mentioned this pull request Aug 30, 2021

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

Merged

vasild force-pushed the respect_onlynet_for_tor branch from a2ebcda to baa6cb1 Compare August 30, 2021 13:15

vasild closed this Nov 8, 2021

bitcoin locked and limited conversation to collaborators Nov 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tor: respect non-onion -onlynet= for outgoing Tor connections #22651

tor: respect non-onion -onlynet= for outgoing Tor connections #22651

vasild commented Aug 6, 2021

vasild commented Aug 6, 2021

jonatack commented Aug 6, 2021 •

edited

Loading

ghost commented Aug 6, 2021

ghost commented Aug 6, 2021 •

edited by ghost

Loading

jonatack commented Aug 6, 2021

DrahtBot commented Aug 7, 2021 •

edited

Loading

ghost commented Aug 7, 2021

ghost commented Aug 7, 2021

ghost commented Aug 8, 2021 •

edited by ghost

Loading

vasild commented Aug 9, 2021

ghost commented Aug 9, 2021

vasild commented Aug 11, 2021

vasild commented Aug 11, 2021

vasild commented Aug 11, 2021

ghost commented Aug 11, 2021 •

edited by ghost

Loading

jonatack left a comment

vasild commented Aug 11, 2021

vasild commented Aug 19, 2021

jonatack commented Aug 19, 2021 •

edited

Loading

Rspigler commented Aug 19, 2021

ghost commented Aug 20, 2021

laanwj commented Aug 26, 2021

vasild commented Aug 30, 2021

vasild commented Aug 30, 2021 •

edited

Loading

jonatack commented Aug 30, 2021

luke-jr commented Sep 14, 2021

Talkless commented Nov 6, 2021

vasild commented Nov 8, 2021

tor: respect non-onion -onlynet= for outgoing Tor connections #22651

tor: respect non-onion -onlynet= for outgoing Tor connections #22651

Conversation

vasild commented Aug 6, 2021

vasild commented Aug 6, 2021

jonatack commented Aug 6, 2021 • edited Loading

ghost commented Aug 6, 2021

ghost commented Aug 6, 2021 • edited by ghost Loading

jonatack commented Aug 6, 2021

DrahtBot commented Aug 7, 2021 • edited Loading

Conflicts

ghost commented Aug 7, 2021

ghost commented Aug 7, 2021

ghost commented Aug 8, 2021 • edited by ghost Loading

vasild commented Aug 9, 2021

ghost commented Aug 9, 2021

vasild commented Aug 11, 2021

vasild commented Aug 11, 2021

vasild commented Aug 11, 2021

ghost commented Aug 11, 2021 • edited by ghost Loading

jonatack left a comment

Choose a reason for hiding this comment

vasild commented Aug 11, 2021

vasild commented Aug 19, 2021

jonatack commented Aug 19, 2021 • edited Loading

Rspigler commented Aug 19, 2021

ghost commented Aug 20, 2021

laanwj commented Aug 26, 2021

vasild commented Aug 30, 2021

vasild commented Aug 30, 2021 • edited Loading

jonatack commented Aug 30, 2021

luke-jr commented Sep 14, 2021

Talkless commented Nov 6, 2021

vasild commented Nov 8, 2021

jonatack commented Aug 6, 2021 •

edited

Loading

ghost commented Aug 6, 2021 •

edited by ghost

Loading

DrahtBot commented Aug 7, 2021 •

edited

Loading

ghost commented Aug 8, 2021 •

edited by ghost

Loading

ghost commented Aug 11, 2021 •

edited by ghost

Loading

jonatack commented Aug 19, 2021 •

edited

Loading

vasild commented Aug 30, 2021 •

edited

Loading