feat: refresh circuit relay client reservations before expiry - #272
Merged
Conversation
specs/relay/circuit-v2 makes reservation refresh the client's
responsibility ("the reservation becomes invalid after this time and
it's the responsibility of the client to refresh") and ties validity
to the connection to the relay ("if the peer disconnects, the
reservation is no longer valid"). listenCircuit previously performed a
single RESERVE and ignored rsvExpire, so a node would silently become
unreachable via the relay once its reservation lapsed.
listenCircuit now tracks the granted expiry and runs a background loop
(ReservationRefreshConfig, tuned like go-libp2p's autorelay finder:
2 minute margin, checked every minute) that re-issues RESERVE on the
existing hop connection ahead of expiry. A disconnect notifier
withdraws the circuit listen address immediately if that specific
connection to the relay is lost, and a failed refresh (transport error
or non-OK STATUS) withdraws it too, so a dead reservation stops being
advertised in switchListenAddrs instead of dialers hitting a live-looking
but dead address.
Withdrawal is a new general primitive, switchWithdrawListener, that
removes one ActiveListener from the Switch without touching the rest of
its listeners or connections -- switchClose already had inline logic to
tear down all listeners together, but nothing let a single listener stop
advertising on its own before this.
The disconnect notifier matched the specific connection the RESERVE went out on, so a client with a second connection to the same relay withdrew its circuit listen address while the relay still honoured the reservation. The two reference implementations each pick one model and apply it on both sides. go-libp2p is peer-keyed throughout: the relay server returns early from its disconnect notifiee while Connectedness(p) == Connected (circuitv2/relay/relay.go), and the client's relay_finder skips the event unless Connectedness == NotConnected before deleting from its map[peer.ID]*Reservation (autorelay/relay_finder.go). rust-libp2p is connection-keyed throughout, on the server (HashMap<PeerId, HashMap<ConnectionId, Reservation>>) and on the client (reservation_addresses keyed by ConnectionId). This repo adopted the go-libp2p model on the server side in #255, so the client has to match it. Mixing the two also breaks against a real go-libp2p relay, which keeps a reservation while any connection from us remains: per-connection matching drops a listen address that is still routable. Withdraw the listener only when lookupConn finds no connection left to the relay peer, mirroring registerReservationCleanup.
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.
Spec
specs/relay/circuit-v2.md, Reservation section:Fixes #268 (depends on #266, already merged).
What changed
listenCircuit(src/LibP2P/NAT/Relay/Transport.hs) previously performed one RESERVE, ignoredrsvExpire, and never refreshed — a node would silently become unreachable via the relay once its reservation lapsed.rsvExpirefrom the RESERVE response. If the relay omits it, the RESERVE (initial or refresh) is treated as a failure — there's no expiry to schedule a refresh against, so trusting an undated reservation would defeat the whole point of this change.refreshLoop, spawned once the initial reservation is granted, polls (rrcPollInterval) and re-issues RESERVE on a freshhopstream over the same connection to the relay once the reservation is withinrrcMarginof expiry.swDisconnectNotifiershook from fix: invalidate relay reservations when the peer's last connection closes #271) matches the specific connection used for the reservation (byconnStateTVaridentity, not peer id) and immediately withdraws the circuit listen address if that connection closes.STATUS) also withdraws the listen address.switchWithdrawListener(src/LibP2P/Switch/Listen.hs) removes oneActiveListenerfrom the Switch — cancels its accept loop, closes it, and drops it fromswListeners— without touching any other listener or connection.switchClosehad equivalent inline logic for tearing down all listeners together; nothing previously let a single listener stop advertising on its own.circuitTransportandlistenCircuitnow take aReservationRefreshConfig;NATConfiggainedncReservationRefresh(default viadefaultReservationRefreshConfig) soregisterNATHandlersthreads it through. Both are re-exported from theLibP2Pfacade alongside the existingcircuitTransport/CircuitStateexports.go-libp2p comparison
Checked
p2p/host/autorelay/relay_finder.go(the refresh loop lives there, not inp2p/protocol/circuitv2/client/reservation.go, which only does a single RESERVE):rsvpExpirationSlack = 2 * time.Minute,rsvpRefreshInterval = time.Minute— refresh once expiry is within 2 minutes, checked every minute. Mirrored exactly asdefaultReservationRefreshConfig(rrcMargin = 120,rrcPollInterval= 1 minute in µs).switchWithdrawListenerwithdraws the listen address but never callscloseConnectionon the relay connection itself.reservation.go'sReservetreats an expiration in the past (which includes a zero/absentexpire, since protobuf-optional fields decode to the zero value) as a hard error. Mirrored viareservationExpiry, which fails outright — for both the initial RESERVE and a refresh — whenrsvExpireis missing, rather than defaulting or best-effort-continuing.One divergence: go-libp2p's relay-loss handling flows through its general connection-manager "unprotect" mechanism, which is polling/eviction-based within its connmgr, not an immediate synchronous callback. This codebase already has a synchronous
swDisconnectNotifiershook (added in #271 for the server-side reservation cleanup), so the client side uses the same mechanism for symmetry and to get immediate (not next-poll) withdrawal on disconnect.Design decisions
Peer-keyed loss detection, matching the server side. The disconnect notifier withdraws the circuit listen address only once
lookupConnfinds no connection left to the relay peer, mirroringregisterReservationCleanupinNAT.hs.The two reference implementations each pick one model and apply it on both sides. go-libp2p is peer-keyed throughout: the relay server returns early from its disconnect notifiee while
Connectedness(p) == Connected(circuitv2/relay/relay.go), and the client'srelay_finderskips the event unlessConnectedness == NotConnectedbefore deleting from itsmap[peer.ID]*Reservation(autorelay/relay_finder.go). rust-libp2p is connection-keyed throughout, on the server (HashMap<PeerId, HashMap<ConnectionId, Reservation>>) and on the client (reservation_addresseskeyed byConnectionId).This repo adopted the go-libp2p model on the server side in relay: invalidate reservations when their owning connection disconnects #255, so the client matches it. Mixing the two also misbehaves against a real go-libp2p relay, which keeps a reservation while any connection from us remains: per-connection matching would drop a listen address that is still routable.
What was tested
New
test/LibP2P/NAT/Relay/ReservationRefreshSpec.hs, three real in-process switches over loopback TCP, following the existingTransportSpec.hs/ReservationLifecycleSpec.hsconventions:closeConnection(no polling), followingReservationLifecycleSpec's established pattern for synchronous notifier-driven assertions.hopprotocol stream handler) that grants exactly one RESERVE then refuses every subsequent one, since the real relay server here always grants a refresh from an existing holder even at capacity. Confirms an unrelated plain-TCP connection into the same target Switch survives (ping still round-trips) and that Switch's other listener is untouched.Also updated
TransportSpec.hsandReservationLifecycleSpec.hsfor the newcircuitTransport/NATConfigsignatures (no behavioral changes to those tests).Full suite: 1153 examples, 0 failures (was 1150 baseline + 3 new).
What I deliberately left out
RelayLimit/_mLimit(issue relay: enforce client-side circuit limits (data/duration) #269) — out of scope per the brief, untouched.listenCircuitis called twice for the same relay (the second registration overwrites the first'sInboundQueueinCircuitState); this is a pre-existing property ofregisterQueue, not something this change introduces or was asked to address.