p2p: don't exclusively drop tor/i2p outgoing cxns in idle loop #8330
Conversation
| const auto it = m_max_out_peers.find(zone); | ||
| if (it == m_max_out_peers.end()) | ||
| { | ||
| MWARNING(epee::net_utils::zone_to_string(zone) << " max out peers not set, using default"); |
There was a problem hiding this comment.
Do you really want to spam people who set no limit ? I assume this'd do. It maybe a rare case, but it's legitimate. Or is a max always set even if the user doesn't set one ?
There was a problem hiding this comment.
Maybe I'm missing something but I can't see how it wouldn't get set by a regular user; it should always get set in here even if a user sets a limit of 0 or doesn't include the --out-peers flag: https://github.com/j-berman/monero/blob/299fbd007687534a3b5abb87c2b3f2f596ecdacb/src/p2p/net_node.inl#L388
The only way I believe it wouldn't get set is if a dev is doing something custom where they skip setting it before calling a function that reads it, like in the unit test I updated in this PR (which was hanging before I made this change). I.e. it should always be set explicitly before it's read in normal use.
Maybe there's a better way to handle this? Default to 0 in case it's not set, and maybe only warn once?
There was a problem hiding this comment.
I did not look. If it's impossible with "normal" usage, it's fine by me.
There was a problem hiding this comment.
I did not look. If it's impossible with "normal" usage, it's fine by me.
| @@ -171,7 +181,7 @@ namespace cryptonote | |||
| epee::math_helper::once_a_time_milliseconds<100> m_standby_checker; | |||
| epee::math_helper::once_a_time_seconds<101> m_sync_search_checker; | |||
| epee::math_helper::once_a_time_seconds<43> m_bad_peer_checker; | |||
| std::atomic<unsigned int> m_max_out_peers; | |||
| std::unordered_map<epee::net_utils::zone, std::atomic<unsigned int>> m_max_out_peers; | |||
There was a problem hiding this comment.
The map should be atomic to match the previous semantics.
|
@j-berman please squash |
Context
update_sync_searchruns in the idle loop, checking to see if we have outgoing connections >= the max allowed. If so, a sync'd connection gets dropped so that the daemon can try to connect to nodes that need to sync.Before this PR
Currently it's comparing the tally of clearnet + tor + i2p outgoing connections to the max of outgoing clearnet peers; but, the tally of clearnet + tor + i2p outgoing peers is usually always going to be greater than the max allowed outgoing clearnet peers. Because of this, it's usually just going to drop a tor or i2p connection right here (because they are last when iterating over all connections), even if the daemon has just 1 outgoing tor or i2p connection.
This seems to be another source of tor/i2p connection reliability issues mentioned in #6938, #6631, #8251.
This PR
This PR makes sure to tally outgoing connections by zone, then compare to the max by zone. If the max peer count set for tor is 10 for example, and there are <10 outgoing tor connections, an outgoing tor connection won't get dropped.
Future PR
I left a couple TODO's to investigate updating the pruning logic correctly. Didn't seem as trivial to figure out, so I left the logic how it currently is (just comparing the tally to the clearnet max).