Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(swarm): improve PeerAddresses configurability #5574

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ libp2p-dcutr = { version = "0.12.0", path = "protocols/dcutr" }
libp2p-dns = { version = "0.42.0", path = "transports/dns" }
libp2p-floodsub = { version = "0.45.0", path = "protocols/floodsub" }
libp2p-gossipsub = { version = "0.47.0", path = "protocols/gossipsub" }
libp2p-identify = { version = "0.45.0", path = "protocols/identify" }
libp2p-identify = { version = "0.46.0", path = "protocols/identify" }
libp2p-identity = { version = "0.2.9" }
libp2p-kad = { version = "0.47.0", path = "protocols/kad" }
libp2p-mdns = { version = "0.46.0", path = "protocols/mdns" }
Expand All @@ -102,7 +102,7 @@ libp2p-rendezvous = { version = "0.15.0", path = "protocols/rendezvous" }
libp2p-request-response = { version = "0.27.0", path = "protocols/request-response" }
libp2p-server = { version = "0.12.7", path = "misc/server" }
libp2p-stream = { version = "0.2.0-alpha", path = "protocols/stream" }
libp2p-swarm = { version = "0.45.1", path = "swarm" }
libp2p-swarm = { version = "0.45.2", path = "swarm" }
libp2p-swarm-derive = { version = "=0.35.0", path = "swarm-derive" } # `libp2p-swarm-derive` may not be compatible with different `libp2p-swarm` non-breaking releases. E.g. `libp2p-swarm` might introduce a new enum variant `FromSwarm` (which is `#[non-exhaustive]`) in a non-breaking release. Older versions of `libp2p-swarm-derive` would not forward this enum variant within the `NetworkBehaviour` hierarchy. Thus the version pinning is required.
libp2p-swarm-test = { version = "0.4.0", path = "swarm-test" }
libp2p-tcp = { version = "0.42.0", path = "transports/tcp" }
Expand Down
5 changes: 5 additions & 0 deletions protocols/identify/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
## 0.46.0

