fix: invalidate relay reservations when the peer's last connection closes - #271
Merged
Conversation
…oses RelayState kept reservations keyed by peer with only an expiration, and the Switch teardown path never told the relay about a disconnect. A reservation therefore held its slot until expiry, and handleConnect briefly treated it as valid before opening the stop stream failed. That permits reservation-cap exhaustion through reserve-then-disconnect churn. specs/relay/circuit-v2: "the reservation remains valid until its expiration, as long as there is an active connection from the peer to the relay. If the peer disconnects, the reservation is no longer valid." The reservation is bound to the peer, not to the connection the RESERVE arrived on, so a peer keeping a second connection keeps its reservation. This follows go-libp2p, whose relay disconnect notifiee returns early while Connectedness(p) == Connected. (rust-libp2p instead keys reservations per connection with a per-peer cap; both readings are spec-conformant, and the per-peer model is the one this codebase's rsReservations already implements.) Add swDisconnectNotifiers to the Switch, fired from closeConnection -- already the single idempotent teardown path -- after the pool removal has committed and before the muxer is closed. Notifiers run synchronously so the reservation is gone by the time teardown returns, and each is isolated so a failing notifier cannot abort it. Because the closing connection has already left the pool, the liveness check needs no special-casing. registerNATHandlers subscribes registerReservationCleanup, which drops the peer's reservation only when lookupConn finds no connection left. ActiveReservation, handleReserve and handleConnect are unchanged. Closes #255.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #255.
Problem
RelayState.rsReservationskeyed reservations by peer with only an expiration, and the Switch teardown path never notified the relay of a disconnect. A reservation held its slot until expiry, andhandleConnectbriefly treated it as valid before opening the stop stream failed. That permits reservation-cap exhaustion through reserve-then-disconnect churn.Spec
specs/relay/circuit-v2:The reservation is tied to an active connection from the peer, not to the connection the RESERVE arrived on.
Which reference semantics
The two reference implementations differ, and both are spec-conformant:
rsvp map[peer.ID]time.Time, one reservation per peer. Its disconnect notifiee returns early whileConnectedness(p) == Connected, so a peer holding a second connection keeps its reservation.connections: HashMap<PeerId, HashMap<ConnectionId, Reservation>>withmax_reservations_per_peer: 4; each reservation dies with its own connection.This PR follows go-libp2p, because
rsReservationsis alreadyMap PeerId ActiveReservationandrcMaxReservations = 128already matches both implementations' defaults. The rust model would require restructuring the reservation store and adding a per-peer cap.Issue #255 originally asked for the opposite behaviour ("a second connection from the same PeerId cannot accidentally keep a reservation alive"); the issue body has been corrected to match the spec wording and the reference implementations.
Change
SwitchgainsswDisconnectNotifiers :: TVar [Connection -> IO ()].closeConnection— already the single, idempotent teardown path — fires them synchronously, after the pool-removal STM transaction has committed and beforemuxClose. Synchronous firing means the reservation is gone by the time teardown returns, closing the window where a concurrent CONNECT would still see it. Each notifier is exception-isolated so a failing one cannot abort teardown.lookupConnwith no special-casing.registerNATHandlerssubscribesregisterReservationCleanup, which deletes the peer's reservation only when no connection to it remains.ActiveReservation,handleReserveandhandleConnectare unchanged — no new fields, no signature changes.Tests
New
LibP2P.NAT.Relay.ReservationLifecycleSpec, driving real switches over loopback TCP and callingcloseConnectionon the relay's own side so the assertions are deterministic rather than timing-dependent:rcMaxReservations = 1: refused before, granted after)Full suite: 1150 examples, 0 failures.
Note on verification
The Haskell toolchain was unavailable on the host for this change, so the build and test runs were done in the repo's own
haskell:9.10-slim-bookwormimage.