P2P: Change ClientPool to PeerSet
#337
Conversation
github-actions
bot
added
A-p2p
| pub use client_wrappers::ClientDropGuard; | ||
| use client_wrappers::StoredClient; | ||
|
|
||
| /// A request to the peer-set. |
There was a problem hiding this comment.
| /// A request to the peer-set. | |
| /// A request to the [`PeerSet`]. |
There was a problem hiding this comment.
Nvm nvm, it's defined in the same file. Same for PeerSetResponse btw.
There was a problem hiding this comment.
It's in scope but it's private
| /// Peers with more cumulative difficulty than the given cumulative difficulty. | ||
| /// | ||
| /// Returned peers will be remembered and won't be returned from subsequent calls until the guard is dropped. | ||
| PeersWithMorePoW(u128), | ||
| /// A random outbound peer. | ||
| /// | ||
| /// The returned peer will be remembered and won't be returned from subsequent calls until the guard is dropped. |
There was a problem hiding this comment.
I think the returned ... docs could be moved to the response variants.
There was a problem hiding this comment.
Added them on both to make sure these docs are seen.
There was a problem hiding this comment.
Okay but these are now duplicated and can get out of sync.
Reasoning for comment on the response is that the return type is next to it. Other crates are like this, consensus, address-book, blockchain, txpool, etc.
| config | ||
| .max_inbound_connections | ||
| .checked_add(config.outbound_connections) | ||
| .unwrap(), |
There was a problem hiding this comment.
This is small but I think overflows in these types of cases usually saturate in most programs instead of crashing for UX reasons (better to saturate on already unrealistic and irrelevantly large number over crashing on said irrelevant number).
User input causing overflow is unrealistic yet possible if someone does this in the CLI/config:
max_inbound_connections = 99999999999999999999
which I could imagine at least a few people doing. I think cuprated continuing with MAX over crashing would be expected, or... monerod catches large input during CLI/config in stuff like --out-peers, --limit-rate, etc.
$ ./monerod --out-peers 18446744073709551615
Failed to parse arguments: the argument ('18446744073709551615') for option '--out-peers' is invalid
If we're either saturating or early returning during CLI, these unwraps can be removed.
| /// Peers with more cumulative difficulty than the given cumulative difficulty. | ||
| /// | ||
| /// Returned peers will be remembered and won't be returned from subsequent calls until the guard is dropped. | ||
| PeersWithMorePoW(u128), | ||
| /// A random outbound peer. | ||
| /// | ||
| /// The returned peer will be remembered and won't be returned from subsequent calls until the guard is dropped. |
There was a problem hiding this comment.
Okay but these are now duplicated and can get out of sync.
Reasoning for comment on the response is that the return type is next to it. Other crates are like this, consensus, address-book, blockchain, txpool, etc.
What
Changes the
ClientPool, aDashMapof unused connected peers to thePeerSet, a service containing all connected peers.Why
We need access to all connected peers for certain RPC requests + it's just clearer to have all connected peers in one place.
How
A new type
WeakClientwas created so we can still give out handles from thePeerSet