p2p: Diversify automatic outbound connections with respect to networks

amitiuttarwar commented at 7:46 pm on March 6, 2023: contributor

This is joint work with mzumsande.

This is a proposal to diversify outbound connections with respect to reachable networks. The existing logic evaluates peers for connection based purely on the frequency of available addresses in AddrMan. This PR adds logic to automatically connect to alternate reachable networks and adds eviction logic that protects one existing connection to each network.

For instance, if AddrMan is populated primarily with IPv4 and IPv6 addresses and only a handful of onion addresses, it is likely that we won’t establish any automatic outbound connections to Tor, even if we’re capable of doing so. For smaller networks like CJDNS, this is even more of an issue and often requires adding manual peers to ensure regularly being connected to the network.

Connecting to multiple networks improves resistance to eclipse attacks for individual nodes. It also benefits the entire p2p network by increasing partition resistance and privacy in general.

The automatic connections to alternate networks is done defensively, by first filling all outbound slots with random addresses (as in the status quo) and then adding additional peers from reachable networks the node is currently not connected to. This approach ensures that outbound slots are not left unfilled while attempting to connect to a network that may be unavailable due to a technical issue or misconfiguration that bitcoind cannot detect.

Once an additional peer is added and we have one more outbound connection than we want, outbound eviction ensures that peers are protected if they are the only ones for their network.

Manual connections are also taken into account: If a user already establishes manual connections to a trusted peer from a network, there is no longer a need to make extra efforts to ensure we also have an automatic connection to it (although this may of course happen by random selection).

DrahtBot commented at 7:46 pm on March 6, 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	vasild, naumenkogs
Concept ACK	ghost, 0xB10C, brunoerg, kristapsk, fanquake
Stale ACK	mzumsande, ajtowns, willcl-ark

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#28222 (Use shared_ptr for CNode inside CConnman by willcl-ark)
#27912 (net: disconnect inside AttemptToEvictConnection by willcl-ark)
#27509 (Relay own transactions only via short-lived Tor or I2P connections by vasild)
#26114 (net: Make AddrFetch connections to fixed seeds by mzumsande)
#25572 (refactor: Introduce EvictionManager and use it for the inbound eviction logic by dergoegge)
#25268 (refactor: Introduce EvictionManager by dergoegge)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label P2P on Mar 6, 2023

amitiuttarwar commented at 7:53 pm on March 6, 2023: contributor

We have 3 open questions that we are interested in hearing feedback from reviewers. We have considered many potential interactions and have laid out a couple specific ones along with tradeoffs of different approaches.

IBD

The first is regarding the interactions between this patch & IBD. Peers on privacy network might be slower than clearnet peers (eg. Tor or I2P). Our current proposal does not have any special case logic for disabling the network prioritization during IBD.

This could mean that our overall IBD speed is reduced by not having the fastest peers possible. The network peers could also be intermittently disconnected for stalling & get the node into a loop where it, for example, disconnects a Tor peer, connects to an IPV4 peer, re-adds a Tor peer and then keeps repeating. Finally, there could be interactions with the slow peers causing delays in the response time of the faster peers based on the specifics of our IBD logic. One solution is to just disable the network-specific outbound logic during IBD.

On the other hand, a node can get slow peers by random chance anyways. Whether randomly connecting to altnet peers or because a clearnet peer just happens to be slow. If they are slow, but not slow enough to be evicted due to the stalling limit, that will already have a significant impact on the IBD rate. The issues and potential solutions with improving IBD can be seen as tangential to this proposal, and we could favor not adding custom workarounds that serve as a bandaid to mask the root cause. Enabling alternative networks during IBD is already known to slow down the sync dramatically, and we can just add a warning in the documentation to help users become more aware.

Peers protected from eviction

This patch adds eviction protection for special network peers, but it only gets triggered when we have an extra outbound-full-relay peer (aka 9). This occurs when we detect a stale tip or because we added another special network peer. In these situations we’d want to ensure that we have at least one non-protected peer that can be disconnected so the node can return to the steady state of 8 full-relay outbound peers.

Currently, we protect 4 peers via m_chain_sync.m_protect, which are the first peers that send us headers corresponding to our best chain. This means 4 peers could be protected with this logic, and 3 more peers being protected by their network. If we added support for two more alternative networks, a user activating all networks could run into a problem where they have 9 protected peers in these circumstances, and unable to return to the steady state of 8 full-relay outbound peers.

Since it would require adding support for at least one more network to run into this issue, we have not addressed it in this patch. There are multiple viable mitigation strategies: One option is to reduce the number of peers we protect with the headers logic (via m_protect). Another is to limit the network-specific logic to 2 random networks or such.

Network peer protection

A potential concern of this patch is that the logic that protects network peers from eviction could be “over-protective”. For example, keeping a Tor connection only because it is the only Tor connection, not because it’s providing us meaningful data.

Any concerns around this are completely bounded by the fact that the protection logic is limited to when we have an extra peer, either through stale tip detection or through the new temporary connections for network specific peers.

However, if there is still concern, @ajtowns had an interesting idea to increase our likelihood of adding a 2nd peer from alternate networks. We could update the stale tip logic to choose the network of the extra outbound differently. Right now, the network odds reflect the proportions represented in AddrMan. Instead, we could amend the logic to select based on the proportion of networks we are connected to. This would increase our likelihood of connecting to the network, because, for example, you’re more likely to have ⅛ I2P peers than ⅛ of your addrman be I2P addresses.

Increasing the chances of connecting to a privacy network in that context has two benefits: It serves the original intention of the stale top logic to be a last-ditch effort against a perceived eclipse attack. Overtaking multiple networks increases the cost of attack, so this is theoretically a more useful attempt than clearnet. For this proposed patch, it would mean the node is more likely to add another peer from the alternate network, so the node is more able to replace a “bad” network peer with hopefully a better one. This would be effective since the stale tip logic tends to run a couple times a day.

This could be treated as a separate chunk of work to be evaluated in a follow-up, or it could be incorporated into this PR if reviewers believe it could help mitigate concerns about edge case possibilities.

amitiuttarwar force-pushed on Mar 6, 2023

vasild commented at 11:36 am on March 7, 2023: contributor

Concept ACK.

This was somewhere on my TODO, thanks for picking it up!

The automatic connections to alternate networks is done defensively, by first filling all outbound slots with random addresses (as in the status quo) and then adding additional peers from reachable networks the node is currently not connected to. This approach ensures that outbound slots are not left unfilled while attempting to connect to a network that may be unavailable due to a technical issue or misconfiguration that bitcoind cannot detect.

Hmm, this is maybe ok. Another approach would be to try to open to e.g. Tor while still having < 8 outbound connections. To counter the issue of staying with < 8 connections and not being able to open to Tor because e.g. the Tor proxy is broken, then we could give up after a few attempts and then fall back to the old way of filling with random (aka IPv4 ;-)) addresses. If we overshoot the 8 connections (like described above), then for some time we will have more, until eviction kicks in. How much time is that exactly? If that is short then it would be sub-optimal to open a IPv4 connection and drop it shortly after in cases where Tor connectivity is working smoothly. If it is long then we have practically increased the limit of 8 which will cause more traffic.

In the above, the clearnet networks IPv4 and IPv6 are counted as one network because diversification between these doesn’t make sense and is impossible in some network environments.

Why it doesn’t make sense? I think it is an extra effort to obtain IPv4 addresses if one has IPv6 addresses and the other way around. Why impossible in some network environments? After all you know if you are going to attempt to connect to 5.6.7.8 or 2030:c0fe:....

Manual connections are also taken into account: If a user already establishes manual connections to a trusted peer from a network, there is no longer a need to make extra efforts

+1

IBD

I think there is no need to add special logic during IBD (e.g. disable this PR during IBD). Clearnet peers can also be slow and non-clearnet peers may be fast-enough. IBD should be able to handle slow/fast peers already without special hand-holding.

Peers protected from eviction

I think we don’t need to worry about this now because even if we treat IPv4 and IPv6 as separate networks and all 4 from m_protected are from one network (e.g. IPv4), then we have 4 more slots for IPv6, Tor, I2P, CJDNS. If we implement more networks, maybe by that time we will have increased the default 8 or I would suggest to then start disconnecting from m_protected.

Network peer protection A potential concern of this patch is that the logic that protects network peers from eviction could be “over-protective”. For example, keeping a Tor connection only because it is the only Tor connection, not because it’s providing us meaningful data.

I don’t see it as “over-protective”. Can we not have some metric of whether a peer is “providing us a meaningful data”? I am ok to deal with this in a followup.

Taking a step back, the root problem here is that addrman has many IPv4 addresses and few I2P ones (for example). If it would have equal number of addresses from each network, then this problem would not exist because then picking up a random one would have a 1/5 chance of selecting any network (thus there would not be any bias towards IPv4). Would it be simpler and solve the problem equally well if we only change addrman internally to return an address from any given network with a 1/5 chance? Then no need to change the eviction logic or the ThreadOpenConnections logic. And no need to guarantee explicitly in the code that at least one connection exists to each network. It will happen by itself most of the time.

mzumsande commented at 4:59 pm on March 7, 2023: contributor

Thanks! My thoughts on just some of your points:

I think we don’t need to worry about this now because even if we treat IPv4 and IPv6 as separate networks and all 4 from m_protected are from one network (e.g. IPv4), then we have 4 more slots for IPv6, Tor, I2P, CJDNS.

I think that scenario would already be problematic: In this case, all 8 outbound peers could be protected, and while there would be no need to add network-related extra peers, we could still add an extra outbound due to stale tip (StartExtraBlockRelayPeers()), and then we wouldn’t have a peer to evict.

I don’t see it as “over-protective”. Can we not have some metric of whether a peer is “providing us a meaningful data”? I am ok to deal with this in a followup.

I agree. We have lots of different metrics for that (e.g. all Misbehaving() calls, ConsiderEviction() for stale tip, inactivity checks) that would apply to network-specific peers in the same way as for unprotected peers. Maybe some of these can be improved, but I think this should be unrelated to this PR.

Would it be simpler and solve the problem equally well if we only change addrman internally to return an address from any given network with a 1/5 chance? Then no need to change the eviction logic or the ThreadOpenConnections logic. And no need to guarantee explicitly in the code that at least one connection exists to each network. It will happen by itself most of the time.

That would certainly be simpler (although I found the necessary non-addrman changes in this PR to be surprisingly small), but as you wrote we’d only have a connection to each network with a certain probability. I did a quick simulation (too lazy to try and do the math), and it appears that with 5 networks and 8 outbounds, the chance to be connected to all of them is just 32% (62% for 4 nets, 88% for 3, 99% for 2). I would have to think a bit more about the consequence on eclipse attacks for this - for example, a small network like CJDNS is probably pretty easy to eclipse currently, and if we configure IPv4 and CJDNS to be reachable and choose each connection with a base probability of 50% to CJDNS, that could be pretty scary… But just noting that the two approaches aren’t exclusive and implementing both would be possible.

One interesting idea for possible follow-ups would be optional logic that ties certain behavior to networks (e.g. broadcasting wallet transactions only to peers from anonymity networks). In order for that to work, it would be good to have stronger guarantee that we are actually connected to these networks.

vasild commented at 9:29 am on March 8, 2023: contributor

I think that scenario would already be problematic … and then we wouldn’t have a peer to evict.

“… then start disconnecting from m_protected”

the math

I think it goes like this: (with 5 networks) the chance of not selecting IPv4 for the first peer is 4/5, for the first and second peer is (4/5)^2. The chance of not selecting IPv4 in all 8 choices is p=(4/5)^8. So, the chance of not selecting IPv4 or not selecting IPv6 is 2*p. The chance of not selecting IPv4, or IPv6 or Tor or I2P or CJDNS is 5*p. In other words, the chance of not being connected to at least one network is 5*p, and thus the chance of being connected to all networks is 1-5*p, or 16% (60% for 4 nets, 88% for 3 nets, 99% for 2 nets and 100% for 1 net).

I think this is enough to ditch the idea of random-chance Select(). Also, maybe it would not be so much more simpler than this PR.

offtopic:

One interesting idea for possible follow-ups would be optional logic that ties certain behavior to networks (e.g. broadcasting wallet transactions only to peers from anonymity networks).

Yes, please! :) To take this one step further, it would be even better to create a temporary connection(s) to Tor/I2P, broadcast our transaction(s) and close the connection(s) (with I2P transient address). This way, if we broadcast another tx after e.g. 1 hour the recipient will not be able to conclude that both transactions come from the same origin.

In order for that to work, it would be good to have stronger guarantee that we are actually connected to these networks.

Would not be needed with temporary tx-broadcast connections.

mzumsande commented at 11:43 pm on March 8, 2023: contributor

I think it goes like this: (…)

I think this is enough to ditch the idea of random-chance Select(). Also, maybe it would not be so much more simpler than this PR.

I agree with that conclusion, but I think you can’t just add the probabilities like that. I think the correct formula is $$p={k! S(n,k) \over k^n}$$ where k is the number of networks, $n=8$ the number of outbounds and $S(n,k)$ the Stirling number of the second kind, see here for an explanation - the resulting 32.256% for all 5 networks agrees very well with my simulation results. (sorry, this is getting offtopic, and I’ll stop now - will address your other open comments soon, unless amiti is first)

ghost commented at 8:17 am on March 9, 2023: none

Concept ACK

0xB10C commented at 1:17 pm on March 12, 2023: contributor

Concept ACK: I think, having network diversity in outbound connections is helpful and improves partition resistance.

In the above, the clearnet networks IPv4 and IPv6 are counted as one network because diversification between these doesn’t make sense and is impossible in some network environments.

Why it doesn’t make sense? I think it is an extra effort to obtain IPv4 addresses if one has IPv6 addresses and the other way around. Why impossible in some network environments? After all you know if you are going to attempt to connect to 5.6.7.8 or 2030:c0fe:….

I also not sure if I agree with counting IPv4 and IPv6 as one network and I think this could need a bit more reasoning. Is it because they are both clearnet? Is it a workaround to don’t have 8 protected peers if all networks are enabled (which we’d also face when adding another network like yggdrasil as you mentioned)?

I had the feeling that protecting only one IPv4 or IPv6 could, for example, reduce the number of inbound connections IPv6-only nodes get. However, I’m not sure if that’s really the case (I need to think about this more).

mzumsande commented at 7:57 pm on March 13, 2023: contributor

Hmm, this is maybe ok. Another approach would be to try to open to e.g. Tor while still having < 8 outbound connections. To counter the issue of staying with < 8 connections and not being able to open to Tor because e.g. the Tor proxy is broken, then we could give up after a few attempts and then fall back to the old way of filling with random (aka IPv4 ;-)) addresses. If we overshoot the 8 connections (like described above), then for some time we will have more, until eviction kicks in. How much time is that exactly? If that is short then it would be sub-optimal to open a IPv4 connection and drop it shortly after in cases where Tor connectivity is working smoothly. If it is long then we have practically increased the limit of 8 which will cause more traffic.

We thought about this, of course, because it’s the most straightforward approach. However, don’t like it as much as the approach here for several reasons:

it requires introducing an arbitrary waiting time, during which we might have less outbound connections, especially if this would happen for multiple networks at the same time.
having too few outbound connections is a a potential security risk for the node and slows down important processes such as IBD, the only downsides of having an extra outbound connection is to have a bit of extra traffic, plus temporarily requiring more inbound capacity from the network.
churning through a few short-lived extra connections has several unrelated benefits, we do it voluntarily anyway (feeler connections), so I’d think it’s no big deal.
the conditions which precluded us from connecting to a network might correct over time (maybe we had a lot of bad addresses in our addrman in the beginning; maybe some technical problem). Just trying a couple of times right after startup, and then giving up wouldn’t be ideal in this scenario.

I guess it boils down to that in general, I think temporarily having more outbound connections than our target is preferable to temporarily having less connections.

mzumsande commented at 7:59 pm on March 13, 2023: contributor

