fix(relay): enforce per-peer reservation limit at boundary#6365
fix(relay): enforce per-peer reservation limit at boundary#6365failuresmith wants to merge 2 commits into
Conversation
| let relay_addr = Multiaddr::empty().with(Protocol::Memory(rand::random::<u64>())); | ||
| let mut relay = build_relay_with_config(relay::Config { | ||
| max_reservations: 10, | ||
| max_reservations_per_peer: 1, | ||
| reservation_duration: Duration::from_secs(60), | ||
| ..relay::Config::default() | ||
| }); | ||
| let relay_peer_id = *relay.local_peer_id(); |
There was a problem hiding this comment.
The test is very much appreciated, thanks!
The test right now creates two clients with the same peer ID, because the client behavior can have only one reservation per connection at a time.
I think we can simplify this quite a lot by instead adding two listen addresses for the relay and have one client try to use both addresses (which internally will then create two connections between client and relay).
There was a problem hiding this comment.
I like your idea of simplifying this to a single client.
My only concern is that it may not cover exactly the same case. From what I can tell, a second reservation to the same relay is already handled specially, and once there is a direct connection, the client may reuse it rather than open a second one.
Because of that, I wasn’t sure that using two relay listen addresses would reliably give us two simultaneous client-relay connections with the same PeerId, which is the case this regression was meant to cover.
That said, I may be missing something. If this does reliably create two connections while still testing the same boundary condition, I’d be happy to change it.
Summary
PeerIdRoot cause
The relay admission check used
>instead of>=when evaluatingmax_reservations_per_peer, which allowed one extra reservation when a peer was already exactly at the configured limit.Validation
cargo fmt --checkcargo test -p libp2p-relay