This is somewhat hackish, but a very small change, in case it is desired for 22.0.

The underlying problem is that CNetAddr::GetGroup() (edit: this is now NetGroupManager::GetGroup()) and the places where it is called are build around the assumption that it is easier to obtain addresses with common prefix (e.g. belonging to one IP subnet). However this assumption is not true for Tor, I2P and CJDNS addresses.

For example, we would avoid connecting to aabcd.onion if we already have connection to aaxyz.onion, assuming that the common prefix makes it more likely they belong to the same actor.

CNetAddr::GetGroup() NetGroupManager::GetGroup only makes sense for the IPv4 and IPv6 networks. To resolve this properly a bigger refactor would be required - where we call GetGroup() only for IPv4 and IPv6 addresses and treat Tor/I2P/CJDNS separately.

The underlying problem is that CNetAddr::GetGroup() and the places where it is called are build around the assumption that it is easier to obtain addresses with common prefix (e.g. belonging to one IP subnet).

Doesn't CNetAddr::GetGroup() in netaddress.cpp already have special casing for Tor/I2P/CJDNS (nBits = 4 , resulting in num_bytes == 0 ) that would make this unnecessary (because it already treats aabcd.onion the same as aaxyz.onion and every other Tor address)?

I don't think so - currently CNetAddr::GetGroup() treats aabcd.onion and aaxyz.onion the same, but not the same as every other Tor address. The way I read it, CNetAddr::GetGroup() will return a vector of 2 bytes for a Tor address: the first byte will be 0x03 (NET_ONION), set here:

the second byte is set here:

and will consist of the first 4 bits of the Tor address, followed by 4 1-bits.

You are right, I missed the spot where the second byte is set.

Your eyes, on their own, decided to skip the line that contains ] | ((1 << (8 - nBits)) - 1)); (mine did) 🤯 🤣

Yes, I wonder why 😀

Another question (still not sure I have completely understood the bucketing...):
Wouldn't this mean that addresses advertised to us by peers with a Tor address would always map to the same bucket/position, regardless of the source? And, as a result:

1. if we only have Tor peers, we would only ever reach a multiplicity of one in the New tables, so that the nRefCount mechanism to give addrs advertised to us by many diverse peers more weight in New would no longer work in this situation?
2. We'd only ever use 64 of the 1024 buckets for addrs received from Tor addresses

The relevant code is here.

(talking about the "new" buckets)

Correct. This PR makes all Tor sources be treated as a single source, so the same address received from different Tor sources will end up in the same bucket/position. Overall, all addresses received by Tor sources will be dispersed amongst 64 ADDRMAN_NEW_BUCKETS_PER_SOURCE_GROUP buckets.

Notice that this is already the case for Tor peers that have connected to us (incoming from our point of view) because all of them look to us as coming from 127.0.0.1, so all of them belong to a single group. This plays nicely because an attacker looking to fill a victim's addrman new buckets can generate many different Tor addresses and connect from all of them to the victim (mimicking different sources), but all will be treated as one source (127.0.0.1). This is not the case for I2P and CJDNS.

Maybe this patch is too aggressive and should treat as one group sources that are incoming Tor/I2P/CJDNS and leave the 16 groups (4 bits) for sources that we have connected to (outgoing).

Concept ACK, will look more deeply.

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

Reviews

See the guideline for information on the review process.

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

Conflicts

No conflicts as of last run.

Please also consider adding Yggdrasil to the list. #23531

228a8c975a...71cb144cb4: rebase due to conflicts

This is much relevant as it was when I opened it. I am just not sure whether the change may have some unexpected adverse effects. I will eventually reconsider this and maybe move it from "draft" to "open".

Concept ACK

Yes. But I think I should not give up on treating Tor sources as one source because this is what it is in reality. And then, addrman should be able to accommodate 10s of thousands of addresses from one source (if the source is Tor, I2P or CJDNS) and still not allow a single malicious entity to flood it.

71cb144cb4...9d2cc3f8ab: rebase due to conflicts

To resolve this properly a bigger refactor would be required - where we call GetGroup() only for IPv4 and IPv6 addresses and treat Tor/I2P/CJDNS separately.

Part of this was already done in #27374 which limited GetGroup() calls for IPv4/6 peers.

Remaining pieces of code that depend on the output of GetGroup() and pass non-IPv4/6 addresses are:

• the eviction logic (see nKeyedNetGroup in src/node/eviction.cpp) and
• addrman bucketing (see GetTriedBucket() and GetNewBucket() in addrman.cpp).

Is this ready for review?

@0xB10C this is a bit hackish and, as an undesirable side-effect, will change the number of buckets in addrman that are available for tor/i2p/cjdns addresses. I would rather embark on a bigger change where GetGroup() is only called for IPv4/6 addresses and the distinction between IPv4/6=use GetGroup() VS tor/i2p/cjdns=do something else be made in the higher level code (eviction logic and addrman).

🤔 There hasn't been much activity lately and the CI seems to be failing.

If no one reviewed the current pull request by commit hash, a rebase can be considered. While the CI failure may be a false positive, the CI hasn't been running for some time, so there may be a real issue hiding as well. A rebase triggers the latest CI and makes sure that no silent merge conflicts have snuck in.

Closing this because I think the proper solution is a bigger change as mentioned above in #22563 (comment):

If the address is IPv4 or IPv6 then use GetGroup()
Else do something else (for Tor, I2P and CJDNS addresses).

Currently GetGroup() is called for all types of addresses from:
AddrInfo::GetTriedBucket()
AddrInfo::GetNewBucket()
CConnman::CalculateKeyedNetGroup() // saved in CNode::nKeyedNetGroup and later used during eviction
CConnman::ThreadOpenConnections() // already only called for IPv4 or IPv6 addresses, good!

If somebody works on this, I would be happy to review. Otherwise maybe I will pick it at a later time.