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

stratospher · 2023-03-30T14:59:39Z

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 p2p: Improve diversification of new connections #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 Fix net grouping of non-IP networks #27369)

DrahtBot · 2023-03-30T14:59:43Z

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.

sdaftuar

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.

sdaftuar · 2023-03-30T15:37:32Z

src/net.cpp

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).

thanks! done.

mzumsande

Concept ACK

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

bitcoin/src/net.cpp

Lines 1803 to 1805 in 328087d

    
           if (!addr.IsValid() || IsLocal(addr) || !IsReachable(addr) || 
        
               !HasAllDesirableServiceFlags(addr.nServices) || 
        
               setConnected.count(m_netgroupman.GetGroup(addr))) continue;

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

bitcoin/src/net.cpp

Line 1890 in 328087d

    
           OpenNetworkConnection(addrConnect, (int)setConnected.size() >= std::min(nMaxConnections - 1, 2), &grant, nullptr, conn_type);

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?

mzumsande · 2023-03-30T17:40:51Z

src/net.cpp

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.

thanks! done.

vasild · 2023-03-31T05:15:43Z

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() 😞. 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 · 2023-03-31T10:29:37Z

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 · 2023-03-31T12:07:06Z

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().

src/netaddress.cpp

fanquake · 2023-03-31T12:26:04Z

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

A new circular dependency in the form of "netaddress -> netbase -> netaddress" appears to have been introduced.

^---- failure generated from lint-circular-dependencies.py

sdaftuar · 2023-03-31T13:35:10Z

bitcoin/src/net.cpp

Line 1890 in 328087d

OpenNetworkConnection(addrConnect, (int)setConnected.size() >= std::min(nMaxConnections - 1, 2), &grant, nullptr, conn_type);

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.

sdaftuar · 2023-03-31T13:53:23Z

src/netaddress.cpp

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

I agree, see below.

sdaftuar · 2023-03-31T13:55:05Z

src/netaddress.cpp

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.

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.

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.

vasild · 2023-04-02T17:59:55Z

src/netaddress.cpp

I think this method belongs to CConnman, something like:

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

amitiuttarwar · 2023-04-03T03:17:05Z

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 72e8ffd 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

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).

mzumsande · 2023-04-03T17:26:08Z

src/netaddress.cpp

I agree, see below.

sdaftuar · 2023-04-03T18:10:47Z

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 · 2023-04-05T19:19:06Z

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.

…works Co-authored-by: Suhas Daftuar <[email protected]>

stratospher · 2023-04-06T18:47:53Z

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

ACK b5585ba

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

vasild

ACK b5585ba

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.

vasild · 2023-04-11T11:39:49Z

src/net.cpp

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


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

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

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...

jonatack · 2023-04-13T19:13:35Z

Concept ACK, will review.

achow101 · 2023-04-13T22:13:30Z

ACK b5585ba

…for tor…

jonatack

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.)

jonatack · 2023-04-14T18:36:34Z

src/net.cpp

        int nOutboundBlockRelay = 0;
-        std::set<std::vector<unsigned char> > setConnected;
+        int outbound_privacy_network_peers = 0;
+        std::set<std::vector<unsigned char>> setConnected; // netgroups of our ipv4/ipv6 outbound peers


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.

11bb31c p2p: "skip netgroup diversity of new connections for tor/i2p/cjdns" follow-up (Jon Atack) Pull request description: In #27374 the role of the `setConnected` data structure in `CConnman::ThreadOpenConnections` changed from the set of outbound peer netgroups to those of outbound IPv4/6 peers only. In accordance with the changed semantics, this pull fixes a code comment regarding feeler connections and updates the naming of `setConnected` to `outbound_ipv46_peer_netgroups`. Addresses #27374 (comment). ACKs for top commit: mzumsande: Code Review ACK 11bb31c vasild: ACK 11bb31c ryanofsky: Code review ACK 11bb31c Tree-SHA512: df9151a6cce53c279e549683a9f30fdc23d513dc664cfee1cf0eb8ec80b2848d32c80a92cc0a9f47d967f305864975ffb339fe0eaa80bc3bef1b28406419eb96

