net: don’t bind on 0.0.0.0 if binds are restricted to Tor #20234

pull vasild wants to merge 1 commits into bitcoin:master from vasild:bind changing 5 files +129 −29
  1. vasild commented at 7:25 pm on October 24, 2020: member

    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 we should honor the restriction and bind only to the specified address.

    Before this change, if no -bind is given then we would bind to 0.0.0.0:8333 and to 127.0.0.1:8334 (incoming Tor) which is ok - the user does not care to restrict the binding.

    However, if only -bind=addr:port=onion is given (without ordinary -bind=) then we would bind to addr:port and to 0.0.0.0:8333 in addition.

    Change the above to not do the additional bind: if only -bind=addr:port=onion is given (without ordinary -bind=) then bind to addr:port (only) and consider incoming connections to that as Tor and do not advertise it. I.e. a Tor-only node.

  2. vasild commented at 7:26 pm on October 24, 2020: member
    This is a followup to #19991, cc @hebasto
  3. luke-jr commented at 7:47 pm on October 24, 2020: member
    How does Tor continue to function after this?
  4. DrahtBot added the label P2P on Oct 24, 2020
  5. DrahtBot commented at 5:25 am on October 25, 2020: 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:

    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.

  6. MarcoFalke added this to the milestone 0.21.0 on Oct 28, 2020
  7. jonatack commented at 5:42 pm on October 28, 2020: member
    Concept ACK
  8. in src/net.cpp:2337 in db2d6f3238 outdated 
    2406+    for (const auto& addrBind : options.vWhiteBinds) {
2407         fBound |= Bind(addrBind.m_service, (BF_EXPLICIT | BF_REPORT_ERROR), addrBind.m_flags);
2408     }
2409-    if (binds.empty() && whiteBinds.empty()) {
2410+    for (const auto& addr_bind : options.onion_binds) {
2411+        fBound |= Bind(addr_bind, BF_EXPLICIT | BF_REPORT_ERROR | BF_DONT_ADVERTISE, NetPermissionFlags::PF_NONE);
    jonatack commented at 6:25 pm on October 28, 2020:
    The report error flag was previously absent for the onion binds, while present for the others. This was added in bb145c9050203, @hebasto WDYT, just an oversight?
    vasild commented at 8:20 am on October 30, 2020:

    Yeah. #19991 (review)

    I think it is reasonable to inform the user that his request couldn’t be fulfilled. E.g. if he specifies -bind=1.2.3.4:8334=onion and we can’t bind on that address and port.

    hebasto commented at 1:17 pm on October 30, 2020:

    @hebasto WDYT, just an oversight?

    Good catch! I think the BF_REPORT_ERROR flag was missed for no reason.

    vasild commented at 12:38 pm on July 6, 2021:

    Stripping down this PR I had to remove the BF_REPORT_ERROR flag.

    If one runs two bitcoinds with explicit -bind=... that do not conflict one may expect that they would start successfully. The functional testing framework expects this! However they both try to bind on the same default onion target 127.0.0.1:8334, one of them succeeds and the other fails. That failure is muted by the absence of BF_REPORT_ERROR. Apparently no test cares whether that bind has failed or not. To observe the problem, in master, add BF_REPORT_ERROR and run ./test/functional/test_runner.py, observe random tests failures with errors like “unable to bind to 127.0.0.1:8334”.

    (the full version of this PR used to fix that and thus the presence of BF_REPORT_ERROR did not cause tests to brick)

  9. in test/functional/feature_bind_extra.py:114 in db2d6f3238 outdated 
    109+        self.setup_nodes()
110+
111+    def run_test(self):
112+        for i in range(len(self.expected)):
113+            pid = self.nodes[i].process.pid
114+            assert_equal(set(get_bind_addrs(pid)), set(self.expected[i][1]))
    jonatack commented at 7:09 pm on October 28, 2020:

    This assertion fails for me with errors like

    0AssertionError: not(
1{('00000000', 11340), ('7f000001', 16340), ('00000000000000000000000000000000', 11340), ('7f000001', 18445)} ==
2{('00000000', 11340), ('7f000001', 16340), ('7f000001', 18445)})

    This fixes it, but it’s not pretty and might be treating the symptoms more than the problem:

     0--- a/test/functional/test_framework/netutil.py
 1+++ b/test/functional/test_framework/netutil.py
 2@@ -81,7 +81,10 @@ def get_bind_addrs(pid):
 3     bind_addrs = []
 4     for conn in netstat('tcp') + netstat('tcp6'):
 5         if conn[3] == STATE_LISTEN and conn[4] in inodes:
 6-            bind_addrs.append(conn[1])
 7+            if conn[1][0] == '00000000000000000000000000000000':
 8+                bind_addrs.append(('00000000', conn[1][1]))
 9+            else:
10+                bind_addrs.append(conn[1])
11     return bind_addrs
    vasild commented at 8:16 am on October 30, 2020:

    Finally I figured out what is going on!

    test_ipv6_local() tries to connect to ::1 in order to check if the system has IPv6 support.

    Some (all?) Travis machines do support IPv6, but ::1 is not configured on the loopback interface (!?). On those machines connecting to ::1 fails with [Errno 99] Cannot assign requested address. However it is possible to successfully bind on ::.

    bitcoind tries to bind on :: and ::1 and the above makes it difficult for the test to set reasonable expectations.

    Resolved by ignoring all IPv6 addresses.

  10. jonatack commented at 7:11 pm on October 28, 2020: member
    Approach ACK/almost ACK db2d6f323836f286f37d4e826c6edcfce6cde29b
  11. in test/functional/feature_bind_extra.py:2 in db2d6f3238 outdated 
    0@@ -0,0 +1,117 @@
1+#!/usr/bin/env python3
2+# Copyright (c) 2014-2019 The Bitcoin Core developers
    sipa commented at 9:34 pm on October 28, 2020:

    I am not a lawyer, but my understanding is that:

    • The copyright notice here has no legal meaning, and copyright is retained by whoever authored it regardless of what is stated in the file’s header.
    • “Bitcoin Core developers” is no legal entity, and can’t own any copyright. It’s just there to indicate “various contributors over time” without the need for being specific.

    So there is no problem with using either your own name, or using the collective term - except the latter avoids the need for keeping it up to date as more contributors work on it. Most code I’ve written for Bitcoin Core is in files with “Bitcoin Core developers” notices, FWIW.

    luke-jr commented at 0:37 am on October 29, 2020:

    This PR is by @vasild. He can legally put any copyright line he wants on code he wrote (IANAL).

    And again, there is no “Bitcoin Core developers” entity. It simply means each author retains his own copyright.

    sipa commented at 2:02 am on October 29, 2020:
    @JabbaDesilijicTiure My comment was purely about the individual files’ copyright notices. The project falls under the MIT license as specified in the COPYING file at the root of the repository.
    luke-jr commented at 4:30 am on October 29, 2020:
    @JabbaDesilijicTiure You don’t know what you’re talking about. Copyright does not depend on any notice at all. There are plenty of places you can learn more - this isn’t the place for it.
    fanquake commented at 10:59 am on October 29, 2020:
    @JabbaDesilijicTiure I’ve removed your comment. This repository is obviously not the place for that.
  12. fanquake deleted a comment on Oct 29, 2020
  13. laanwj commented at 11:14 am on October 29, 2020: member
    @vasild, like any contributor, is part of “The Bitcoin Core developers” This is not the place for copyright discussion in any case. Please keep the discussion focused on the code change done here.
  14. gmaxwell commented at 11:28 am on October 29, 2020: contributor

    Unless I am misunderstanding this PR, this will break tor configuration for any user with an explicit bind configuration. Not binding to localhost is extremely weird, I can’t think of any other program I’ve encountered doing that.

    Is there an actual usecase for not binding localhost?

    Might it be better to just document that localhost binds are implicit and then, if there is a need for not binding to localhost, have an argument to specifically disable that?

    In other words, I think what you’re actually pointing to is just a doc bug where the documentation isn’t pedantically correct, but where the behaviour is what 99.999% of users would prefer. If so– change the doc.

  15. vasild commented at 12:09 pm on October 29, 2020: member

    @gmaxwell

    this will break tor configuration for any user with an explicit bind configuration. Not binding to localhost is extremely weird

    You found a bug! Not in this PR, but in 0.20.1:

    0$ bitcoind --version
1Bitcoin Core version v0.20.1.0-36355f128f8b
2
3$ bitcoind -bind=1.1.1.1:8333 -listenonion=1
4...
52020-10-29T11:51:58Z tor: Got service ID 7hgc4pgtuexasesn, advertising service 7hgc4pgtuexasesn.onion:8333
6...

    bitcoind tells the Tor daemon to redirect incoming connections to 127.0.0.1:8333:

    https://github.com/bitcoin/bitcoin/blob/v0.20.1/src/torcontrol.cpp#L539

    but then it does not listen on 127.0.0.1:8333:

    0$ sockstat -l |grep bitcoin
1vd       bitcoind   95898 6  tcp4   127.0.0.1:8332        *:*
2vd       bitcoind   95898 21 tcp4   1.1.1.1:8333          *:*
3$ telnet 127.0.0.1 8333
4Trying 127.0.0.1...
5telnet: connect to address 127.0.0.1: Connection refused

    That bug was fixed by #19991 by removing the hardcoded 127.0.0.1.

    When only -bind= is provided (without -bind=...=onion), this PR would keep the same behavior as 0.20.1 as above - it would bind only on 1.1.1.1:8333 as requested. However it would tell the Tor daemon to redirect connections to 1.1.1.1:8333 instead of the bogus 127.0.0.1:8333 - as a side effect of the change from #19991 now we use the proper address instead of hardcoding 127.0.0.1.

  16. gmaxwell commented at 12:32 pm on October 29, 2020: contributor
    rpc bind is a separate configuration from ‘bind’, as is whitebind, perhaps the onion should be its own bind statement (that users normally never touch)
  17. laanwj deleted a comment on Oct 30, 2020
  18. vasild force-pushed on Oct 30, 2020
  19. vasild commented at 8:23 am on October 30, 2020: member
    Fixed the test failure on Travis (confirmed on my local fork).
  20. vasild closed this on Oct 30, 2020
  21. vasild reopened this on Oct 30, 2020
  22. hebasto commented at 2:06 pm on October 30, 2020: member
    • If only -bind=addr is given (without -bind=...=onion) then we would bind to addr and to 127.0.0.1:8334 in addition…

    One could expect binding to the default 127.0.0.1:8334 as no -bind=...=onion is provided explicitly.

    … which may be unexpected assuming the perception of -bind=addr is “bind only to addr”.

    If perception of an option is ambiguous the option docs require improvement, no?

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

    Again, binding to the default 0.0.0.0:8333 is reasonable as no -bind=... is provided explicitly.

    I understand, that all statements above look like -bind=... and -bind=...=onion are two independent options. Are they?

  23. vasild commented at 2:32 pm on October 30, 2020: member

    I understand, that all statements above look like -bind=... and -bind=...=onion are two independent options. Are they?

    I think all 3 of -bind=..., -bind=...=onion and -whitebind= should be considered “the same” because all of them control where to listen for P2P connections.

    For example, if -whitebind= is present and -bind= is not, then we do not bind on 0.0.0.0 anymore:

    0$ bitcoind -whitebind=1.1.1.1:8888
1...
2$ sockstat -l |grep bitcoind
3vd       bitcoind   46471 6  tcp4   127.0.0.1:8332        *:*
4vd       bitcoind   46471 19 tcp4   1.1.1.1:8888          *:*
5$
  24. hebasto commented at 2:39 pm on October 30, 2020: member

    For example, if -whitebind= is present and -bind= is not, then we do not bind on 0.0.0.0 anymore:

    This behavior seems to need to be properly documented as well :)

  25. vasild commented at 11:32 am on November 4, 2020: member

    This PR seems to have derailed in direction of documentation, but I think the code needs to be changed.

    Take any popular server - Apache, PostgreSQL, MySQL, OpenSSH, Tor, Bitcoin Core 0.20.1 (but not the current master). They all listen for some service and if the user explicitly specifies to listen/bind only on a given address:port, they would not listen for that service on other addresses/ports in addition.

    Apache, for example has Listen 80 which binds on 0.0.0.0:80, but when changed to Listen 1.1.1.1:80 it would only bind on 1.1.1.1:80.

    The current master does not abide to the above and even if requested to bind on a specific address:port to listen for P2P, it may bind to 127.0.0.1 or 0.0.0.0 in addition (details are in the OP). IMO this violates the principle of least surprise.

  26. laanwj removed this from the milestone 0.21.0 on Nov 5, 2020
  27. laanwj added this to the milestone 22.0 on Nov 5, 2020
  28. MarcoFalke commented at 7:07 pm on November 5, 2020: member
    The 0.21 milestone has been removed for now. And this is not a regression anyway, AFAICT
  29. DrahtBot added the label Needs rebase on Nov 18, 2020
  30. ghost commented at 8:58 am on December 6, 2020: none

    I understand, that all statements above look like -bind=... and -bind=...=onion are two independent options. Are they?

    It looks to me that they are.

    I find the current -bind option documentation with the optional =onion suffix rather confusing.

    Could it be possible to introduce a new option -onionbind (analog to names rpcbind, whitebind) to set the onion bind address explicitly and prevent confusing mixing?

    Both -bind and -onionbind should be able to be unset (e.g. set to ’none’) to explicitly disable them. E.g. if one would only want to “onion bind” and not to bind to clearnet at all, one could set -bind=none, while for -onionbind the default would take effect.

    Here a suggestion of the documentation that could be then, which I think would be clearer:

    0  -bind=<addr>[:<port>]|none
1       Bind to given address and always listen on it (default: 0.0.0.0). Use
2       [host]:port notation for IPv6.
3
4  -onionbind=<addr>[:<port>]|none
5       Bind onion to given address, always listen on it and tag any incoming
6       connections to that address and port as incoming Tor connections.
7       (defaults: 127.0.0.1:8334, testnet: 127.0.0.1:18334,
8       signet: 127.0.0.1:38334, regtest: 127.0.0.1:18445)
9       Use [host]:port notation for IPv6.

    Unfortunately, I only now came to know of to the =onion suffix introduced in #19991.

    Edit: gmaxwell suggested similar (https://github.com/bitcoin/bitcoin/pull/20234#issuecomment-718722537) “perhaps the onion should be its own bind statement (that users normally never touch)”

  31. vasild force-pushed on Dec 18, 2020
  32. vasild commented at 12:49 pm on December 18, 2020: member
    Rebased due to conflicts. @wodry what you are suggesting is out of the scope of this PR.
  33. DrahtBot removed the label Needs rebase on Dec 18, 2020
  34. DrahtBot added the label Needs rebase on Jan 7, 2021
  35. vasild force-pushed on Jan 14, 2021
  36. vasild commented at 10:46 am on January 14, 2021: member
    da8e632fb…9580d0605: rebase due to conflicts
  37. DrahtBot removed the label Needs rebase on Jan 14, 2021
  38. DrahtBot added the label Needs rebase on May 19, 2021
  39. vasild force-pushed on May 19, 2021
  40. vasild commented at 11:39 am on May 19, 2021: member
    9580d06052...a6b8fc9012: rebase due to conflicts
  41. jonatack commented at 11:41 am on May 19, 2021: member
    Concept ACK
  42. DrahtBot removed the label Needs rebase on May 19, 2021
  43. laanwj commented at 1:40 pm on May 25, 2021: member
    The idea for extending the -bind syntax, instead of adding -onionbind was that not every supported overlay network needs its own set of options. I think this was a good idea in itself. We already have way too many different bitcoind options.
  44. in test/functional/feature_bind_extra.py:27 in a6b8fc9012 outdated 
    23+    assert_equal,
24+    p2p_port,
25+    rpc_port,
26+)
27+
28+# From chainparamsbase.cpp:CreateBaseChainParams().
    jonatack commented at 4:06 pm on June 14, 2021: 
    0# From chainparamsbase.cpp:CreateBaseChainParams(), regtest default port 18444
    vasild commented at 12:14 pm on July 6, 2021:
    Both “regtest” and “18444” are redundant with the line that follows: REGTEST_TOR_TARGET_PORT = 18445
  45. in test/functional/feature_bind_extra.py:32 in a6b8fc9012 outdated 
    31+class BindExtraTest(BitcoinTestFramework):
32+    def set_test_params(self):
33+        # Avoid any -bind= on the command line. Force the framework to avoid
34+        # adding -bind=127.0.0.1.
35+        self.setup_clean_chain = True
36+        self.bind_to_localhost_only = False
    jonatack commented at 4:08 pm on June 14, 2021: 
    0     def set_test_params(self):
1+        self.setup_clean_chain = True
2         # Avoid any -bind= on the command line. Force the framework to avoid
3         # adding -bind=127.0.0.1.
4-        self.setup_clean_chain = True
5         self.bind_to_localhost_only = False
    vasild commented at 12:15 pm on July 6, 2021:
    Done
  46. in test/functional/feature_bind_extra.py:70 in a6b8fc9012 outdated 
    65+        )