Why it doesn’t make sense? I think it is an extra effort to obtain IPv4 addresses if one has IPv6 addresses and the other way around. Why impossible in some network environments? After all you know if you are going to attempt to connect to 5.6.7.8 or 2030:c0fe:….

I also not sure if I agree with counting IPv4 and IPv6 as one network and I think this could need a bit more reasoning. Is it because they are both clearnet? Is it a workaround to don’t have 8 protected peers if all networks are enabled (which we’d also face when adding another network like yggdrasil as you mentioned)?

I think our reasoning was:

They are both clearnet, so it’s a bit unclear to me if there is much of a benefit in diversification here. Though maybe there is an extra effort for a potential eclipse attacker to cover both IPv4 and IPv6?
IPv4-only network setups exist, I’ve experienced it more than once that I would be within a network of some organization and could only connect to IPv4 peers, but bitcoind wasn’t able to detect that so I’d see regular attempts to make connection to IPv6 peers which all failed immediately.

So basically, we saw no clear upsides, and some downsides, and that’s why we decided to add it. But I’m not insistent on this if people think that there is enough benefit in having connections to both IPv4 and IPv6 peers - it would also make the code cleaner if we didn’t have to apply this exception in multiple places.

It had nothing to do with the number of 8 protected peers. I’m sure we could find other solutions around that edge case problem, e.g. adding logic to just not try an extra outbound in the first place when we could get into a position where we don’t have an unprotected peer to evict, or softening the protection during eviction as vasild suggested.

ajtowns commented at 5:27 am on March 15, 2023: contributor

I’m not sure it has any influence on this pr, but did you consider doing something similar for block-relay-only peers?

Something like: connect to one generic block-relay-only peer, plus an additional block-relay-only peer for every network we support? I think that should only improve things, including during IBD. It wouldn’t do anything to improve tx propagation of course, so wouldn’t make this PR redundant.

amitiuttarwar commented at 8:29 pm on March 19, 2023: contributor

thank you everyone for the thoughtful feedback! some questions & responses -

RE: logic to initially overshoot number of connections

@vasild

If we overshoot the 8 connections (like described above), then for some time we will have more, until eviction kicks in. How much time is that exactly? If that is short then it would be sub-optimal to open a IPv4 connection and drop it shortly after in cases where Tor connectivity is working smoothly. If it is long then we have practically increased the limit of 8 which will cause more traffic.

Since we already have short-lived connections through feelers, rotating extra block relay only connections, and stale tip detection, can you clarify the disadvantage of adding another? Especially since we wouldn’t expect this new mechanism to occur frequently, does it seem acceptable or intrinsically different somehow?

RE: ipv4 vs ipv6

@vasild: Are you suggesting that treating them as separate networks increases the cost of attack? Does the delta in energy required from the attacker feel meaningful? @0xB10C

I had the feeling that protecting only one IPv4 or IPv6 could, for example, reduce the number of inbound connections IPv6-only nodes get. However, I’m not sure if that’s really the case (I need to think about this more).

Hm interesting. Our current baseline is that we have no protections based on network. By introducing network protections, we would expect the number of inbounds for IPV4 and IPV6 to drop compared to the current conditions. It’s likely that conflating the two networks would impact the distribution between them, but would that be problematic?

Future improvements

Agreed these are orthogonal to the current proposal, but are very interesting ideas worth further exploration! - temporary tx broadcast connections - block-relay-only connections per network

brunoerg commented at 6:17 pm on March 21, 2023: contributor

Concept ACK

brunoerg commented at 8:40 pm on March 21, 2023: contributor

Something like: connect to one generic block-relay-only peer, plus an additional block-relay-only peer for every network we support? I think that should only improve things, including during IBD. It wouldn’t do anything to improve tx propagation of course, so wouldn’t make this PR redundant.

Also, should we have (at least) one anchor per network instead of only two “generic” ones?

amitiuttarwar commented at 0:00 am on March 24, 2023: contributor

added a commit that increases fuzz coverage for select by network @brunoerg

Also, should we have (at least) one anchor per network instead of only two “generic” ones?

yeah, def think it’s worth considering in relation to increasing block-relay-peers. @mzumsande & I have started brainstorming & plan to open an issue once we formulate some initial ideas. in the meanwhile, feel free to reach out directly if you’re interested in discussing further :)

vasild commented at 9:10 am on April 7, 2023: contributor

If we overshoot the 8 connections (like described above), then for some time we will have more, until eviction kicks in. How much time is that exactly? If that is short then it would be sub-optimal to open a IPv4 connection and drop it shortly after in cases where Tor connectivity is working smoothly. If it is long then we have practically increased the limit of 8 which will cause more traffic.

Since we already have short-lived connections through feelers, rotating extra block relay only connections, and stale tip detection, can you clarify the disadvantage of adding another? Especially since we wouldn’t expect this new mechanism to occur frequently, does it seem acceptable or intrinsically different somehow?

You did not answer my question “How much time is that exactly?” but your reply seems to imply an answer “short”, correct me if I misunderstand. The disadvantage of doing that is “it would be sub-optimal to open a IPv4 connection and drop it shortly after in cases where Tor connectivity is working smoothly”.

@vasild: Are you suggesting that treating them as separate networks increases the cost of attack?

Yes.

Does the delta in energy required from the attacker feel meaningful?

That question is a bit vague. I guess “yes” for some values of “meaningful”.

achow101 referenced this in commit 3a93957a5d on Apr 20, 2023

DrahtBot added the label Needs rebase on Apr 20, 2023

amitiuttarwar force-pushed on Apr 21, 2023

DrahtBot removed the label Needs rebase on Apr 21, 2023

sidhujag referenced this in commit 6004bb5ebe on Apr 21, 2023

amitiuttarwar force-pushed on May 24, 2023

amitiuttarwar commented at 8:16 pm on May 24, 2023: contributor

this PR is ready for review!

did a couple of clean-up steps:

removed the fuzz commit since coverage is now handled in #27549
rebased on master
updated the OP to reflect current status

amitiuttarwar marked this as ready for review on May 24, 2023

amitiuttarwar force-pushed on May 25, 2023

DrahtBot added the label CI failed on May 25, 2023

fanquake commented at 9:26 am on May 25, 2023: member

https://cirrus-ci.com/task/4751246913961984?logs=ci#L2724:

0test/denialofservice_tests.cpp(134): Entering test case "stale_tip_peer_management"
1test/denialofservice_tests.cpp(212): error: in "denialofservice_tests/stale_tip_peer_management": check vNodes[i]->fDisconnect == false has failed
2test/denialofservice_tests.cpp(214): error: in "denialofservice_tests/stale_tip_peer_management": check vNodes[max_outbound_full_relay - 2]->fDisconnect == true has failed
3test/denialofservice_tests.cpp(134): Leaving test case "stale_tip_peer_management"; testing time: 7571us

mzumsande commented at 3:34 pm on May 25, 2023: contributor

https://cirrus-ci.com/task/4751246913961984?logs=ci#L2724:

0test/denialofservice_tests.cpp(134): Entering test case "stale_tip_peer_management"
1test/denialofservice_tests.cpp(212): error: in "denialofservice_tests/stale_tip_peer_management": check vNodes[i]->fDisconnect == false has failed
2test/denialofservice_tests.cpp(214): error: in "denialofservice_tests/stale_tip_peer_management": check vNodes[max_outbound_full_relay - 2]->fDisconnect == true has failed
3test/denialofservice_tests.cpp(134): Leaving test case "stale_tip_peer_management"; testing time: 7571us

Ah, I think the problem with the test is that AddRandomOutboundPeer creates random IPv4 addresses, and sometimes these aren’t routable (e.g. protected ranges), so that they are not assigned to NET_IPV4 but NET_UNROUTABLE, which then makes the test fail. This happens intermittently. @amitiuttarwar I haven’t looked into solutions yet, but may just try another IPv4 address if this is the case?

brunoerg commented at 7:39 pm on May 25, 2023: contributor

Ah, I think the problem with the test is that AddRandomOutboundPeer creates random IPv4 addresses, and sometimes these aren’t routable (e.g. protected ranges), so that they are not assigned to NET_IPV4 but NET_UNROUTABLE, which then makes the test fail. This happens intermittently. @amitiuttarwar I haven’t looked into solutions yet, but may just try another IPv4 address if this is the case?

Not sure, I ran the test several times (debugging) and could get the error but didn’t get a NET_UNROUTABLE address.

amitiuttarwar force-pushed on May 27, 2023

amitiuttarwar commented at 1:25 am on May 27, 2023: contributor

Ah, I think the problem with the test is that AddRandomOutboundPeer creates random IPv4 addresses, and sometimes these aren’t routable (e.g. protected ranges), so that they are not assigned to NET_IPV4 but NET_UNROUTABLE, which then makes the test fail.

great find @mzumsande ! I was able to reproduce & observe this behavior, so I added a loop in AddRandomOutboundPeer to try again if the selected address is not routable. hopefully this works, I ran the current version >1000 times and didn’t see any failures locally. @brunoerg - how did you observe if the addrs are routable? note that in the failure situation it’s not the onion peer that causes the issue, its a IPv4 peer from the initial setup

DrahtBot removed the label CI failed on May 27, 2023

in src/test/denialofservice_tests.cpp:113 in 399e371942 outdated

126-    node.fSuccessfullyConnected = true;
127-
128-    connman.AddTestNode(node);
129+    auto routable = false;
130+
131+    while (!routable) {

brunoerg commented at 3:18 pm on May 27, 2023:

Instead of pushing the node to vNodes and removing it if it’s not routable, I think you could do something like:

 0static void AddRandomOutboundPeer(NodeId& id, std::vector<CNode*>& vNodes, PeerManager& peerLogic, ConnmanTestMsg& connman, ConnectionType connType, bool onion_peer = false)
 1{
 2    CAddress addr;
 3
 4    if (onion_peer) {
 5        auto tor_addr{g_insecure_rand_ctx.randbytes(ADDR_TORV3_SIZE)};
 6        BOOST_REQUIRE(addr.SetSpecial(OnionToString(tor_addr)));
 7    }
 8
 9    while (!addr.IsRoutable()) {
10        addr = CAddress(ip(g_insecure_rand_ctx.randbits(32)), NODE_NONE);
11    }
12
13    vNodes.emplace_back(new CNode{id++,
14            /*sock=*/nullptr,
15            addr,
16            /*nKeyedNetGroupIn=*/0,
17            /*nLocalHostNonceIn=*/0,
18            CAddress(),
19            /*addrNameIn=*/"",
20            connType,
21            /*inbound_onion=*/false});
22    CNode &node = *vNodes.back();
23
24    node.SetCommonVersion(PROTOCOL_VERSION);
25    peerLogic.InitializeNode(node, ServiceFlags(NODE_NETWORK | NODE_WITNESS));
26    node.fSuccessfullyConnected = true;
27    connman.AddTestNode(node);
28}

You can check if the addr is routable directly, no need to do it in CNode.
Check onion_peer before trying to create a routable addr.

amitiuttarwar commented at 5:20 pm on May 27, 2023:

great feedback! that’s much cleaner. incorporated in the latest push

amitiuttarwar force-pushed on May 27, 2023

in src/net_processing.cpp:5165 in 377e81e2e4 outdated

5156@@ -5155,6 +5157,12 @@ void PeerManagerImpl::EvictExtraOutboundPeers(std::chrono::seconds now)
5157             if (state == nullptr) return; // shouldn't be possible, but just in case
5158             // Don't evict our protected peers
5159             if (state->m_chain_sync.m_protect) return;
5160+            // Protect a peer if it's the only one for its network, counting IPv4 and
5161+            // IPv6 as one network.
5162+            // Both outbound-full-relay and manual connections are counted for this,
5163+            // but not block-relay-only connections because the goal
5164+            // is to have at least one tx-relaying connection to each reachable network.
5165+            if (m_connman.GetFullOutboundAndManualCount(pnode->addr.GetNetwork()) <= 1) return;

brunoerg commented at 6:27 pm on May 29, 2023:

We want to protect a peer if it’s the only one for its network. However, we can run a node supporting just one network. In this case, I suppose that calling GetFullOutboundAndManualCount might be unnecessary, we would iterating the whole m_nodes with no need.

In this case, I think we could have a function like SupportsMultipleNetworks to check whether we support more than one network, e.g.:

 0bool CConnman::SupportsMultipleNetworks() const
 1{
 2    int count = 0;
 3    for (int n = 0; n < NET_MAX; n++) {
 4        enum Network net = (enum Network)n;
 5        if (net == NET_UNROUTABLE || net == NET_INTERNAL) continue;
 6        if (IsReachable(net)) {
 7            if (net == NET_IPV4) n++;
 8            count++;
 9        }
10    }
11    return count > 0;
12}

if so, we can call GetFullOutboundAndManualCount:

0if (m_connman.SupportsMultipleNetworks() && m_connman.GetFullOutboundAndManualCount(pnode->addr.GetNetwork()) <= 1) return;

amitiuttarwar commented at 8:10 pm on June 5, 2023:

yeah, I see what you’re saying - that we can avoid iterating through all peers when we are only running on one network.

I’ve implemented a version of your suggestion in https://github.com/amitiuttarwar/bitcoin/commit/1805698096ae29e8dd6ecd1be9348a04f612602c. I changed the function implementation and also invoked from the additional call site in MaybePickPreferredNetwork. It seems viable.

I haven’t pushed it here yet because I’m still trying to figure out a slightly cleaner solution. I noticed that we are introducing a lot of functions to CConnman that iterate through the networks to check similar-but-slightly-different attributes. GetReachableEmptyNetworks already exists, this PR introduces MaybePickPreferredNetwork, and now we may also add this function, SupportsMultipleNetworks. given that CConnman is already quite large & frequently imported, I’m trying to see if there’s a way we could consolidate these similar functions together. will report back later this week :)

vasild commented at 1:41 pm on June 6, 2023:

GetFullOutboundAndManualCount() will iterate on all nodes, and is called from ForEachNode() which iterates on all nodes. This makes it O(number of nodes ^ 2). This will also do a recursive lock on m_nodes_mutex and there is an ongoing effort to eradicate recursive locks from the code base.

So, what about collecting the stats (a network->count map) just once, before the ForEachNode() call?

amitiuttarwar commented at 7:55 pm on June 11, 2023:

latest push collects stats as a CConnman member, so we don’t have to worry about iterating through all nodes

amitiuttarwar commented at 8:04 pm on June 11, 2023:

updated to have a small connman member to keep track of counts. now we don’t iterate through all peers anymore :)

brunoerg commented at 7:19 pm on May 29, 2023: contributor

Just a suggestion, but I think we could cover GetFullOutboundAndManualCount in fuzz.

 0diff --git a/src/test/fuzz/connman.cpp b/src/test/fuzz/connman.cpp
 1index f81658b83..f5867f8b4 100644
 2--- a/src/test/fuzz/connman.cpp
 3+++ b/src/test/fuzz/connman.cpp
 4@@ -128,4 +128,6 @@ FUZZ_TARGET_INIT(connman, initialize_connman)
 5     (void)connman.GetTotalBytesSent();
 6     (void)connman.GetTryNewOutboundPeer();
 7     (void)connman.GetUseAddrmanOutgoing();
 8+    const Network net{fuzzed_data_provider.PickValueInArray({Network::NET_IPV4, Network::NET_IPV6, Network::NET_INTERNAL, Network::NET_ONION})};
 9+    (void)connman.GetFullOutboundAndManualCount(net);
10 }

in src/net.cpp:1905 in 377e81e2e4 outdated

