p2p: skip netgroup diversity of new connections for tor/i2p/cjdns

stratospher commented at 2:59 pm on March 30, 2023: contributor

Follow up for #27264.

In order to make sure that our persistent outbound slots belong to different netgroups, distinct net groups of our peers are added to setConnected. We’d only open a persistent outbound connection to peers which have a different netgroup compared to those netgroups present in setConnected.

Current GetGroup() logic assumes route-based diversification behaviour for tor/i2p/cjdns addresses (addresses are public key based and not route-based). Distinct netgroups possible (according to the current GetGroup() logic) for:

tor => 030f, 031f, .. 03ff (16 possibilities)
i2p => 040f, 041f, .. 04ff (16 possibilities)
cjdns => 05fc0f, 05fc1f, … 05fcff (16 possibilities)

setConnected is used in ThreadOpenConnections() before making outbound and anchor connections to new peers so that they belong to distinct netgroups.

behaviour on master

if we run a node only on tor/i2p/cjdns
- we wouldn’t be able to open more than 16 outbound connections(manual, block-relay-only anchor, outbound full relay, block-relay-only connections) because we run out of possible netgroups.
- see #27264 (comment)
- tested by changing MAX_OUTBOUND_FULL_RELAY_CONNECTIONS to 17 with onlynet=onion and observed how node wouldn’t make more than 16 outbound connections.

behaviour on PR

netgroup diversity checks are skipped for tor/i2p/cjdns addresses.
we don’t insert tor/i2p/cjdns address in setConnected and GetGroup doesn’t get called on tor/i2p/cjdns(see #27369)

DrahtBot commented at 2:59 pm on March 30, 2023: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	mzumsande, vasild, achow101
Concept ACK	sdaftuar, jonatack

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.

DrahtBot added the label P2P on Mar 30, 2023

in src/net.cpp:1713 in 542ac836f1 outdated

1709@@ -1710,6 +1710,11 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect)
1710                 if (pnode->IsFullOutboundConn()) nOutboundFullRelay++;
1711                 if (pnode->IsBlockOnlyConn()) nOutboundBlockRelay++;
1712 
1713+                CAddress address{pnode->addr};

sdaftuar commented at 3:37 pm on March 30, 2023:

nit: If you touch this again, I think it would be worth adding a comment explaining why this code is here (and probably also add a comment to where the lookup in setConnected happens down below, near line 1806, so that if someone is touching the code in just one place they are aware of this issue).

stratospher commented at 12:08 pm on March 31, 2023:

thanks! done.

sdaftuar approved

sdaftuar commented at 3:38 pm on March 30, 2023: member

utACK.

I think the other way we could write this would be to leave setConnected unchanged, and change the logic where we look up in setConnected to only do the lookup for non-IPV4/IPV6 peers. It doesn’t seem to me like it matters much which way we do this, since I believe netgroups for different networks cannot collide, so this looks fine to me.

fanquake added this to the milestone 25.0 on Mar 30, 2023

in src/net.cpp:1714 in 542ac836f1 outdated

