fix(identify): swallow StreamStateError on close

[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.

[wjmelements]

I also tried a binary search approach. The speedup was negligible until there were an unreasonable number of addresses so I don't think it justifies the additional complexity.

  ⎿  --- 10 addresses, 20 protocols ---
     linear                         5000 iterations      43.8 ms  (8.8 µs/op)
     binary search                  5000 iterations      32.2 ms  (6.4 µs/op)
       speedup: 1.36x
       address count matches: 10

     --- 20 addresses, 20 protocols ---
     linear                         5000 iterations      37.3 ms  (7.5 µs/op)
     binary search                  5000 iterations      38.5 ms  (7.7 µs/op)
       speedup: 0.97x
       address count matches: 20

     --- 100 addresses, 20 protocols ---
     linear                         2000 iterations      35.1 ms  (17.6 µs/op)
     binary search                  2000 iterations      33.3 ms  (16.6 µs/op)
       speedup: 1.06x
       address count matches: 100

     --- 500 addresses, 20 protocols ---
     linear                         500 iterations    4445.9 ms  (8891.8 µs/op)
     binary search                  500 iterations     199.6 ms  (399.1 µs/op)
       speedup: 22.28x
       address count matches: 500

     --- 1000 addresses, 20 protocols ---
     linear                         100 iterations    2066.3 ms  (20662.7 µs/op)
     binary search                  100 iterations     157.9 ms  (1578.8 µs/op)
       speedup: 13.09x
       address count matches: 1000