1901@@ -1871,7 +1902,14 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect)
1902                 }
1903             } else {
1904                 // Not a feeler
1905-                std::tie(addr, addr_last_try) = addrman.Select();
1906+                if (preferred_net.has_value()) {

brunoerg commented at 0:51 am on May 30, 2023:

In 377e81e2e409470d00e69247d0d9e980773ab73e, I suppose this verification could be avoided. In Select, new_only is false by default and preferred_net is optional and it will only have a value when MaybePickPreferredNetworkreturns true. So, if preferred_net doesn’t have any value, I suppose that addrman.Select(false, preferred_net) and addrman.Select() will have the same effect.

 0@@ -1901,15 +1915,7 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect)
 1                     std::tie(addr, addr_last_try) = addrman.Select(true);
 2                 }
 3             } else {
 4-                // Not a feeler
 5-                if (preferred_net.has_value()) {
 6-                    // pick an extra outbound peer from a preferred network.
 7-                    // The eviction logic in net_processing makes sure that
 8-                    //  a peer from another network will be evicted.
 9-                    std::tie(addr, addr_last_try) = addrman.Select(false, preferred_net);
10-                } else {
11-                    std::tie(addr, addr_last_try) = addrman.Select();
12-                }
13+                std::tie(addr, addr_last_try) = addrman.Select(false, preferred_net);

amitiuttarwar commented at 7:53 pm on June 5, 2023:

nice simplification! incorporated in latest push

amitiuttarwar force-pushed on Jun 5, 2023

amitiuttarwar commented at 8:23 pm on June 5, 2023: contributor

thanks for the review @brunoerg ! latest push incorporated fuzz test & ThreadOpenConnections simplification. I’m still working on the other feedback (as explained above).

one question about your suggested patch in the fuzz test: const Network net{fuzzed_data_provider.PickValueInArray({Network::NET_IPV4, Network::NET_IPV6, Network::NET_INTERNAL, Network::NET_ONION})}; is there a reason to select from the subset of networks? in the patch I pushed up, I also added the other ones. I saw that ConsumeNetAddr used the same subset, but I am guessing that’s because of other challenges in creating network addresses. just wanted to check to see if there’s something I’m missing.

IPv4 & IPv6 treatment

Context refresher: the open question is whether IPv4 & IPv6 should be bundled together as “clearnet” or treated as two separate networks. The current proposal opts for the former.

After chatting this through with @mzumsande, we are still leaning towards bundling into a single clearnet. Here is our understanding & rationale. We are curious to hear more feedback from reviewers, and are open to being persuaded that the alternative approach is beneficial through concrete examples.

We do not see clear value in treating them separately: from our research, it seems that most cloud providers allow users to expose IPv4 or IPv6 addresses with trivial setup. If users are running a node on a different setup, eg. local, they can also use a VPN to expose the opposite. The amount of effort required to obtain the opposite address generally seems minimal, which means diversifying would not meaningfully increase the cost of attack for a malicious entity.
The potential downside is clear: by default, bitcoind considers both IPv4 & IPv6 to be reachable. However, both Martin & I have experienced running nodes within a network setup such that only connection attempts to IPv4 nodes were successful. A node in this environment running current master will still have failed IPv6 connection attempts. But the problem (aka frequency of failed attempts) would increase in severity if we added this patch with logic that treated IPv4 & IPv6 separately.
If we wanted to treat these two networks separately and not worsen the rate of failed connections, we would have to add additional logic to prevent infinite periodic attempts (eg. to IPv6 nodes in the previous example). This additional complexity is viable, but does not feel justified given not having a clear value add of separating the two networks.
Just to clarify, having too many peers be protected from eviction is not a current concern regardless of the approach selected here. @vasild, @0xB10C - thoughts?

vasild commented at 11:43 am on June 6, 2023: contributor

I am slowly coming back to this PR…

we are still leaning towards bundling into a single clearnet … - thoughts?

Ok :)

in src/test/denialofservice_tests.cpp:221 in df76eb535b outdated

216+
217+    for (int i = 0; i < max_outbound_full_relay - 2; ++i) {
218+        BOOST_CHECK(vNodes[i]->fDisconnect == false);
219+    }
220+    BOOST_CHECK(vNodes[max_outbound_full_relay - 2]->fDisconnect == true);
221+    BOOST_CHECK(vNodes.back()->fDisconnect == false);

vasild commented at 10:23 am on June 7, 2023:

This test seems correct but is a bit obscure as it juggles with vNodes[max_outbound_full_relay - 2] and vNodes.back(). Here it does not check the value of vNodes[max_outbound_full_relay - 1]->fDisconnect (it is supposed to be true, left unmodified from the previous test).

Maybe it would help a bit if:

before the newly added test snippet set vNodes[max_outbound_full_relay - 1]->fDisconnect to false, so that all of them are false then the new test starts.
back() is not used and [max_outbound_full_relay + 1] is used instead (and [max_outbound_full_relay + 2] at the end).

amitiuttarwar commented at 7:55 pm on June 11, 2023:

updated to incorporate your suggestions, thanks!

in src/net.h:1044 in df76eb535b outdated

1027+     *
1028+     * @param[out]    network        Preferred network, if found.
1029+     *
1030+     * @return           bool        Whether a preferred network was found.
1031+     */
1032+    bool MaybePickPreferredNetwork(std::optional<Network>& network);

vasild commented at 10:28 am on June 7, 2023:

The return value and the optional output argument are redundant. Better to return std::optional and take no arguments:

0std::optional<Network> MaybePickPreferredNetwork()

(would have to be called outside of else if (... which I find clearer).

Or drop the optional:

0bool MaybePickPreferredNetwork(Network& network);

amitiuttarwar commented at 2:53 am on June 8, 2023:

yeah, the reason for this function signature was to enable calling from within the else if(...) conditional. pulling it out would mean having to run MaybePickPreferredNetwork every time ThreadOpenConnections runs, which seems unnecessary?

vasild commented at 12:42 pm on June 8, 2023:

yes, that would be unnecessary

amitiuttarwar commented at 5:50 pm on June 8, 2023:

okay, going to resolve this thread. but lmk if you have any other suggestions, it’s definitely a bit odd..

vasild commented at 1:13 pm on June 26, 2023:

Hmm, just realized that this is marked as resolved but is not resolved in the code. I am not sure, did you intentionally leave it as it is in the code now or did you forget to drop the optional like I suggested above?

amitiuttarwar commented at 4:19 pm on June 26, 2023:

sorry, I guess I misinterpreted this conversation. let’s clarify :)

your initial comment had two suggestions

remove the out param & return optional instead of bool
drop the optional from the out param

it looks like I focused & responded to the issue with # 1, but overlooked # 2. to restate, I think # 1 would be nice in isolation, but based on the caller, doesn’t make the most sense here.

that said, # 2 of dropping the optional seems reasonable. the caller is already responsible for checking the return value so the optional isn’t adding much value. will update.

thanks for catching, sorry about that!

amitiuttarwar commented at 11:29 pm on June 30, 2023:

removing the optional from the out param means that the compiler can’t guarantee that the value of network was set. so preferred_net needs a default value, which I set to NET_UNROUTABLE and then checked for later in the function. this approach doesn’t feel much cleaner to me than the previous of having network be optional, but I don’t feel strongly either way. or, is there a better solution I’m not seeing right now?

amitiuttarwar commented at 2:44 am on July 6, 2023:

reverted to the original optional arg in the latest push (https://github.com/bitcoin/bitcoin/compare/7a1c663e3727695a081a735c37819241341f3730..1e65aae8068ecf313a7c3b176dfc1326b3b67fbe) - checking for preferred_net meant I changed the default behavior. in order to fix that I would have had to add another else statement which seems more confusing than the original implementation, so went back

jonatack commented at 8:09 pm on August 9, 2023:

The return value and the optional output argument are redundant. Better to return std::optional and take no arguments:
0std::optional<Network> MaybePickPreferredNetwork()

Agree. This method could also be const and have a Clang thread-safety annotation and related assertions.

in src/net.cpp:1636 in df76eb535b outdated

1631+{
1632+    std::vector<Network> preferred_networks;
1633+    for (int n = 0; n < NET_MAX; n++) {
1634+        enum Network net = (enum Network)n;
1635+        if (net == NET_UNROUTABLE || net == NET_INTERNAL) continue;
1636+        if (IsReachable(net) && GetFullOutboundAndManualCount(net) == 0 && addrman.Size(net) != 0) {

vasild commented at 10:55 am on June 7, 2023:

GetFullOutboundAndManualCount(net) is called for each network and each call iterates all nodes, collecting stats just for the given network, ignoring the others. It can collect all the stats with one iteration (network->count map) and be called just once, before this loop (vs for each netowork).

amitiuttarwar commented at 7:56 pm on June 11, 2023:

moved to member to prevent iterating through all nodes

in src/net.cpp:1640 in df76eb535b outdated

1635+        if (net == NET_UNROUTABLE || net == NET_INTERNAL) continue;
1636+        if (IsReachable(net) && GetFullOutboundAndManualCount(net) == 0 && addrman.Size(net) != 0) {
1637+            preferred_networks.push_back(net);
1638+        }
1639+    }
1640+    if (preferred_networks.size() > 0) {

vasild commented at 11:16 am on June 7, 2023:

There is no need to collect more than one element in preferred_networks, the previous loop can be stopped as soon as a match is found and the shuffling can be done on the order in which networks are iterated (untested):

 0std::optional<Network> CConnman::MaybePickPreferredNetwork()
 1{
 2    std::array<Network, 5> nets{NET_IPV4, NET_IPV6, NET_ONION, NET_I2P, NET_CJDNS};
 3    Shuffle(nets.begin(), nets.end(), FastRandomContext());
 4    auto peers_per_net = GetFullOutboundAndManualCounts();
 5    for (const auto net : nets) {
 6        if (IsReachable(net) && peers_per_net[net] == 0 && addrman.Size(net) != 0) {
 7            return net;
 8        }
 9    }
10    return std::nullopt;
11}

amitiuttarwar commented at 7:56 pm on June 11, 2023:

done, thanks!

amitiuttarwar commented at 8:01 pm on June 11, 2023:

question: I noticed you used array in your implementation but we were using vector. is there a strong reason for that?

in src/net.cpp:1619 in df76eb535b outdated

1625+        }
1626+    }
1627+    return outbound_peers;
1628+}
1629+
1630+bool CConnman::MaybePickPreferredNetwork(std::optional<Network>& network)

vasild commented at 11:30 am on June 7, 2023:

The way this is implemented would give clearnet 2x chance of being returned over another network with 0 peers. Is this the intention?

For example: let all of IPv4, IPv6 and Tor be reachable and there are 0 peers to each of those networks. Clearnet (IPv4 or IPv6) will be returned in 67% of the cases and Tor in 33%. Should that not be clearnet 50%, Tor 50%?

ajtowns commented at 2:55 am on June 8, 2023:

I think the requirement here is only “try each network with reasonable probability” – you want to be connected to all of the networks, so which one comes first doesn’t matter very much.

If you don’t have ipv4 or ipv6 connections but are going through this codepath, then that means you have all your outbounds connected via privacy networks, but haven’t chosen to -onlynet=tor or similar. So biassing towards clearnet probably makes sense anyway: clearnet connections probably have higher bandwidth/lower latency so will give you blocks/txs quicker, reducing the load on the privacy networks.

vasild commented at 12:59 pm on June 8, 2023:

Ok, I am just checking because elsewhere the intention is to treat clearnet as one network and this code does not do that. I do not have a strong opinion either way.

amitiuttarwar commented at 7:57 pm on June 11, 2023:

we have now removed any clearnet bundling, so this quirk is no longer relevant

vasild commented at 11:36 am on June 7, 2023: contributor

Approach ACK df76eb535bbd215da17cee8e0f480af9e94e9e70

ajtowns commented at 2:47 am on June 8, 2023: contributor

The potential downside is clear: by default, bitcoind considers both IPv4 & IPv6 to be reachable. However, both Martin & I have experienced running nodes within a network setup such that only connection attempts to IPv4 nodes were successful. A node in this environment running current master will still have failed IPv6 connection attempts. But the problem (aka frequency of failed attempts) would increase in severity if we added this patch with logic that treated IPv4 & IPv6 separately.

I’m not convinced this is a problem in the first place – failed connection attempts on networks that don’t exist don’t seem to impose any significant costs?

It seems simpler to me to just treat each network as if it’s meaningfully distinct, and try to remain connected to each one if possible. I’m not sure trying to be more clever about that (eg: we want one of ipv4 or ipv6 because those connections are fast; we want new networks like cjdns because otherwise they might be really hard to bootstrap because few outbound connections are made; we want i2p because it’s harder to eclipse than tor; etc) actually has much benefit?

If we are worried about failed connections for some reason, we could lower the frequency from 1 minute to perhaps 5 minutes. I think the worst case scenario there is when you have all 8 outbounds via one network (ipv4 eg), want to make a connection to another network (eg cjdns), but have all 3 remaining networks (ipv6, tor, i2p) marked as reachable but not actually functioning. Then you’ve got a 75% chance of MaybePickPreferredNetwork wasting your time, and having to wait another interval. That seems to have about a 4x multiplier in the worst case, so you’d only be making the extra net outbound connection perhaps every 20 minutes, instead of every 5 minutes. But for a long-lived node, that’s probably not too bad even so. And in the ordinary case where you only perhaps have ipv6 as reachable but not working, it’s only a 2x multiplier.

vasild commented at 12:59 pm on June 8, 2023: contributor

On treating IPv4 and IPv6 as one network - I am ok either way. I see the merit in amiti’s considerations above, but also on @ajtowns’s. One more thing to consider - for an attacker it is not too difficult to get IPv6 connectivity but that is also the case for Tor. I mean it is not too difficult to get Tor connectivity either, but that is still some extra effort.

amitiuttarwar force-pushed on Jun 11, 2023

amitiuttarwar commented at 8:03 pm on June 11, 2023: contributor

thank you for the reviews @ajtowns & @vasild 🙌

significant changes with latest push:

introduce a new member on CConnman called m_network_conn_counts which is a map from Network to an atomic that keeps track of the number of OUTBOUND_FULL_RELAY and MANUAL connections on the node broken down by network. this means we don’t have to iterate through all nodes anymore.
changed the interval from 1 min to 5 min.
got rid of any “clearnet” bundling, now we treat IPV4 & IPV6 as separate networks, just like any other.

one note: naming is HARD. there’s a lot of “full-outbound-or-manual” in the code, which is quite a mouthful. I couldn’t come up with any good names for the combo, but open to suggestions. I think “full-tx-outbound” would be the combined name, but it’s confusing since OUTBOUND_FULL_RELAY is the name of the automatic connections. Instead of renaming all of OUTBOUND_FULL_RELAY to AUTOMATIC_OUTBOUND_FULL_RELAY or something, I opted for the mouthful of “full outbound or manual”.

DrahtBot added the label CI failed on Jun 11, 2023

amitiuttarwar force-pushed on Jun 11, 2023

DrahtBot removed the label CI failed on Jun 12, 2023

in src/test/util/net.h:33 in a5e431daf0 outdated

28@@ -29,7 +29,10 @@ struct ConnmanTestMsg : public CConnman {
29     {
30         LOCK(m_nodes_mutex);
31         m_nodes.push_back(&node);
32+
33+        if (node.IsManualOrFullOutboundConn()) { m_network_conn_counts[node.ConnectedThroughNetwork()] += 1; }

mzumsande commented at 2:37 pm on June 12, 2023:

nit: This belongs in the first commit, not the second.

amitiuttarwar commented at 8:25 pm on June 14, 2023:

thanks, fixed

in src/net.cpp:578 in d1142f031c outdated

574@@ -575,6 +575,8 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo
575                              });
576     pnode->AddRef();
577 
578+    if (pnode->IsManualOrFullOutboundConn()) { m_network_conn_counts[pnode->ConnectedThroughNetwork()] += 1; }

mzumsande commented at 2:48 pm on June 12, 2023:

Is the use of ConnectedThroughNetwork() instead of addr.GetNetwork() on purpose? The name and the extra logic for inbound onions suggest that it’s tailored towards inbound connections, and there is the subtle difference that it calls addr.GetNetClass() instead of addr.GetNetwork(), which means a different behavior for linked IPV4 addresses.

I think that in general it would be good to have more consistency between these two functions, which is out of scope for this PR. But we should have consistency in what function is called here (the net_processing call to MultipleManualOrFullOutboundConns uses GetNetwork())

amitiuttarwar commented at 8:27 pm on June 14, 2023:

good catch! I didn’t realize that difference between GetNetClass() and GetNetwork(). updated this site to use GetNetwork() in the latest push.

amitiuttarwar force-pushed on Jun 14, 2023

DrahtBot added the label CI failed on Jun 14, 2023

vasild commented at 4:17 pm on June 15, 2023: contributor

Yes. Array does not involve heap allocation - it is simpler and cheaper structure than vector. If the size is known upfront and it is not going to be resized later then array is the obvious choice.

vasild commented at 4:29 pm on June 15, 2023: contributor

On 11 June 2023 22:03:39 CEST, Amiti Uttarwar @.***> wrote:

thank you for the reviews @ajtowns & @vasild 🙌

significant changes with latest push:

introduce a new member on CConnman called m_network_conn_counts which is a map from Network to an atomic that keeps track of the number of OUTBOUND_FULL_RELAY and MANUAL connections on the node broken down by network. this means we don’t have to iterate through all nodes anymore.

I did not look at the code yet, but why should it be atomic? If there could be concurrent access then the map itself needs to be protected, not just the individual values.

changed the interval from 1 min to 5 min.

got rid of any “clearnet” bundling, now we treat IPV4 & IPV6 as separate networks, just like any other.

one note: naming is HARD. there’s a lot of “full-outbound-or-manual” in the code, which is quite a mouthful. I couldn’t come up with any good names for the combo, but open to suggestions. I think “full-tx-outbound” would be the combined name, but it’s confusing since OUTBOUND_FULL_RELAY is the name of the automatic connections. Instead of renaming all of OUTBOUND_FULL_RELAY to AUTOMATIC_OUTBOUND_FULL_RELAY or something, I opted for the mouthful of “full outbound or manual”.

full-outbound-or-manual looks ok, it is obvious. full-tx-outbound is obscure, only a select few will know what it means, and even they/we will forget it after 6 months

in src/net.cpp:582 in f653a45f4f outdated

577@@ -575,6 +578,9 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo
578                              });
579     pnode->AddRef();
580 
581+    // update connection count by network
582+    if (pnode->IsManualOrFullOutboundConn()) { m_network_conn_counts[addrConnect.GetNetwork()] += 1; }