DrahtBot added the P2P label Mar 30, 2023

This was referenced Mar 30, 2023

Fix net grouping of non-IP networks #27369

Open

p2p: Improve diversification of new connections #27264

Merged

sdaftuar approved these changes Mar 30, 2023

View reviewed changes

fanquake added this to the 25.0 milestone Mar 30, 2023

mzumsande reviewed Mar 30, 2023

View reviewed changes

stratospher force-pushed the followup-27264 branch from 542ac83 to 8b65760 Compare March 31, 2023 12:01

stratospher commented Mar 31, 2023

View reviewed changes

src/netaddress.cpp Outdated Show resolved Hide resolved

sdaftuar reviewed Mar 31, 2023

View reviewed changes

This was referenced Mar 31, 2023

Handle CJDNS from LookupSubNet() #27071

Merged

p2p: delete anchors.dat after trying to connect to that peers #24034

Closed

net, refactor: extract Network and BIP155Network logic to node/network #27385

Closed

vasild reviewed Apr 2, 2023

View reviewed changes

mzumsande reviewed Apr 3, 2023

View reviewed changes

src/netaddress.cpp Outdated

Copy link

Contributor

mzumsande Apr 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, see below.

stratospher force-pushed the followup-27264 branch from 8b65760 to f85e365 Compare April 6, 2023 18:32

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

b5585ba

…works Co-authored-by: Suhas Daftuar <[email protected]>

stratospher force-pushed the followup-27264 branch from f85e365 to b5585ba Compare April 6, 2023 18:43

mzumsande reviewed Apr 7, 2023

View reviewed changes

rajarshimaitra mentioned this pull request Apr 10, 2023

p2p: skip netgroup diversity of new connections for tor/i2p/cjdns #27374 Bitshala/BitcoinCore-PR-Review-Club#23

Closed

vasild approved these changes Apr 11, 2023

View reviewed changes

achow101 merged commit 2bfe43d into bitcoin:master Apr 13, 2023

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 14, 2023

Merge bitcoin#27374: p2p: skip netgroup diversity of new connections …

2243787

…for tor…

jonatack mentioned this pull request Apr 14, 2023

p2p: skip netgroup diversity follow-up #27467

Merged

jonatack reviewed Apr 14, 2023

View reviewed changes

mzumsande mentioned this pull request Jun 15, 2023

net: do not break when addr is not from a distinct network group #27863

Merged

This was referenced Aug 29, 2023

addrman: treat Tor/I2P/CJDNS as a single group #22563

Closed

BIP324 connection support #28196

Merged

amitiuttarwar mentioned this pull request Sep 12, 2023

Increase # of block-relay-only connections #28462

Open

bitcoin locked and limited conversation to collaborators Apr 13, 2024

stratospher deleted the followup-27264 branch July 17, 2024 06:40

	if (!addr.IsValid() \|\| IsLocal(addr) \|\| !IsReachable(addr) \|\|
	!HasAllDesirableServiceFlags(addr.nServices) \|\|
	setConnected.count(m_netgroupman.GetGroup(addr))) continue;

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

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

Conversation

stratospher commented Mar 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Mar 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

sdaftuar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mzumsande left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vasild commented Mar 31, 2023

Uh oh!

stratospher commented Mar 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stratospher commented Mar 31, 2023

Uh oh!

Uh oh!

fanquake commented Mar 31, 2023

Uh oh!

sdaftuar commented Mar 31, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amitiuttarwar commented Apr 3, 2023

Uh oh!

mzumsande left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sdaftuar commented Apr 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mzumsande commented Apr 5, 2023

Uh oh!

stratospher commented Apr 6, 2023

Uh oh!

mzumsande left a comment

Choose a reason for hiding this comment

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonatack commented Apr 13, 2023

Uh oh!

achow101 commented Apr 13, 2023

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

stratospher commented Mar 30, 2023 •

edited

Loading

DrahtBot commented Mar 30, 2023 •

edited

Loading

stratospher commented Mar 31, 2023 •

edited

Loading

sdaftuar commented Apr 3, 2023 •

edited

Loading