net: Use alternative port for incoming Tor connections #19991

pull hebasto wants to merge 8 commits into bitcoin:master from hebasto:200922-tor changing 8 files +125 −56
  1. hebasto commented at 11:44 AM on September 22, 2020: member

    This PR adds ability to label incoming Tor connections as different from normal localhost connections.

    Closes #8973. Closes #16693.

    Default onion service target ports are:

    • 8334 on mainnnet
    • 18334 on testnet
    • 38334 on signet
    • 18445 on regtest

    To set the onion service target socket manually the extended -bind config option could be used:

    $ src/bitcoind -help | grep -A 6 -e '-bind'
  -bind=<addr>[:<port>][=onion]
       Bind to given address and always listen on it (default: 0.0.0.0). Use
       [host]:port notation for IPv6. Append =onion to tag any incoming
       connections to that address and port as incoming Tor connections
       (default: 127.0.0.1:8334=onion, testnet: 127.0.0.1:18334=onion,
       signet: 127.0.0.1:38334=onion, regtest: 127.0.0.1:18445=onion)

    Since pr19991.02 update this PR is an alternative to #19043.

  2. fanquake added the label P2P on Sep 22, 2020
  3. DrahtBot commented at 12:23 PM on September 22, 2020: member

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #19288 (tests: Add fuzzing harness for TorController by practicalswift)
    • #19064 (refactor: Cleanup thread ctor calls by hebasto)
    • #19043 (torcontrol: add -tortarget config by MDrollette)

    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.

  4. in src/net.cpp:2335 in 6e05a443bb outdated 
    2299 | @@ -2300,11 +2300,14 @@ bool CConnman::InitBinds(const std::vector<CService>& binds, const std::vector<N
2300 |      }
2301 |      if (binds.empty() && whiteBinds.empty()) {
2302 |          struct in_addr inaddr_any;
2303 | -        inaddr_any.s_addr = INADDR_ANY;
2304 | +        inaddr_any.s_addr = htonl(INADDR_ANY);
2305 |          struct in6_addr inaddr6_any = IN6ADDR_ANY_INIT;
    Saibato commented at 1:06 PM on September 22, 2020:

    nit. Why casting 0 to 0 , 0 is 0 in every byte-order? I like this to be a constant and not a hookable function.

    kristapsk commented at 1:39 PM on September 22, 2020:

    Is it guaranteed that INADDR_ANY will always be 0, on any operating system?

    Saibato commented at 3:05 PM on September 22, 2020:

    rfc1122 section-3.2.1.3

    hebasto commented at 7:43 PM on September 22, 2020:

    Why casting 0 to 0 , 0 is 0 in every byte-order?

    Firstly, https://www.man7.org/linux/man-pages/man7/ip.7.html:

    in_addr should be assigned one of the INADDR_* values using htonl...

    Secondly, it is inconsistent to have s_addr = INADDR_ANY in a line, and s_addr = htonl(INADDR_LOOPBACK) in another one.

    Saibato commented at 8:27 AM on September 23, 2020:

    Yup i guess its just a generation thing, its a nit, but i always liked to some degree how the bitcoin client did network things different than expected, U will never know who wrote the example codes and how drunk or biased by a gun on there head they where at those times when the foundations u all take for granted where coded. INADDR_LOOPBACK is a different case and hey what can u do if the constant is not in net order, i remember the rant of Hobbit when he bashed on Berkeley sockets implementation ( also on many other reasons) . Htonl() was one of the first predictable ways the :Gesseler had to go". And if u bind to "this host" or maybe not makes a difference.

    I will not way in this more, made my point and u are in charge its now ur world, but please never trade readability for function.

    sipa commented at 8:35 AM on September 23, 2020:

    @saibato Are you ok?

    vasild commented at 10:00 AM on September 23, 2020:

    htonl() is the proper one even though the number is a palindrome. We had some discussion about that here and here.

    Saibato commented at 12:37 PM on September 23, 2020:

    @sipa

    @Saibato Are you ok?

    Thx :)

    <details><summary>click here :package: :tada: . to find out more</summary> <br> I case u forgot, just assign INADDR_ANY without htonl is up till now in source code right after some first commits. from November 2009 onward. Even you later never changed that behavior and so far all run good with this.

    I get nervous when things in bitcoin are changed by a drive by commit just because ...

    Why even touch it? Suppose Satoshi knew what we did in cold war and those where those magic lines that made bitcon a stable server. I doubt that u lads are like we, but u all have the potential to grow and learn.

    i like it when i can trigger u to mature, but ok is that all?

        // The sockaddr_in structure specifies the address family,
    // IP address, and port for the socket that is being bound
    struct sockaddr_in sockaddr;
    memset(&sockaddr, 0, sizeof(sockaddr));
    sockaddr.sin_family = AF_INET;
    sockaddr.sin_addr.s_addr = INADDR_ANY; // bind to all IPs on this computer
    sockaddr.sin_port = DEFAULT_PORT;


    commit 93cfb02acb13b74198b8e0ab72f039313ecf9f1f
