Allow to specify the number of Tor connections and clearnet connections separately

ghost commented at 6:05 pm on July 12, 2020: none

Is your feature request related to a problem? Please describe. Maybe but that’s not the (only) reason for my feature request. I have the expression I’m losing inbound Tor connections relative to clearnet connections once the default maximum of 125 connections is reached for a listening node

EDIT (almost 2 weeks after the creation of the issue):

I determine the number of inbound tor connections as:

bitcoin-cli getpeerinfo | jq '.[].addr' | grep 127.0.0.1 | wc -l

Note: I get 132 max connections because I add some outbound tor connections by hand.

Some statistics after restarting bitcoind: date: total connections/inbound tor connections 14 juli: 100/17 16 juli: 120/15 22 juli: 131/10 24 juli: 130/7 25 juli: 130/5 26 juli: 132/4 27 juli: 129/3

So it’s very likely I lose inbound tor connections once the maximum number of connections is reached. Please add the bug label.

Describe the solution you’d like Allow to specify the number of Tor connections and clearnet connections separately. For example, numberOnion=25 would reserve 25 Tor connections of the default 125 maximum number of connections.

Describe alternatives you’ve considered Fix the bug first :-)

Additional context Maybe this issue should be split into 2 issues. One with the feature request and one describing the bug.

unknown added the label Feature on Jul 12, 2020

gmaxwell commented at 6:23 pm on July 12, 2020: contributor

The downside of this request as phrased is that you end up with an additional end user tunable with no particular guidance on how to use it. There is nothing wrong with it otherwise, but I think sumbtc’s issue is more a symptom of the eviction logic being tor blind when it shouldn’t be.

This is particularly concerning because tor peers are latency and reliability disadvantaged so on a mixed network host they’re not likely to win at any of the anti-eviction criteria. That’s an oversight.

I think that at a minimum the eviction logic should be made onion-aware by adding a step before the size()/2 longest uptime peers that protects the size()/4 longest uptime localhost peers, and then subtracts what it protected from the number of longest uptime peers it protects… so still half longest uptime, but up to half of those from localhost even if they weren’t the longest uptime overall.

One could go further and split other criteria the same way, but I doubt its worth the complexity to go that far, maybe just the blocks criteria if someone wanted to go further.

ghost commented at 6:35 pm on July 12, 2020: none

@gmaxwell Feel free to phrase (close or reopen) the issue in a different way. I’m just a user, not a developer.

gmaxwell commented at 0:23 am on July 13, 2020: contributor

@sumBTC Yep you did everything fine. It’s understood that the response to a issue might not be exactly what the issue was literally asking for. Thanks for reporting the behaviour, it’s absolutely an oversight.

jonatack commented at 5:18 am on July 13, 2020: member

I think that at a minimum the eviction logic should be made onion-aware by adding a step before the size()/2 longest uptime peers that protects the size()/4 longest uptime localhost peers, and then subtracts what it protected from the number of longest uptime peers it protects… so still half longest uptime, but up to half of those from localhost even if they weren’t the longest uptime overall. @gmaxwell I’d be interested to propose an implementation, unless you plan to.

gmaxwell commented at 10:44 am on July 13, 2020: contributor

@jonatack I don’t plan to though if you are interested in my comments I’d be happy to comment on one if you did.

ghost commented at 6:48 am on July 31, 2020: none

@jonatack If you have a suggestion for a fix, I’ll be happy to insert it in the code and recompile Bitcoin Core (0.20.0) and test it. I always compile the Bitcoin software myself. Testing it out will take some time (a few weeks probably).

jonatack commented at 7:01 am on July 31, 2020: member

@sumBTC very nice – soon, will definitely ping you. I’ve been watching this, and like you, seem to be having trouble keeping inbound onion connections.

ghost commented at 7:11 am on July 31, 2020: none

@jonatack Good to know you confirm the issue! Is it possible/needed to add the “bug” label?

jonatack commented at 7:14 am on July 31, 2020: member

@sumBTC possibly, but the main thing is that you signaled it (thanks!), there’s an initial diagnosis, and it’s being worked on :)

ghost commented at 8:55 am on August 18, 2020: none

@jonatack @sdaftuar @gmaxwell I’ve copied the changes as proposed in #19670 to net.cpp, added a few print statements to some file for monitoring and recompiled bitcoind. Things are looking good now. For default values (125 max connections) the possible eviction kicks-in at 114 inbound peers and onion peers are well protected. I now see up to 125 connections of which around 21 are inbound onion peers. However, at the moment my testing capabilities are limited because sometimes I have problems with my external internet cable (oxidation and leakage which will be repaired September 25). I expect more in depth analyses from your side with the -netinfo dashboard but from what I’ve seen the issue is resolved and can be closed. Suggestion: maybe this a good opportunity to also think about what happens to outbound onion peers?

jonatack commented at 9:18 am on August 18, 2020: member

@sumBTC thanks for coming back on this with your feedback. While writing a patch, I found myself gated on how to write functional tests before proposing the change, which is why I opened #19731 in addition to #19643. Until the test coverage groundwork is done or possibly just the patch proposed in #19670 is merged, it may be fine to leave this issue open? Looking forward to hearing more feedback from you over time.

ghost commented at 9:32 am on August 18, 2020: none

@jonatack thanks for all your hard work, kind words and immediate feedback! I’ll leave it to the core developers to close this issue.

jonatack commented at 10:41 am on August 18, 2020: member

@sumBTC another PR was opened as well: #19728. Your issue was a valuable suggestion.

laanwj referenced this in commit 7f609f68d8 on Aug 24, 2020

sidhujag referenced this in commit 29ed917b69 on Aug 24, 2020

laanwj closed this on Sep 3, 2020

laanwj referenced this in commit dede9eb924 on Mar 30, 2021

DrahtBot locked this on Feb 15, 2022

Allow to specify the number of Tor connections and clearnet connections separately #19500