feat: limit wallet base node peer outbound connections #6307
Conversation
Test Results (CI) 3 files 113 suites 36m 3s ⏱️ For more details on these failures, see this check. Results for commit 9bdb87a. ♻️ This comment has been updated with latest results. |
ghpbot-tari-project
added
P-acks_required
hansieodendaal
force-pushed
the
ho_limit_wallet_peer_connections
branch
2 times, most recently
from
b60fd69
to
60debf2
Compare
hansieodendaal
force-pushed
the
ho_limit_wallet_peer_connections
branch
3 times, most recently
from
809ac8c
to
fcf0fd8
Compare
hansieodendaal
force-pushed
the
ho_limit_wallet_peer_connections
branch
from
fcf0fd8
to
7a599d6
Compare
hansieodendaal
force-pushed
the
ho_limit_wallet_peer_connections
branch
from
7a599d6
to
39604cc
Compare
hansieodendaal
force-pushed
the
ho_limit_wallet_peer_connections
branch
4 times, most recently
from
13c7cc6
to
de151bf
Compare
hansieodendaal
force-pushed
the
ho_limit_wallet_peer_connections
branch
from
de151bf
to
a1059b7
Compare
Added functionality to limit the number of base node peer connections that a wallet can have, based on a config setting. The furtherest nodes will be disconnected, but nodes on the allow list (e.g. connected base node) will be ignored.
hansieodendaal
force-pushed
the
ho_limit_wallet_peer_connections
branch
from
a1059b7
to
0b94bcb
Compare
There was a problem hiding this comment.
Just need to remove assert!s (should never actually panic anyway) otherwise utACK
I think disconnect reasons would be a great addition, Minimize has a less clear intention. From what I understand "Connection minimisation mode" is like a hard limit on connections (inbound or outbound) to nodes which should only be enabled for the wallet. This seems like it addresses a symptom of a bug that causes a higher amount of connections than desired (should be neighbour pool size + random pool size + a few extra nodes that have connected to you in the base node case N/A for wallet). This method will lead to a number of connects and then disconnects (question: Does this settle down after a while?).
Ideally, we should address this bug so that the connections are not made to begin with, or non-neighbours are disconnected after network discovery. But this does address an issue directly so happy for this to go in (not tested). Well done!
| #[derive(Debug, Clone, Copy, Eq, PartialEq)] | ||
| pub enum Minimized { | ||
| Yes, | ||
| No, | ||
| } |
There was a problem hiding this comment.
It appears that this is only used to improve logs.
Not necessary for now but I'd suggest making this a general reason enum (you can use thiserror for the human readable text or impl Display)
| #[derive(Debug, Clone, Copy, Eq, PartialEq)] | |
| pub enum Minimized { | |
| Yes, | |
| No, | |
| } | |
| #[derive(Debug, Clone, Copy, thiserror::Error)] | |
| pub enum DisconnectReason { | |
| #[error("Remote disconnected")] | |
| RemoteDisconnected, | |
| #[error("Connection was culled because it was inactive")] | |
| ConnectionCulledInactive, | |
| #[error("Connection was culled to reduce number of connections")] | |
| ConnectionCulledMinimise | |
| #[error("Local node explicit disconnect")] | |
| LocalDisconnected | |
| } |
There was a problem hiding this comment.
Issue created for this: #6331
ghpbot-tari-project
removed
P-reviews_required
ghpbot-tari-project
added
the
P-reviews_required
Yes, with these changes we see a couple of connects and disconnects, but all race conditions have been taken care of, especially between the dht service, saf service and connectivity service. When the communication node peer quota have been reached, each new outbound connection due to network discovery, dht or user selection will be weighed up against the actual number of connections and distances. For saf, requesting saf messages are only allowed for the peer quota if the new connection is an actual neighbour. When I worked on this PR, and with the many system-level tests I have been doing, I thought some of the things implemented here should be the normal way for base nodes as well. As you observed, currently the base nodes will just connect to every other base node that is out there. |
ghpbot-tari-project
removed
the
P-reviews_required
Description
Added functionality to limit the number of base node peer connections that a wallet can have, based on a config setting. The furtherest nodes will be disconnected, but nodes on the allow list (e.g. connected base node) will be ignored. Request to a newly connected base node for SAF messages will also be limited to the closest connections only.
Motivation and Context
Wallets do not need to be as aggressive in establishing and keeping many connections compared to a base node.
How Has This Been Tested?
System-level testing on
nextnetSome results for a fresh wallet are below with using these settings:
In contrast to the wallet, these results for a fresh base node during the same period:
What process can a PR reviewer use to test or verify this change?
nextnetBreaking Changes