66+
67+        # Node1, no -bind=...=onion, thus no extra port for Tor target.
68+        self.expected.append(
69+            [
70+                ['-bind=127.0.0.1:{}'.format(port)],
    jonatack commented at 4:34 pm on June 14, 2021: 
    0                [f'-bind=127.0.0.1:{port}'],
    vasild commented at 12:15 pm on July 6, 2021:
    Done
  47. in test/functional/feature_bind_extra.py:79 in a6b8fc9012 outdated 
    74+        port += 1
75+
76+        # Node2, no normal -bind, thus only the Tor target.
77+        self.expected.append(
78+            [
79+                ['-bind=127.0.0.1:{}=onion'.format(port)],
    jonatack commented at 4:35 pm on June 14, 2021: 
    0                [f'-bind=127.0.0.1:{port}=onion'],
    vasild commented at 12:15 pm on July 6, 2021:
    Done
  48. in test/functional/feature_bind_extra.py:88 in a6b8fc9012 outdated 
    83+        port += 1
84+
85+        # Node3, both -bind and -bind=...=onion.
86+        self.expected.append(
87+            [
88+                ['-bind=127.0.0.1:{}'.format(port), '-bind=127.0.0.1:{}=onion'.format(port + 1)],
    jonatack commented at 4:37 pm on June 14, 2021: 
    0                [f'-bind=127.0.0.1:{port}', f'-bind=127.0.0.1:{port + 1}=onion'],
    vasild commented at 12:15 pm on July 6, 2021:
    Done
  49. in test/functional/feature_bind_extra.py:110 in a6b8fc9012 outdated 
    105+            actual = set(get_bind_addrs(pid))
106+            # Remove IPv6 addresses because on some CI environments "::1" is not configured
107+            # on the system (so our test_ipv6_local() would return False), but it is
108+            # possible to bind on "::". This makes it unpredictable whether to expect
109+            # that bitcoind has bound on "::1" (for RPC) and "::" (for P2P).
110+            actual_without_ipv6 = set(filter(lambda e: len(e[0]) != 32, actual))
    jonatack commented at 4:49 pm on June 14, 2021:
    pprinting these values shows that they increase on each run of the test, IDK if our test framework should be cleaning them up.
    vasild commented at 12:16 pm on July 6, 2021:
    You mean that actual_without_ipv6 contains more entries on each run of the test? Then the test should fail?
    jonatack commented at 8:17 am on July 7, 2021:
    IIRC no, the values in the entries increased. This no longer occurs with the latest push a0048331a1d2d (and processes and sockstats are stable).
  50. in test/functional/feature_bind_extra.py:72 in a6b8fc9012 outdated 
    94+        # Add RPC ports to the list of expected ports to bind to for all nodes.
95+        # They are not relevant for this test.
96+        for i in range(len(self.expected)):
97+            self.expected[i][1].append((loopback_ipv4, rpc_port(i)))
98+
99+        self.extra_args = list(map(lambda e: e[0], self.expected))
    jonatack commented at 4:54 pm on June 14, 2021: 
    0+        # Set extra_args to:
1+        # [[],
2+        #  ['-bind=127.0.0.1:<port>'],
3+        #  ['-bind=127.0.0.1:<port+1>=onion'],
4+        #  ['-bind=127.0.0.1:<port+2>', '-bind=127.0.0.1:<port+3>=onion']]
5         self.extra_args = list(map(lambda e: e[0], self.expected))
    vasild commented at 12:17 pm on July 6, 2021:
    The comment would be prone to becoming outdated.
  51. jonatack commented at 5:04 pm on June 14, 2021: member
    ACK a6b8fc90129be2ae78f5d38a537dc64353124439 code review, debug built rebased to current master 3a2c84a6b5144f4ee1181, some light testing, ran the test repeatedly with various added pprint statements, verified that the test fails on master.
  52. achow101 commented at 6:29 pm on June 14, 2021: member

    ACK a6b8fc90129be2ae78f5d38a537dc64353124439

    Reviewed code and verified that the test fails on master and passes with this PR.

  53. in src/init.cpp:1743 in a6b8fc9012 outdated 
    1750+    } else if (!connOptions.vBinds.empty()) {
1751+        onion_service_target = connOptions.vBinds.front();
1752+    } else if (!connOptions.vWhiteBinds.empty()) {
1753+        onion_service_target = connOptions.vWhiteBinds.front().m_service;
1754+    } else {
1755+        assert(connOptions.bind_on_any);
    jonatack commented at 1:50 pm on June 18, 2021:
    see discussion at https://www.erisian.com.au/bitcoin-core-dev/log-2021-06-18.html#l-200 regarding naming or a comment here
    laanwj commented at 5:50 pm on June 18, 2021:
    I’m no longer sure a comment or rename is necessary. I do think the interaction with -listen is not completely obvious when reading this code.
  54. laanwj commented at 6:33 am on June 21, 2021: member
    Code review ACK a6b8fc90129be2ae78f5d38a537dc64353124439
  55. in src/init.cpp:1739 in a6b8fc9012 outdated 
    1746+
1747+    CService onion_service_target;
1748+    if (!connOptions.onion_binds.empty()) {
1749+        onion_service_target = connOptions.onion_binds.front();
1750+    } else if (!connOptions.vBinds.empty()) {
1751+        onion_service_target = connOptions.vBinds.front();
    hebasto commented at 12:32 pm on June 29, 2021:
    Are we still able to distinguish incoming connection via Tor from one via clearnet?
    vasild commented at 12:54 pm on June 29, 2021:
    In this case - no (only -bind=addr is given, without -bind=...=onion).
    vasild commented at 12:17 pm on July 6, 2021:
    Removed this code.
  56. hebasto commented at 12:27 pm on July 1, 2021: member
    • 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 #19991.

    -bind=addr does not mention onion network, but it does change onion-related behavior, i.e., it prevents recognizing of incoming connections that come via Tor.

    This new behavior is confusing (at least to me), and I don’t agree with it.

  58. vasild commented at 2:37 pm on July 1, 2021: member

    @hebasto, that is similar to how it operates with other networks - if the user restricts the binds to only some networks, then all other networks are ditched (affected) wrt listening/binding.

    For example, assume a machine has an IPv4 address 1.2.3.4 and an IPv6 address. By default if no -bind is given it would bind to both addresses. If -bind=1.2.3.4 is given it would not bind on the IPv6 address. Now this statement:

    “-bind=addr does not mention IPv6, but it does change IPv6-related behavior…”

    Would this change to -bind= description help?

    0  -bind=<addr>[:<port>][=onion]
1-       Bind to given address...
2+       Bind only to given address...
  59. hebasto commented at 2:45 pm on July 1, 2021: member

    For example, assume a machine has an IPv4 address 1.2.3.4 and an IPv6 address. By default if no -bind is given it would bind to both addresses. If -bind=1.2.3.4 is given it would not bind on the IPv6 address.

    Right. But my concerns are not about binding itself, rather about behavior that users might expect – the distinguishing incoming Tor connections.

  60. jonatack commented at 3:01 pm on July 1, 2021: member

    distinguishing incoming Tor connections

    Right, CNode::m_inbound_onion and CNode::ConnectedThroughNetwork() need to remain pertinent, as the logic is used by the inbound eviction protection and by getpeerinfo, -netinfo, and the GUI peers window.

  61. vasild commented at 3:05 pm on July 1, 2021: member

    I see. So it is about judging which expectation to fail:

    • Fail the expectation that -bind=addr only binds to addr (leave this PR unmerged). This might have security implications: if a user specifies -bind=1.2.3.4:8333 and expects that this is the only listen address:port and sets up his firewall rules according to that expectation. Especially that this was the case before Oct 2020 (see below).

    • Fail the expectation that we distinguish incoming Tor connections if -bind=addr is given (merge this PR). We do that only after #19991 which was merged 9 months ago (in Oct 2020). Btw, in peer eviction we semi-distinguish Tor connections even in that case - we protect localhost peers in addition to Tor ones.

  62. vasild commented at 8:08 am on July 2, 2021: member
    @hebasto, what do you think about the second change in this PR? That is - the second item from the PR description: “* If only -bind=addr=onion is given…”. If that is not contentious then maybe it would make sense that I split the PR in two.
  63. hebasto commented at 8:44 am on July 2, 2021: member

    @hebasto, what do you think about the second change in this PR? That is - the second item from the PR description: “* If only -bind=addr=onion is given…”. If that is not contentious then maybe it would make sense that I split the PR in two.

    sgtm

  64. vasild force-pushed on Jul 6, 2021
  65. vasild force-pushed on Jul 6, 2021
  66. vasild commented at 12:12 pm on July 6, 2021: member
    a6b8fc9...a004833: remove the controversial part of this PR (the behavior when -bind=... is given without -bind=...=onion – behave as in master), take review suggestions
  67. vasild commented at 12:18 pm on July 6, 2021: member

    @hebasto, what do you think about the second change in this PR? That is - the second item from the PR description: “* If only -bind=addr=onion is given…”. If that is not contentious then maybe it would make sense that I split the PR in two.

    sgtm

    Done! Updated PR title and description.

  68. vasild renamed this:
    net: don't extra bind for Tor if binds are restricted
    net: don't bind on 0.0.0.0 if binds are restricted to Tor
    on Jul 6, 2021
  69. Rspigler commented at 11:59 pm on July 6, 2021: contributor
    Concept ACK
  70. in test/functional/feature_bind_extra.py:87 in a0048331a1 outdated 
    82+
83+        # Node2, no -bind, expected to bind on any + tor target.
84+        #self.expected.append(
85+        #    [
86+        #        [],
87+        #        [(any_ipv4, p2p_port(0)), (loopback_ipv4, REGTEST_TOR_TARGET_PORT)]
    jonatack commented at 10:08 am on July 7, 2021:

    Per offline discussion with @vasild:

    0        #        [(any_ipv4, p2p_port(2)), (loopback_ipv4, REGTEST_TOR_TARGET_PORT)]

    and running the commented-out tests requires not having bitcoind -regtest up due to port conflict.

    0$ netstat -na | grep 18445
1tcp        0      0 127.0.0.1:18445         0.0.0.0:*               LISTEN

    Maybe remove the commented-out tests for now.

    vasild commented at 1:49 pm on July 7, 2021:
    Done.
  71. jonatack commented at 10:10 am on July 7, 2021: member

    ACK a0048331a1d2dda24cbdc7b41041ae00bf46ee7a

    Verified the first test (Node0) fails on master, the second test passes (perhaps inverse their order).

  72. net: don't bind on 0.0.0.0 if binds are restricted to Tor 
    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 we should honor the restriction and bind only to the
specified address.

Before this change, if no `-bind` is given then we would bind to
`0.0.0.0:8333` and to `127.0.0.1:8334` (incoming Tor) which is ok -
the user does not care to restrict the binding.

However, if only `-bind=addr:port=onion` is given (without ordinary
`-bind=`) then we would bind to `addr:port` _and_ to `0.0.0.0:8333` in
addition.

Change the above to not do the additional bind: if only
`-bind=addr:port=onion` is given (without ordinary `-bind=`) then bind
to `addr:port` (only) and consider incoming connections to that as Tor
and do not advertise it. I.e. a Tor-only node.
    2feec3ce31
  73. vasild force-pushed on Jul 7, 2021
  74. vasild commented at 1:49 pm on July 7, 2021: member
    a0048331a1...2feec3ce31: remove commented tests - they can be resurrected later if needed and contained a typo (thanks, @jonatack for testing even commented out stuff!)
  75. laanwj commented at 2:38 pm on July 8, 2021: member
    Code review ACK 2feec3ce3130961f98ceb030951d0e46d3b9096c
  76. jonatack commented at 2:39 pm on July 8, 2021: member
    utACK 2feec3ce3130961f98ceb030951d0e46d3b9096c per git diff a004833 2feec3c
  77. jonatack commented at 2:45 pm on July 8, 2021: member
    Note: This PR is tagged for 0.22, but 2 Tor/I2P PRs that should probably be priority for 0.22 but are not tagged are #22179 and #22211.
  78. hebasto approved
  79. hebasto commented at 2:53 pm on July 8, 2021: member

    ACK 2feec3ce3130961f98ceb030951d0e46d3b9096c, tested on Linux Mint 20.1 (x86_64):

    0$ src/bitcoind -signet
1...
22021-07-08T14:50:35Z [init] Bound to 127.0.0.1:38334
32021-07-08T14:50:35Z [init] Bound to [::]:38333
42021-07-08T14:50:35Z [init] Bound to 0.0.0.0:38333
5...

    0$ src/bitcoind -signet -bind=127.0.0.1:38334=onion
1...
22021-07-08T14:52:02Z [init] Bound to 127.0.0.1:38334
3...
  80. laanwj merged this on Jul 12, 2021
  81. laanwj closed this on Jul 12, 2021
  82. vasild deleted the branch on Jul 12, 2021
  83. sidhujag referenced this in commit 31e7f60245 on Jul 14, 2021
  84. gwillen referenced this in commit a4a4d73a44 on Jun 1, 2022
  85. DrahtBot locked this on Aug 16, 2022


vasild luke-jr DrahtBot jonatack hebasto sipa fanquake laanwj gmaxwell MarcoFalke ghost achow101 gruve-p Rspigler

Labels
P2P

Milestone
22.0

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: 2024-12-18 18:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me