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

ghost · 2020-07-12T18:05:31Z

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.

The text was updated successfully, but these errors were encountered:

gmaxwell · 2020-07-12T18:23:38Z

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 · 2020-07-12T18:35:08Z

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

gmaxwell · 2020-07-13T00:23:43Z

@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 · 2020-07-13T05:18:25Z

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 · 2020-07-13T10:44:59Z

@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 · 2020-07-31T06:48:15Z

@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 · 2020-07-31T07:01:47Z

@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 · 2020-07-31T07:11:33Z

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

jonatack · 2020-07-31T07:14:35Z

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

ghost · 2020-08-18T08:55:49Z

@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 · 2020-08-18T09:18:15Z

@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 · 2020-08-18T09:32:40Z

@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 · 2020-08-18T10:41:09Z

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

…ck/last_transaction in getpeerinfo 5da9621 doc: release note for getpeerinfo last_block/last_transaction (Jon Atack) cfef5a2 test: rpc_net.py logging and test naming improvements (Jon Atack) 21c57ba test: getpeerinfo last_block and last_transaction tests (Jon Atack) 8a560a7 rpc: expose nLastBlockTime/TXTime as getpeerinfo last_block/transaction (Jon Atack) 02fbe3a net: add nLastBlockTime/TXTime to CNodeStats, CNode::copyStats (Jon Atack) Pull request description: This PR adds inbound peer eviction criteria `nLastBlockTime` and `nLastTXTime` to `CNodeStats` and `CNode::copyStats`, which then allows exposing them in the next commit as `last_transaction` and `last_block` Unix Epoch Time fields in RPC `getpeerinfo`. This may be useful for writing missing eviction tests. I'd also like to add `lasttx` and `lastblk` columns to the `-netinfo` dashboard as described in #19643 (comment). Relevant discussion at the p2p irc meeting http://www.erisian.com.au/bitcoin-core-dev/log-2020-08-11.html#l-549: ```text <jonatack> i was specifically trying to observe and figure out how to test #19500 <jonatack> which made me realise that i didn't know what was going on with my peer conns in enough detail <jonatack> i'm running bitcoin locally with nLastBlockTime and nLastTXTime added to getpeerinfo for my peer connections dashboard <jonatack> sipa: is there a good reason why that (eviction criteria) data is not exposed through getpeerinfo currently? <sipa> jonatack: nope; i suspect just nobody ever added it <jonatack> sipa: thanks. will propose. ``` The last commit is optional, but I think it would be good to have logging in `rpc_net.py`. ACKs for top commit: jnewbery: Code review ACK 5da9621 theStack: Code Review ACK 5da9621 darosior: ACK 5da9621 Tree-SHA512: 2db164bc979c014837a676e890869a128beb7cf40114853239e7280f57e768bcb43bff6c1ea76a61556212135281863b5290b50ff9d24fce16c5b89b55d4cd70

…ast block/last_transaction in getpeerinfo 5da9621 doc: release note for getpeerinfo last_block/last_transaction (Jon Atack) cfef5a2 test: rpc_net.py logging and test naming improvements (Jon Atack) 21c57ba test: getpeerinfo last_block and last_transaction tests (Jon Atack) 8a560a7 rpc: expose nLastBlockTime/TXTime as getpeerinfo last_block/transaction (Jon Atack) 02fbe3a net: add nLastBlockTime/TXTime to CNodeStats, CNode::copyStats (Jon Atack) Pull request description: This PR adds inbound peer eviction criteria `nLastBlockTime` and `nLastTXTime` to `CNodeStats` and `CNode::copyStats`, which then allows exposing them in the next commit as `last_transaction` and `last_block` Unix Epoch Time fields in RPC `getpeerinfo`. This may be useful for writing missing eviction tests. I'd also like to add `lasttx` and `lastblk` columns to the `-netinfo` dashboard as described in bitcoin#19643 (comment). Relevant discussion at the p2p irc meeting http://www.erisian.com.au/bitcoin-core-dev/log-2020-08-11.html#l-549: ```text <jonatack> i was specifically trying to observe and figure out how to test bitcoin#19500 <jonatack> which made me realise that i didn't know what was going on with my peer conns in enough detail <jonatack> i'm running bitcoin locally with nLastBlockTime and nLastTXTime added to getpeerinfo for my peer connections dashboard <jonatack> sipa: is there a good reason why that (eviction criteria) data is not exposed through getpeerinfo currently? <sipa> jonatack: nope; i suspect just nobody ever added it <jonatack> sipa: thanks. will propose. ``` The last commit is optional, but I think it would be good to have logging in `rpc_net.py`. ACKs for top commit: jnewbery: Code review ACK 5da9621 theStack: Code Review ACK 5da9621 darosior: ACK 5da9621 Tree-SHA512: 2db164bc979c014837a676e890869a128beb7cf40114853239e7280f57e768bcb43bff6c1ea76a61556212135281863b5290b50ff9d24fce16c5b89b55d4cd70