vasild commented at 1:30 pm on June 23, 2023:

style: remove the braces or put the statement on a new line. From doc/developer-notes.md:

If an if only has a single-statement then-clause, it can appear on the same line as the if, without braces.

(same when decrementing)

vasild commented at 1:31 pm on June 23, 2023:

Might as well use ++ instead += 1:

0++m_network_conn_counts[addrConnect.GetNetwork()]

(same when decrementing)

amitiuttarwar commented at 6:24 pm on June 25, 2023:

updated

amitiuttarwar commented at 6:26 pm on June 25, 2023:

done

in src/net.cpp:2411 in f653a45f4f outdated

2403@@ -2360,6 +2404,11 @@ bool CConnman::Start(CScheduler& scheduler, const Options& connOptions)
2404         semAddnode = std::make_unique<CSemaphore>(nMaxAddnode);
2405     }
2406 
2407+    for (int n = 0; n < NET_MAX; n++) {
2408+        enum Network net = (enum Network)n;
2409+        m_network_conn_counts[net] = 0;
2410+    }
2411+

vasild commented at 1:35 pm on June 23, 2023:

This is unnecessary, you can just drop it. The first time operator[] is used for a given key a default-initialized value is inserted for that key, which in our case is 0.

amitiuttarwar commented at 6:27 pm on June 25, 2023:

changed to array, so this is no longer relevant

amitiuttarwar commented at 11:29 pm on June 30, 2023:

changed back to a map, so this is relevant again. removed the explicit initialization due to the default behavior you mentioned. thanks!

vasild commented at 7:51 am on August 4, 2023:

changed back to array and this was not added back but it is not needed because the array is default-initialized: std::array<...> foo = {};

in src/net.h:1056 in f653a45f4f outdated

1051@@ -1023,6 +1052,9 @@ class CConnman
1052     std::chrono::seconds nMaxOutboundCycleStartTime GUARDED_BY(m_total_bytes_sent_mutex) {0};
1053     uint64_t nMaxOutboundLimit GUARDED_BY(m_total_bytes_sent_mutex);
1054 
1055+    // Stores number of full-tx connections (outbound and manual) per network
1056+    std::map<Network, std::atomic<unsigned int>> m_network_conn_counts;

vasild commented at 1:46 pm on June 23, 2023:

This map can be accessed from multiple threads and has to be protected by a mutex. It is too dangerous to assume that it will never be modified. operator[] which is used later in the code is not const. For example, it is not guaranteed that it will not cause some internal reorg of the structure.

std::unordered_map suits better here because we don’t need an order and its access and insertion complexity is O(1).

If you really like using unprotected structure of atomics, then maybe std::array would be ok (but have to index using Network which smells bad). Another option - would it be possible to make this map or array const?

amitiuttarwar commented at 2:02 am on June 25, 2023:

my latest thinking was to change this data struct to an array, add explicit numeric values to the Network enum (for index values) & then protect the array with m_nodes_mutex to keep the values consistent between the relevant data structures tracking node data. what do you think of this approach?

(I had coded it up, but then managed to spill liquid on my laptop which fried it as I was trying to push the changes. still working to get my current setup back to functional to make the changes 😬)

Another option - would it be possible to make this map or array const?

not sure I follow. what would it mean for the array to be const? isn’t the point to update the values? 😛

vasild commented at 9:17 am on June 26, 2023:

change this data struct to an array, add explicit numeric values to the Network enum (for index values)

That seems unnecessary, it is good to rely as less as possible on the actual values of the Network enums. That is - the code better not break if NET_ONION is not 3, for example. Use them just as labels. std::unordered_map fits best with that - std::unordered_map<Network, size_t> would work even if NET_* are changed to be strings!

… protect the array with m_nodes_mutex to keep the values consistent between the relevant data structures tracking node data

Yes, that sounds like a good idea!

… spill liquid on my laptop …

Oh, good luck recovering! :crossed_fingers:

isn’t the point to update the values?

Right, that does not make sense. I was thinking about something like this:

0    const std::map<Network, size_t> m{{NET_IPV4, 0}, {NET_IPV6, 0}, ...};
1    ++m[NET_IPV4]; // modify only the value, not the map structure, not possible

vasild commented at 1:51 pm on June 23, 2023: contributor

Reviewed up to 6e416fb222

amitiuttarwar force-pushed on Jun 25, 2023

amitiuttarwar commented at 6:32 pm on June 25, 2023: contributor

main update with latest push: changed the data structure to an array, indexed by the net message types & protected by the m_nodes_mutex. thanks for the thoughtful reviews! curious to hear thoughts on this new approach

also added a log statement to ThreadOpenConnections so we can observe frequency of the network connections in our nodes, and incorporated style review suggestions

in src/net.cpp:1139 in c11659b864 outdated

1134@@ -1135,6 +1135,9 @@ void CConnman::DisconnectNodes()
1135                 // close socket and cleanup
1136                 pnode->CloseSocketDisconnect();
1137 
1138+                // update connection count by network
1139+                if (pnode->IsManualOrFullOutboundConn()) m_network_conn_counts[pnode->ConnectedThroughNetwork()]--;

mzumsande commented at 1:14 am on June 26, 2023:

here and in test/util/net.h - this is still using ConnectedThroughNetwork (see previous comment).

amitiuttarwar commented at 11:33 pm on June 30, 2023:

thanks! okay, fixed and confirmed that none of the current patch invokes ConnectedThroughNetwork anymore :)

in src/net.h:1041 in c11659b864 outdated

1037@@ -1023,6 +1038,7 @@ class CConnman
1038     std::chrono::seconds nMaxOutboundCycleStartTime GUARDED_BY(m_total_bytes_sent_mutex) {0};
1039     uint64_t nMaxOutboundLimit GUARDED_BY(m_total_bytes_sent_mutex);
1040 
1041+

mzumsande commented at 1:14 am on June 26, 2023:

nit: unrelated whitespace change

amitiuttarwar commented at 11:30 pm on June 30, 2023:

fixed

in src/net.cpp:2043 in c11659b864 outdated