1709@@ -1710,6 +1710,11 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect)
1710                 if (pnode->IsFullOutboundConn()) nOutboundFullRelay++;
1711                 if (pnode->IsBlockOnlyConn()) nOutboundBlockRelay++;
1712 
1713+                CAddress address{pnode->addr};
1714+                if (address.IsTor() || address.IsI2P() || address.IsCJDNS()){

mzumsande commented at 5:40 pm on March 30, 2023:

How about putting this into a seperate function (something like bool LimitOutboundByNetgroup(Network n)), and using a switch statement there, similar to the existing IsAddrV1Compatible()? That way, the compiler would prevent us from forgetting about this if we ever add another network.

stratospher commented at 12:07 pm on March 31, 2023:

thanks! done.

mzumsande commented at 6:11 pm on March 30, 2023: contributor

Concept ACK

I think the same logic needs to be applied to the anchors in https://github.com/bitcoin/bitcoin/blob/328087d16f4362a23a72575fee930a275efbf3dd/src/net.cpp#L1803-L1805 Otherwise we could choose e.g. two onion anchors from the same onion netgroup, but after a restart refuse to reconnect to one, which defeats the point of anchors.

I think the other way we could write this would be to leave setConnected unchanged, and change the logic where we look up in setConnected to only do the lookup for non-IPV4/IPV6 peers.

setConnected is used also at https://github.com/bitcoin/bitcoin/blob/328087d16f4362a23a72575fee930a275efbf3dd/src/net.cpp#L1890 to determine the fCountFailure bool, which tells addrman to possibly deprioritize selecting an address after various failed attempts (nAttempts). As far as I can see, this logic should equally be applied to addrs from alt networks, so it might be better to leave setConnected unchanged as sdaftuar suggested?

vasild commented at 5:15 am on March 31, 2023: contributor

Does this PR trick the code to behave as if we are not connected to Tor (by not inserting into setConnected) even if we are connected? It looks to me that trying to solve the problem of “cannot open many connections to Tor” this would create another problem: “too many connections to Tor” - it would allow all 8 outbound to be to Tor and will not try to diversify to IPv4 (or other networks).

I feel that we should lookup -onlynet or allow connections to Tor up to a limit to avoid “too many connections to Tor”. Or maybe, increase the 4 bits to 5 bits in GetGroup() :disappointed:. In the long term, I guess, it would be best to expand the concept of “network group” to “network group with route based diversification” (IP) + “network group without route based diversification” (tor/i2p/cjdns).

PS This is very much related to #27213 PS run clang-format

stratospher commented at 10:29 am on March 31, 2023: contributor

It doesn’t seem to me like it matters much which way we do this, since I believe netgroups for different networks cannot collide, so this looks fine to me.

I think the same logic needs to be applied to the anchors in

i didn’t add tor/i2p/cjdns peer addresses in setConnected to skip the netgroup diversity check when making both outbound and anchor connections. (now understood from the above comments why it won’t work)

As far as I can see, this logic should equally be applied to addrs from alt networks, so it might be better to leave setConnected unchanged as sdaftuar suggested?

I feel that we should lookup -onlynet or allow connections to Tor up to a limit to avoid “too many connections to Tor”. @mzumsande, @vasild that’s true! summarising the 2 options suggested:

Add an additional check inside LimitOutboundByNetgroup() to skip netgroup diversity checks for outbound connections belonging to pure Tor-only/I2P-only/CJDNS-only networks. Then use this as a gatekeeper before choosing diverse netgroup addresses to make outbound connections to.
increase the 4 bits to 5 bits in GetGroup()
- this seems to be the simplest option! but i’m unsure about how it will affect other areas in the codebase where GetGroup() is used.

curious to know what everyone thinks about option 2? (otherwise we can go with option 1 for now)

stratospher force-pushed on Mar 31, 2023

stratospher commented at 12:07 pm on March 31, 2023: contributor

updated the PR to do option 1:

Added an additional check inside LimitOutboundByNetgroup() to skip netgroup diversity checks for outbound connections belonging to pure Tor-only/I2P-only/CJDNS-only networks.
used this function as a gatekeeper before choosing diverse netgroup addresses to make outbound connections to.

still curious to know opinions about increasing 4 bits to 5 bits in GetGroup().

in src/netaddress.cpp:526 in baa513042b outdated

521+{
522+    const auto onlynets = gArgs.GetArgs("-onlynet");
523+    if (onlynets.size() != 1) {
524+        return true;
525+    }
526+    switch (m_net) {

stratospher commented at 12:13 pm on March 31, 2023:

we would reach here when size of onlynets is 1. it’s possible that m_net doesn’t belong to the network we mention in onlynets but that would be handled in reachability check in ThreadOpenConnections() and we wouldn’t use that address anyways.

EDIT: new patch suggested in #27374 (comment) uses a better approach without the gatekeeper function.

fanquake commented at 12:26 pm on March 31, 2023: member

https://github.com/bitcoin/bitcoin/pull/27374/checks?check_run_id=12424651425

0A new circular dependency in the form of "netaddress -> netbase -> netaddress" appears to have been introduced.
1
2^---- failure generated from lint-circular-dependencies.py

sdaftuar commented at 1:35 pm on March 31, 2023: member

https://github.com/bitcoin/bitcoin/blob/328087d16f4362a23a72575fee930a275efbf3dd/src/net.cpp#L1890

to determine the fCountFailure bool, which tells addrman to possibly deprioritize selecting an address after various failed attempts (nAttempts). As far as I can see, this logic should equally be applied to addrs from alt networks, so it might be better to leave setConnected unchanged as sdaftuar suggested? @lightlike Good observation, I missed this. I think that bit of code that we’re using in our OpenNetworkConnection() call is really trying to determine how many open connections we have, not how many distinct netgroups we’re connected to? So from that perspective, I believe that line of code is written incorrectly; if we are running only on Tor and we happen to have 8 peers from the same netgroup, then presumably we are sufficiently connected that any failures to connect should be recorded in addrman.

So I think the right change here is to modify that line to just sum the number of outbound connections (full-relay, block-relay, addr-fetch, manual, and feelers) and use that in place of setConnected.size().

Does this PR trick the code to behave as if we are not connected to Tor (by not inserting into setConnected) even if we are connected? It looks to me that trying to solve the problem of “cannot open many connections to Tor” this would create another problem: “too many connections to Tor” - it would allow all 8 outbound to be to Tor and will not try to diversify to IPv4 (or other networks). @vasild While it’s possible that we’d make all 8 of our outbound connections to Tor peers, I think this is an unlikely problem, because we limit the number of Tor connections to be a fraction of our addrman (I can’t recall what that fraction is though, can someone remind us?). I believe that an adversary looking to exploit this can really only do so for (eg) new nodes that have a relatively small addrman, by flooding such a node with onion addresses; but that attack is already available today (since we have 16 different onion netgroups, an adversary creating a zillion onion addresses to eclipse a victim will just have the ones that are in different netgroups be the ones that eclipse a peer, instead of ones in the same netgroup).

So I don’t think that this PR makes that problem any worse, and we can wait until we have something like #27213 to solve this problem more generally.

I feel that we should lookup -onlynet or allow connections to Tor up to a limit to avoid “too many connections to Tor”.

I think this is unnecessary and would be counter to what we’re trying to fix here. If a node is running on both ipv4 and tor, I think we should still not require netgroup diversity across the tor peers, because such diversity is meaningless. And as I explained above I don’t think the implicit limiting of tor peers that might arise from this would be needed or desirable.

still curious to know opinions about increasing 4 bits to 5 bits in GetGroup()

This would double the number of addrman slots that can be used by Tor peers, right? My off the cuff guess is that this would be undesirable, but I haven’t gone back to figure out what fraction of the addrman this would be yet.

in src/netaddress.cpp:524 in baa513042b outdated

516@@ -515,6 +517,29 @@ bool CNetAddr::IsAddrV1Compatible() const
517     assert(false);
518 }
519 
520+bool CNetAddr::LimitOutboundByNetgroup() const
521+{
522+    const auto onlynets = gArgs.GetArgs("-onlynet");
523+    if (onlynets.size() != 1) {
524+        return true;

sdaftuar commented at 1:53 pm on March 31, 2023:

As I mention in my comment elsewhere, I think we can drop this logic.

mzumsande commented at 5:26 pm on April 3, 2023:

I agree, see below.

in src/netaddress.cpp:541 in baa513042b outdated

536+    case NET_MAX: // m_net is never and should not be set to NET_MAX
537+        assert(false);
538+    } // no default case, so the compiler can warn about missing cases
539+
540+    assert(false);
541+}

sdaftuar commented at 1:55 pm on March 31, 2023:

I think we should generally avoid adding assert() to networking code, unless we’re sure that it’s really better to crash than to continue, eg because the node is in an inconsistent or corrupt state and is unable to continue running. That doesn’t seem to be the case here, so I think we could use Assume(false) instead, so that we only get crashes in debug builds and our CI environment.

vasild commented at 4:22 pm on April 2, 2023:

If this assert is hit, it means that somebody has added a new entry in enum Network, has forgotten to update this function, has ignored the compiler warning and has ignored the CI failure. Or a memory corruption has occurred and CNetAddr::m_net has been overwritten with random bytes. I think assert(false) is the appropriate response in both cases.

sdaftuar commented at 5:11 pm on April 2, 2023:

I really disagree on this point. We have had many examples over the years of asserts that people have added to our networking code for reasons that seem justifiable (along the lines of the reasons you’ve given), only to discover later that (usually due to an unexpected combination of code changes elsewhere) we ended up opening up a remote crash bug in deployed software that can take down the network – in situations where we could easily have written code more defensively so that our software wouldn’t crash.

We have Assume() exactly to avoid situations where a crash is worse than trying to gracefully recover, and we should err on the side of using that, particularly in networking code that could be triggered by an adversary against the whole network.

in src/netaddress.cpp:520 in 8b657606ad outdated

516@@ -515,6 +517,29 @@ bool CNetAddr::IsAddrV1Compatible() const
517     assert(false);
518 }
519 
520+bool CNetAddr::LimitOutboundByNetgroup() const

vasild commented at 5:59 pm on April 2, 2023:

I think this method belongs to CConnman, something like:

0bool CConnman::AllowOutboundByNetgroup(const std::set<std::vector<unsigned char>>& connected_netgroups,
1                                       const CAddress& addr)
2{                           
3    return connected_netgroups.count(m_netgroupman.GetGroup(addr)) == 0 ||
4           (gArgs.GetArgs("-onlynet").size() == 1 && IsReachable(addr.GetNetwork()) &&
5            (addr.IsTor() || addr.IsI2P() || addr.IsCJDNS())); 
6}

amitiuttarwar commented at 3:17 am on April 3, 2023: contributor

RE fCountFailure bool: @mzumsande

to determine the fCountFailure bool, which tells addrman to possibly deprioritize selecting an address after various failed attempts (nAttempts). As far as I can see, this logic should equally be applied to addrs from alt networks, so it might be better to leave setConnected unchanged as sdaftuar suggested? @sdaftuar I think that bit of code that we’re using in our OpenNetworkConnection() call is really trying to determine how many open connections we have, not how many distinct netgroups we’re connected to? So from that perspective, I believe that line of code is written incorrectly; if we are running only on Tor and we happen to have 8 peers from the same netgroup, then presumably we are sufficiently connected that any failures to connect should be recorded in addrman.

So I think the right change here is to modify that line to just sum the number of outbound connections (full-relay, block-relay, addr-fetch, manual, and feelers) and use that in place of setConnected.size().

looks like this code was introduced in #8065, with the primary motivation stated as preventing inaccurate incrementing of the addrman counter when a node is offline. this reinforces what I suspected from reading the code, so the suggestion to update the logic to the sum of outbound connections makes sense

however, the OP of that post also states:

This is still somewhat incomplete protection because our outbound peers could be down but not timed out or might all be on ’local’ networks (although the requirement for multiple netgroups helps).

which, I don’t fully understand. is this referring to a scenario where a node is only able to connect to other nodes within a local network environment, so would have frequent failures when trying to connect to the bitcoin network at large? and if this is the case, how could we distinguish this behavior pattern from a node running only on Tor and having all outbound peers from the same netgroup?

RE 4 bits to 5 bits in GetGroup(): I agree with @mzumsande in #27369 (comment)

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

The rule not to make more than one outbound connection to a netgroup

Netgroup-based bucketing in addrman

[…]

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

The implications of changing from 4 to 5 bits in GetGroup are nuanced because of the impacts on addrman distributions, so I think we should evaluate it separately from this PR, which is currently marked for the 25.0 milestone.

mzumsande commented at 5:28 pm on April 3, 2023: contributor

It looks to me that trying to solve the problem of “cannot open many connections to Tor” this would create another problem: “too many connections to Tor” - it would allow all 8 outbound to be to Tor and will not try to diversify to IPv4 (or other networks).

@vasild While it’s possible that we’d make all 8 of our outbound connections to Tor peers, I think this is an unlikely problem, because we limit the number of Tor connections to be a fraction of our addrman (I can’t recall what that fraction is though, can someone remind us?). I believe that an adversary looking to exploit this can really only do so for (eg) new nodes that have a relatively small addrman, by flooding such a node with onion addresses; but that attack is already available today (since we have 16 different onion netgroups, an adversary creating a zillion onion addresses to eclipse a victim will just have the ones that are in different netgroups be the ones that eclipse a peer, instead of ones in the same netgroup).

I agree with sdaftuar. I think that keeping the netgroup-based limiting in the case where we can connect to more than 1 network is unnecessary, because the current AddrMan logic puts the alternative networks already at a disadvantage compared to clearnet, and the current netgroup just amplifies that by sometimes not picking an alt-net peer and then picking another clearnet peer instead. So while having too many alt-net peers is not typically an issue in non-adverse scenarios, the netgroup restriction doesn’t even really help that much in an adverse scenario of an attacker trying to eclipse us, because with 10 outbound peers and 16 netgroups, having all alt-net peers would still be possible (unless we also have manual connections!)

(I can’t recall what that fraction is though, can someone remind us?)

From a single fixed source, we can fill 16 buckets (1024 addrs) with onion addresses - that is, for example the case for inbound onion peers, because we see them as 127.0.0.1. We can fill 64 buckets with IPv4 or IPv6 addresses from a single source.

Counting multiple sources from the same alternative network, they can fill up a theoretical maximum of 256 buckets (~16k addresses) or 1/4 of the new tables I think (but due to collisions from the the modulo operations in GetNewBucket and GetBucketPosition that depend on nKey, it is typically more like ~200 buckets in reality). On the other hand, multiple IPv4 and IPv6 sources can fill up all 1024 buckets in the new table.

which, I don’t fully understand. is this referring to a scenario where a node is only able to connect to other nodes within a local network environment, so would have frequent failures when trying to connect to the bitcoin network at large? and if this is the case, how could we distinguish this behavior pattern from a node running only on Tor and having all outbound peers from the same netgroup?

Yes, I think that’s the scenario - a temporary network failure that wouldn’t apply to peers you are locally connected to. I don’t think it’s a problem if you are running -onlynet=onion because even if the peer you are connected to is within your network, you would fail to connect to it because you wouldn’t be connecting to them directly but through onion routing (which would fail if the internet is down).

sdaftuar commented at 6:10 pm on April 3, 2023: member

however, the OP of that post also states:

This is still somewhat incomplete protection because our outbound peers could be down but not timed out or might all be on ’local’ networks (although the requirement for multiple netgroups helps). @amitiuttarwar Thanks for finding this, I hadn’t thought about the issue of wanting to distinguish between local and non-local IPV4 networks for the purposes of detecting whether a node’s network connectivity is down.

This means my suggestion of just summing outbound peers, rather than looking at netgroup diversity of peers, isn’t great. I now think we could sum the IPV4/IPV6 entries in setConnected and add that to the total number of outbound alt-network peers (basically treating every alt-network peer as a distinct entry in setConnected), and use that for this check?

EDIT: for the avoidance of doubt, I wrote a patch to implement my suggested approach here.

mzumsande commented at 7:19 pm on April 5, 2023: contributor

Maybe also add a short explanation before the OpenNetworkConnection() call about the rationale for what we pass for fCountFailure from #8065 (Good find @amitiuttarwar!), i.e. the scenario where we can’t connect to the internet but still may have a local connection. This was not obvious at all to me.

stratospher force-pushed on Apr 6, 2023

p2p: skip netgroup diversity of new connections for tor/i2p/cjdns networks

Co-authored-by: Suhas Daftuar <sdaftuar@gmail.com>

b5585ba5f9

stratospher force-pushed on Apr 6, 2023

stratospher commented at 6:47 pm on April 6, 2023: contributor

Thanks a lot @sdaftuar! I’ve used the simplified approach in your patch!

Counting multiple sources from the same alternative network, they can fill up a theoretical maximum of 256 buckets (~16k addresses) or 1/4 of the new tables I think (but due to collisions from the the modulo operations in GetNewBucket and GetBucketPosition that depend on nKey, it is typically more like ~200 buckets in reality). On the other hand, multiple IPv4 and IPv6 sources can fill up all 1024 buckets in the new table.

tried a similar calculation for tried tables - which depends on netgroup of tor address(16 group possibilities) and hash1%8 which has 8 possibilities. that’s 2^7 bucket possibilities out of 2^8 tried table buckets which is 1/2 the tried table.

in an ideal theoretical ipv4 + tor scenario, the chance of all 8 outbound connections turning out to be tor would be = (50% new + 50 % tried)^8

if src of all tor address is tor = (0.5 * 0.25 + 0.5 * 0.5)^8 = approx 0.04% (even less than this real scenario)
if src of all tor address is ipv4 = (0.5 * 1 + 0.5 * 0.5)^8 = approx 10% (even less than this in real scenario)

i guess these are the upper bound probability of this scenario happening. it is anyways covered in the comments above related to how netgroup restrictions for Tor/I2P/CJDNS don’t make much difference in adverse situations.

This would double the number of addrman slots that can be used by Tor peers, right? My off the cuff guess is that this would be undesirable, but I haven’t gone back to figure out what fraction of the addrman this would be yet.

The implications of changing from 4 to 5 bits in GetGroup are nuanced because of the impacts on addrman distributions, so I think we should evaluate it separately from this PR

thanks for explaining! makes sense, noticed how this would result in tor/i2p/cjdns addresses filling the entire addrman.

I now think we could sum the IPV4/IPV6 entries in setConnected and add that to the total number of outbound alt-network peers (basically treating every alt-network peer as a distinct entry in setConnected), and use that for this check?

I liked this approach!

setConnected.size() >= min(nMaxConnections - 1, 2)

since we include manual connections in setConnected, something else which would change now is we’d record failure attempts when we attempt an outbound connection and are already sufficiently connected to just manual peers. i think this is desirable/aligned with the original intent of why setConnected.size() >= min(nMaxConnections - 1, 2) was written.

nMaxConnections do not include manual connections right? (nMaxConnections limit is default 125, manual connection limit is default 8 (not included in 125)). and setConnected.size() would include them. in a rare scenario in resource constrained systems (if ever nMaxConnections=2) and we only have manual connections, it’s possible that we end up recording addrman failure attempts. but i guess that’s alright.

mzumsande commented at 5:50 pm on April 7, 2023: contributor

ACK b5585ba5f97a19d1b435d9ab69b5a55cfd45dd70

I reviewed the code and verified that we now make outgoing connections to peers with lexicographically adjacent onion addresses.

in src/net.cpp:1906 in b5585ba5f9

1903-            OpenNetworkConnection(addrConnect, (int)setConnected.size() >= std::min(nMaxConnections - 1, 2), &grant, nullptr, conn_type);
1904+            // Record addrman failure attempts when node has at least 2 persistent outbound connections to peers with
1905+            // different netgroups in ipv4/ipv6 networks + all peers in Tor/I2P/CJDNS networks.
1906+            // Don't record addrman failure attempts when node is offline. This can be identified since all local
1907+            // network connections(if any) belong in the same netgroup and size of setConnected would only be 1.
1908+            OpenNetworkConnection(addrConnect, (int)setConnected.size() + outbound_privacy_network_peers >= std::min(nMaxConnections - 1, 2), &grant, nullptr, conn_type);

vasild commented at 11:39 am on April 11, 2023:

This (int)setConnected.size() >= std::min(nMaxConnections - 1, 2) was introduced in c769c4af11fc58dd4813d328c7f71042bc577676.

I think that for the purposes of checking whether we are offline, being connected to just one Tor peer means that we are not. This is because the Tor network is global (same for I2P or CJDNS). There is no such thing as “local area network” of Tor nodes (e.g. 10.0.0.0/24) or localhost Tor address (like 127.0.0.1 in IPv4). If we are connected to one Tor node, it means we are not offline and can possibly connect to other Tor nodes as well.

Maybe out of the scope of this PR, but would it not be better to change that logic to: “if we are connected to at least one CNetAddr::IsRoutable() IPv4/6 address or at least one Tor address or at least one I2P or at least one CJDNS address, then we are not offline”?

cc @gmaxwell

sdaftuar commented at 12:29 pm on April 11, 2023:

In #8065, @gmaxwell pointed out:

This is still somewhat incomplete protection because our outbound peers could be down but not timed out

We never really know if our peers are connected or not, because when your network goes down it usually takes some amount of time for each connection to time out. Requiring two connections to be up (versus just 1 connection to e.g. Tor, or some other non-local ipv4/ipv6 network) is a tradeoff around how long we spend in a state where we’re punishing addrman entries while we might be offline and just haven’t noticed it yet, vs not punishing entries for being down when our network is up.

My guess is that most nodes on the network don’t have local network peers, and so the effect of this logic for most users since it was deployed in #8065 – requiring 2 non-local outbound connections before we start punishing addrman entries for failed attempts – will be the same after this PR is merged.

If someone can figure out that it’s more optimal for this to be 1 or 3 or some other value, I think we can pick that up in a new PR; however I’d suspect it doesn’t matter too much, and that 2 is likely better than 1…

vasild approved

vasild commented at 11:42 am on April 11, 2023: contributor

ACK b5585ba5f97a19d1b435d9ab69b5a55cfd45dd70

I think this fixes the problem described in #27264 (comment) and is ok to be merged in its current form.

Could it create another problem of connecting too much to Tor and not try to diversify to IPv4? Yes. Is this unlikely problem? Yes, with a common addrman that is filled mostly with IPv4 addresses. With a non-common addrman? I do not know.

jonatack commented at 7:13 pm on April 13, 2023: contributor

Concept ACK, will review.

achow101 commented at 10:13 pm on April 13, 2023: member

ACK b5585ba5f97a19d1b435d9ab69b5a55cfd45dd70

achow101 merged this on Apr 13, 2023

achow101 closed this on Apr 13, 2023

sidhujag referenced this in commit 2243787e44 on Apr 14, 2023

jonatack referenced this in commit d2834e1c96 on Apr 14, 2023

in src/net.cpp:1710 in b5585ba5f9

1707+        // Only connect out to one peer per ipv4/ipv6 network group (/16 for IPv4).
1708         int nOutboundFullRelay = 0;
1709         int nOutboundBlockRelay = 0;
1710-        std::set<std::vector<unsigned char> > setConnected;
1711+        int outbound_privacy_network_peers = 0;
1712+        std::set<std::vector<unsigned char>> setConnected; // netgroups of our ipv4/ipv6 outbound peers

jonatack commented at 6:36 pm on April 14, 2023:

The code doc at https://github.com/bitcoin/bitcoin/pull/27374/files#diff-00021eed586a482abdb09d6cdada1d90115abe988a91421851960e26658bed02R1858 is now out of date.

The naming of setConnected should probably be updated to its new semantics.

Addressed in #27467.

jonatack commented at 7:19 pm on April 14, 2023: contributor

Post-merge ACK.

I am not completely sure the additional complexity involved is worth it versus reverting 72e8ffd (#27264), but the change itself here seems fine, modulo a couple of minor loose ends.

if we run a node only on tor/i2p/cjdns

Via interactions with users, my understanding is that it may possibly be fairly often the case that node operators are running on non-clearnet networks only. (See also these polls for fun: users https://twitter.com/jonatack/status/1629173539067527171 / bitcoin devs https://twitter.com/jonatack/status/1631717328416014336).

Perhaps increasingly so, based on the increase in the number of Tor v3 and I2P peers reported by -addrinfo over the past year or so, now up to 15k onion and 1300 I2P peers, i.e. recently active / non-IsTerrible ones. (I2P peers in particular increased significantly recently after 2 node-in-a-box packages added support in December 2022.)

ryanofsky referenced this in commit 456af7a955 on Jun 9, 2023

bitcoin locked this on Apr 13, 2024

p2p: skip netgroup diversity of new connections for tor/i2p/cjdns #27374

Reviews

Conflicts