net, init: derive default onion port if a user specified a -port #31223

pull mzumsande wants to merge 3 commits into bitcoin:master from mzumsande:202410_portplus1 changing 9 files +94 −19
  1. mzumsande commented at 6:01 pm on November 5, 2024: contributor

    This resolves #31133 (setups with multiple local nodes each using a different -port no longer working with v28.0, see the issue description for more details) by deriving the default onion listening port to be the value specified by -port incremented by 1 (idea by vasild / laanwj). Note that with this fix, the chosen -port values of two local nodes cannot be adjacent, otherwise there will be port collisions again.

    From the discussion in the linked issue, this was the most popular option, followed by doing nothing and telling affected users to change their setups to use -bind instead of -port. But more opinions are certainly welcome!

    I think that if we decide to do something about the problem described in the issue, we should do so soon (in 28.1.), so I opened this PR. Fixes #31133

  2. DrahtBot commented at 6:01 pm on November 5, 2024: contributor

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

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31223.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK tdb3
    Concept ACK Christewart, laanwj, i-am-yuvi

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #31260 (scripted-diff: Type-safe settings retrieval by ryanofsky)

    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.

  3. mzumsande referenced this in commit a0be9a6b08 on Nov 5, 2024
  4. mzumsande force-pushed on Nov 5, 2024
  5. Christewart commented at 6:35 pm on November 5, 2024: contributor
    concept ACK
  6. in src/torcontrol.cpp:718 in 2dccdc69d7 outdated
    715+CService DefaultOnionServiceTarget(std::optional<uint16_t> port)
    716 {
    717     struct in_addr onion_service_target;
    718     onion_service_target.s_addr = htonl(INADDR_LOOPBACK);
    719-    return {onion_service_target, BaseParams().OnionServiceTargetPort()};
    720+    return {onion_service_target, port.has_value() ? port.value() : BaseParams().OnionServiceTargetPort()};
    


    tdb3 commented at 0:53 am on November 6, 2024:

    std::optional::value_or() might work nicely here.

    e.g.

     0diff --git a/src/torcontrol.cpp b/src/torcontrol.cpp
     1index d56216a7cb0..b518cf1bb93 100644
     2--- a/src/torcontrol.cpp
     3+++ b/src/torcontrol.cpp
     4@@ -715,5 +715,5 @@ CService DefaultOnionServiceTarget(std::optional<uint16_t> port)
     5 {
     6     struct in_addr onion_service_target;
     7     onion_service_target.s_addr = htonl(INADDR_LOOPBACK);
     8-    return {onion_service_target, port.has_value() ? port.value() : BaseParams().OnionServiceTargetPort()};
     9+    return {onion_service_target, port.value_or(BaseParams().OnionServiceTargetPort())};
    10 }
    

    mzumsande commented at 3:25 pm on November 6, 2024:
    Thanks, changed to that.
  7. tdb3 commented at 1:49 am on November 6, 2024: contributor

    Approach ACK

    This seems like a reasonable and straightforward way to alleviate the issue.

    Seemed to work nicely with two node instances run as follows:

    0build/src/bitcoind -port=11000 -rpcport=11100 -datadir=/mnt/tmp/n1/
    
    0build/src/bitcoind -port=12000 -rpcport=12100 -datadir=/mnt/tmp/n2/
    
     0ss -tulpn
     1...
     2
     3... 127.0.0.1:11100                    0.0.0.0:*           users:(("bitcoind",pid=192480,fd=12))       
     4... 127.0.0.1:11001                    0.0.0.0:*           users:(("bitcoind",pid=192480,fd=29))       
     5... 127.0.0.1:12100                    0.0.0.0:*           users:(("bitcoind",pid=192511,fd=12))       
     6... 127.0.0.1:12001                    0.0.0.0:*           users:(("bitcoind",pid=192511,fd=29))                                                         
     7... 0.0.0.0:11000                    0.0.0.0:*           users:(("bitcoind",pid=192480,fd=32))       
     8... 0.0.0.0:12000                    0.0.0.0:*           users:(("bitcoind",pid=192511,fd=32))
     9
    10...
    
  8. laanwj commented at 10:15 am on November 6, 2024: member
    Concept ACK
  9. laanwj added the label P2P on Nov 6, 2024
  10. laanwj added this to the milestone 28.1 on Nov 6, 2024
  11. laanwj added the label Needs backport (28.x) on Nov 6, 2024
  12. mzumsande referenced this in commit e382bdcc54 on Nov 6, 2024
  13. mzumsande force-pushed on Nov 6, 2024
  14. in doc/release-notes-31223.md:6 in e382bdcc54 outdated
    0@@ -0,0 +1,6 @@
    1+P2P and network changes
    2+-----------------------
    3+When the `-port` configuration option is used, the default onion listening port will now
    4+be derived to be that port + 1 instead of being set to a fixed value.
    5+This re-allows setups with multiple local nodes using different `-port`, which
    6+would lead to a startup failure in v28.0 due to a port collision error. (#31223)
    


    vasild commented at 4:50 pm on November 6, 2024:

    This implies that it’s not possible in v28.0, but it is if -bind=...=onion is used. Maybe clarify:

    0This re-allows setups with multiple local nodes using different `-port`, which
    1would lead to a startup failure in v28.0 if `-bind=...=onion` is not used due to a port collision error. (#31223)
    

    mzumsande commented at 9:16 pm on November 8, 2024:
    I think that -bind=...=onion in connection with -port doesn’t resolve the problem completely, because that node wouldn’t have any other binds than the onion bind (in particular no bind-on-any) and therefore couldn’t receive any non-onion inbound connections. Not sure if there is a way to avoid the problem other than not using -port in the first place.

    vasild commented at 2:02 pm on November 12, 2024:

    I meant -bind= as a means to disable the onion bind on 127.0.0.1:8334 to avoid the port collision, but I typed -bind=...=onion instead :(

    One can use both depending on what’s needed: -bind=1.2.3.4:5555 -bind=127.0.0.1:8335=onion.

  15. in src/init.cpp:1917 in e382bdcc54 outdated
    1913+            // If -port was specified but no onion bind, derive the onion listening port by incrementing
    1914+            // that port by 1. This avoids port collisions in case of setups with multiple local nodes.
    1915+            onion_port = args.GetIntArg("-port", 0) + 1;
    1916+            if (IsBadPort(*onion_port)) {
    1917+                InitWarning(BadPortWarning("-port", *onion_port));
    1918+            }
    


    vasild commented at 4:56 pm on November 6, 2024:
    If -port=5999 is given (5999 is not bad, but 6000 is), then this would produce a warning like “-port request to listen on port 6000 …” which could be confusing because the user did not specify 6000. But more importantly I think we don’t need this bad port check at all because this is completely local thing, only between bitcoin and the Tor router. That port is not announced to the outside world, so it is ok to just drop check.

    mzumsande commented at 8:58 pm on November 8, 2024:
    will drop as suggested. I guess I was also thinking about local port contention: If bitcoind binds to a port from the list that is usually used by other protocols, then this might affect other programs from starting up or working correctly. Do you think that is an issue we should care about?

    vasild commented at 2:09 pm on November 12, 2024:

    Do you think that is an issue we should care about?

    No.

    I have found simple/dumb programs easier to configure compared to programs that try to be intelligent on behalf of the user.

    Shaw’s Principle: Build a system that even a fool can use, and only a fool will want to use it.

    This is a bit subjective and I may have a bias here, but if I put Port 80 in /etc/ssh/sshd_config I can’t imagine sshd saying something like “Hey, you are binding sshd on port 80, your web server may not work well”. :-D

  16. in src/init.cpp:1919 in e382bdcc54 outdated
    1915+            onion_port = args.GetIntArg("-port", 0) + 1;
    1916+            if (IsBadPort(*onion_port)) {
    1917+                InitWarning(BadPortWarning("-port", *onion_port));
    1918+            }
    1919+        }
    1920+        onion_service_target = DefaultOnionServiceTarget(onion_port);
    


    vasild commented at 5:24 pm on November 6, 2024:

    I think it is simpler and is ok to use reduce all this to:

      0diff --git i/src/init.cpp w/src/init.cpp
      1index c10f792c7d..7e141dbd4c 100644
      2--- i/src/init.cpp
      3+++ w/src/init.cpp
      4@@ -518,13 +518,13 @@ void SetupServerArgs(ArgsManager& argsman, bool can_listen_ipc)
      5                  " If <type> is not supplied or if <type> = 1, indexes for all known types are enabled.",
      6                  ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
      7 
      8     argsman.AddArg("-addnode=<ip>", strprintf("Add a node to connect to and attempt to keep the connection open (see the addnode RPC help for more info). This option can be specified multiple times to add multiple nodes; connections are limited to %u at a time and are counted separately from the -maxconnections limit.", MAX_ADDNODE_CONNECTIONS), ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::CONNECTION);
      9     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);
     10     argsman.AddArg("-bantime=<n>", strprintf("Default duration (in seconds) of manually configured bans (default: %u)", DEFAULT_MISBEHAVING_BANTIME), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
     11-    argsman.AddArg("-bind=<addr>[:<port>][=onion]", strprintf("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:%u=onion, testnet3: 127.0.0.1:%u=onion, testnet4: 127.0.0.1:%u=onion, signet: 127.0.0.1:%u=onion, regtest: 127.0.0.1:%u=onion)", defaultBaseParams->OnionServiceTargetPort(), testnetBaseParams->OnionServiceTargetPort(), testnet4BaseParams->OnionServiceTargetPort(), signetBaseParams->OnionServiceTargetPort(), regtestBaseParams->OnionServiceTargetPort()), ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::CONNECTION);
     12+    argsman.AddArg("-bind=<addr>[:<port>][=onion]", strprintf("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:%u=onion, testnet3: 127.0.0.1:%u=onion, testnet4: 127.0.0.1:%u=onion, signet: 127.0.0.1:%u=onion, regtest: 127.0.0.1:%u=onion)", defaultChainParams->GetDefaultPort() + 1, testnetChainParams->GetDefaultPort() + 1, testnet4ChainParams->GetDefaultPort() + 1, signetChainParams->GetDefaultPort() + 1, regtestChainParams->GetDefaultPort() + 1), ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::CONNECTION);
     13     argsman.AddArg("-cjdnsreachable", "If set, then this host is configured for CJDNS (connecting to fc00::/8 addresses would lead us to the CJDNS network, see doc/cjdns.md) (default: 0)", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
     14     argsman.AddArg("-connect=<ip>", "Connect only to the specified node; -noconnect disables automatic connections (the rules for this peer are the same as for -addnode). This option can be specified multiple times to connect to multiple nodes.", ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::CONNECTION);
     15     argsman.AddArg("-discover", "Discover own IP addresses (default: 1 when listening and no -externalip or -proxy)", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
     16     argsman.AddArg("-dns", strprintf("Allow DNS lookups for -addnode, -seednode and -connect (default: %u)", DEFAULT_NAME_LOOKUP), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
     17     argsman.AddArg("-dnsseed", strprintf("Query for peer addresses via DNS lookup, if low on addresses (default: %u unless -connect used or -maxconnections=0)", DEFAULT_DNSSEED), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
     18     argsman.AddArg("-externalip=<ip>", "Specify your own public address", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
     19@@ -1844,12 +1844,14 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
     20     connOptions.whitelist_relay = args.GetBoolArg("-whitelistrelay", DEFAULT_WHITELISTRELAY);
     21 
     22     // Port to bind to if `-bind=addr` is provided without a `:port` suffix.
     23     const uint16_t default_bind_port =
     24         static_cast<uint16_t>(args.GetIntArg("-port", Params().GetDefaultPort()));
     25 
     26+    const uint16_t default_bind_port_onion = default_bind_port + 1;
     27+
     28     const auto BadPortWarning = [](const char* prefix, uint16_t port) {
     29         return strprintf(_("%s request to listen on port %u. This port is considered \"bad\" and "
     30                            "thus it is unlikely that any peer will connect to it. See "
     31                            "doc/p2p-bad-ports.md for details and a full list."),
     32                          prefix,
     33                          port);
     34@@ -1868,13 +1870,13 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
     35                 continue;
     36             }
     37         } else {
     38             const std::string network_type = bind_arg.substr(index + 1);
     39             if (network_type == "onion") {
     40                 const std::string truncated_bind_arg = bind_arg.substr(0, index);
     41-                bind_addr = Lookup(truncated_bind_arg, BaseParams().OnionServiceTargetPort(), false);
     42+                bind_addr = Lookup(truncated_bind_arg, default_bind_port_onion, false);
     43                 if (bind_addr.has_value()) {
     44                     connOptions.onion_binds.push_back(bind_addr.value());
     45                     continue;
     46                 }
     47             }
     48         }
     49@@ -1904,22 +1906,13 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
     50     CService onion_service_target;
     51     if (!connOptions.onion_binds.empty()) {
     52         onion_service_target = connOptions.onion_binds.front();
     53     } else if (!connOptions.vBinds.empty()) {
     54         onion_service_target = connOptions.vBinds.front();
     55     } else {
     56-        std::optional<uint16_t> onion_port;
     57-        if (args.IsArgSet("-port")) {
     58-            // If -port was specified but no onion bind, derive the onion listening port by incrementing
     59-            // that port by 1. This avoids port collisions in case of setups with multiple local nodes.
     60-            onion_port = args.GetIntArg("-port", 0) + 1;
     61-            if (IsBadPort(*onion_port)) {
     62-                InitWarning(BadPortWarning("-port", *onion_port));
     63-            }
     64-        }
     65-        onion_service_target = DefaultOnionServiceTarget(onion_port);
     66+        onion_service_target = DefaultOnionServiceTarget(default_bind_port_onion);
     67         connOptions.onion_binds.push_back(onion_service_target);
     68     }
     69 
     70     if (args.GetBoolArg("-listenonion", DEFAULT_LISTEN_ONION)) {
     71         if (connOptions.onion_binds.size() > 1) {
     72             InitWarning(strprintf(_("More than one onion bind address is provided. Using %s "
     73diff --git i/src/torcontrol.h w/src/torcontrol.h
     74index f57bf9e65f..0b66201cf1 100644
     75--- i/src/torcontrol.h
     76+++ w/src/torcontrol.h
     77@@ -24,13 +24,13 @@ extern const std::string DEFAULT_TOR_CONTROL;
     78 static const bool DEFAULT_LISTEN_ONION = true;
     79 
     80 void StartTorControl(CService onion_service_target);
     81 void InterruptTorControl();
     82 void StopTorControl();
     83 
     84-CService DefaultOnionServiceTarget(std::optional<uint16_t> port);
     85+CService DefaultOnionServiceTarget(uint16_t port);
     86 
     87 /** Reply from Tor, can be single or multi-line */
     88 class TorControlReply
     89 {
     90 public:
     91     TorControlReply() { Clear(); }
     92diff --git i/src/torcontrol.cpp w/src/torcontrol.cpp
     93index d56216a7cb..60df916f16 100644
     94--- i/src/torcontrol.cpp
     95+++ w/src/torcontrol.cpp
     96@@ -708,12 +708,12 @@ void StopTorControl()
     97         torControlThread.join();
     98         event_base_free(gBase);
     99         gBase = nullptr;
    100     }
    101 }
    102 
    103-CService DefaultOnionServiceTarget(std::optional<uint16_t> port)
    104+CService DefaultOnionServiceTarget(uint16_t port)
    105 {
    106     struct in_addr onion_service_target;
    107     onion_service_target.s_addr = htonl(INADDR_LOOPBACK);
    108-    return {onion_service_target, port.has_value() ? port.value() : BaseParams().OnionServiceTargetPort()};
    109+    return {onion_service_target, port};
    110 }
    111diff --git i/src/chainparamsbase.h w/src/chainparamsbase.h
    112index c75a70cb96..adbd6a5174 100644
    113--- i/src/chainparamsbase.h
    114+++ w/src/chainparamsbase.h
    115@@ -19,21 +19,19 @@ class ArgsManager;
    116  */
    117 class CBaseChainParams
    118 {
    119 public:
    120     const std::string& DataDir() const { return strDataDir; }
    121     uint16_t RPCPort() const { return m_rpc_port; }
    122-    uint16_t OnionServiceTargetPort() const { return m_onion_service_target_port; }
    123 
    124     CBaseChainParams() = delete;
    125-    CBaseChainParams(const std::string& data_dir, uint16_t rpc_port, uint16_t onion_service_target_port)
    126-        : m_rpc_port(rpc_port), m_onion_service_target_port(onion_service_target_port), strDataDir(data_dir) {}
    127+    CBaseChainParams(const std::string& data_dir, uint16_t rpc_port)
    128+        : m_rpc_port(rpc_port), strDataDir(data_dir) {}
    129 
    130 private:
    131     const uint16_t m_rpc_port;
    132-    const uint16_t m_onion_service_target_port;
    133     std::string strDataDir;
    134 };
    135 
    136 /**
    137  * Creates and returns a std::unique_ptr<CBaseChainParams> of the chosen chain.
    138  */
    139diff --git i/src/chainparamsbase.cpp w/src/chainparamsbase.cpp
    140index aadd04e509..060d519d92 100644
    141--- i/src/chainparamsbase.cpp
    142+++ w/src/chainparamsbase.cpp
    143@@ -38,21 +38,21 @@ const CBaseChainParams& BaseParams()
    144  * been chosen arbitrarily to keep ranges of used ports tight.
    145  */
    146 std::unique_ptr<CBaseChainParams> CreateBaseChainParams(const ChainType chain)
    147 {
    148     switch (chain) {
    149     case ChainType::MAIN:
    150-        return std::make_unique<CBaseChainParams>("", 8332, 8334);
    151+        return std::make_unique<CBaseChainParams>("", 8332);
    152     case ChainType::TESTNET:
    153-        return std::make_unique<CBaseChainParams>("testnet3", 18332, 18334);
    154+        return std::make_unique<CBaseChainParams>("testnet3", 18332);
    155     case ChainType::TESTNET4:
    156-        return std::make_unique<CBaseChainParams>("testnet4", 48332, 48334);
    157+        return std::make_unique<CBaseChainParams>("testnet4", 48332);
    158     case ChainType::SIGNET:
    159-        return std::make_unique<CBaseChainParams>("signet", 38332, 38334);
    160+        return std::make_unique<CBaseChainParams>("signet", 38332);
    161     case ChainType::REGTEST:
    162-        return std::make_unique<CBaseChainParams>("regtest", 18443, 18445);
    163+        return std::make_unique<CBaseChainParams>("regtest", 18443);
    164     }
    165     assert(false);
    166 }
    167 
    168 void SelectBaseParams(const ChainType chain)
    169 {
    

    which will also handle variants like -port=5555 -bind=127.0.0.1=onion (will result in listening on 127.0.0.1:5556 for incoming Tor connections).


    mzumsande commented at 9:11 pm on November 8, 2024:

    Sound interesting - will try to test this a bit more / and will probably change it to that early next week.

    Is this interaction between -port and -bind without port useful / necessary for anything or more of a gimmick? (it already exists on master) I kind of think as -port as an option I’d choose if I didn’t want to use -bind (and would use -bind with a port if I used it at all), so the combination seems a bit confusing.


    vasild commented at 2:16 pm on November 12, 2024:

    I don’t know how useful -port=5555 -bind=127.0.0.1=onion is or how many people use it. But it is supported on master and thus we better not brick it. And this makes our lives harder.

    Out of the scope of this PR, I am in favor of dropping support for -bind=addresswithoutport. Just to make things simpler for us and maybe also for the users that will not have to wonder what happens if some weird combination of options are used, e.g. port=1111 bind=2.2.2.2:3333.


    mzumsande commented at 7:02 pm on November 14, 2024:
    took the suggestion now!
  17. vasild commented at 5:47 pm on November 6, 2024: contributor

    Another alternative: don’t listen for Tor on regtest by default (were all reports for this for regtest?).

    What about setups that use e.g. -port=5000 and have a manually configured the Tor hidden service in torrc that redirects to 127.0.0.1:8334? E.g.:

    0HiddenServiceDir /var/db/tor/myservice/
    1HiddenServiceVersion 3
    2HiddenServicePort 8333 127.0.0.1:8334
    

    This change would silently break those setups - because we will now be listening on 127.0.0.1:5001 and other peers that try to connect to the node will get a failure because the Tor router gets connection refused on 127.0.0.1:8334 and this may remain unnoticed for a long time.

  18. i-am-yuvi commented at 7:28 am on November 8, 2024: none
    Concept ACK
  19. mzumsande commented at 10:45 pm on November 8, 2024: contributor

    Another alternative: don’t listen for Tor on regtest by default (were all reports for this for regtest?).

    I don’t know, but I could see people / orgs having local setups with multiple nodes on mainnet, e.g. with one node facing random peers from the internet/allowing inbounds, and others that only connect to local trusted peers. In general, I don’t like regtest-specific rules so I would see them as a last resort if nothing else works.

    What about setups that use e.g. -port=5000 and have a manually configured the Tor hidden service in torrc that redirects to 127.0.0.1:8334?

    Good point, I agree that this is a downside of the port +1 approach. I’m really not sure how widespread this kind of behavior is, at least our tor.md doesn’t recomment using -port for anything related to tor.

    Haven’t pushed any changes yet, will do a bit more testing for vasild’s suggested change / also look into a functional test next week.

  20. laanwj commented at 1:01 pm on November 9, 2024: member

    Another alternative: don’t listen for Tor on regtest by default (were all reports for this for regtest?).

    Agree with @mzumsande in that i don’t particularly like this; remember that regtest is primarily for testing mainnet behavior (and mainnet code paths) in our functional tests, so we want the behavior to be as close to that as possible. Every extra flag needed makes for more test-only code.

    What about setups that use e.g. -port=5000 and have a manually configured the Tor hidden service in torrc that redirects to 127.0.0.1:8334?

    It’s important to make this clear in the release notes. They’ll either have to update their Tor configuation, or specify an explicit torbind on 8334 using -bind.

    All in all i think this is much better, and more expected, behavior than a hardcoded, unchangable port.

  21. vasild commented at 2:26 pm on November 12, 2024: contributor

    It’s important to make this clear in the release notes.

    Note that the problem this PR is trying to resolve has a release note and an easy workaround. We are here because some people (many?) didn’t read the release notes even after encountering the problem (second bitcoind refuses to start because of port collision). A startup failure is an error that cannot be ignored. Whereas a silent breakage of the hidden service could remain unnoticed for a long time - one would have to check the number of incoming Tor connections, wonder why they are 0 over long period of time and investigate… I just wonder if the cure (this PR) can turn out to be worse than the disease.

  22. net, init: derive default onion port if a user specified a -port
    After port collisions are no longer tolerated but lead to
    a startup failure in v28.0, local setups of multiple nodes,
    each with a different -port value would not be possible anymore
    due to collision of the onion default port - even if the nodes
    were using tor or not interested in receiving onion inbound connections.
    
    Fix this by deriving the onion listening port to be -port + 1.
    (idea by vasild / laanwj)
    
    Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
    0e2b12b92a
  23. mzumsande force-pushed on Nov 14, 2024
  24. mzumsande referenced this in commit 74f5106d8a on Nov 14, 2024
  25. mzumsande commented at 7:15 pm on November 14, 2024: contributor

    Updated PR (took much longer than expected due to a medical issue, sorry) and I still plan on adding a functional test, hopefully tomorrow.

    • took vasild’s suggestion, so that OnionServiceTargetPort() is removed from chainparamsbase
    • expanded release note
  26. in doc/release-notes-31223.md:8 in 74f5106d8a outdated
    0@@ -0,0 +1,8 @@
    1+P2P and network changes
    2+-----------------------
    3+When the `-port` configuration option is used, the default onion listening port will now
    4+be derived to be that port + 1 instead of being set to a fixed value (8334 on mainnet).
    5+This re-allows setups with multiple local nodes using different `-port` and not using `-bind`,
    6+which would lead to a startup failure in v28.0 due to a port collision.
    7+Note that a `HiddenServicePort` manually configured in `torrc` may need adjustment if used in
    8+connection with the `-port` option. (#31223)
    


    vasild commented at 9:08 am on November 15, 2024:

    Maybe further elaborate this with an example:

    If you are using -port= with non-standard value, for example -port=5555 and not using -bind=...=onion, previously Bitcoin Core would listen for incoming Tor connections on 127.0.0.1:8334. Now it would listen on 127.0.0.1:5556 (-port plus one). If you configured the hidden service manually in torrc now you have to change it from HiddenServicePort 8333 127.0.0.1:8334 to HiddenServicePort 8333 127.0.0.1:5556, or configure bitcoind with -bind=127.0.0.1:8334=onion to get the previous behavior.


    mzumsande commented at 9:29 pm on November 15, 2024:
    done (in a slightly adjusted form)
  27. vasild commented at 9:22 am on November 15, 2024: contributor

    I reviewed the code and it looks safe and will achieve the intended purpose.

    Since I introduced the current behavior which this PR is aiming to fix, I understand that I may have a bias towards downplaying the current issue (port collision) and a biased preference towards “It’s fine, don’t do anything”. However, even with this realization, I can’t help but think whether this will introduce a bigger problem than the one it solves: silent breakage vs obvious breakage.

    Thus I will neither approve nor disapprove this. In other words - I do not object this if enough other people think it is good.

  28. test: add functional test for -port behavior 997757dd2b
  29. Add release note for #31223
    Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
    1dd3af8fbc
  30. mzumsande force-pushed on Nov 15, 2024
  31. mzumsande commented at 9:39 pm on November 15, 2024: contributor

    Updated Release note and added a commit that adds a functional test for various aspects of -port that have been discussed in this PR an the issue.

    When it comes to the question whether the issue should be fixed in 28.1, I guess it also comes down to how widespread we think a scenario with -port and a manual torrc is, vs how many more people doing local testing setups would be affected by the -port change from 28.0. I can’t say I have a good feeling for either - to be honest I would have expected way more people to complain about their local setups not working anymore, than actually did in the last couple of weeks.

    But, to be clear, if reviewers think it’s better not to make another change now and tell affected users to use -bind instead of -port, I’m happy to close this / maybe still add a functional test for -port so that future changes in this area are more likely to be caught by tests.

  32. tdb3 approved
  33. tdb3 commented at 1:44 am on November 17, 2024: contributor

    Code review ACK 1dd3af8fbc350c6f1efa8ae6449e67e1b42ccff4

    if reviewers think it’s better not to make another change now and tell affected users to use -bind instead of -port

    I don’t feel very strongly either way, but I lean toward implementing the port+1 approach in this PR.

    still add a functional test for -port so that future changes in this area are more likely to be caught by tests.

    I support this. It’s descriptive and helps catch issues later.

  34. DrahtBot requested review from laanwj on Nov 17, 2024

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-03 15:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me