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(identify): make timeout and concurrent streams configurable #5654

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
2 changes: 1 addition & 1 deletion Cargo.lock

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

2 changes: 1 addition & 1 deletion 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.48.0", path = "protocols/gossipsub" }
libp2p-identify = { version = "0.45.1", path = "protocols/identify" }
libp2p-identify = { version = "0.45.2", 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 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.45.2

- Make stream timeout and maximum number of concurrent streams configurable on identify.
See [PR 5654](https://github.com/libp2p/rust-libp2p/pull/5654).

## 0.45.1

- Add `hide_listen_addrs` option to prevent leaking (local) listen addresses.
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.1"
version = "0.45.2"
authors = ["Parity Technologies <[email protected]>"]
license = "MIT"
repository = "https://github.com/libp2p/rust-libp2p"
Expand Down
34 changes: 34 additions & 0 deletions protocols/identify/src/behaviour.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,9 @@ use std::{
time::Duration,
};

const STREAM_TIMEOUT: Duration = Duration::from_secs(60);
const MAX_CONCURRENT_STREAMS_PER_CONNECTION: usize = 10;

/// Whether an [`Multiaddr`] is a valid for the QUIC transport.
fn is_quic_addr(addr: &Multiaddr, v1: bool) -> bool {
use Protocol::*;
Expand Down Expand Up @@ -153,6 +156,17 @@ pub struct Config {
///
/// Disabled by default.
pub hide_listen_addrs: bool,

/// Maximum duration for which an identification request stream may remain
/// active before timing out.
///
/// Defaults to 1 minute.
pub stream_timeout: Duration,

/// Maximum number of concurrent identification request streams per connection.
///
/// Defaults to 10.
pub max_concurrent_streams_per_connection: usize,
}

impl Config {
Expand All @@ -167,6 +181,8 @@ impl Config {
push_listen_addr_updates: false,
cache_size: 100,
hide_listen_addrs: false,
stream_timeout: STREAM_TIMEOUT,
max_concurrent_streams_per_connection: MAX_CONCURRENT_STREAMS_PER_CONNECTION,
}
}

Expand Down Expand Up @@ -202,6 +218,20 @@ impl Config {
self.hide_listen_addrs = b;
self
}

/// Configures maximum allowed duration for which an active identification request
/// stream is allowed to timeout.
pub fn with_stream_timeout(mut self, t: Duration) -> Self {
self.stream_timeout = t;
self
}

/// Configures the maximum number of concurrent identification request streams
/// allowed per connection.
pub fn with_max_concurrent_streams_per_connection(mut self, s: usize) -> Self {
self.max_concurrent_streams_per_connection = s;
self
}
}

impl Behaviour {
Expand Down Expand Up @@ -350,6 +380,8 @@ impl NetworkBehaviour for Behaviour {
self.config.agent_version.clone(),
remote_addr.clone(),
self.all_addresses(),
self.config.stream_timeout,
self.config.max_concurrent_streams_per_connection,
))
}

Expand Down Expand Up @@ -382,6 +414,8 @@ impl NetworkBehaviour for Behaviour {
self.config.agent_version.clone(),
addr.clone(), // TODO: This is weird? That is the public address we dialed, shouldn't need to tell the other party?
self.all_addresses(),
self.config.stream_timeout,
self.config.max_concurrent_streams_per_connection,
))
}

Expand Down
9 changes: 4 additions & 5 deletions protocols/identify/src/handler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,6 @@
use std::{task::Context, task::Poll, time::Duration};
use tracing::Level;

const STREAM_TIMEOUT: Duration = Duration::from_secs(60);
const MAX_CONCURRENT_STREAMS_PER_CONNECTION: usize = 10;

/// Protocol handler for sending and receiving identification requests.
///
/// Outbound requests are sent periodically. The handler performs expects
Expand Down Expand Up @@ -116,7 +113,7 @@

impl Handler {
/// Creates a new `Handler`.
pub fn new(

Check failure on line 116 in protocols/identify/src/handler.rs

View workflow job for this annotation

GitHub Actions / clippy (1.80.0)

this function has too many arguments (9/7)

Check failure on line 116 in protocols/identify/src/handler.rs

View workflow job for this annotation

GitHub Actions / clippy (beta)

this function has too many arguments (9/7)
interval: Duration,
remote_peer_id: PeerId,
public_key: PublicKey,
Expand All @@ -124,13 +121,15 @@
agent_version: String,
observed_addr: Multiaddr,
external_addresses: HashSet<Multiaddr>,
stream_timeout: Duration,
max_concurrent_streams_per_connection: usize,
Copy link
Member

Choose a reason for hiding this comment

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

This is triggering a too many argument linting from clippy. Maybe we could have a struct that takes in the options and pass it to the handler. Thoughts?

CC @jxs

Copy link
Author

Choose a reason for hiding this comment

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

This is triggering a too many argument linting from clippy. Maybe we could have a struct that takes in the options and pass it to the handler. Thoughts?

sure. will add it inside a struct.

Copy link
Author

Choose a reason for hiding this comment

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

Hi @dariusc93 rather than creating a new struct can I directly pass behaviour config into the handler as most of the data is being used from there?

Copy link
Author

Choose a reason for hiding this comment

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

Hi @dariusc93 have passed config into hander and checks have passed.

) -> Self {
Self {
remote_peer_id,
events: SmallVec::new(),
active_streams: futures_bounded::FuturesSet::new(
STREAM_TIMEOUT,
MAX_CONCURRENT_STREAMS_PER_CONNECTION,
stream_timeout,
max_concurrent_streams_per_connection,
),
trigger_next_identify: Delay::new(Duration::ZERO),
exchanged_one_periodic_identify: false,
Expand Down
Loading