…eviction protection test coverage 0cca08a Add unit test coverage for our onion peer eviction protection (Jon Atack) caa21f5 Protect onion+localhost peers in ProtectEvictionCandidatesByRatio() (Jon Atack) 8f1a53e Use EraseLastKElements() throughout SelectNodeToEvict() (Jon Atack) 8b1e156 Add m_inbound_onion to AttemptToEvictConnection() (Jon Atack) 72e30e8 Add unit tests for ProtectEvictionCandidatesByRatio() (Jon Atack) ca63b53 Use std::unordered_set instead of std::vector in IsEvicted() (Jon Atack) 41f84d5 Move peer eviction tests to a separate test file (Jon Atack) f126cbd Extract ProtectEvictionCandidatesByRatio from SelectNodeToEvict (Jon Atack) Pull request description: Now that #19991 and #20210 have been merged, we can determine inbound onion peers using `CNode::m_inbound_onion` and add it to the localhost peers protection in `AttemptToEvictConnection`, which was added in #19670 to address issue #19500. Update 28 February 2021: I've updated this to follow gmaxwell's suggestion in #20197 (comment). This branch now protects up to 1/4 onion peers (connected via our tor control service), if any, sorted by longest uptime. If any (or all) onion slots remain after that operation, they are then allocated to protect localhost peers, or a minimum of 2 localhost peers in the case that no onion slots remain and 2 or more onion peers were protected, sorted as before by longest uptime. This patch also adds test coverage for the longest uptime, localhost, and onion peer eviction protection logic to build on the welcome initial unit testing of #20477. Suggest reviewing the commits that move code with `colorMoved = dimmed-zebra` and `colorMovedWs = allow-indentation-change`. Closes #11537. ACKs for top commit: laanwj: Code review ACK 0cca08a vasild: ACK 0cca08a Tree-SHA512: 2f5a63f942acaae7882920fc61f0185dcd51da85e5b736df9d1fc72343726dd17da740e02f30fa5dc5eb3b2d8345707aed96031bec143d48a2497a610aa19abd

ghost added the Feature label Jul 12, 2020

jonatack mentioned this issue Aug 2, 2020

Add -netinfo peer connections dashboard #19643

Merged

sdaftuar mentioned this issue Aug 5, 2020

Protect localhost and block-relay-only peers from eviction #19670

Merged

fanquake linked a pull request Aug 6, 2020 that will close this issue

Protect localhost and block-relay-only peers from eviction #19670

Merged

laanwj closed this as completed in #19670 Sep 3, 2020

This was referenced Oct 20, 2020

p2p: improve onion detection in AttemptToEvictConnection #20193

Closed

p2p: protect onions in AttemptToEvictConnection(), add eviction protection test coverage #20197

Merged

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ghost commented Jul 12, 2020 •

edited by ghost

gmaxwell commented Jul 12, 2020

ghost commented Jul 12, 2020 •

edited by ghost

gmaxwell commented Jul 13, 2020 •

edited

jonatack commented Jul 13, 2020

gmaxwell commented Jul 13, 2020

ghost commented Jul 31, 2020 •

edited by ghost

jonatack commented Jul 31, 2020

ghost commented Jul 31, 2020 •

edited by ghost

jonatack commented Jul 31, 2020

ghost commented Aug 18, 2020

jonatack commented Aug 18, 2020 •

edited

ghost commented Aug 18, 2020

jonatack commented Aug 18, 2020

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

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

Comments

ghost commented Jul 12, 2020 • edited by ghost

gmaxwell commented Jul 12, 2020

ghost commented Jul 12, 2020 • edited by ghost

gmaxwell commented Jul 13, 2020 • edited

jonatack commented Jul 13, 2020

gmaxwell commented Jul 13, 2020

ghost commented Jul 31, 2020 • edited by ghost

jonatack commented Jul 31, 2020

ghost commented Jul 31, 2020 • edited by ghost

jonatack commented Jul 31, 2020

ghost commented Aug 18, 2020

jonatack commented Aug 18, 2020 • edited

ghost commented Aug 18, 2020

jonatack commented Aug 18, 2020

ghost commented Jul 12, 2020 •

edited by ghost

ghost commented Jul 12, 2020 •

edited by ghost

gmaxwell commented Jul 13, 2020 •

edited

ghost commented Jul 31, 2020 •

edited by ghost

ghost commented Jul 31, 2020 •

edited by ghost

jonatack commented Aug 18, 2020 •

edited