- Update to the new `PeerAddresses` API with `PeerAddressesConfig`, changing `with_cache_size` to `with_cache_config`.
See [PR 5574](https://github.com/libp2p/rust-libp2p/pull/5574).

## 0.45.0

- Address translation is moved here from `libp2p-core`.
Expand Down
2 changes: 1 addition & 1 deletion protocols/identify/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ name = "libp2p-identify"
edition = "2021"
rust-version = { workspace = true }
description = "Nodes identification protocol for libp2p"
version = "0.45.0"
version = "0.46.0"
authors = ["Parity Technologies <[email protected]>"]
license = "MIT"
repository = "https://github.com/libp2p/rust-libp2p"
Expand Down
35 changes: 13 additions & 22 deletions protocols/identify/src/behaviour.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,12 @@ use libp2p_identity::PublicKey;
use libp2p_swarm::behaviour::{ConnectionClosed, ConnectionEstablished, DialFailure, FromSwarm};
use libp2p_swarm::{
ConnectionDenied, DialError, ExternalAddresses, ListenAddresses, NetworkBehaviour,
NotifyHandler, PeerAddresses, StreamUpgradeError, THandlerInEvent, ToSwarm,
_address_translation,
NotifyHandler, PeerAddresses, PeerAddressesConfig, StreamUpgradeError, THandlerInEvent,
ToSwarm, _address_translation,
};
use libp2p_swarm::{ConnectionId, THandler, THandlerOutEvent};

use std::collections::hash_map::Entry;
use std::num::NonZeroUsize;
use std::{
collections::{HashMap, HashSet, VecDeque},
task::Context,
Expand Down Expand Up @@ -142,11 +141,8 @@ pub struct Config {
/// Disabled by default.
pub push_listen_addr_updates: bool,

/// How many entries of discovered peers to keep before we discard
/// the least-recently used one.
///
/// Disabled by default.
pub cache_size: usize,
/// Configuration for the LRU cache of discovered peers.
pub cache_config: Option<PeerAddressesConfig>,
}

impl Config {
Expand All @@ -159,7 +155,7 @@ impl Config {
local_public_key,
interval: Duration::from_secs(5 * 60),
push_listen_addr_updates: false,
cache_size: 100,
cache_config: Some(Default::default()),
}
}

Expand All @@ -184,20 +180,19 @@ impl Config {
self
}

/// Configures the size of the LRU cache, caching addresses of discovered peers.
pub fn with_cache_size(mut self, cache_size: usize) -> Self {
self.cache_size = cache_size;
/// Configuration for the LRU cache responsible for caching addresses of discovered peers.
///
/// If set to [`None`], caching is disabled.
pub fn with_cache_config(mut self, cache_config: Option<PeerAddressesConfig>) -> Self {
self.cache_config = cache_config;
Copy link
Member

Choose a reason for hiding this comment

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

We should probably keep with_cache_size, deprecating it for with_cache_config and just have use that internally so we could probably introduce this as a patch update. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

CC @jxs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If this is possible, I'm definitely doing that. I did not think of that thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's done. I'll let you resolve this if you're ok with it. 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. It was slightly more complicated than expected because since every attributes of identify::Config are public, I cannot remove cache_size if we want to stay in 0.45.1 and not upgrade to 0.46.0.

Just a thought, but I checked the other "Config" object across the project and some do expose the attributes but most of them don't. Maybe we should homogenize that and make every attributes private to not have this kind of problem later on ? It would be a pain to make many new major updates but it would once and for all. If you think there is something here, I will happily open an issue to discuss this mater more.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dariusc93 I'm closing this conversation. Feel free to re-open it if you have other suggestions about it.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with François on this one, can we just introduce a breaking change, and take the minor release cycle to also do #5660?

self
}
}

impl Behaviour {
/// Creates a new identify [`Behaviour`].
pub fn new(config: Config) -> Self {
let discovered_peers = match NonZeroUsize::new(config.cache_size) {
None => PeerCache::disabled(),
Some(size) => PeerCache::enabled(size),
};
let discovered_peers = PeerCache::new(config.cache_config.clone());

Self {
config,
Expand Down Expand Up @@ -602,12 +597,8 @@ fn multiaddr_matches_peer_id(addr: &Multiaddr, peer_id: &PeerId) -> bool {
struct PeerCache(Option<PeerAddresses>);

impl PeerCache {
fn disabled() -> Self {
Self(None)
}

fn enabled(size: NonZeroUsize) -> Self {
Self(Some(PeerAddresses::new(size)))
fn new(cache_config: Option<PeerAddressesConfig>) -> Self {
Self(cache_config.map(PeerAddresses::new))
}

fn get(&mut self, peer: &PeerId) -> Vec<Multiaddr> {
Expand Down
8 changes: 6 additions & 2 deletions protocols/identify/tests/smoke.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
use futures::StreamExt;
use libp2p_identify as identify;
use libp2p_swarm::{Swarm, SwarmEvent};
use libp2p_swarm::{PeerAddressesConfig, Swarm, SwarmEvent};
use libp2p_swarm_test::SwarmExt;
use std::collections::HashSet;
use std::iter;
use std::num::NonZeroUsize;
use std::time::{Duration, Instant};
use tracing_subscriber::EnvFilter;

Expand Down Expand Up @@ -161,7 +162,10 @@ async fn emits_unique_listen_addresses() {
identify::Config::new("a".to_string(), identity.public())
.with_agent_version("b".to_string())
.with_interval(Duration::from_secs(1))
.with_cache_size(10),
.with_cache_config(Some(PeerAddressesConfig {
number_of_peers: NonZeroUsize::new(10).expect("10 != 0"),
..Default::default()
})),
)
});
let mut swarm2 = Swarm::new_ephemeral(|identity| {
Expand Down
5 changes: 5 additions & 0 deletions swarm/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
## 0.45.2

- Add `PeerAddressesConfig` and the possibility to configure the number of addresses cached per peer.
See [PR 5574](https://github.com/libp2p/rust-libp2p/pull/5574).

## 0.45.1

- Update `libp2p-swarm-derive` to version `0.35.0`, see [PR 5545]
Expand Down
2 changes: 1 addition & 1 deletion swarm/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ name = "libp2p-swarm"
edition = "2021"
rust-version = { workspace = true }
description = "The libp2p swarm"
version = "0.45.1"
version = "0.45.2"
authors = ["Parity Technologies <[email protected]>"]
license = "MIT"
repository = "https://github.com/libp2p/rust-libp2p"
Expand Down
2 changes: 1 addition & 1 deletion swarm/src/behaviour.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ pub mod toggle;

pub use external_addresses::ExternalAddresses;
pub use listen_addresses::ListenAddresses;
pub use peer_addresses::PeerAddresses;
pub use peer_addresses::{PeerAddresses, PeerAddressesConfig};

use crate::connection::ConnectionId;
use crate::dial_opts::DialOpts;
Expand Down
43 changes: 33 additions & 10 deletions swarm/src/behaviour/peer_addresses.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,39 @@ use lru::LruCache;

use std::num::NonZeroUsize;

#[derive(Debug, Clone)]
/// Configuration of a [`PeerAddresses`] instance.
pub struct PeerAddressesConfig {
/// Capacity of the [`PeerAddresses`] cache.
pub number_of_peers: NonZeroUsize,

/// Maximum number of cached addresses per peer.
pub number_of_addresses_by_peer: NonZeroUsize,
}

impl Default for PeerAddressesConfig {
fn default() -> Self {
Self {
number_of_peers: NonZeroUsize::new(100).expect("100 != 0"),
dariusc93 marked this conversation as resolved.
Show resolved Hide resolved
number_of_addresses_by_peer: NonZeroUsize::new(10).expect("10 != 0"),
}
}
}

/// Struct for tracking peers' external addresses of the [`Swarm`](crate::Swarm).
#[derive(Debug)]
pub struct PeerAddresses(LruCache<PeerId, LruCache<Multiaddr, ()>>);
pub struct PeerAddresses {
config: PeerAddressesConfig,
inner: LruCache<PeerId, LruCache<Multiaddr, ()>>,
}

impl PeerAddresses {
/// Creates a [`PeerAddresses`] cache with capacity for the given number of peers.
///
/// For each peer, we will at most store 10 addresses.
pub fn new(number_of_peers: NonZeroUsize) -> Self {
Self(LruCache::new(number_of_peers))
/// For each peer, we will at most store `config.number_of_addresses_by_peer` addresses.
pub fn new(config: PeerAddressesConfig) -> Self {
let inner = LruCache::new(config.number_of_peers);
Self { config, inner }
}

/// Feed a [`FromSwarm`] event to this struct.
Expand Down Expand Up @@ -50,12 +73,12 @@ impl PeerAddresses {
pub fn add(&mut self, peer: PeerId, address: Multiaddr) -> bool {
match prepare_addr(&peer, &address) {
Ok(address) => {
if let Some(cached) = self.0.get_mut(&peer) {
if let Some(cached) = self.inner.get_mut(&peer) {
cached.put(address, ()).is_none()
} else {
let mut set = LruCache::new(NonZeroUsize::new(10).expect("10 > 0"));
let mut set = LruCache::new(self.config.number_of_addresses_by_peer);
set.put(address, ());
self.0.put(peer, set);
self.inner.put(peer, set);

true
}
Expand All @@ -66,7 +89,7 @@ impl PeerAddresses {

/// Returns peer's external addresses.
pub fn get(&mut self, peer: &PeerId) -> impl Iterator<Item = Multiaddr> + '_ {
self.0
self.inner
.get(peer)
.into_iter()
.flat_map(|c| c.iter().map(|(m, ())| m))
Expand All @@ -76,7 +99,7 @@ impl PeerAddresses {
/// Removes address from peer addresses cache.
/// Returns true if the address was removed.
pub fn remove(&mut self, peer: &PeerId, address: &Multiaddr) -> bool {
match self.0.get_mut(peer) {
match self.inner.get_mut(peer) {
Some(addrs) => match prepare_addr(peer, address) {
Ok(address) => addrs.pop(&address).is_some(),
Err(_) => false,
Expand All @@ -92,7 +115,7 @@ fn prepare_addr(peer: &PeerId, addr: &Multiaddr) -> Result<Multiaddr, Multiaddr>

impl Default for PeerAddresses {
fn default() -> Self {
Self(LruCache::new(NonZeroUsize::new(100).unwrap()))
Self::new(Default::default())
}
}

Expand Down
3 changes: 2 additions & 1 deletion swarm/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,8 @@ pub use behaviour::{
AddressChange, CloseConnection, ConnectionClosed, DialFailure, ExpiredListenAddr,
ExternalAddrExpired, ExternalAddresses, FromSwarm, ListenAddresses, ListenFailure,
ListenerClosed, ListenerError, NetworkBehaviour, NewExternalAddrCandidate,
NewExternalAddrOfPeer, NewListenAddr, NotifyHandler, PeerAddresses, ToSwarm,
NewExternalAddrOfPeer, NewListenAddr, NotifyHandler, PeerAddresses, PeerAddressesConfig,
ToSwarm,
};
pub use connection::pool::ConnectionCounters;
pub use connection::{ConnectionError, ConnectionId, SupportedProtocols};
Expand Down
Loading