feat: run identify on every new connection and populate the peer store - #273
Merged
Conversation
swPeerStore was only ever written by handleIdentifyPush, so it filled only when a remote pushed to us. requestIdentify returned IdentifyInfo without storing it, and nothing called it when a connection came up, so nothing was known about a peer we dialled or accepted. specs/relay/DCUtR depends on this being populated: "Upon observing the new connection, the inbound peer (here B) checks the addresses advertised by A via identify." go-libp2p drives its IDService from the swarm's Connected notification for the same reason. registerIdentifyHandlers now also subscribes an on-connect notifier, so any Switch that registers identify gets this without further wiring and the Switch core keeps no dependency on the protocol. The notifier is a thin wrapper over identifyPeer, which is exported so callers that need to observe the outcome -- tests, and the DCUtR integration in #258 -- can run it directly and read the result. requestIdentify now closes its stream on every exit path and bounds the exchange with a 5s timeout, matching go-libp2p's identify.DefaultTimeout. Running it per connection would otherwise leak a half-open stream and a thread for every connection, since a peer can negotiate and then never answer. The remaining stream-ownership work -- AutoNAT both sides, and dispatchStream's ownership model -- stays with #257. Storing goes through a shared storeIdentify used by both the push responder and identifyPeer, so both merge into what is already known via mergeIdentify instead of replacing it. Unblocks #258. Closes #267.
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 #267. Unblocks #258.
Problem
swPeerStorewas written only byhandleIdentifyPush, i.e. only when a remote pushed to us.requestIdentifyreturnedIdentifyInfowithout storing it, and nothing invoked it when a connection came up —Dial.hsandListen.hscontain no reference to it. So nothing was known about a peer we dialled or accepted.Spec
Identify is how a peer learns the remote's public key, listen addresses, protocols and the address it observes for us.
specs/relay/DCUtRdepends on it having already run:go-libp2p drives its
IDServicefrom the swarm's Connected notification for the same reason.Change
registerIdentifyHandlersadditionally subscribes an on-connect notifier onswNotifiers. Any Switch that registers identify gets this with no further wiring, and the Switch core keeps no dependency on the protocol — the same shape GossipSub already uses.identifyPeer :: Switch -> Connection -> IO (Either String ()), which is exported. Callers that need to observe the outcome — tests, and the DCUtR integration in dcutr: production integration reuses the relay connection instead of dialing direct #258 — call it directly and read the result. The notifier itself discards it.storeIdentify, used by both the push responder andidentifyPeer, so both merge into what is already known viamergeIdentifyrather than replacing it. A push is a partial update and must not erase fields it omits.network.WithAllowLimitedConn(ctx, "identify")).Stream lifetime
requestIdentifydid not close its stream. That was harmless while it had no callers; running it per connection would leak a half-open stream and a thread for every connection, since a peer may negotiate and then never answer.It now closes on every exit path — success, negotiation refusal, decode failure, timeout, exception — and bounds the exchange with a 5s timeout, matching go-libp2p's
identify.DefaultTimeout(which it applies to all id interactions in both directions). Its signature is unchanged; a timeout surfaces asLeft.The rest of the stream-ownership work stays with #257: AutoNAT on both sides, and
dispatchStream's ownership model, which affects every protocol and is not something this PR should decide.Deliberately not included
A wait-for-identify primitive. go-libp2p has
IdentifyWait, and its hole punch service takes the identify service as a required dependency to wait on it. Here the identify notifier and #258's DCUtR notifier will both fire on the same connection with no ordering guarantee — but #258 can resolve that deterministically by callingidentifyPeeritself and reading the result, without new Switch state to keep and clean up. If #258 turns out to need a shared completion signal, it can be added then against concrete requirements rather than guessed at now.Tests
New
LibP2P.Protocol.Identify.IdentifyOnConnectSpec, two real switches over loopback TCP:Full suite: 1159 examples, 0 failures.
Note on verification
The Haskell toolchain is unavailable on this host, so the build and test runs were done in the repo's own
haskell:9.10-slim-bookwormimage.