Fix net grouping of non-IP networks #27369

1. 
   vasild commented at 11:39 am on March 30, 2023: member

   Current behaviour

   NetGroupManager::GetGroup() was designed with IP networks in mind, where for example 10.20.30.40 and 10.20.30.45 are “close” and likely to belong to the same entity, while 100.20.30.40 is “distant” from them. This does not apply however for Tor, I2P and CJDNS where the address is “random” bytes. Actually, Tor, I2P and CJDNS addresses that have common prefix are harder to obtain than ones that don’t have a common prefix (the opposite compared to IP).

   Currently NetGroupManager::GetGroup() is doing prefixing on Tor, I2P and CJDNS addresses and assumes same properties apply as for IP addresses. This magically “works” to some extent but has drawbacks. Some serious changes are needed to remove that assumption from the code.

   See #22563 #27264 (comment)

   Expected behaviour

   • Don’t do prefixing on Tor, I2P and CJDNS and don’t assume proximity based on that.
   • Treat all addresses from Tor as one group (because it is very easy to obtain huge amount of them). Same for I2P and CJDNS.
   • Make sure addrman can still store enough addresses from those networks.
   • Make sure a single source from Tor, I2P and CJDNS cannot flood addrman with junk due to the previous point.
2. 
   stratospher commented at 11:54 am on March 30, 2023: contributor

   #27264 (comment) - workaround it by not inserting tor/i2p/cjdns address in setConnected for now? so GetGroup wouldn’t be called on these networks.
3. 
   mzumsande commented at 2:21 pm on March 30, 2023: member

   fyi @amitiuttarwar @naumenkogs @sdaftuar

   I think there are two aspects to this that don’t necessarily need be addressed at the same time:

   1. The rule not to make more than one outbound connection to a netgroup
   2. Netgroup-based bucketing in addrman

   I think 1) is more urgent right now, because the scenario outlined in #27264 (comment) - it’s likely that there are nodes out there that are -onlynet=onion and try to compensate for the added eclipsing risk by having 8 manual connections - those might run into problems filling up all of their outbound slots with current master.

   So, in the short term (25.0), I think that we should either temporarily revert 72e8ffd7f8dbf908e65da6d012ede914596737ab or do what @stratospher suggested, if that could still make it into the release.

   I don’t have a good suggestion for the long term right now.

4. 
   sdaftuar commented at 2:46 pm on March 30, 2023: member

   #27264 (comment) - workaround it by not inserting tor/i2p/cjdns address in setConnected for now? so GetGroup wouldn’t be called on these networks.

   This seems clearly correct to me – I think it was an oversight that setConnected picks up netgroups for non-IPV4/IPV6 networks in the first place.

5. 
   stratospher commented at 3:02 pm on March 30, 2023: contributor

   opened #27374 to skip tor/i2p/cjdns addresses in setConnected for now.
6. achow101 referenced this in commit 2bfe43db16 on Apr 13, 2023

Contributors
vasild stratospher mzumsande sdaftuar