2037@@ -2035,6 +2038,9 @@ void CConnman::OpenNetworkConnection(const CAddress& addrConnect, bool fCountFai
2038     {
2039         LOCK(m_nodes_mutex);
2040         m_nodes.push_back(pnode);
2041+
2042+        // update connection count by network
2043+        if (pnode->IsManualOrFullOutboundConn()) m_network_conn_counts[addrConnect.GetNetwork()]++;

mzumsande commented at 1:19 am on June 26, 2023:

I think addrConnect can’t be used here, because it can be empty if pszDest is set (that’s why the CI fails too). You could use pnode->addr instead, which is set from either addrConnect or pszDest in ConnectNode()!

amitiuttarwar commented at 11:33 pm on June 30, 2023:

ah, good catch. updated

in src/net.h:470 in 564bf851cc outdated

464@@ -465,6 +465,21 @@ class CNode
465         return m_conn_type == ConnectionType::MANUAL;
466     }
467 
468+    bool IsManualOrFullOutboundConn() const {
469+        switch (m_conn_type) {
470+            case ConnectionType::INBOUND:

vasild commented at 1:18 pm on June 26, 2023:

style: use clang-format, this should be:

0    bool IsManualOrFullOutboundConn() const 
1    {
2        switch (m_conn_type) {
3        case ConnectionType::INBOUND:
4...

amitiuttarwar commented at 11:34 pm on June 30, 2023:

updated and double checked that clang-format doesn’t have any other suggestions for the patch

in src/test/util/net.h:36 in 564bf851cc outdated

28@@ -29,7 +29,10 @@ struct ConnmanTestMsg : public CConnman {
29     {
30         LOCK(m_nodes_mutex);
31         m_nodes.push_back(&node);
32+
33+        if (node.IsManualOrFullOutboundConn()) m_network_conn_counts[node.ConnectedThroughNetwork()]++;
34     }
35+
36     void ClearTestNodes()

vasild commented at 1:25 pm on June 26, 2023:

unrelated whitespace change

amitiuttarwar commented at 11:11 pm on June 30, 2023:

hm, this one was intentional. I edited AddTestNode and it was previously missing a whitespace between functions

in src/net.h:1085 in 564bf851cc outdated

1077@@ -1048,6 +1078,9 @@ class CConnman
1078     std::atomic<NodeId> nLastNodeId{0};
1079     unsigned int nPrevNodeCount{0};
1080 
1081+    // Stores number of full-tx connections (outbound and manual) per network
1082+    std::array<unsigned int, Network::NET_MAX> m_network_conn_counts GUARDED_BY(m_nodes_mutex) = {};

vasild commented at 1:29 pm on June 26, 2023:

I suggested std::array in another comment as a way to workaround the lack of mutex protection. Now that this is protected by a mutex and no need to tweak or workaround concurrency issues, I think the neatest option is:

0    std::unordered_map<Network, size_t> m_network_conn_counts GUARDED_BY(m_nodes_mutex);

The std::array usage adds the requirement for sequential values of NET_* that are without gaps and start from 0 and have NET_MAX. That makes the code more fragile and harder to change in the future.

amitiuttarwar commented at 11:35 pm on June 30, 2023:

ah I see. okay updated :)

ajtowns commented at 1:52 am on July 6, 2023:

We already require NET_* to go from 0 to NET_MAX with no gaps every time we do for (int n = 0; n < NET_MAX; ++n). It’s a standard expectation of enums and it’s documented as such in netaddress.h. It’s not an added requirement, and doesn’t make the code fragile or harder to change…

vasild commented at 1:50 pm on June 26, 2023: contributor

Approach ACK 564bf851cccd34e859f23b6e6b00debebba9a3c2

maflcko commented at 10:58 am on June 28, 2023: member

CI failure in feature_proxy.py:

https://cirrus-ci.com/task/4981950109188096?logs=ci#L5325

 0 node0 stderr net.cpp:1142:113: runtime error: unsigned integer overflow: 0 - 1 cannot be represented in type 'value_type' (aka 'unsigned int')
 1    [#0](/bitcoin-bitcoin/0/) 0x55dbd8047436 in CConnman::DisconnectNodes() src/net.cpp:1142:113
 2    [#1](/bitcoin-bitcoin/1/) 0x55dbd807d233 in CConnman::ThreadSocketHandler() src/net.cpp:1393:9
 3    [#2](/bitcoin-bitcoin/2/) 0x55dbd807d233 in CConnman::Start(CScheduler&, CConnman::Options const&)::$_1::operator()() const src/net.cpp:2430:75
 4    [#3](/bitcoin-bitcoin/3/) 0x55dbd807d233 in void std::__invoke_impl<void, CConnman::Start(CScheduler&, CConnman::Options const&)::$_1&>(std::__invoke_other, CConnman::Start(CScheduler&, CConnman::Options const&)::$_1&) /usr/bin/../lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/bits/invoke.h:61:14
 5    [#4](/bitcoin-bitcoin/4/) 0x55dbd807d233 in std::enable_if<is_invocable_r_v<void, CConnman::Start(CScheduler&, CConnman::Options const&)::$_1&>, void>::type std::__invoke_r<void, CConnman::Start(CScheduler&, CConnman::Options const&)::$_1&>(CConnman::Start(CScheduler&, CConnman::Options const&)::$_1&) /usr/bin/../lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/bits/invoke.h:111:2
 6    [#5](/bitcoin-bitcoin/5/) 0x55dbd807d233 in std::_Function_handler<void (), CConnman::Start(CScheduler&, CConnman::Options const&)::$_1>::_M_invoke(std::_Any_data const&) /usr/bin/../lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/bits/std_function.h:290:9
 7    [#6](/bitcoin-bitcoin/6/) 0x55dbd94ac316 in std::function<void ()>::operator()() const /usr/bin/../lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/bits/std_function.h:591:9
 8    [#7](/bitcoin-bitcoin/7/) 0x55dbd94ac316 in util::TraceThread(std::basic_string_view<char, std::char_traits<char>>, std::function<void ()>) src/util/thread.cpp:21:9
 9    [#8](/bitcoin-bitcoin/8/) 0x55dbd807cfc8 in void std::__invoke_impl<void, void (*)(std::basic_string_view<char, std::char_traits<char>>, std::function<void ()>), char const*, CConnman::Start(CScheduler&, CConnman::Options const&)::$_1>(std::__invoke_other, void (*&&)(std::basic_string_view<char, std::char_traits<char>>, std::function<void ()>), char const*&&, CConnman::Start(CScheduler&, CConnman::Options const&)::$_1&&) /usr/bin/../lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/bits/invoke.h:61:14
10    [#9](/bitcoin-bitcoin/9/) 0x55dbd807cfc8 in std::__invoke_result<void (*)(std::basic_string_view<char, std::char_traits<char>>, std::function<void ()>), char const*, CConnman::Start(CScheduler&, CConnman::Options const&)::$_1>::type std::__invoke<void (*)(std::basic_string_view<char, std::char_traits<char>>, std::function<void ()>), char const*, CConnman::Start(CScheduler&, CConnman::Options const&)::$_1>(void (*&&)(std::basic_string_view<char, std::char_traits<char>>, std::function<void ()>), char const*&&, CConnman::Start(CScheduler&, CConnman::Options const&)::$_1&&) /usr/bin/../lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/bits/invoke.h:96:14
11    [#10](/bitcoin-bitcoin/10/) 0x55dbd807cfc8 in void std::thread::_Invoker<std::tuple<void (*)(std::basic_string_view<char, std::char_traits<char>>, std::function<void ()>), char const*, CConnman::Start(CScheduler&, CConnman::Options const&)::$_1>>::_M_invoke<0ul, 1ul, 2ul>(std::_Index_tuple<0ul, 1ul, 2ul>) /usr/bin/../lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/bits/std_thread.h:252:13
12    [#11](/bitcoin-bitcoin/11/) 0x55dbd807cfc8 in std::thread::_Invoker<std::tuple<void (*)(std::basic_string_view<char, std::char_traits<char>>, std::function<void ()>), char const*, CConnman::Start(CScheduler&, CConnman::Options const&)::$_1>>::operator()() /usr/bin/../lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/bits/std_thread.h:259:11
13    [#12](/bitcoin-bitcoin/12/) 0x55dbd807cfc8 in std::thread::_State_impl<std::thread::_Invoker<std::tuple<void (*)(std::basic_string_view<char, std::char_traits<char>>, std::function<void ()>), char const*, CConnman::Start(CScheduler&, CConnman::Options const&)::$_1>>>::_M_run() /usr/bin/../lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/bits/std_thread.h:210:13
14    [#13](/bitcoin-bitcoin/13/) 0x7f22df2e63e2  (/lib/x86_64-linux-gnu/libstdc++.so.6+0xe63e2) (BuildId: 7e8507d31f51627e18fc00bd04ee5a2e01069a96)
15    [#14](/bitcoin-bitcoin/14/) 0x7f22dee8f189  (/lib/x86_64-linux-gnu/libc.so.6+0x8f189) (BuildId: bdb8aa3b1b60f9d43e1c70ba98158e05f765efdc)
16    [#15](/bitcoin-bitcoin/15/) 0x7f22def1dbcf  (/lib/x86_64-linux-gnu/libc.so.6+0x11dbcf) (BuildId: bdb8aa3b1b60f9d43e1c70ba98158e05f765efdc)
17SUMMARY: UndefinedBehaviorSanitizer: unsigned-integer-overflow net.cpp:1142:113 in

vasild commented at 11:39 am on June 28, 2023: contributor

… unsigned integer overflow …

Maybe collecting the number of “full outbound or manual” connections on the fly was not so bad? I am not objecting pre-collecting and maintaining that number in CConnman, just thinking aloud. That looks a bit more tricky and maybe more prone to bugs (like the above). It is also not generic - the “full outbound or manual” number is used only in this specific case and unlikely to be used elsewhere in the future. A more generic approach, that could possibly be reused by other code, would be to store the number per connection type and then this specific code would sum the numbers for OUTBOUND_FULL_RELAY and MANUAL, but that’s maybe over-engineering at this point?

kristapsk commented at 11:49 am on June 28, 2023: contributor

Concept ACK

amitiuttarwar force-pushed on Jun 30, 2023

amitiuttarwar commented at 11:39 pm on June 30, 2023: contributor

latest push returned to map & addressed review comments @vasild I very much agree with your line of questioning. storing as a data structure definitely has some explicit tradeoffs to counting the number on the fly. neither is a clear winner for me, so I’m fine implementing whatever reviewers prefer.

It is also not generic - the “full outbound or manual” number is used only in this specific case and unlikely to be used elsewhere in the future. A more generic approach, that could possibly be reused by other code, would be to store the number per connection type and then this specific code would sum the numbers for OUTBOUND_FULL_RELAY and MANUAL, but that’s maybe over-engineering at this point?

yeah, that’s exactly the reasoning / conclusion I drew. it wouldn’t be too difficult to adapt this data structure in the future, so I think it makes sense to keep it restricted to our use case for now

DrahtBot removed the label CI failed on Jul 1, 2023

amitiuttarwar force-pushed on Jul 6, 2023

amitiuttarwar commented at 2:46 am on July 6, 2023: contributor

small change to revert the function signature MaybePickPreferredNetwork to have network be an optional. can see #27213 (review) if you’re interested in more detail.

DrahtBot added the label CI failed on Jul 6, 2023

vasild approved

vasild commented at 9:40 am on July 6, 2023: contributor

ACK 1e65aae8068ecf313a7c3b176dfc1326b3b67fbe

vasild commented at 9:42 am on July 6, 2023: contributor

What is (was?) the reason for #27213 (comment)?

in src/net.cpp:94 in 1e65aae806 outdated

89@@ -90,6 +90,9 @@ static constexpr std::chrono::seconds MAX_UPLOAD_TIMEFRAME{60 * 60 * 24};
90 // A random time period (0 to 1 seconds) is added to feeler connections to prevent synchronization.
91 static constexpr auto FEELER_SLEEP_WINDOW{1s};
92 
93+/** Frequency to attempt extra connections to reachable networks we're not connected to yet **/
94+static constexpr auto EXTRA_NETWORK_PEER_INTERVAL = 5min;

willcl-ark commented at 1:17 pm on July 6, 2023:

nit: could use brace initialization here

willcl-ark commented at 2:05 pm on July 6, 2023:

The commit message says every ~1min, but this means it will be ~5min via GetExponentialRand I think? This seems to be more in line with timings I have observed during testing, e.g.

02023-07-05T14:04:26Z [net] Making network specific connection to ipv6_addr:8333
12023-07-05T14:15:16Z [net] Making network specific connection to onion_addr.onion:8333

amitiuttarwar commented at 5:34 pm on July 6, 2023:

you’re right. the commit message is a relic of the past interval. will update both of these if I retouch the code

amitiuttarwar commented at 11:40 pm on July 30, 2023:

incorporated brace initialization in 4428c6d (#28189)

amitiuttarwar commented at 6:40 pm on August 3, 2023:

updated the commit message and incorporated brace initialization in latest push

mzumsande commented at 2:17 pm on July 6, 2023: contributor

What is (was?) the reason for #27213 (comment)?

The unsigned integer overflow was caused by #27213 (review) (fixed now!) - the counter wasn’t incremented for new pszDest connections, but still decremented on disconnection, thus the overflow.

willcl-ark commented at 2:59 pm on July 6, 2023: member

Approach and lightly tested ACK.

Slight tangent, but also slightly related to the discussion on whether IPV4 and IPV6 should be grouped… I noticed during testing that a majority of my network specific connection attempts were to IPV6 addresses, even though I don’t have an IPV6 address and can’t make connections to it… This appears to be because all networks are set as Reachable by default unless specifically unset (which is not new to this PR)?

This has the effect on this PR that many of the every~5 minute network specific connections are wasted on ipv6 attempts.

DrahtBot removed the label CI failed on Jul 6, 2023

amitiuttarwar commented at 5:53 pm on July 6, 2023: contributor

@willcl-ark thanks for the review!

Slight tangent, but also slightly related to the discussion on whether IPV4 and IPV6 should be grouped… I noticed during testing that a majority of my network specific connection attempts were to IPV6 addresses, even though I don’t have an IPV6 address and can’t make connections to it… This appears to be because all networks are set as Reachable by default unless specifically unset (which is not new to this PR)?

This has the effect on this PR that many of the every~5 minute network specific connections are wasted on ipv6 attempts.

completely related, thanks for sharing! this makes sense based on the current implementation. the original proposal had special case logic to prevent this situation, but the direction of review has led to deciding this is an acceptable tradeoff. incase helpful, #27213 (comment) provides a recap, #27213 (comment) provides context for switching over.

willcl-ark commented at 10:09 am on July 7, 2023: member

This has the effect on this PR that many of the every~5 minute network specific connections are wasted on ipv6 attempts.

completely related, thanks for sharing! this makes sense based on the current implementation. the original proposal had special case logic to prevent this situation, but the direction of review has led to deciding this is an acceptable tradeoff. incase helpful, #27213 (comment) provides a recap, #27213 (comment) provides context for switching over.

Well that will teach me not to read all the previous comments fully before reviewing!

Is there some historical reason why we consider IPV6 to be active, even when it’s not? I mean sure, attempting connections is pretty cheap, but still?

  02023-07-06T14:04:26Z [net] Making network specific connection to [2001:569:7605:d700:d88a:94bb:8158:c0bf]:8333
  12023-07-06T14:15:16Z [net] Making network specific connection to 7lazlb2a4nfzhpzfjqdhuqmobq6vdeel46owxwmsfxqvjqlkpv4hdxyd.onion:8333
  22023-07-06T14:19:13Z [net] Making network specific connection to [2401:e180:8843:7fa:da:90f:6481:f37c]:8333
  32023-07-06T14:25:56Z [net] Making network specific connection to [240e:3a1:5c12:a671:9963:442d:8afb:1f66]:8333
  42023-07-06T14:32:32Z [net] Making network specific connection to [2607:fb90:8a84:3b5b:877c:b950:e861:ad3f]:8333
  52023-07-06T14:32:53Z [net] Making network specific connection to [2604:3d09:a684:6500::b730]:8333
  62023-07-06T14:33:40Z [net] Making network specific connection to [2603:90c8:5:ae54::1134]:8333
  72023-07-06T14:50:31Z [net] Making network specific connection to [2601:448:c400:1000::ef15]:8333
  82023-07-06T14:51:00Z [net] Making network specific connection to [2a02:1810:2f09:6bf0::1000]:8333
  92023-07-06T14:55:12Z [net] Making network specific connection to [2601:680:cd80:4560:f014:1dde:a3aa:b6dd]:8333
 102023-07-06T14:59:24Z [net] Making network specific connection to [2001:ac8:28:6f::10]:8333
 112023-07-06T15:04:21Z [net] Making network specific connection to [2604:3d09:407d:ce00::3e19]:8333
 122023-07-06T15:09:44Z [net] Making network specific connection to [2a01:540:c3d9:9700:54e4:647d:63a7:82f0]:8333
 132023-07-06T15:11:39Z [net] Making network specific connection to [2001:0:284a:364:3c95:aefb:4345:b866]:8333
 142023-07-06T15:19:18Z [net] Making network specific connection to [2001:999:609:d40e::2]:8333
 152023-07-06T15:20:20Z [net] Making network specific connection to [2a01:cb04:2c4:c400:585e:f634:ed61:5a1f]:8333
 162023-07-06T15:26:01Z [net] Making network specific connection to [2605:59c8:5151:9110:3fc2:81b3:8d94:1823]:8333
 172023-07-06T15:26:24Z [net] Making network specific connection to [2a02:8308:8081:f300:3b8:7ec0:2837:1b57]:8333
 182023-07-06T15:39:03Z [net] Making network specific connection to [2407:3640:2107:1278::1]:8333
 192023-07-06T15:40:57Z [net] Making network specific connection to [2003:dc:5f2c:7b9:7285:c2ff:fe2e:5c7f]:8333
 202023-07-06T15:48:09Z [net] Making network specific connection to [2001:0:8ff4:dc96:30be:3e97:3f57:fe7e]:8333
 212023-07-06T15:51:41Z [net] Making network specific connection to [2600:381:a504:62c8:cfc:e4a2:8ea:9bc0]:8333
 222023-07-06T15:58:41Z [net] Making network specific connection to [2406:da14:335:b603:b5e8:7570:be85:8ea1]:8333
 232023-07-06T16:02:54Z [net] Making network specific connection to [2001:1c03:4f03:1700:3c1b:8053:95fd:859b]:8333
 242023-07-06T16:04:02Z [net] Making network specific connection to [2001:41d0:a:6b1d::1]:8333
 252023-07-06T16:07:31Z [net] Making network specific connection to [2601:240:4b00:262:4434:21e8:7414:bb02]:8333
 262023-07-06T16:15:13Z [net] Making network specific connection to [2800:e2:c780:1859:6d64:7105:b90d:3bc9]:8333
 272023-07-06T16:17:52Z [net] Making network specific connection to [2001:0:2851:782c:38b3:8887:435d:7379]:8333
 282023-07-06T16:23:45Z [net] Making network specific connection to [2a02:a212:a1c1:7800:b9e4:53a7:67ab:b5e3]:8333
 292023-07-06T16:31:18Z [net] Making network specific connection to [2603:7000:ac00:750e:1479:7b85:fdc5:a792]:8333
 302023-07-06T16:45:07Z [net] Making network specific connection to [2607:fea8:5d20:a300::f770]:8333
 312023-07-06T16:50:47Z [net] Making network specific connection to [2a03:4000:f:670::2]:8333
 322023-07-06T16:57:45Z [net] Making network specific connection to [2001:1608:1b:f9::1]:17482
 332023-07-06T17:04:27Z [net] Making network specific connection to [2601:701:200:b010:6944:55f8:f635:4495]:8333
 342023-07-06T17:05:47Z [net] Making network specific connection to [2800:e2:b500:10f:989c:82a9:e249:f6bb]:8333
 352023-07-06T17:06:00Z [net] Making network specific connection to [2a03:b0c0:1:e0::6a5:f001]:8333
 362023-07-06T17:11:37Z [net] Making network specific connection to [2a0b:6204:20cc:7500:5073:d1cf:bc80:7ed5]:8333
 372023-07-06T17:11:42Z [net] Making network specific connection to [2a01:e0a:149:fcb0:7a2b:cbff:fe4d:1828]:8333
 382023-07-06T17:14:30Z [net] Making network specific connection to [2001:da8:1002:310:fdba:2f06:f2d7:fd53]:8333
 392023-07-06T17:29:31Z [net] Making network specific connection to [2600:100b:a000:1015:8d24:9f00:9926:9a61]:8333
 402023-07-06T17:31:11Z [net] Making network specific connection to [2a0d:3344:1b18:10::f43]:8333
 412023-07-06T17:37:24Z [net] Making network specific connection to [2607:fb91:83f:862:acd4:4936:c03e:853b]:8333
 422023-07-06T17:41:12Z [net] Making network specific connection to [2603:9004:900:af02:95ed:2928:d5ee:8404]:8333
 432023-07-06T17:50:08Z [net] Making network specific connection to [2001:41d0:602:3315::]:8333
 442023-07-06T18:00:48Z [net] Making network specific connection to [2003:c2:471c:1155:c825:a4e0:d618:ea6f]:8333
 452023-07-06T18:03:44Z [net] Making network specific connection to [2603:9000:a707:1f86::157d]:8333
 462023-07-06T18:05:15Z [net] Making network specific connection to [2603:7080:9302:9b73::1b91]:8333
 472023-07-06T18:19:09Z [net] Making network specific connection to [2a00:23c8:7e01:1a01:922:48dd:5234:3779]:8333
 482023-07-06T18:23:51Z [net] Making network specific connection to [2601:206:8383:5930::87d5]:8333
 492023-07-06T18:31:28Z [net] Making network specific connection to [2620:9b::191d:d8b7]:8333
 502023-07-06T18:34:04Z [net] Making network specific connection to [2001:4c4e:11d1:9f00:fcea:e214:8392:4917]:8333
 512023-07-06T18:34:24Z [net] Making network specific connection to [2600:1700:70:5f30:e8e6:306a:8f60:a029]:8333
 522023-07-06T18:37:18Z [net] Making network specific connection to [2400:6180:100:d0::7f9:e00e]:8333
 532023-07-06T18:44:31Z [net] Making network specific connection to [2a03:b0c0:1:d0::30:f001]:8555
 542023-07-06T18:51:54Z [net] Making network specific connection to [2a02:908:821:6160:2daa:a061:5fd1:cb9f]:8333
 552023-07-06T19:03:57Z [net] Making network specific connection to [2a02:8388:e5c3:4a80:201:2eff:fe82:b3cc]:8333
 562023-07-06T19:04:26Z [net] Making network specific connection to [2600:1700:6184:9200::42]:8333
 572023-07-06T19:06:40Z [net] Making network specific connection to [2601:300:4003:dbb0::99cd]:8333
 582023-07-06T19:09:08Z [net] Making network specific connection to [2a0d:3344:1b62:6610::9e2]:8333
 592023-07-06T19:14:06Z [net] Making network specific connection to [2607:fcd0:ccc0:1d02::b37e]:8333
 602023-07-06T19:30:14Z [net] Making network specific connection to [2003:c8:cf22:b98b:d92e:b84a:afb9:ffdd]:8333
 612023-07-06T19:33:46Z [net] Making network specific connection to [1342:b5e:d22a:cf7:39e8:bea0:c8ce:b627]:8333
 622023-07-06T19:36:21Z [net] Making network specific connection to [2001:8b0:daf:e5be:fe88:ada6:e89:d890]:8333
 632023-07-06T19:39:19Z [net] Making network specific connection to [2001:41d0:203:bb0a::]:8333
 642023-07-06T19:40:52Z [net] Making network specific connection to [2800:150:13e:7b:99fa:830:c73d:dedd]:8333
 652023-07-06T19:44:24Z [net] Making network specific connection to [2603:9000:dd1a:5800:d04c:b91c:c3ec:6c2b]:8333
 662023-07-06T19:44:34Z [net] Making network specific connection to [2603:7081:7842:a846::1aa9]:8333
 672023-07-06T19:48:04Z [net] Making network specific connection to [2a01:e34:ecae:a390:a0da:f5d:2b1d:9643]:8333
 682023-07-06T19:58:38Z [net] Making network specific connection to [2a03:4000:54:fac:a8fb:beff:fe0f:de18]:8333
 692023-07-06T20:00:05Z [net] Making network specific connection to [2001:da8:1002:310:1d5b:7d7a:dcd0:e546]:8333
 702023-07-06T20:02:28Z [net] Making network specific connection to [2607:fea8:45da:6800::e471]:8333
 712023-07-06T20:08:03Z [net] Making network specific connection to [2604:2d80:9f8b:6e00::1001]:8333
 722023-07-06T20:09:25Z [net] Making network specific connection to [2600:6c56:6508:c8:815b:578:a8e6:a646]:8333
 732023-07-06T20:16:18Z [net] Making network specific connection to [2a02:ce0:1801:d672:1494:98f7:4c2c:6d10]:8333
 742023-07-06T20:16:25Z [net] Making network specific connection to [2a01:cb19:8486:b100:e8d6:1cca:99bd:65bc]:8333
 752023-07-06T20:22:26Z [net] Making network specific connection to [2a01:e11:100c:70:e531:ed0a:7844:dd36]:8333
 762023-07-06T20:25:57Z [net] Making network specific connection to [2604:3d08:a67e:b000::f338]:8333
 772023-07-06T20:42:28Z [net] Making network specific connection to [2002:4473:1121:0:527:d5a8:cc33:9b63]:8333
 782023-07-06T20:44:22Z [net] Making network specific connection to [2800:2181:5400:ec:7652:b00b:a33c:64be]:8333
 792023-07-06T20:46:43Z [net] Making network specific connection to [2a02:a03f:add0:fe00:4c38:d828:5f89:ea5a]:8333
 802023-07-06T20:54:47Z [net] Making network specific connection to [2601:647:4000:7690:8a1:af9d:e225:14d1]:8333
 812023-07-06T20:57:03Z [net] Making network specific connection to [2806:104e:c:654a:1981:7ddb:d93:87ec]:8333
 822023-07-06T21:15:29Z [net] Making network specific connection to [2002:6205:27e:0:1004:c04c:9f00:782e]:8333
 832023-07-06T21:15:30Z [net] Making network specific connection to [2a02:2454:9933:ca00::ec73]:8333
 842023-07-06T21:19:52Z [net] Making network specific connection to [2001:569:7e07:bd00:10da:ba84:bf02:c293]:8333
 852023-07-06T21:20:36Z [net] Making network specific connection to [2a01:4f9:2b:1bde::2]:8333
 862023-07-06T21:25:31Z [net] Making network specific connection to [2a0b:f4c2::3]:8333
 872023-07-06T21:30:31Z [net] Making network specific connection to [2a01:cb08:8268:9500:88e1:8698:fe3:974e]:8333
 882023-07-06T21:31:03Z [net] Making network specific connection to [2001:9e8:f099:2000:79af:765e:7bbe:8c36]:8333
 892023-07-06T21:36:56Z [net] Making network specific connection to [2603:7000:4600:500:7da2:6e48:a92b:2765]:8333
 902023-07-06T21:47:35Z [net] Making network specific connection to [2a02:587:1a11:1d00:6755:ad79:9339:ff78]:8333
 912023-07-06T21:54:13Z [net] Making network specific connection to [2600:4040:7209:2300:5573:7ebe:3622:efde]:8333
 922023-07-06T21:56:57Z [net] Making network specific connection to [2003:e6:1725:4000:1844:7011:ba02:3b8a]:8333
 932023-07-06T21:59:39Z [net] Making network specific connection to [240e:3b5:3471:fcc1:951c:670d:c4cc:554]:8333
 942023-07-06T22:02:29Z [net] Making network specific connection to [2a03:1b20:a:f011::a03e]:54915
 952023-07-06T22:05:44Z [net] Making network specific connection to [2600:1700:170:14c0::35]:8333
 962023-07-06T22:13:21Z [net] Making network specific connection to [2a01:cb1d:84c0:ec00:2:8284:39b5:bee8]:8333
 972023-07-06T22:15:56Z [net] Making network specific connection to [2a01:4f8:1c1c:f22c::1]:8333
 982023-07-06T22:19:22Z [net] Making network specific connection to [2001:4dd0:3564:0:9c1c:cc31:9fe8:5505]:8333
 992023-07-06T22:35:18Z [net] Making network specific connection to [2605:59c8:2062:9210::98e]:8333
1002023-07-06T22:38:43Z [net] Making network specific connection to [2601:983:827f:5850:e5a6:b90a:11b5:e3af]:8333
1012023-07-06T22:39:19Z [net] Making network specific connection to [2400:2413:d641:9900:8596:b918:ea3b:ce3c]:8333
1022023-07-06T22:43:59Z [net] Making network specific connection to [2402:800:9eb0:3f6c:e0c4:ee35:9e2e:ff10]:8333
1032023-07-06T22:44:47Z [net] Making network specific connection to [2001:0:2851:b9f0:4f6:62:8b12:b535]:8333
1042023-07-06T22:49:47Z [net] Making network specific connection to [2a01:4f9:2a:32e6::2]:8333
1052023-07-06T22:59:26Z [net] Making network specific connection to [2804:56c:c148:c700:cf18:6fee:1521:997f]:8333
1062023-07-06T23:03:38Z [net] Making network specific connection to [2405:9800:b960:363a:c067:5ce4:c878:30ac]:8333
1072023-07-06T23:34:05Z [net] Making network specific connection to [2600:1700:1d00:5c60:bc36:2c4d:df89:5c36]:8333
1082023-07-06T23:38:37Z [net] Making network specific connection to [2400:3b00:20:c:bacb:29ff:feab:8886]:8333
1092023-07-06T23:39:01Z [net] Making network specific connection to [2001:5b0:66c0:d888:1c20:581b:5a97:439e]:8333
1102023-07-06T23:41:57Z [net] Making network specific connection to [2600:1700:170:14c0::43]:8333
1112023-07-06T23:45:45Z [net] Making network specific connection to [2001:ee0:4a3e:46b0:d427:da63:566:6080]:8333
1122023-07-06T23:45:53Z [net] Making network specific connection to [2a02:c207:3002:7468::1]:8333
1132023-07-06T23:47:50Z [net] Making network specific connection to [2001:1600:10:100::69d]:8333
1142023-07-06T23:49:26Z [net] Making network specific connection to [2800:300:6f41:7800::3]:8333
1152023-07-06T23:50:03Z [net] Making network specific connection to [2601:983:8280:9f20::4c33]:8333
1162023-07-07T00:00:08Z [net] Making network specific connection to [2600:4040:7be8:ee00:ccce:926:661f:d6b8]:8333
1172023-07-07T00:02:26Z [net] Making network specific connection to [2a02:810d:a200:149c:4d61:c376:892:7e0c]:8333
1182023-07-07T00:07:49Z [net] Making network specific connection to [2a01:4f9:6b:4c2a::2]:8333
1192023-07-07T00:25:14Z [net] Making network specific connection to [2003:c6:7719:c900:93c2:2ac8:e62c:19bc]:8333
1202023-07-07T00:25:30Z [net] Making network specific connection to [2a01:4f8:200:53af::2]:8333
1212023-07-07T00:32:34Z [net] Making network specific connection to [2402:7500:95b:e295:adf0:1280:e14:ff80]:8333
1222023-07-07T00:35:50Z [net] Making network specific connection to [2001:9e8:a7ea:c700:899b:709:f9cb:b15]:8333
1232023-07-07T00:36:49Z [net] Making network specific connection to [2001:8003:2c32:b900:fd15:5cb8:24a1:38df]:8333
1242023-07-07T00:39:59Z [net] Making network specific connection to udvbimwjhkd7j3fwadzefyieppzxkm6abdkwrpo2or4avjhvnvtqk6ad.onion:8333
1252023-07-07T00:44:20Z [net] Making network specific connection to [2003:f0:f718:d700:5f82:5c67:394c:1c1a]:8333
1262023-07-07T00:45:01Z [net] Making network specific connection to hxmwwegj3lrnu2lpmr5rncuolbxmcq3synvpesvn46seq4hivpoyjeyd.onion:8333
1272023-07-07T00:45:43Z [net] Making network specific connection to [2409:8a00:7981:6c00:e0fb:a501:b561:6649]:8333
1282023-07-07T00:51:55Z [net] Making network specific connection to [2a03:f680:fe04:2f6d:1110:151c:433b:5c59]:8333
1292023-07-07T00:55:23Z [net] Making network specific connection to [2603:8000:6d06:c19d:5ed3:bd68:b176:4432]:8333
1302023-07-07T00:59:08Z [net] Making network specific connection to [2600:6c50:6700:f715::1e46]:8333
1312023-07-07T01:24:29Z [net] Making network specific connection to [2a09:bac1:3b60:140::16:179]:8333
1322023-07-07T01:26:42Z [net] Making network specific connection to [2600:1700:62b3:32b0::21]:8333
1332023-07-07T01:44:20Z [net] Making network specific connection to [2603:3001:390a:7dc1:fd7:7457:2c56:8560]:8333
1342023-07-07T01:45:34Z [net] Making network specific connection to [2605:ba00:5107:f13d::c9c]:8333
1352023-07-07T01:50:02Z [net] Making network specific connection to [2607:fb91:152a:80df:2441:841a:0:ee8]:8333
1362023-07-07T01:59:02Z [net] Making network specific connection to [2001:bc8:1201:706:8634:97ff:fe11:bca8]:8333
1372023-07-07T02:05:19Z [net] Making network specific connection to [2601:589:4d7e:60a0:8518:dd87:b870:cdac]:8333
1382023-07-07T02:12:45Z [net] Making network specific connection to [2605:59c8:3045:f210:d0f6:b7d4:664a:6298]:8333
1392023-07-07T02:20:52Z [net] Making network specific connection to [2601:283:4b82:7fc0:e514:7666:4132:dfb4]:8333
1402023-07-07T02:30:30Z [net] Making network specific connection to [2407:7000:9892:1000:b1e3:865b:f4bf:96e6]:8333
1412023-07-07T02:35:32Z [net] Making network specific connection to [2001:0:284a:364:c6e:fb35:4f86:4d5c]:8333
1422023-07-07T02:48:16Z [net] Making network specific connection to [2001:1c00:2004:2000:f93f:417f:40d6:2f98]:8333
1432023-07-07T02:52:39Z [net] Making network specific connection to [2001:67c:289c:4::78]:8333
1442023-07-07T02:55:13Z [net] Making network specific connection to [2603:6000:b000:d666:3aa4:fa57:321:809a]:8333
1452023-07-07T03:04:14Z [net] Making network specific connection to [2607:fb90:9f2a:d9bf:6d4:c4ff:fed2:cfc9]:8333
1462023-07-07T03:10:07Z [net] Making network specific connection to [2001:9e8:4ac4:ec00:4335:6809:33d4:eb4a]:8333
1472023-07-07T03:13:57Z [net] Making network specific connection to [2a03:7846:7514:101:839a:c15e:6cb1:5771]:8333
1482023-07-07T03:17:14Z [net] Making network specific connection to [2a01:4b00:8648:8000:d888:1d8c:1818:213e]:8333
1492023-07-07T03:22:10Z [net] Making network specific connection to [2a00:23c8:7e01:1a01:f534:944b:397d:a9c6]:8333
1502023-07-07T03:24:18Z [net] Making network specific connection to [2001:8a0:f92b:de00:6468:8ba1:51e6:531f]:8333
1512023-07-07T03:28:20Z [net] Making network specific connection to [2a00:23c6:5e05:9401:251e:3e0d:8d3a:ebfe]:8333
1522023-07-07T03:32:17Z [net] Making network specific connection to [2602:ffc8:2:104::18]:8333
1532023-07-07T03:34:08Z [net] Making network specific connection to [2806:103e:22:846f::7]:8333
1542023-07-07T03:41:03Z [net] Making network specific connection to [2001:56a:f9b7:3900:69ad:63e5:fe49:37a9]:8333
1552023-07-07T03:43:36Z [net] Making network specific connection to [2a02:8109:3bbf:f41c:290d:4dc8:9923:23]:8333
1562023-07-07T03:55:28Z [net] Making network specific connection to [2a01:36d:111:6537:3fb8:f46a:8aaf:f509]:8333
1572023-07-07T04:08:36Z [net] Making network specific connection to [2a0d:6fc2:5420:5f00:64f1:d4c8:387a:8acb]:8333
1582023-07-07T04:09:20Z [net] Making network specific connection to [2601:681:8b80:10b0:91a6:3d6a:5903:f8ec]:8333
1592023-07-07T04:18:35Z [net] Making network specific connection to [2001:a61:3aff:6201:8b03:860b:d1fb:3cef]:8333
1602023-07-07T04:19:57Z [net] Making network specific connection to [2601:443:c100:59fc:5499:952a:17c7:2a47]:8333
1612023-07-07T04:54:30Z [net] Making network specific connection to [2a01:298:f7:1:51f9:79cd:7ca6:d8b2]:8333
1622023-07-07T04:55:04Z [net] Making network specific connection to [2600:1700:16d2:beaf::2000]:8333
1632023-07-07T04:55:53Z [net] Making network specific connection to [2a00:23c4:a71e:d301:2018:b0a1:3d8a:4c58]:8333
1642023-07-07T05:03:03Z [net] Making network specific connection to [2600:6c40:5c00:135e:8086:57fb:8445:1976]:8333
1652023-07-07T05:05:13Z [net] Making network specific connection to [2001:8a0:dec8:9a00:b8eb:fb:a4d7:360e]:8333
1662023-07-07T05:07:39Z [net] Making network specific connection to [2a01:e0a:320:39a0:6e9b:2c17:3975:a341]:8333
1672023-07-07T05:14:23Z [net] Making network specific connection to [2605:6400:30:f174::42]:8333
1682023-07-07T05:23:25Z [net] Making network specific connection to [2002:5bc:3e12::5bc:3e12]:8333
1692023-07-07T05:24:02Z [net] Making network specific connection to [2a01:598:9290:9950:90a6:677:13f6:8eb6]:8333
1702023-07-07T05:24:41Z [net] Making network specific connection to [2a02:1210:4ac4:a400:310f:a516:44a9:2de1]:8333
1712023-07-07T05:31:40Z [net] Making network specific connection to [2a0d:3344:19b2:8f10::d95]:8333
1722023-07-07T05:37:58Z [net] Making network specific connection to [2607:fea8:c21e:400::512c]:8333
1732023-07-07T05:39:31Z [net] Making network specific connection to [2a00:23c6:c8af:5601:b4fe:cbae:fce1:be84]:8333
1742023-07-07T05:43:36Z [net] Making network specific connection to [2001:8a0:d91f:6200:9363:daa3:9fd1:78b9]:8333
1752023-07-07T05:44:45Z [net] Making network specific connection to [2600:1001:a100:c26c:2aed:af3b:54a4:12df]:8333
1762023-07-07T05:45:08Z [net] Making network specific connection to [2001:9e8:8e25:5a00:459b:ff39:d304:1e96]:8333
1772023-07-07T05:54:02Z [net] Making network specific connection to [2804:d45:a305:1300:4320:ec0b:df48:2314]:8333
1782023-07-07T05:57:44Z [net] Making network specific connection to [2603:6011:7d00:f41b:20bf:be69:b0d2:36a3]:8333
1792023-07-07T05:58:20Z [net] Making network specific connection to [2003:c9:4f0f:a24f:210d:2795:3488:d8f2]:8333
1802023-07-07T05:58:57Z [net] Making network specific connection to [2003:e9:f0c:bd00:8550:ab03:d4fe:7286]:8333
1812023-07-07T06:07:31Z [net] Making network specific connection to [240e:39c:61:a140:a639:b3ff:fe54:93c4]:8333
1822023-07-07T06:10:17Z [net] Making network specific connection to [2003:cc:738:5800:dd00:81e3:b390:7a40]:8333
1832023-07-07T06:17:38Z [net] Making network specific connection to [2603:6010:f700:da2f::84f]:8333
1842023-07-07T06:20:26Z [net] Making network specific connection to [2603:3004:7e7:b800::fcc8]:8333
1852023-07-07T06:27:06Z [net] Making network specific connection to [2607:fb90:3c21:c041:86e:2e97:6a7b:2691]:8333
1862023-07-07T06:27:44Z [net] Making network specific connection to [2603:300a:21c3:4900:c83b:c1d2:2eee:9558]:8333
1872023-07-07T06:29:17Z [net] Making network specific connection to [2a02:8428:c488:7001:edaa:6bb3:810d:e9e6]:8333
1882023-07-07T06:37:46Z [net] Making network specific connection to [2603:8080:e900:2389:e008:e9c:473c:2253]:8333
1892023-07-07T06:45:32Z [net] Making network specific connection to [2606:9580:100:e:d075:a67e:1f9c:139d]:8333
1902023-07-07T07:01:01Z [net] Making network specific connection to [2a01:112f:42e1:e600:6d72:5128:84ef:f9e6]:8333
1912023-07-07T07:09:54Z [net] Making network specific connection to [2001:18c0:9e4:7400::a8e2]:8333
1922023-07-07T07:10:19Z [net] Making network specific connection to [2a02:9b0:403b:ac61:189e:2c0e:ac5:a]:8333
1932023-07-07T07:14:21Z [net] Making network specific connection to [2600:1014:a020:2187:8edc:d4ff:fe81:59c2]:8333
1942023-07-07T07:14:29Z [net] Making network specific connection to [2804:14c:657d:4030:28b4:eff:fe9b:8894]:8333
1952023-07-07T07:24:23Z [net] Making network specific connection to [2804:4308:99:5f00:153:78b1:769c:169f]:8333
1962023-07-07T07:29:40Z [net] Making network specific connection to [2a02:2f05:6806:a600:64b8:5656:cc2f:59f4]:8333
1972023-07-07T07:29:53Z [net] Making network specific connection to [fec0::6c05:e753:618:4306]:8333
1982023-07-07T07:39:24Z [net] Making network specific connection to [2003:f4:4f17:26f0:7285:c2ff:fe7f:6213]:8333
1992023-07-07T08:01:34Z [net] Making network specific connection to [2603:8000:4100:2994::138f]:8333
2002023-07-07T08:10:53Z [net] Making network specific connection to [2a02:8071:51f0:a9e0:4027:8a2c:1f5b:b414]:8333
2012023-07-07T08:16:11Z [net] Making network specific connection to [2a02:a470:8f22:1:593f:dc27:6917:997]:8333
2022023-07-07T08:20:32Z [net] Making network specific connection to [2602:feb4:1c8:d400:1890:f152:a5b4:241d]:8333
2032023-07-07T08:20:43Z [net] Making network specific connection to [2002:5477:7501:0:38fb:bb60:559b:138c]:8333

amitiuttarwar commented at 10:31 pm on July 7, 2023: contributor

Is there some historical reason why we consider IPV6 to be active, even when it’s not? I mean sure, attempting connections is pretty cheap, but still?

good question! looks like #1021 introduced support of IPV6. there’s some PR convo that indicates it started with defaulting off, but review direction lead to it being on.

maybe we could consider a deeper fix that would allow nodes to detect whether IPV6 is actually connectable or not, and enable / disable whether that network is considered reachable. that would impact the behavior of this feature, but could be considered separately and have a wider impact. on master, nodes that are currently unable to connect to IPV6 are still going to be persisting addresses to addrman & attempting connections periodically.

vasild commented at 3:05 pm on July 10, 2023: contributor

a majority of my network specific connection attempts were to IPV6 addresses, even though I don’t have an IPV6 address and can’t make connections to it

Yeah. That is also an issue on master, without this PR. Maybe it deserves a separate PR. Opened #28061 to track that.

in src/net_processing.cpp:5164 in d0ac2d6602 outdated

5160@@ -5159,6 +5161,9 @@ void PeerManagerImpl::EvictExtraOutboundPeers(std::chrono::seconds now)
5161             if (state == nullptr) return; // shouldn't be possible, but just in case
5162             // Don't evict our protected peers
5163             if (state->m_chain_sync.m_protect) return;
5164+            // If this is the only connection on a particular network that is

mzumsande commented at 3:25 pm on July 26, 2023:

commit d0ac2d6602ab5fd32d8d22b49488f77b33d6f0a9: In the commit message, “(counting IPv4 and IPv6 as one network)” is no longer true

amitiuttarwar commented at 6:26 pm on July 26, 2023:

gah, thanks. will update if I retouch.

amitiuttarwar commented at 6:42 pm on August 3, 2023:

updated in latest push. no longer any references to the bundling in the commit messages

mzumsande commented at 6:13 pm on July 26, 2023: contributor

ACK 1e65aae8068ecf313a7c3b176dfc1326b3b67fbe (with the caveat that I’m coauthor)

I reviewed the commits and tested this on mainnet while supporting multiple networks - everything worked as expected, it would usually be all clearnet connections first, some of which are then slowly replaced by i2p / cjdns / tor so that diversity increases.

in src/net.cpp:1139 in 9b97c34dbe outdated

1134@@ -1135,6 +1135,9 @@ void CConnman::DisconnectNodes()
1135                 // close socket and cleanup
1136                 pnode->CloseSocketDisconnect();
1137 
1138+                // update connection count by network
1139+                if (pnode->IsManualOrFullOutboundConn()) m_network_conn_counts[pnode->addr.GetNetwork()]--;

ajtowns commented at 1:44 am on July 27, 2023:

dev notes says ++i preferred over i++, here and elsewhere

amitiuttarwar commented at 11:40 pm on July 30, 2023:

updated in 4428c6d (#28189)

amitiuttarwar commented at 6:43 pm on August 3, 2023:

updated in latest push. I think I caught them all

in src/net.h:1082 in 9b97c34dbe outdated

1063@@ -1048,6 +1064,9 @@ class CConnman
1064     std::atomic<NodeId> nLastNodeId{0};
1065     unsigned int nPrevNodeCount{0};
1066 
1067+    // Stores number of full-tx connections (outbound and manual) per network
1068+    std::unordered_map<Network, size_t> m_network_conn_counts GUARDED_BY(m_nodes_mutex);

ajtowns commented at 1:45 am on July 27, 2023:

Having a map where a std::array would be fine is kind of lame… That said, this is the same setup as in addrman_impl.h for m_network_counts.

amitiuttarwar commented at 11:45 pm on July 30, 2023:

for anyone following along in review, some historical context of why it was initially changed from std::array to std::unordered_map here: #27213 (review) @ajtowns I agree with your reasoning in #27213 (review) that an array doesn’t tangibly make the code more brittle. since an array has a significantly smaller memory footprint, I think it’s slightly preferable. incorporated in 84495cd (#28189)

vasild commented at 10:32 am on July 31, 2023:

Having a map where a std::array would be fine is kind of lame

Why? Semantically the closest container to what is needed here is a map. Do you mean from performance point of view? I would go with map first and consider an optimization only if it has been demonstrated that the optimization results in faster code or smaller memory footprint.

To be explicit - I would also ack this PR if this is changed to array. I think the map vs array discussion is minor for this PR and shouldn’t block it.

However, if this gets as map in master, taking an extra effort in a separate PR to change it to array would better be backed by some performance or memory measurements to demonstrate that there is an actual reason to change it.

ajtowns commented at 7:21 am on August 2, 2023:

A std::array can be viewed as a specific form of map that uses keys from a range of integers starting at 0 that’s known at compile time, with either few or no missing elements. Where that’s the case, an array is more efficient (smaller storage, no dynamic allocation needed, faster lookups, better caching behaviour etc) and more convenient (eg, x = a[3] works even if a is a const& which isn’t true for a map, requiring a [much more convoluted approach]https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1275648306) instead).

The “don’t optimise unless it’s necessary” philosophy doesn’t mean “write slow code until you find out faster code is needed”, it means “write simple code until you find out faster code is needed”, but an array is simpler than a map in the first place. Using something a bit more complicated in its place, when that’s both not useful and slower isn’t the end of the world, but it’s still kind of lame.

amitiuttarwar commented at 6:51 pm on August 3, 2023:

updated to an std:array in the latest push

in src/net.cpp:1616 in d0ac2d6602 outdated

1606@@ -1607,6 +1607,12 @@ std::unordered_set<Network> CConnman::GetReachableEmptyNetworks() const
1607     return networks;
1608 }
1609 
1610+bool CConnman::MultipleManualOrFullOutboundConns(Network net)
1611+{
1612+    LOCK(m_nodes_mutex);
1613+    return m_network_conn_counts[net] > 1;

ajtowns commented at 1:56 am on July 27, 2023:

Could be a const member function, if you weren’t using map::operator[]. That would mean either switching to an array, where you can use [] for just a lookup, or doing a manual find:

0if (auto it = m_network_conn_counts.find(net) && it != m_network_conn_counts.end()) {
1    return it->second > 0;
2}
3return false;

amitiuttarwar commented at 11:46 pm on July 30, 2023:

updated in 84495cd (#28189) by turning m_network_counts to an array

amitiuttarwar commented at 6:51 pm on August 3, 2023:

incorporated here in latest push

in src/net.cpp:1840 in 1e65aae806 outdated

1835+            // and then protecting "only" peers from a network during outbound eviction.
1836+            // This is not attempted if the user changed -maxconnections to a value
1837+            // so low that less than MAX_OUTBOUND_FULL_RELAY_CONNECTIONS are made,
1838+            // to prevent interactions with otherwise protected outbound peers.
1839+            next_extra_network_peer = GetExponentialRand(now, EXTRA_NETWORK_PEER_INTERVAL);
1840+            network_peer = true;

ajtowns commented at 2:04 am on July 27, 2023:

Isn’t network_peer == true the same as preferred_net != std::nullopt ? Could drop the bool.

amitiuttarwar commented at 11:46 pm on July 30, 2023:

done in 006b8dd (#28189)

amitiuttarwar commented at 6:52 pm on August 3, 2023:

done in latest push

in src/net.cpp:1945 in 1e65aae806 outdated

1940@@ -1904,6 +1941,9 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect)
1941                 }
1942                 LogPrint(BCLog::NET, "Making feeler connection to %s\n", addrConnect.ToStringAddrPort());
1943             }
1944+
1945+            if (network_peer) LogPrint(BCLog::NET, "Making network specific connection to %s\n", addrConnect.ToStringAddrPort());

ajtowns commented at 2:05 am on July 27, 2023:

Maybe report the preferred_net name here as well?

amitiuttarwar commented at 8:46 pm on July 30, 2023:

I considered that, but wouldn’t it be pretty self-evident from the address format? happy to add if it still feels useful.

vasild commented at 10:07 am on July 31, 2023:

CJDNS vs IPv6 difference may not be very obvious to some people.

amitiuttarwar commented at 6:55 pm on August 3, 2023:

added the network to print statement

ajtowns commented at 2:09 am on July 27, 2023: contributor

utACK 1e65aae8068ecf313a7c3b176dfc1326b3b67fbe

Various nits only. I think adding some redundant net-category debug logs about trying to make ipv6 connections even on systems with no ipv6 connectivity is fine given it’s at about the same rate as there’s log entries for the extra block relay only connections anyway.

in src/net.cpp:1622 in 1e65aae806 outdated

1615@@ -1613,6 +1616,22 @@ bool CConnman::MultipleManualOrFullOutboundConns(Network net)
1616     return m_network_conn_counts[net] > 1;
1617 }
1618 
1619+bool CConnman::MaybePickPreferredNetwork(std::optional<Network>& network)
1620+{
1621+    std::array<Network, 5> nets{NET_IPV4, NET_IPV6, NET_ONION, NET_I2P, NET_CJDNS};
1622+    Shuffle(nets.begin(), nets.end(), FastRandomContext());

naumenkogs commented at 10:24 am on July 27, 2023:

nit: may be worth explaining that this randomization helps to get to NET_I2P option without traversing the entire NET_ONION set of potentially rubbish or buggy stuff :)

amitiuttarwar commented at 4:03 pm on July 30, 2023:

I don’t quite follow. if we have multiple networks that match, the randomization seems valuable in all the cases?

naumenkogs commented at 9:05 am on July 31, 2023:

I was pointing out a specific (probably stronger-than-rest) benefit of randomization. So that if one wants to drop the randomization from the code, they consider that.

Feel free to ignore it.

in src/net.cpp:1893 in 1e65aae806 outdated

1890@@ -1857,7 +1891,10 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect)
1891                 }
1892             } else {
1893                 // Not a feeler
1894-                std::tie(addr, addr_last_try) = addrman.Select();
1895+                // If preferred_net has a value set, pick an extra outbound
1896+                // peer from that network. The eviction logic in net_processing

naumenkogs commented at 10:28 am on July 27, 2023:

Do we have any risk of TOR and I2P peers bouncing each other forever? Say, you have 7 ip4 peers and 1 Tor peer. I2P bounces Tor. In the next step, Tor (maybe another node, or maybe the same) bounces I2P node back.

ajtowns commented at 11:58 am on July 27, 2023:

This example should be prevented by the “Protect extra full outbound peers by network” commit, shouldn’t it? ie the process would be:

7 ipv4 + 1 tor
7 ipv4 + 1 tor + 1 i2p (via “network specific connection”)
6 ipv4 + 1 tor + 1 i2p (tor and i2p nodes protected from eviction due to if (!m_connman.MultipleManualOrFullOutboundConns(pnode->addr.GetNetwork())) return; code path)

jonatack commented at 0:51 am on August 12, 2023:

Do we have any risk of TOR and I2P peers bouncing each other forever?

I added logging and have been testing this, and haven’t seen that occur thanks to the eviction protection as mentioned by AJ.

However, the observation and testing did find addnode entries being connected to as full outbound peers by the new logic and then maintained (protected) in that incorrect role. Addnode peers are important here, as they are the way that nodes have been able to ensure connections to the various networks, particularly ones with fewer peers in addrman or on the network.

To avoid using our limited outbound slots for addnode peers, and to ensure that the peers a user selects to be addnode benefit from their intended privileges (non-eviction, not disconnected or discouraged for bad behavior, etc.), it may be worth preventing that.

Finding this uncovered that addnode CJDNS peers are currently not detected as connected in CConnman::GetAddedNodeInfo (i.e. fConnected is always false), so:

CConnman::ThreadOpenAddedConnections() continually retries to connect to them about once a minute
RPC getaddednodeinfo incorrectly shows them as not connected

I’ve attempted to address these issues in #28248, along with adding the logging that permitted seeing them.

Edit: also opened #28895 that is extracted from #28248.

mzumsande commented at 2:44 pm on July 27, 2023: contributor

Maybe a release note would be nice? Not because this would break anything, but so that users who are currently performing active measures to stay connected to multiple networks (e.g. -addnode together with regular monitoring if the added node is still online) know that there is another option now.

amitiuttarwar commented at 11:39 pm on July 30, 2023: contributor

thank you for the reviews everyone 🙌

In terms of next steps, we could either keep this PR as is in aims of merging soon & address follow-ups separately, or incorporate the review comments here. I’ve recapped the current status of the PR and am curious to hear feedback as to what route seems preferable.

the current tip on this PR has 3 review ACKs (vasild, mzumsande with co-author caveat, ajtowns) and 1 lightly tested approach ACK (willcl-ark)
the outstanding review comments are relatively small & mostly can be addressed in a follow up. I opened #28189 to address reviewer suggestions of style, reverting m_network_conn_counts to an array & adding a release note
there are two review comments relating to outdated commit messages that can’t be addressed in a follow up: 1, 2

since this PR has been open for a while & the remaining suggestions are relatively small, my preference is to continue trying to get enough review here for confidence to merge, then address the open improvements separately. that said, if maintainers or reviewers prefer otherwise, I am happy to incorporate those changes here.

amitiuttarwar commented at 11:47 pm on July 30, 2023: contributor

@mzumsande - release note added in a6d270d (#28189)

naumenkogs commented at 9:15 am on July 31, 2023: member

ACK 1e65aae8068ecf313a7c3b176dfc1326b3b67fbe

vasild commented at 10:16 am on July 31, 2023: contributor

I would be happy to re-ack if followups are incorporated here. That would also keep git history cleaner. Can’t amend commit messages in a followup, it will remain as “IPv4 and IPv6 as one network” forever in git history, could confuse the future generations (or me after a few months have passed and I have forgotten everything :disappointed:).

in src/net.cpp:1615 in 1e65aae806 outdated

1609@@ -1604,6 +1610,28 @@ std::unordered_set<Network> CConnman::GetReachableEmptyNetworks() const
1610     return networks;
1611 }
1612 
1613+bool CConnman::MultipleManualOrFullOutboundConns(Network net)
1614+{
1615+    LOCK(m_nodes_mutex);

willcl-ark commented at 3:07 pm on August 1, 2023:

0    AssertLockHeld(m_nodes_mutex);

We lock m_nodes_mutex here, but this is only called from inside ForEachNode, which itself has already locked the same mutex. It’s no problem because m-Nodes_mutex is Recursive, but as there is a passive effort to replace Recursive Mutexes, if you do end up touching this again perhaps you could assert the lock is held at runtime with AssertLockHeld and decorate header declaration with EXCLUSIVE_LOCKS_REQUIRED for compile time analysis, instead?

vasild commented at 3:42 pm on August 1, 2023:

:+1:

amitiuttarwar commented at 7:12 pm on August 3, 2023:

good catch! totally agree with the suggestion - def agree that we shouldn’t introduce more uses of recursive mutexes, esp when not necessary.

that said, I’m struggling a bit to communicate with the compiler-

when I add both the runtime AssertLockHeld and annotation of EXCLUSIVE_LOCKS_REQUIRED, I’m getting a compile time warning from -Wthread-safety-analysis

 0diff --git a/src/net.cpp b/src/net.cpp
 1index a504289fa5..228d081aa6 100644
 2--- a/src/net.cpp
 3+++ b/src/net.cpp
 4@@ -1612,7 +1612,7 @@ std::unordered_set<Network> CConnman::GetReachableEmptyNetworks() const
 5
 6 bool CConnman::MultipleManualOrFullOutboundConns(Network net) const
 7 {
 8-    LOCK(m_nodes_mutex);
 9+    AssertLockHeld(m_nodes_mutex);
10     return m_network_conn_counts[net] > 1;
11 }
12
13diff --git a/src/net.h b/src/net.h
14index 55f5631d43..0a3de19281 100644
15--- a/src/net.h
16+++ b/src/net.h
17@@ -909,7 +909,7 @@ public:
18     /** Return true if we should disconnect the peer for failing an inactivity check. */
19     bool ShouldRunInactivityChecks(const CNode& node, std::chrono::seconds now) const;
20
21-    bool MultipleManualOrFullOutboundConns(Network net) const;
22+    bool MultipleManualOrFullOutboundConns(Network net) const EXCLUSIVE_LOCKS_REQUIRED(m_nodes_mutex);

0net_processing.cpp:5166:28: warning: calling function 'MultipleManualOrFullOutboundConns' requires holding mutex 'm_connman.m_nodes_mutex' exclusively [-Wthread-safety-analysis]
1            if (!m_connman.MultipleManualOrFullOutboundConns(pnode->addr.GetNetwork())) return;

and if I remove EXCLUSIVE_LOCKS_REQUIRED from the declaration, it complains about AssertLockHeld and m_network_conn_counts

0net.cpp:1615:5: warning: calling function 'AssertLockHeldInternal<AnnotatedMixin<std::recursive_mutex>>' requires holding mutex 'm_nodes_mutex' exclusively [-Wthread-safety-analysis]
1    AssertLockHeld(m_nodes_mutex);
2    ^
3./sync.h:143:28: note: expanded from macro 'AssertLockHeld'
4#define AssertLockHeld(cs) AssertLockHeldInternal(#cs, __FILE__, __LINE__, &cs)
5                           ^
6net.cpp:1616:12: warning: reading variable 'm_network_conn_counts' requires holding mutex 'm_nodes_mutex' [-Wthread-safety-analysis]
7    return m_network_conn_counts[net] > 1;
8           ^

I think this is an issue because ForEachNode is called in a lamdba which gets evaluated at runtime? I’m not sure how I can work around this, open to any suggestions :)

ajtowns commented at 8:24 pm on August 3, 2023:

The ForEachNode lambda is already annotated with EXCLUSIVE_LOCKS_REQUIRED(::cs_main) so all that ’s needed is to add m_connman.m_nodes_mutex to that. That doesn’t work directly, because m_nodes_mutex is private and the ForEachNode is in a different class, but you can work around that by declaring:

0public:
1    // alias for thread safety annotations only, not defined.
2    RecursiveMutex& GetNodesMutex() const LOCK_RETURNED(m_nodes_mutex);

in CConnman, and writing:

0        m_connman.ForEachNode([&](CNode* pnode) EXCLUSIVE_LOCKS_REQUIRED(::cs_main, m_connman.GetNodesMutex()) {

amitiuttarwar commented at 1:13 am on August 4, 2023:

oh interesting. do I understand this correctly: the LOCK_RETURNED annotation tells thread safety analysis that this function will return m_nodes_mutex, even though it’s just an empty function, which lets the lambda use EXCLUSIVE_LOCKS_REQUIRED. this works because it doesn’t need to actually return the mutex there, it just needs to promise the compiler it will be there, which is handled elsewhere.

amitiuttarwar commented at 3:20 am on August 4, 2023:

hm, thinking about this more… I believe this workaround diminishes some of the value of the compile-time notation. for example, if ForEachNode no longer acquired m_nodes_mutex, then the compiler wouldn’t be able to flag the issue, but the AssertLockHeld would catch it at runtime.

the advantage is that this one weak check allows to maintain the compiler checks at the other levels (of functions and members that are protected), and we remove the recursive locking of the mutex.

this seems reasonable to me, so I’ll incorporate.

ajtowns commented at 5:41 am on August 4, 2023:

Because ForEachNode takes a std::function, the thread safety annotations on the lambda are dropped, so the compiler can’t verify that ForEachNode has actually taken all the locks that the lambda expects to have held. Problem is that doing it any other way (eg templates) requires ForEachNode to be annotated to require any extra locks that the lambda requires that were already held by the caller.

willcl-ark approved

willcl-ark commented at 3:22 pm on August 1, 2023: member

ACK 1e65aae806

Left one more observation since last time related to the mutex locking. I’m not sure how important the Recusive Mutex-removal project is in general and whether it’s worth changing though.

I would also be happy to re-review if the nits were addressed in here FWIW :)

fanquake commented at 3:25 pm on August 1, 2023: member

Going to suggest closing #28189 and just dealing with the final changes here. We can’t change commit messages after the fact, so if that’s a concern for some, we should correct them. I think with range-diff, and two reviewers already commited to re-ACKing, we can integrate the final changes and get this merged without issue.

amitiuttarwar force-pushed on Aug 3, 2023

p2p: Introduce data struct to track connection counts by network

Connman uses this new map to keep a count of active OUTBOUND_FULL_RELAY and
MANUAL connections. Unused until next commit.

Co-authored-by: Martin Zumsande <mzumsande@gmail.com>

654d9bc276

amitiuttarwar force-pushed on Aug 3, 2023

amitiuttarwar commented at 7:15 pm on August 3, 2023: contributor

thanks for the reviews & feedback @naumenkogs, @vasild, @willcl-ark & @fanquake

I’ve updated this PR to incorporate all the open feedback & fix up the commit messages. everything should be addressed except for ~these two~ one open item: ~1. question about locking improvement #27213 (review)~ (updated using suggestion from #27213 (review)) 2. release note which is kept in the follow-up, to prevent possible review churn on language feedback

should be ready for re-review. thanks!

p2p: Protect extra full outbound peers by network

If a peer is the only one of its network, protect it from eviction.
This improves the diversity of outbound connections with respect to
reachable networks.

Co-authored-by: Amiti Uttarwar <amiti@uttarwar.org>

034f61f83b

test: Add test for outbound protection by network

Co-authored-by: Amiti Uttarwar <amiti@uttarwar.org>

65cff00cee

p2p: network-specific management of outbound connections

Diversify outbound connections with respect to
networks: Every ~5 minutes, try to add an extra connection
to a reachable network which we currently don't have a connection to.
This is done defensively - only try management with respect to networks
after all existing outbound slots are filled.
The resulting situation with an extra outbound peer will be handled
by the extra outbound eviction logic, which protects peers from
eviction if they are the only ones for their network.

Co-authored-by: Amiti Uttarwar <amiti@uttarwar.org>

1b52d16d07

amitiuttarwar force-pushed on Aug 4, 2023

vasild approved

vasild commented at 7:45 am on August 4, 2023: contributor

ACK 1b52d16d07be3b5d968157913f04d9cd1e2d3678

DrahtBot requested review from ajtowns on Aug 4, 2023

DrahtBot requested review from mzumsande on Aug 4, 2023

DrahtBot requested review from naumenkogs on Aug 4, 2023

DrahtBot requested review from willcl-ark on Aug 4, 2023

willcl-ark commented at 8:33 am on August 4, 2023: member

ACK 1e65aae806

Updates shown by git range-diff 1e65aae806...1b52d16 look correct to me and glad to get the nits in this initial pull.

naumenkogs commented at 8:42 am on August 4, 2023: member

ACK 1b52d16d07be3b5d968157913f04d9cd1e2d3678

DrahtBot removed review request from naumenkogs on Aug 4, 2023

bitcoin deleted a comment on Aug 6, 2023

fanquake commented at 4:44 pm on August 6, 2023: member

@willcl-ark note that you ACK’d the wrong commit here.

fanquake merged this on Aug 6, 2023

fanquake closed this on Aug 6, 2023

fanquake referenced this in commit ef3f9f389f on Aug 9, 2023

sidhujag referenced this in commit d30b47dc61 on Aug 9, 2023

sidhujag referenced this in commit 78f4ce9dda on Aug 9, 2023

in src/net.cpp:1943 in 1b52d16d07

1938@@ -1895,6 +1939,9 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect)
1939                 }
1940                 LogPrint(BCLog::NET, "Making feeler connection to %s\n", addrConnect.ToStringAddrPort());
1941             }
1942+
1943+            if (preferred_net != std::nullopt) LogPrint(BCLog::NET, "Making network specific connection to %s on %s.\n", addrConnect.ToStringAddrPort(), GetNetworkName(preferred_net.value()));

jonatack commented at 8:02 pm on August 9, 2023:

Suggestions for this log entry:

use LogPrintLevel
replace “Making” with “Trying to make” (like the anchor connection logging), as connections to privacy networks regularly fail
add missing hyphen to “network specific” and log the network just after it, instead of at the end where it is less useful (and a bit less clear, e.g. “on onion”)
log the IP address only if the -logips config option is set
add the connection type

0-
1-            if (preferred_net != std::nullopt) LogPrint(BCLog::NET, "Making network specific connection to %s on %s.\n", addrConnect.ToStringAddrPort(), GetNetworkName(preferred_net.value()));
2+            if (preferred_net.has_value()) {
3+                LogPrintLevel(BCLog::NET, BCLog::Level::Debug, "Trying to make a network-specific %s connection %s(%s)\n",
4+                              GetNetworkName(preferred_net.value()),
5+                              fLogIPs ? strprintf("to %s ", addrConnect.ToStringAddrPort()) : "",
6+                              ConnectionTypeAsString(ConnectionType::OUTBOUND_FULL_RELAY));
7+            }

Example output:

[net:debug] Trying to make a network-specific cjdns connection to [fc70:de9d:7fe2:b32:5828:1a3c:d0f:83ec]:8333 (outbound-full-relay)

in src/net_processing.cpp:5166 in 1b52d16d07

5160@@ -5159,6 +5161,9 @@ void PeerManagerImpl::EvictExtraOutboundPeers(std::chrono::seconds now)
5161             if (state == nullptr) return; // shouldn't be possible, but just in case
5162             // Don't evict our protected peers
5163             if (state->m_chain_sync.m_protect) return;
5164+            // If this is the only connection on a particular network that is
5165+            // OUTBOUND_FULL_RELAY or MANUAL, protect it.
5166+            if (!m_connman.MultipleManualOrFullOutboundConns(pnode->addr.GetNetwork())) return;

jonatack commented at 8:03 pm on August 9, 2023:

We log the disconnection and protection actions in EvictExtraOutboundPeers() and it would be good to log this action as well.

While testing and monitoring the changes in this pull, I found the following diff to be helpful.

0-            if (!m_connman.MultipleManualOrFullOutboundConns(pnode->addr.GetNetwork())) return;
1+            if (!m_connman.MultipleManualOrFullOutboundConns(pnode->addr.GetNetwork())) {
2+                LogPrintLevel(BCLog::NET, BCLog::Level::Debug,
3+                              "Protecting from eviction last remaining outbound %s peer=%d that relays transactions%s (%s)\n",
4+                              GetNetworkName(pnode->addr.GetNetwork()), pnode->GetId(),
5+                              fLogIPs ? strprintf(": %s", pnode->addr.ToStringAddrPort()) : "",
6+                              pnode->ConnectionTypeAsString());
7+                return;
8+            }

02023-08-09T18:52:42.433930Z [net:debug] Trying to make a network-specific cjdns connection to [fc70:de9d:7fe2:b32:5828:1a3c:d0f:83ec]:8333 (outbound-full-relay)
12023-08-09T18:53:16.324002Z [net:debug] Protecting from eviction last remaining outbound cjdns peer=26 that relays transactions: [fc70:de9d:7fe2:b32:5828:1a3c:d0f:83ec]:8333 (outbound-full-relay)

in src/net.h:482 in 1b52d16d07

477+        case ConnectionType::MANUAL:
478+                return true;
479+        } // no default case, so the compiler can warn about missing cases
480+
481+        assert(false);
482+    }

jonatack commented at 8:11 pm on August 9, 2023:

This can more simply use the existing helpers.

0bool IsManualOrFullOutboundConn() const { return IsFullOutboundConn() || IsManualConn(); }

in src/net.cpp:1828 in 1b52d16d07

1824@@ -1795,6 +1825,17 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect)
1825             next_feeler = GetExponentialRand(now, FEELER_INTERVAL);
1826             conn_type = ConnectionType::FEELER;
1827             fFeeler = true;
1828+        } else if (nOutboundFullRelay == m_max_outbound_full_relay &&

jonatack commented at 8:28 pm on August 9, 2023:

The first line of this conditional seems redundant, as it is already checked a few lines earlier for the OUTBOUND_FULL_RELAY case.

jonatack commented at 8:43 pm on August 9, 2023: member

Post-merge ACK.

The last commit, p2p: network-specific management of outbound connections, looks like it could use test coverage.

I’ve been testing the changes here these past two days, with added/improved logging to monitor the new behavior.

bitcoin locked this on Aug 11, 2024

p2p: Diversify automatic outbound connections with respect to networks #27213

Reviews

Conflicts

IBD

Peers protected from eviction

Network peer protection

RE: logic to initially overshoot number of connections

RE: ipv4 vs ipv6

Future improvements

IPv4 & IPv6 treatment