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

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.

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.

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?

1. 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

2. 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

3. Stop Node 1 and 2:

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

4. 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

5. Check debug.log:

trying connection 3llpwlxkisrm2iu5oayh2hd6df7q36wdmt6nounenzqcxkasxfk34eid.onion

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

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.

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.

Would this fix also #13378 ?

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.

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

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

Peer info:

xejoddwvi35krze3546qtx6dfmednjs2ccsociyezkyhzdxtemp5v5ad.onion:18444
outbound-full-relay

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.

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"..

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

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

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

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.

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.

5959ece29e...a2ebcda19a: address some suggestions

@vasild, do you plan to update?

Done! :)

Code review re-ACK a2ebcda19a25067cb8183f25f6ffd0a26065fcc5 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)

Concept ACK

reACK a2ebcda

Major changes since last review:

1. tor: respect non-onion -onlynet= for outgoing Tor connections #22651 (comment)
2. tor: respect non-onion -onlynet= for outgoing Tor connections #22651 (comment)

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)

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

@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.

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.

#22834 looks like a better solution IMO

#22834 looks like a better solution IMO

So this PR should be closed?

@vasild I'm confused :)

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

Thanks, @Talkless!