Author:	s_nakamoto <s_nakamoto@1a98c847-1fd6-4fd8-948a-caf3550aa51b>  Fri Nov  6 05:50:05 2009
Committer:	s_nakamoto <s_nakamoto@1a98c847-1fd6-4fd8-948a-caf3550aa51b>  Fri Nov  6 05:50:05 2009

    </details>

  5. Saibato changes_requested
  6. practicalswift commented at 1:17 PM on September 22, 2020: contributor

    Concept ACK

    Being able to differentiate between incoming Tor connections from normal localhost connections would be very useful

  7. glozow commented at 2:47 PM on September 22, 2020: member

    Concept ACK

  8. laanwj commented at 3:05 PM on September 22, 2020: member

    Concept ACK! Thank you for working on this.

  9. in src/init.cpp:454 in 6e05a443bb outdated 
     450 | @@ -451,6 +451,7 @@ void SetupServerArgs(NodeContext& node)
 451 |      argsman.AddArg("-maxtimeadjustment", strprintf("Maximum allowed median peer time offset adjustment. Local perspective of time may be influenced by peers forward or backward by this amount. (default: %u seconds)", DEFAULT_MAX_TIME_ADJUSTMENT), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
 452 |      argsman.AddArg("-maxuploadtarget=<n>", strprintf("Tries to keep outbound traffic under the given target (in MiB per 24h). Limit does not apply to peers with 'download' permission. 0 = no limit (default: %d)", DEFAULT_MAX_UPLOAD_TARGET), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
 453 |      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);
 454 | +    argsman.AddArg("-onionservicetargetport=<port>", strprintf("Listen for connections from Tor onion service on <port> (default: %u, testnet: %u, regtest: %u)", defaultBaseParams->OnionServiceTargetPort(), testnetBaseParams->OnionServiceTargetPort(), regtestBaseParams->OnionServiceTargetPort()), ArgsManager::ALLOW_INT | ArgsManager::NETWORK_ONLY, OptionsCategory::CONNECTION);
    vasild commented at 10:07 AM on September 23, 2020:

    Signet not mentioned?

    hebasto commented at 10:38 AM on September 23, 2020:

    I think this change -- to mention signet in help output for all relevant config options -- should be the part of another PR, no?

    hebasto commented at 9:35 AM on September 26, 2020:

    See #20014.

    hebasto commented at 11:05 AM on September 28, 2020:

    Updated.

  10. vasild commented at 10:34 AM on September 23, 2020: member

    • This will always open 127.0.0.1:8334, even if the new option -onionservicetargetport= is not specified and the system does not use Tor in any way. I think this deserves a release notes mention.

    • Binding to 127.0.0.1 would make it impossible (or hard) to run the Tor daemon on another host. What about changing to -onionservicetarget=bind_address:port without a default value - i.e. don't do the new bind unless -onionservicetarget= is specified? This would also allow to bind to an external address or to 0.0.0.0 (any).

    • Notice that if another peer manages to reach and connect to the new bind without tor, then we would mistakenly think it is an incoming tor connection.

    • I am just curious - what will happen if -onionservicetargetport=8333 is specified?

  11. hebasto commented at 10:48 AM on September 23, 2020: member

    @vasild

    • This will always open 127.0.0.1:8334, even if the new option -onionservicetargetport= is not specified and the system does not use Tor in any way. I think this deserves a release notes mention.

    Target port opening could be dependent on -listenonion option value. What do you think?

    • Binding to 127.0.0.1 would make it impossible (or hard) to run the Tor daemon on another host. What about changing to -onionservicetarget=bind_address:port without a default value - i.e. don't do the new bind unless -onionservicetarget= is specified? This would also allow to bind to an external address or to 0.0.0.0 (any).

    This issue is addressed in #19043, IIUC.

    • Notice that if another peer manages to reach and connect to the new bind without tor, then we would mistakenly think it is an incoming tor connection.

    Hmmm... Need to think about this.

    • I am just curious - what will happen if -onionservicetargetport=8333 is specified?

    It will still work but without strong distinction between Tor and clearnet, no?

  12. naumenkogs commented at 2:17 PM on September 23, 2020: member

    Concept ACK

  13. hebasto commented at 7:40 AM on September 24, 2020: member

    From IRC discussion:

    <vasild> what about extending the -bind=addr:port option to something like -bind=addr:port[=tor], for example: -bind=1.2.3.4:8333 -bind=1.2.3.4:8334=tor <vasild> instead of adding a new option -onionservicetarget= ? <wumpus> making it an attribute makes sense <wumpus> (of the bind) <vasild> I don't know yet how i2p works, but if it works in a similar way (the i2p daemon connects to us, like in tor) then for i2p that would mean just recognizing =i2p in addition to recognizing =tor, instead of adding new option -i2pservicetarget=addr:port <wumpus> right, it's elegant way to think about it and allows for extensibility

  14. jonatack commented at 1:57 PM on September 24, 2020: member

    Concept ACK. Could use test coverage. I'm running #20002 that is built on these changes and those in #19998.

  15. hebasto commented at 9:00 PM on September 24, 2020: member

    @vasild @jonatack

    what about extending the -bind=addr:port option to something like -bind=addr:port[=tor]

    I like that ^ idea very much. Implemented in https://github.com/hebasto/bitcoin/commits/200924-WIP

    As many other PRs are built on top of this one, I'm begging you to make some preliminary review of the suggested branch to avoid unneeded rebasing.

    New version features:

    • extended -bind configure option, now it completely replaces #19043 as well
    • no extra binding when -nolistenonion

    Also a small Q: is it better to drop doc commit cfa17e307adaec81f6036cf4f124d78f9e128c30 in favor of #19961 ?

    UPDATE: already upstreamed.

  16. hebasto commented at 9:27 PM on September 24, 2020: member

    @vasild

    • Notice that if another peer manages to reach and connect to the new bind without tor, then we would mistakenly think it is an incoming tor connection.

    This behavior could be mitigated in #19998 by additional checking of peer.addr and peer.GetAddrLocal().

  17. jonatack commented at 9:51 PM on September 24, 2020: member

    I like that approach, too. Will look at your branch. Edit: done.

  18. hebasto force-pushed on Sep 25, 2020
  19. hebasto commented at 11:41 AM on September 25, 2020: member

    Updated 6e05a443bbe7090a331281290f18bedea13bab0a -> cfa17e307adaec81f6036cf4f124d78f9e128c30 (pr19991.01 -> pr19991.02, diff):

    • extended -bind configure option (credits to @vasild), now this PR completely replaces #19043
    • no extra binding when -nolistenonion
    • added corresponding changes into tor.md
  20. in src/init.cpp:439 in cfa17e307a outdated 
     435 | @@ -436,7 +436,7 @@ void SetupServerArgs(NodeContext& node)
 436 |      argsman.AddArg("-addnode=<ip>", "Add a node to connect to and attempt to keep the connection open (see the `addnode` RPC command help for more info). This option can be specified multiple times to add multiple nodes.", ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::CONNECTION);
 437 |      argsman.AddArg("-asmap=<file>", strprintf("Specify asn mapping used for bucketing of the peers (default: %s). Relative paths will be prefixed by the net-specific datadir location.", DEFAULT_ASMAP_FILENAME), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
 438 |      argsman.AddArg("-bantime=<n>", strprintf("Default duration (in seconds) of manually configured bans (default: %u)", DEFAULT_MISBEHAVING_BANTIME), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
 439 | -    argsman.AddArg("-bind=<addr>", "Bind to given address and always listen on it. Use [host]:port notation for IPv6", ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::CONNECTION);
 440 | +    argsman.AddArg("-bind=<addr>", strprintf("Bind to given address and always listen on it. Use [host]:port notation for IPv6. Use =onion suffix to bind onion service target socket (default: 127.0.0.1:%u=onion, testnet: 127.0.0.1:%u=onion, regtest: 127.0.0.1:%u=onion)", defaultBaseParams->OnionServiceTargetPort(), testnetBaseParams->OnionServiceTargetPort(), regtestBaseParams->OnionServiceTargetPort()), ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::CONNECTION);
    vasild commented at 11:50 AM on September 25, 2020: 
        argsman.AddArg("-bind=<addr>[:<port>][=onion]", strprintf("Bind to given address and always listen on it. Use [host]:port notation for IPv6. Append =onion to tag any incoming connections to that address and port as incoming tor connections (default: mainnet: 127.0.0.1:%u=onion, testnet: 127.0.0.1:%u=onion, regtest: 127.0.0.1:%u=onion)", defaultBaseParams->OnionServiceTargetPort(), testnetBaseParams->OnionServiceTargetPort(), regtestBaseParams->OnionServiceTargetPort()), ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::CONNECTION);
    hebasto commented at 3:49 PM on September 25, 2020:

    Thanks! Updated.

    vasild commented at 12:50 PM on September 29, 2020:

    Thanks! It just occurred to me that the default for -bind (with this PR) is not 127.0.0.1:8334=onion like in the description above. It is rather -bind=0.0.0.0:8333 -bind=127.0.0.1:8334=onion.

    hebasto commented at 4:36 PM on September 29, 2020:

    Updated.

  21. in src/init.cpp:1955 in cfa17e307a outdated 
    1934 | +        } else {
1935 | +            const std::string network_type = bind_arg.substr(index + 1);
1936 | +            if (connOptions.listen_onion && network_type == "onion") {
1937 | +                const std::string truncated_bind_arg = bind_arg.substr(0, index);
1938 | +                if (Lookup(truncated_bind_arg, bind_addr, BaseParams().OnionServiceTargetPort(), false)) {
1939 | +                    connOptions.onion_bind = bind_addr;
    vasild commented at 12:19 PM on September 25, 2020:

    This would accept multiple =onion arguments and discard all but the last one. I.e. if -bind=1.1.1.1:8334=onion -bind=2.2.2.2:8334=onion is given then it will behave as if only -bind=2.2.2.2:8334=onion is given. Should we support more than one onion bind address? Maybe it would make sense on a machine with many interfaces.

    hebasto commented at 2:01 PM on September 25, 2020:

    This would accept multiple =onion arguments and discard all but the last one.

    That is done intentionally, leaving improving for follow ups.

    vasild commented at 1:08 PM on September 29, 2020:

    I think it is not good to introduce one behavior and change it later. Like "ignore all but last -bind=addr:port=onion" and later change it to something else because users may start to depend on the first one.

    If supporting more than one -bind=addr:port=onion is undesired then what about emitting an error in that case?

    hebasto commented at 4:35 PM on September 29, 2020:

    Updated.

  22. in src/init.cpp:1952 in cfa17e307a outdated 
    1931 | +                connOptions.vBinds.push_back(bind_addr);
1932 | +                continue;
1933 | +            }
1934 | +        } else {
1935 | +            const std::string network_type = bind_arg.substr(index + 1);
1936 | +            if (connOptions.listen_onion && network_type == "onion") {
    vasild commented at 12:19 PM on September 25, 2020:

    If the suffix is something else than =onion then this code would silently ignore it. I think we should print an error and refuse to start if the user makes a typo like =omion.

    hebasto commented at 1:58 PM on September 25, 2020: 
    $ src/bitcoind -bind=127.0.0.1:4242=omion | grep omion
2020-09-25T13:57:56Z [init] Command-line arg: bind="127.0.0.1:4242=omion"
Error: Cannot resolve -bind address: '127.0.0.1:4242=omion'
2020-09-25T13:57:58Z [init] Error: Cannot resolve -bind address: '127.0.0.1:4242=omion'
    vasild commented at 1:11 PM on September 29, 2020:

    Sorry, I misread the code.

  23. in src/net.h:214 in cfa17e307a outdated 
     201 | @@ -201,6 +202,8 @@ class CConnman
 202 |          std::vector<NetWhitelistPermissions> vWhitelistedRange;
 203 |          std::vector<NetWhitebindPermissions> vWhiteBinds;
 204 |          std::vector<CService> vBinds;
 205 | +        bool listen_onion{true};
 206 | +        Optional<CService> onion_bind;
    vasild commented at 12:29 PM on September 25, 2020:

    nit: m_ prefix.

    hebasto commented at 1:51 PM on September 25, 2020:

    #19759

    vasild commented at 1:29 PM on September 29, 2020:

    Ok, drop this. I suggested it because members below use m_ prefix. But members above don't!

    hebasto commented at 1:37 PM on September 29, 2020:

    #19759 (comment)

    :D

  24. in src/net.cpp:2335 in cfa17e307a outdated 
    2330 | @@ -2323,7 +2331,7 @@ bool CConnman::Start(CScheduler& scheduler, const Options& connOptions)
2331 |          nMaxOutboundCycleStartTime = 0;
2332 |      }
2333 |  
2334 | -    if (fListen && !InitBinds(connOptions.vBinds, connOptions.vWhiteBinds)) {
2335 | +    if (fListen && !InitBinds(connOptions.vBinds, connOptions.vWhiteBinds, connOptions.onion_bind.get())) {
    vasild commented at 12:36 PM on September 25, 2020:

    This will cause undefined behavior if connOptions.onion_bind is not set, are we sure it is set here?

    hebasto commented at 8:30 PM on September 25, 2020:

    Thanks! Updated.

  25. in src/net.cpp:2342 in cfa17e307a outdated 
    2310 |          fBound |= Bind(CService(inaddr_any, GetListenPort()), !fBound ? BF_REPORT_ERROR : BF_NONE, NetPermissionFlags::PF_NONE);
2311 |      }
2312 | +
2313 | +    if (m_listen_onion) {
2314 | +        fBound |= Bind(onion_bind, BF_EXPLICIT, NetPermissionFlags::PF_NONE);
2315 | +    }
    vasild commented at 12:43 PM on September 25, 2020:

    I don't think the new functionality should be tied to the -listenonion option.

    Its name is confusing - it has nothing to do with "listening" for tor connections. What it does is to automatically create the tor hidden service using the tor-control connection.

    If the hidden service is configured via torrc, then we don't want to create another one automatically via tor-control and thus -listenonion=0, but we still want to bind to a different addr:port.

    hebasto commented at 2:34 PM on September 25, 2020:

    Ok. I think a user should be able to explicitly disable such an additional binding? Any ideas?

    hebasto commented at 3:41 PM on September 25, 2020:

    Why not assign to the "-listenonion" option a new meaning "Listen incoming onion connection, and automatically create the onion service if the Tor control available". It looks pretty back-compatible, no?

    vasild commented at 1:42 PM on September 29, 2020:

    That would couple the two things together under one option, making it impossible to configure them independently:

    N auto create hidden service (-listenonion) extra bind (-bind=addr:port=onion)
    1 no no
    2 no yes
    3 yes no
    4 yes yes
    hebasto commented at 4:35 PM on September 29, 2020:

    Thanks! Updated.

  26. in src/net.cpp:2343 in cfa17e307a outdated 
    2311 |      }
2312 | +
2313 | +    if (m_listen_onion) {
2314 | +        fBound |= Bind(onion_bind, BF_EXPLICIT, NetPermissionFlags::PF_NONE);
2315 | +    }
2316 | +
    vasild commented at 12:55 PM on September 25, 2020:

    Bind() could call AddLocal() which will add the address to mapLocalHost. From there we will eventually advertise it to some peer and it will get gossiped over the network.

    We definitely do not want that because if somebody else manages to connect to it (outside of tor) then we will consider that as an incoming tor connection. This bind address should only be known by bitcoind and the tor daemon.

    hebasto commented at 3:28 PM on September 25, 2020:

    For "127.0.0.1" IsRoutable() returns false, therefore it won't be passed to AddLocal(). But, in general, I agree with you.

    hebasto commented at 3:49 PM on September 25, 2020:

    Updated.

    vasild commented at 2:02 PM on September 29, 2020:

    Yes, this is only relevant if a routable address is supplied to -bind=addr:port=onion.

    The added NetPermissionFlags::PF_ONION works, but is strange because that is not a "permission". It is just a flag telling Bind() "this is a covert bind address, don't advertise it". Isn't enum BindFlags more suitable for it? BF_DONT_ADVERTISE or BF_COVERT?

    In any case - the _ONION in the name may be too narrow, if we want to use the same mechanism for something else in the future.

    hebasto commented at 4:36 PM on September 29, 2020:

    Thanks! Updated.

  27. in src/torcontrol.cpp:92 in cfa17e307a outdated 
      89 |       * connected is the handler that is called when connection is successfully established.
  90 |       * disconnected is a handler that is called when the connection is broken.
  91 |       * Return true on success.
  92 |       */
  93 | -    bool Connect(const std::string &target, const ConnectionCB& connected, const ConnectionCB& disconnected);
  94 | +    bool Connect(const std::string& control_socket, const ConnectionCB& connected, const ConnectionCB& disconnected);
    vasild commented at 12:59 PM on September 25, 2020:

    nit: I think it is wrong to call this "socket" as sockets are usually integers, like file descriptors, not std::strings. I understand why "target" is now undesirable. What about tor_control_center? ;)

    hebasto commented at 1:47 PM on September 25, 2020:

    Maybe control_center (if we are already within Tor-related class)?

    hebasto commented at 3:49 PM on September 25, 2020:

    Updated.

  28. in src/torcontrol.cpp:761 in cfa17e307a outdated 
     756 | @@ -752,7 +757,9 @@ void StartTorControl()
 757 |          return;
 758 |      }
 759 |  
 760 | -    torControlThread = std::thread(std::bind(&TraceThread<void (*)()>, "torcontrol", &TorControlThread));
 761 | +    torControlThread = std::thread(&TraceThread<std::function<void()>>, "torcontrol", [onion_service_target] {
 762 | +        TorControlThread(onion_service_target);
    vasild commented at 1:10 PM on September 25, 2020:

    TorControlThread() only takes a reference to this object and uses it in another thread. I think this is dangerous because the liftetime of the object may be shorter than the lifetime of the thread. Better make TorControlThread()'s argument just CService so that the object is copied.

    hebasto commented at 2:48 PM on September 25, 2020:

    wrt properties of std::thread() constructor and TraceThread() function I don't think it is dangerous. But objects of CService class are small, so going to implement passing by-copy.

    hebasto commented at 3:48 PM on September 25, 2020:

    Updated.

  29. vasild commented at 1:28 PM on September 25, 2020: member

    Just to summarize the default behavior:

    Before this PR

    • If no -bind is given then we will bind on 0.0.0.0:8333 and [::]:8333.
    • If -bind=1.2.3.4:8333 is given then we will bind only on 1.2.3.4:8333.

    With this PR

    • If no -bind is given then we will bind on 0.0.0.0:8333, [::]:8333 and 127.0.0.1:8334.
    • If -bind=1.2.3.4:8333 is given then we will bind on 1.2.3.4:8333 and on 127.0.0.1:8334 (is this ok?).
    • If -bind=1.2.3.4:8333 -bind=1.2.3.4:8334=onion is given then we will bind on 1.2.3.4:8333 and 1.2.3.4:8334.
  30. hebasto commented at 1:42 PM on September 25, 2020: member

    @vasild

    Just to summarize the default behavior:

    Before this PR

    • If no -bind is given then we will bind on 0.0.0.0:8333 and [::]:8333.

    • If -bind=1.2.3.4:8333 is given then we will bind only on 1.2.3.4:8333.

    With this PR

    • If no -bind is given then we will bind on 0.0.0.0:8333, [::]:8333 and 127.0.0.1:8334.

    • If -bind=1.2.3.4:8333 is given then we will bind on 1.2.3.4:8333 and on 127.0.0.1:8334 (is this ok?).

    • If -bind=1.2.3.4:8333 -bind=1.2.3.4:8334=onion is given then we will bind on 1.2.3.4:8333 and 1.2.3.4:8334.

    The node will not bind on 127.0.0.1:8334 if non-default listenonion=0 is provided.

  31. vasild commented at 1:44 PM on September 25, 2020: member

    This could be an alternative way to check if an incoming connection is from tor - to ask the tor daemon if it made it. But it seems this is not possible now and even if made possible in the future then it will only be available if bitcoind has access to tor-control.

  32. hebasto force-pushed on Sep 25, 2020
  33. hebasto commented at 3:48 PM on September 25, 2020: member

    Updated cfa17e307adaec81f6036cf4f124d78f9e128c30 -> 0782aee0a3268e9e4de53613f229d645c2ab06b9 (pr19991.02 -> pr19991.03, diff):

    • addressed some @vasild's comments
  34. hebasto force-pushed on Sep 25, 2020
  35. hebasto commented at 8:29 PM on September 25, 2020: member

    Updated 0782aee0a3268e9e4de53613f229d645c2ab06b9 -> 20349ec2f669c6e06dfbddbcec63b1c3b6cdbc9e (pr19991.03 -> pr19991.04, diff):

    This will cause undefined behavior if connOptions.onion_bind is not set, are we sure it is set here?

  36. in src/chainparamsbase.cpp:53 in 20349ec2f6 outdated 
      48 |      } else if (chain == CBaseChainParams::SIGNET) {
  49 | -        return MakeUnique<CBaseChainParams>("signet", 38332);
  50 | +        return MakeUnique<CBaseChainParams>("signet", 38332, 38334);
  51 |      } else if (chain == CBaseChainParams::REGTEST) {
  52 | -        return MakeUnique<CBaseChainParams>("regtest", 18443);
  53 | +        return MakeUnique<CBaseChainParams>("regtest", 18443, 18445);
    jonatack commented at 6:34 AM on September 28, 2020:

    3271e6d maybe document in this section the why and choice of the additional port <n + 2> in each chain

    hebasto commented at 9:56 AM on September 28, 2020:

    It is an arbitrary choice. Just verified that ports are not reserved by some other service.

    jonatack commented at 10:01 AM on September 28, 2020:

    Thanks. I think it would be helpful to document that and say why there is an extra port for future readers of the code.

    hebasto commented at 11:02 AM on September 28, 2020:

    Updated.

  37. in src/torcontrol.cpp:199 in 20349ec2f6 outdated 
     195 | @@ -193,16 +196,16 @@ void TorControlConnection::eventcb(struct bufferevent *bev, short what, void *ct
 196 |      }
 197 |  }
 198 |  
 199 | -bool TorControlConnection::Connect(const std::string &target, const ConnectionCB& _connected, const ConnectionCB&  _disconnected)
 200 | +bool TorControlConnection::Connect(const std::string& tor_control_center, const ConnectionCB& _connected, const ConnectionCB&  _disconnected)
    jonatack commented at 6:39 AM on September 28, 2020:

    a8b28269 nit, rm extra space

    bool TorControlConnection::Connect(const std::string& tor_control_center, const ConnectionCB& _connected, const ConnectionCB& _disconnected)
    hebasto commented at 11:02 AM on September 28, 2020:

    Updated.

  38. in src/init.cpp:439 in 20349ec2f6 outdated 
     435 | @@ -436,7 +436,7 @@ void SetupServerArgs(NodeContext& node)
 436 |      argsman.AddArg("-addnode=<ip>", "Add a node to connect to and attempt to keep the connection open (see the `addnode` RPC command help for more info). This option can be specified multiple times to add multiple nodes.", ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::CONNECTION);
 437 |      argsman.AddArg("-asmap=<file>", strprintf("Specify asn mapping used for bucketing of the peers (default: %s). Relative paths will be prefixed by the net-specific datadir location.", DEFAULT_ASMAP_FILENAME), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
 438 |      argsman.AddArg("-bantime=<n>", strprintf("Default duration (in seconds) of manually configured bans (default: %u)", DEFAULT_MISBEHAVING_BANTIME), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
 439 | -    argsman.AddArg("-bind=<addr>", "Bind to given address and always listen on it. Use [host]:port notation for IPv6", ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::CONNECTION);
 440 | +    argsman.AddArg("-bind=<addr>[:<port>][=onion]", strprintf("Bind to given address and always listen on it. Use [host]:port notation for IPv6. Append =onion to tag any incoming connections to that address and port as incoming tor connections (default: mainnet: 127.0.0.1:%u=onion, testnet: 127.0.0.1:%u=onion, regtest: 127.0.0.1:%u=onion)", defaultBaseParams->OnionServiceTargetPort(), testnetBaseParams->OnionServiceTargetPort(), regtestBaseParams->OnionServiceTargetPort()), ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::CONNECTION);
    jonatack commented at 8:25 AM on September 28, 2020:

    937faad This would print:

      -bind=<addr>[:<port>][=onion]
       Bind to given address and always listen on it. Use [host]:port notation
       for IPv6. Append =onion to tag any incoming connections to that
       address and port as incoming tor connections (default: mainnet:
       127.0.0.1:8334=onion, testnet: 127.0.0.1:18334=onion, regtest:
       127.0.0.1:18445=onion)
    • Should we warn that only the last =onion argument will be kept if more than one is passed?
    • s/default:/defaults/ or remove the colon after mainnet, testnet, and regtest, to avoid nested colons
    • maybe s/tor/onion/
    jonatack commented at 9:35 AM on September 28, 2020:

    Signet?

    hebasto commented at 9:50 AM on September 28, 2020:

    #20014

    jonatack commented at 9:58 AM on September 28, 2020:

    Yes, #20014 doesn't update -bind, as there's nothing to update before this PR, so it should be updated here if I'm not confused.

    hebasto commented at 11:03 AM on September 28, 2020:

    Thanks! Updated.

    vasild commented at 2:07 PM on September 29, 2020:

    Should we warn that only the last =onion argument will be kept if more than one is passed?

    Right, or even error :bomb: or support more than one =onion.

    hebasto commented at 4:35 PM on September 29, 2020:

    Updated.

  39. in src/torcontrol.cpp:544 in 20349ec2f6 outdated 
     540 | @@ -536,7 +541,7 @@ void TorController::auth_cb(TorControlConnection& _conn, const TorControlReply&
 541 |              private_key = "NEW:RSA1024"; // Explicitly request RSA1024 - see issue #9214
 542 |          // Request onion service, redirect port.
 543 |          // Note that the 'virtual' port is always the default port to avoid decloaking nodes using other ports.
 544 | -        _conn.Command(strprintf("ADD_ONION %s Port=%i,127.0.0.1:%i", private_key, Params().GetDefaultPort(), GetListenPort()),
 545 | +        _conn.Command(strprintf("ADD_ONION %s Port=%i,%s", private_key, Params().GetDefaultPort(), m_target.ToStringIPPort()),
    jonatack commented at 8:59 AM on September 28, 2020:

    0c9e4c55 One thing I did while testing this was add this logging change:

    diff --git a/src/torcontrol.cpp b/src/torcontrol.cpp
index f3583d5754..bafa44645f 100644
--- a/src/torcontrol.cpp
+++ b/src/torcontrol.cpp
@@ -490,7 +490,7 @@ TorController::~TorController()
 void TorController::add_onion_cb(TorControlConnection& _conn, const TorControlReply& reply)
 {
     if (reply.code == 250) {
-        LogPrint(BCLog::TOR, "tor: ADD_ONION successful\n");
+        LogPrint(BCLog::TOR, "tor: ADD_ONION successful on ports %i, %s\n", Params().GetDefaultPort(), m_target.ToStringIPPort());

    which printed:

    2020-09-28T08:52:08Z tor: ADD_ONION successful on ports 8333, 127.0.0.1:8334
    hebasto commented at 10:52 AM on September 28, 2020:

    We already have the following log message:

    2020-09-28T10:51:00Z [init] Bound to 127.0.0.1:18334
    jonatack commented at 11:18 AM on September 28, 2020:

    Oh, quite right, I was grepping on -ni bind ;/

    master

    456006:2020-09-28T10:33:52Z Bound to [::]:8333
456007:2020-09-28T10:33:52Z Bound to 0.0.0.0:8333

    this branch

    456232:2020-09-28T11:16:58Z Bound to [::]:8333
456233:2020-09-28T11:16:58Z Bound to 0.0.0.0:8333
456234:2020-09-28T11:16:58Z Bound to 127.0.0.1:8334
  40. jonatack commented at 9:26 AM on September 28, 2020: member

    Approach ACK. Would it be possible to add test coverage? Perhaps similar to feature_proxy.py.

    (Two minor comments: Perhaps separate commits like net, refactor: Refactor CBaseChainParams::RPCPort() to their own "no behavior change" pure refactoring PR. They lengthen review with an increased diff that isn't essential to the goal of this change. Also, commit refactor: Rename TorController::target to m_control_socket makes more sense when in the same commit as Pass onion service target to Tor controller. When reviewing the commits in order, I was wondering, "why is this being renamed?" Feel free to ignore if you disagree.)

  41. hebasto force-pushed on Sep 28, 2020
  42. hebasto force-pushed on Sep 28, 2020
  43. hebasto commented at 11:18 AM on September 28, 2020: member

    Updated 20349ec2f669c6e06dfbddbcec63b1c3b6cdbc9e -> d0e349140fac872ebf01c3826528a84bd6a89519 (pr19991.04 -> pr19991.06, diff):

    When reviewing the commits in order, I was wondering, "why is this being renamed?"

    Commit message updated.

  44. hebasto commented at 11:35 AM on September 28, 2020: member

    @jonatack

    Would it be possible to add test coverage? Perhaps similar to feature_proxy.py.

    feature_proxy.py tests outbound connections, IIUC.

  45. net: Use network byte order for in_addr.s_addr
    It is documented in the ip(7) manual page.
    b3273cf403
  46. net: Add alternative port for onion service
    This change allows to distinguish incoming Tor connections from others.
    a5266d4546
  47. net, refactor: Refactor CBaseChainParams::RPCPort function fdd3ae4d26
  48. refactor: Rename TorController::target to m_tor_control_center
    `target` is a proper name for the onion service target address and port.
This change is required for the following commit.
    e3f07851f0
  49. hebasto force-pushed on Sep 29, 2020
  50. hebasto commented at 7:06 AM on September 29, 2020: member

    Rebased d0e349140fac872ebf01c3826528a84bd6a89519 -> efa45f5fde37ebf20f318da74d53f056fd71d69f (pr19991.06 -> pr19991.07) due to the conflict with #20014.

  51. laanwj commented at 3:58 PM on September 29, 2020: member

    Tested ACK efa45f5fde37ebf20f318da74d53f056fd71d69f I've verified that this opens a new port (8334, bound on 127.0.0.1 only) and that torcontrol makes incoming connection on the hidden service that it creates go to there. This allows to distinguish incoming Tor connections. This works together with #19954.

  52. hebasto force-pushed on Sep 29, 2020
  53. hebasto force-pushed on Sep 29, 2020
  54. hebasto commented at 4:34 PM on September 29, 2020: member

    Updated efa45f5fde37ebf20f318da74d53f056fd71d69f -> 30f2355d96bf67a30a06c1fcf6d27d242b05491b (pr19991.07 -> pr19991.09, diff).

    1. Addressed @vasild's comment:

    Should we support more than one onion bind address? Maybe it would make sense on a machine with many interfaces.

    and @jonatack's comment:

    Should we warn that only the last =onion argument will be kept if more than one is passed?

    If -listenonion=1 is provided, the node emits a warning about multiple bind addresses.

    1. Addressed @vasild's comment:

    I don't think the new functionality should be tied to the -listenonion option.

    1. Addressed @vasild's comment:

    Isn't enum BindFlags more suitable for it? BF_DONT_ADVERTISE or BF_COVERT?

    1. Addressed @vasild's comment:

    Thanks! It just occurred to me that the default for -bind (with this PR) is not 127.0.0.1:8334=onion like in the description above. It is rather -bind=0.0.0.0:8333 -bind=127.0.0.1:8334=onion.

  55. in src/net.cpp:23 in 30f2355d96 outdated 
      19 | @@ -20,6 +20,7 @@
  20 |  #include <protocol.h>
  21 |  #include <random.h>
  22 |  #include <scheduler.h>
  23 | +#include <torcontrol.h>
    laanwj commented at 10:03 AM on September 30, 2020:

    I really hope we can avoid introducing this circular dependency.

    Looks like this is due to only one line: onion_binds.push_back(DefaultOnionServiceTarget()); let just pass this value in.

    hebasto commented at 1:34 PM on September 30, 2020:

    Thanks! Updated.

  56. hebasto force-pushed on Sep 30, 2020
  57. hebasto commented at 1:34 PM on September 30, 2020: member

    Updated 30f2355d96bf67a30a06c1fcf6d27d242b05491b -> 3c24f75d4ce7d3ce1e30d334ea30873a15633e46 (pr19991.09 -> pr19991.10, diff):

    I really hope we can avoid introducing this circular dependency.

  58. laanwj commented at 1:40 PM on September 30, 2020: member

    Thanks for addressing my comment.

    Re-ACK 3c24f75d4ce7d3ce1e30d334ea30873a15633e46

  59. hebasto force-pushed on Sep 30, 2020
  60. hebasto commented at 5:19 PM on September 30, 2020: member

    ~Updated 3c24f75d4ce7d3ce1e30d334ea30873a15633e46 -> acba1851ef78d88c2442813174c2b559047b61e8 (pr19991.10 -> pr19991.11, diff):~

    • ~type of Options.onion_binds switched from std::vector to std::set that is more appropriate, and makes the following #19998 easier.~

    UPDATE: reverted back, sorry.

  61. hebasto force-pushed on Sep 30, 2020
  62. in src/net.cpp:91 in ffa19a6eb8 outdated 
      83 | @@ -84,6 +84,7 @@ enum BindFlags {
  84 |      BF_NONE         = 0,
  85 |      BF_EXPLICIT     = (1U << 0),
  86 |      BF_REPORT_ERROR = (1U << 1),
  87 | +    BF_DONT_ADVERTISE = (1U << 2),
    Sjors commented at 2:10 PM on October 1, 2020:

    These flags would benefit from some documentation.

    hebasto commented at 4:30 PM on October 1, 2020:

    Thanks! Updated.

  63. Sjors approved
  64. Sjors commented at 2:17 PM on October 1, 2020: member

    tACK 3c24f75d4ce7d3ce1e30d334ea30873a15633e46 though I'm not super familiar with some of this networking code

    I was able to automagically spin up a hidden service that listened on "virtual" 8333 and binds to 8334 (as well as a custom 7000). And I was able to connect another peer to it.

  65. in src/net.cpp:2309 in 3c24f75d4c outdated 
    2304 | @@ -2308,10 +2305,18 @@ bool CConnman::Bind(const CService &addr, unsigned int flags, NetPermissionFlags
2305 |          }
2306 |          return false;
2307 |      }
2308 | +
2309 | +    if (addr.IsRoutable() && fDiscover && !(flags & BF_DONT_ADVERTISE) && (permissions & PF_NOBAN) == 0) {
    vasild commented at 2:52 PM on October 1, 2020:

    nit: !(a & b) is the same as (a & b) == 0, use just either one for consistency, not both.

    hebasto commented at 4:31 PM on October 1, 2020:

    Updated.

  66. in src/init.cpp:1968 in 3c24f75d4c outdated 
    1968 | +    }
1969 | +
1970 | +    if (args.GetBoolArg("-listenonion", DEFAULT_LISTEN_ONION)) {
1971 | +        const auto bind_addr = connOptions.onion_binds.front();
1972 | +        if (connOptions.onion_binds.size() > 1) {
1973 | +            InitWarning(strprintf(_("More than one bind address is provided for automatic Tor onion service. Using %s"), bind_addr.ToStringIPPort()));
    vasild commented at 2:53 PM on October 1, 2020:

    nit:

                InitWarning(strprintf(_("More than one onion bind address is provided. Using %s for the automatically created hidden service."), bind_addr.ToStringIPPort()));
    hebasto commented at 4:31 PM on October 1, 2020:

    Thanks! Updated.

  67. in src/net.cpp:2319 in 3c24f75d4c outdated 
    2315 |  
2316 | -bool CConnman::InitBinds(const std::vector<CService>& binds, const std::vector<NetWhitebindPermissions>& whiteBinds)
2317 | +bool CConnman::InitBinds(
2318 | +    const std::vector<CService>& binds,
2319 | +    const std::vector<NetWhitebindPermissions>& whiteBinds,
2320 | +    std::vector<CService> onion_binds)
    vasild commented at 2:58 PM on October 1, 2020:

    Maybe change the onion_binds argument to const reference?

    hebasto commented at 4:32 PM on October 1, 2020:

    Thanks! Updated.

  68. in test/functional/test_framework/test_framework.py:439 in 3c24f75d4c outdated 
     435 | @@ -436,7 +436,7 @@ def get_bin_from_version(version, bin_name, bin_default):
 436 |          if self.bind_to_localhost_only:
 437 |              extra_confs = [["bind=127.0.0.1"]] * num_nodes
 438 |          else:
 439 | -            extra_confs = [[]] * num_nodes
 440 | +            extra_confs = [["listenonion=0"]] * num_nodes
    vasild commented at 3:02 PM on October 1, 2020:

    Is this change still needed after recent mods to this PR? If "yes", then why is it needed?

    hebasto commented at 4:32 PM on October 1, 2020:

    Thanks! Updated.

  69. vasild approved
  70. vasild commented at 3:14 PM on October 1, 2020: member

    ACK 3c24f75d4

    A few nits and a question below, feel free to ignore.

    This PR will cause an extra bind on 127.0.0.1:8334 by default. I think this deserves a release notes mention. There is no way to switch that off (one can only change the address and/or port), I think that is ok. If it is a concern, then maybe we can tweak it to: if -bind= is explicitly specified then don't do an extra bind and redirect the automatically created tor service (if we create one) to the normal port (8333 or whatever is given to -bind).

  71. net: Pass onion service target to Tor controller 57f17e57c8
  72. net, refactor: Move AddLocal call one level up
    This change simplifies the following commit.
    92bd3c1da4
  73. net: Extend -bind config option with optional network type bb145c9050
  74. doc: Update onion service target port numbers in tor.md 96571b3d4c
  75. hebasto force-pushed on Oct 1, 2020
  76. hebasto commented at 4:30 PM on October 1, 2020: member

    Updated 3c24f75d4ce7d3ce1e30d334ea30873a15633e46 -> 96571b3d4cb4cda0fd3d5a457ae4a12f615de82b (pr19991.10 -> pr19991.12, diff):

  77. Sjors commented at 5:55 PM on October 1, 2020: member

    re-utACK 96571b3d4cb4cda0fd3d5a457ae4a12f615de82b

  78. hebasto commented at 7:43 PM on October 1, 2020: member

    @laanwj Assign 0.21.0 milestone?

  79. in src/net.cpp:2341 in 96571b3d4c 
    2337 |          fBound |= Bind(CService(inaddr6_any, GetListenPort()), BF_NONE, NetPermissionFlags::PF_NONE);
2338 |          fBound |= Bind(CService(inaddr_any, GetListenPort()), !fBound ? BF_REPORT_ERROR : BF_NONE, NetPermissionFlags::PF_NONE);
2339 |      }
2340 | +
2341 | +    for (const auto& addr_bind : onion_binds) {
2342 | +        fBound |= Bind(addr_bind, BF_EXPLICIT | BF_DONT_ADVERTISE, NetPermissionFlags::PF_NONE);
    vasild commented at 9:42 AM on October 2, 2020:

    nit: maybe BF_REPORT_ERROR should be here too, like with binds and whiteBinds?

  80. vasild approved
  81. vasild commented at 9:47 AM on October 2, 2020: member

    ACK 96571b3d4

    Looks good, I just wonder if we can do even better. The following are just some thoughts, feel free to ignore them.

    The semantic of -bind is to restrict the binding only to some address. If not specified, then the user does not care and we bind to 0.0.0.0, if specified then the user cares and we bind only to the given address.

    With this PR, if no -bind is given then we would bind to 0.0.0.0 and to 127.0.0.1 which is ok (the user does not care to restrict the binding). However:

    • If only -bind=addr is given (without -bind=...=onion) then we would bind to addr and to 127.0.0.1 in addition which may be unexpected assuming the perception of -bind=addr is "bind only to addr".

    • If only -bind=addr=onion is given (without ordinary -bind=...) then we would bind to addr and to 0.0.0.0 in addition.

    What about changing the above two cases to not do the additional bind? I.e. if -bind is given then only bind to the specified address:

    • If only -bind=addr is given (without -bind=...=onion) then bind to addr (only). If we are creating tor hidden service then use addr as target. Same behavior as before this PR.

    • If only -bind=addr=onion is given (without ordinary -bind=...) then bind to addr (only), consider incoming connections as tor and do not advertise addr. I.e. a tor-only node.

    <details> <summary>Example patch to achieve the above</summary>

    diff --git i/src/init.cpp w/src/init.cpp
index 773e23d08..d1fed90ae 100644
--- i/src/init.cpp
+++ w/src/init.cpp
@@ -1955,31 +1955,39 @@ bool AppInitMain(const util::Ref& context, NodeContext& node, interfaces::BlockA
                 }
             }
         }
         return InitError(ResolveErrMsg("bind", bind_arg));
     }
 
-    if (connOptions.onion_binds.empty()) {
-        connOptions.onion_binds.push_back(DefaultOnionServiceTarget());
-    }
-
-    if (args.GetBoolArg("-listenonion", DEFAULT_LISTEN_ONION)) {
-        const auto bind_addr = connOptions.onion_binds.front();
-        if (connOptions.onion_binds.size() > 1) {
-            InitWarning(strprintf(_("More than one onion bind address is provided. Using %s for the automatically created Tor onion service."), bind_addr.ToStringIPPort()));
-        }
-        StartTorControl(bind_addr);
-    }
-
     for (const std::string& strBind : args.GetArgs("-whitebind")) {
         NetWhitebindPermissions whitebind;
         bilingual_str error;
         if (!NetWhitebindPermissions::TryParse(strBind, whitebind, error)) return InitError(error);
         connOptions.vWhiteBinds.push_back(whitebind);
     }
 
+    if (args.GetBoolArg("-listenonion", DEFAULT_LISTEN_ONION)) {
+        CService onion_service_target;
+        if (!connOptions.onion_binds.empty()) {
+            onion_service_target = connOptions.onion_binds.front();
+            if (connOptions.onion_binds.size() > 1) {
+                InitWarning(
+                    strprintf(_("More than one onion bind address is provided. Using %s "
+                                "for the automatically created Tor onion service."),
+                              onion_service_target.ToStringIPPort()));
+            }
+        } else if (!connOptions.vBinds.empty()) {
+            onion_service_target = connOptions.vBinds.front();
+        } else if (!connOptions.vWhiteBinds.empty()) {
+            onion_service_target = connOptions.vWhiteBinds.front().m_service;
+        } else {
+            onion_service_target = DefaultOnionServiceTarget();
+        }
+        StartTorControl(onion_service_target);
+    }
+
     for (const auto& net : args.GetArgs("-whitelist")) {
         NetWhitelistPermissions subnet;
         bilingual_str error;
         if (!NetWhitelistPermissions::TryParse(net, subnet, error)) return InitError(error);
         connOptions.vWhitelistedRange.push_back(subnet);
     }
diff --git i/src/net.cpp w/src/net.cpp
index 7f575b66c..2f67d07ed 100644
--- i/src/net.cpp
+++ w/src/net.cpp
@@ -17,12 +17,13 @@
 #include <net_permissions.h>
 #include <netbase.h>
 #include <node/ui_interface.h>
 #include <protocol.h>
 #include <random.h>
 #include <scheduler.h>
+#include <torcontrol.h>
 #include <util/strencodings.h>
 #include <util/translation.h>
 
 #ifdef WIN32
 #include <string.h>
 #else
@@ -2326,24 +2327,24 @@ bool CConnman::InitBinds(
     for (const auto& addrBind : binds) {
         fBound |= Bind(addrBind, (BF_EXPLICIT | BF_REPORT_ERROR), NetPermissionFlags::PF_NONE);
     }
     for (const auto& addrBind : whiteBinds) {
         fBound |= Bind(addrBind.m_service, (BF_EXPLICIT | BF_REPORT_ERROR), addrBind.m_flags);
     }
-    if (binds.empty() && whiteBinds.empty()) {
+    for (const auto& addr_bind : onion_binds) {
+        fBound |= Bind(addr_bind, BF_EXPLICIT | BF_REPORT_ERROR | BF_DONT_ADVERTISE, NetPermissionFlags::PF_NONE);
+    }
+    if (binds.empty() && whiteBinds.empty() && onion_binds.empty()) {
         struct in_addr inaddr_any;
         inaddr_any.s_addr = htonl(INADDR_ANY);
         struct in6_addr inaddr6_any = IN6ADDR_ANY_INIT;
         fBound |= Bind(CService(inaddr6_any, GetListenPort()), BF_NONE, NetPermissionFlags::PF_NONE);
         fBound |= Bind(CService(inaddr_any, GetListenPort()), !fBound ? BF_REPORT_ERROR : BF_NONE, NetPermissionFlags::PF_NONE);
-    }
 
-    for (const auto& addr_bind : onion_binds) {
-        fBound |= Bind(addr_bind, BF_EXPLICIT | BF_DONT_ADVERTISE, NetPermissionFlags::PF_NONE);
+        fBound |= Bind(DefaultOnionServiceTarget(), BF_NONE | BF_DONT_ADVERTISE, NetPermissionFlags::PF_NONE);
     }
-
     return fBound;
 }
 
 bool CConnman::Start(CScheduler& scheduler, const Options& connOptions)
 {
     Init(connOptions);

    </details>

  82. hebasto commented at 9:52 AM on October 2, 2020: member

    @vasild Thanks for your review and suggestion! It seems your patch introduces a new circular dependency. Could we leave improvements for follow ups wrt coming feature freeze deadline?

  83. vasild commented at 10:36 AM on October 2, 2020: member

    I am ok with 96571b3 (ACK-ed above). Feel free to ignore the idea or postpone it.

  84. laanwj added this to the milestone 0.21.0 on Oct 2, 2020
  85. laanwj commented at 11:37 AM on October 2, 2020: member

    Re-ACK 96571b3d4cb4cda0fd3d5a457ae4a12f615de82b

  86. laanwj merged this on Oct 2, 2020
  87. laanwj closed this on Oct 2, 2020
  88. jonatack commented at 11:40 AM on October 2, 2020: member

    @hebasto I am writing test coverage, are you open to bringing it into this PR or do you consider it a follow-up?

  89. laanwj commented at 11:42 AM on October 2, 2020: member

    @jonatack Though it would have been nice to include more testing here, let's leave that for a follow-up PR. this is a good self-contained change, and now we can move on to the other PRs based on it.

  90. hebasto deleted the branch on Oct 2, 2020
  91. sidhujag referenced this in commit 9d14195e7b on Oct 4, 2020
  92. laanwj referenced this in commit f79a4a8952 on Oct 12, 2020
  93. sidhujag referenced this in commit 2180fb4d7a on Oct 13, 2020
  94. laanwj referenced this in commit 0d22482353 on Oct 15, 2020
  95. sidhujag referenced this in commit 37b55282d6 on Oct 16, 2020
  96. vasild referenced this in commit db2d6f3238 on Oct 24, 2020
  97. vasild referenced this in commit d78acd996e on Oct 29, 2020
  98. vasild referenced this in commit d3e6247fec on Oct 29, 2020
  99. vasild referenced this in commit c055a5003b on Oct 29, 2020
  100. vasild referenced this in commit da8e632fbf on Dec 18, 2020
  101. vasild referenced this in commit 9580d06052 on Jan 14, 2021
  102. laanwj referenced this in commit dede9eb924 on Mar 30, 2021
  103. vasild referenced this in commit a6b8fc9012 on May 19, 2021
  104. SimonVrouwe commented at 7:21 AM on July 25, 2021: none

    @hebasto Hello, I ended up here to find reasons why my node (bitcoind v0.21.1) is listening on port 8334 even when listenonion=0 is set. Reading the discussion in this PR, it is not clear to me if this was intended or not.

    @vasild

    ...

    With this PR

    • If no -bind is given then we will bind on 0.0.0.0:8333, [::]:8333 and 127.0.0.1:8334.
    • If -bind=1.2.3.4:8333 is given then we will bind on 1.2.3.4:8333 and on 127.0.0.1:8334 (is this ok?).
    • If -bind=1.2.3.4:8333 -bind=1.2.3.4:8334=onion is given then we will bind on 1.2.3.4:8333 and 1.2.3.4:8334.

    The node will not bind on 127.0.0.1:8334 if non-default listenonion=0 is provided.

    Why is the onion_binds.push_back not placed inside the if (args.GetBoolArg("-listenonion", DEFAULT_LISTEN_ONION))?

    https://github.com/bitcoin/bitcoin/blob/2b5563bb1e6819bf46f561d94213d5fb32751359/src/init.cpp#L1725-L1740

  105. vasild referenced this in commit 43188fa4b7 on Aug 2, 2021
  106. vasild referenced this in commit 293aebe687 on Aug 3, 2021
  107. vasild referenced this in commit 3111e86146 on Aug 3, 2021
  108. random-zebra referenced this in commit b4751e10ce on Aug 11, 2021
  109. vasild referenced this in commit 1597069982 on Aug 17, 2021
  110. vasild referenced this in commit ceeb317cde on Sep 16, 2021
  111. Fabcien referenced this in commit 9608a2657a on Nov 10, 2021
  112. Fabcien referenced this in commit e7f8845c0c on Nov 10, 2021
  113. Fabcien referenced this in commit 07021d773e on Nov 10, 2021
  114. Fabcien referenced this in commit 6efd907572 on Nov 10, 2021
  115. Fabcien referenced this in commit 2587152e2e on Nov 10, 2021
  116. Fabcien referenced this in commit a83f882a3e on Nov 10, 2021
  117. Fabcien referenced this in commit 313d69774e on Nov 10, 2021
  118. Fabcien referenced this in commit 9d1355217e on Nov 10, 2021
  119. vasild referenced this in commit 0739037d4c on Jun 13, 2022
  120. DrahtBot locked this on Aug 18, 2022
Contributors
hebastoDrahtBotSaibatokristapsksipavasildpracticalswiftglozowlaanwjnaumenkogsjonatackSjorsSimonVrouwe
Labels
P2P

Milestone
0.21.0

Linked (view graph)
#8973 Incoming tor connections should use alternative port#16693 ADD_ONION assumes bitcoind to reside in hardcoded 127.0.0.1#19043 torcontrol: add -tortarget config#19998 net: Add CNode::ConnectedThroughNetwork member function#20002 net, rpc, cli: expose peer network in getpeerinfo; simplify/improve -netinfo#20193 p2p: improve onion detection in AttemptToEvictConnection#20197 p2p: protect onions in AttemptToEvictConnection(), add eviction protection test coverage
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-02 18:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me