fix(identify): handle multiple envelopes from go

[wjmelements]

Reviewer @tabcat
This was causing failures with identify when I was doing a quick Helia bitswap with Kubo.
Kubo closes the stream fully immediately after sending the identify response.
The JS identify client calls pb.unwrap().unwrap().close() after pb.read(); this throws StreamStateError because byteStream.unwrap() tries to push the internal read buffer onto a stream whose readStatus is closed.
The error is caught and the stream aborted, discarding a successfully-read message, so consumeIdentifyMessage is never called, and peer:identify never fires.
Consequently, Helia never gets onConnect called for Kubo peers.
(I'm not too familiar with this code so maybe there is a better way to fix this.)

Changes

• wrap the close() call in its own try/catch so a failure there doesn't discard the already-read message

[tabcat]

Hi @wjmelements do you think you could reproduce this behavior inside of a test?
A stack trace would also be super helpful 🙏

[tabcat]

byteStream.unwrap() tries to push the internal read buffer onto a stream whose readStatus is closed.v

byteStream will only push the read buffer back to the stream if there are bytes leftover. any idea what those bytes are?

[wjmelements]

I pushed a test that reproduces the same exception I was seeing.

any idea what those bytes are?

It's pb.Identify{SignedPeerRecord: sr}. The relevant function is writeChunkedIdentifyMsg in p2p/protocol/identify/id.go of go-libp2p (my version is v0.48.0 but this has been the behavior for a few years).

To summarize that code, the identify response gets split up if there is a SignedPeerRecord and the message will be larger than legacyIDSize(2 KB); the first message is the identify message without the SignedPeerRecord and the second is another with only the SignedPeerRecord.

[achingbrain]

the identify response gets split up if there is a SignedPeerRecord and the message will be larger than legacyIDSize(2 KB); the first message is the identify message without the SignedPeerRecord and the second is another with only the SignedPeerRecord.

This is not spec-compliant behaviour, the spec clearly says the remote should send an Identify message and close(s) the stream, not one or more Identify messages (or words to that effect). I've opened libp2p/go-libp2p#3483

To move forward here we should probably read multiple identify messages until the stream closes (with an appropriate timeout) and compose the final message from whatever we receive.

We definitely want to prefer signed peer records if they are available.

[tabcat]

It looks like go iterates over 10 messages with a 5 second timeout for the identify stream. It collects all the messages and merges them before writing to the peer store. All identify responses are dropped if the stream times out.

[wjmelements]

It looks like go iterates over 10 messages with a 5 second timeout for the identify stream. It collects all the messages and merges them before writing to the peer store. All identify responses are dropped if the stream times out.

Ok, I plan to implement similar behavior for js.

[achingbrain]

There is an update to the Identify spec with message-splitting taking shape here.

Any changes made here should make the js implementation conform to the newly more detailed spec.

[tabcat]

apologies for the delay. where this pr landed:

receive: handles up to 10 lp-framed identify messages and merges them. lets us interop with go-libp2p which splits the signedPeerRecord into a second message when identify is over 2kb. original close-crash gets fixed by
wrapping the close in try/catch with stream.abort() if close throws. the read-loop catch is lenient, any error after at least one successful read keeps the messages we got and breaks.

send: still single-message. sending multi-message would crash older js-libp2p receivers that still have the close-crash. this pr is what lets them stop crashing so once it's deployed we can turn on multi-send too, depending on how the spec changes solidify.

cross-impl: looked at the 12 active libp2p stacks at libp2p/libp2p. 4kb is the practical outbound ceiling, rust-libp2p and litep2p both cap there. today's identify is comfortably under that without post-quantum keys so this
pr doesn't add an outbound cap, trim, or sort. those become useful when envelopes grow (pq identity, more circuit-relay addresses, etc) and can be added in follow-ups. still need to run some interop tests with other implementations locally.

deferred: multi-message send, outbound 4kb cap + trim, defaultMultiaddrSorter (only meaningful when splitting), partial-diff identify-push. spec pr libp2p/specs#709 still has open questions around envelope sizes for
post-quantum signatures, can update when that settles.

[tabcat]

tested against:

• go-v0.46, go-v0.47, go-v0.48
• rust-v0.53, rust-v0.54, rust-v0.55, rust-v0.56
• nim-v1.14
• jvm-v1.2
• js-v1.x, js-v2.x, js-v3.x
• js-head ↔ js-head

using libp2p/test-plans. no failures.