fix: opt-in to toplogy notifications on transient connections

[achingbrain]

Adds a notifyOnTransient option when registering a network topology to opt-in to being notified when peers that support the registered protocol connect over transient connections. False by default.

The logic has been switched to check each connection's transient property and only notify if the user has opted in.

The side effect here is that if notifyOnTransient is true, and the peer ends up opening a direct connection (for example they dial us via circuit relay, open a stream to do the WebRTC SDP exchange, then open a WebRTC connection), identify will run on the second connection so the topology will receive two notifications.

This is not a breaking change since the previous behaviour would have been to only notify on the initial transient connection, which you can't do data-heavy things like bitswap over, or long-lived things like GossipSub so is probably a bug.

Fixes #2036

[achingbrain]

Still needs a bit of work, it's missing the case where a peer opens a transient connection, then opens a second non-transient connection.

Dismissing approval given with the new changes there is remaining work to get the topology to correctly notify for transient connections as well as direct connections

[joeltg]

Hate to nag but is there an ETA on this? GossipSub over WebRTC is completely broken so this is blocking new releases for us 😖

[achingbrain]

Still needs a bit of work, it's missing the case where a peer opens a transient connection, then opens a second non-transient connection.

I've added a test for this. The topology will get a notification on each connection that is opened and it'll be down the implementation to not duplicate peer operations, etc. It'll only be notified on transient connections if it ops in to notifications, otherwise it'll just get them for non-transient connections.

[joeltg]

I've been testing this branch locally and I think there's still something missing.

Currently, the registrar only notifies topologies of new connections from within the Registrar._onPeerUpdate handler, which diffs the updated protocols with the existing stored protocols to get two filtered protocol arrays added and removed. Then, for each topology registered for the protocols in added, it calls the topology's onConnect callback with every open connection, skipping transient connections that aren't opted-into.

The problem with this is with the protocol diff / filtering at the beginning. For example, when I try to use GossipSub over WebRTC in the browser, the following happens:

1. peer A opens a transient connection to peer B via circuit-relay
2. both peers perform identify over the circuit-relay connection, exchanging all registered protocols and updating their peer store. the gossipsub topology is NOT notified because the connection is transient
3. the peers establish a WebRTC connection
4. both peers perform identify again, over the WebRTC connection, exchanging the same set of protocols
5. nothing happens because added is empty

[joeltg]

I don't think there's a straightforward one- or two-line fix for this. I have a patch joeltg@bef1927 that adds a topologyPeers: Map<string, PeerSet> to the registrar (one for every topology) and maintains those to tell whether it should notify peers or not, and this works, although it reverts the behavior to notifying topologies exactly once per peer and not once per connection. Feel free to PR/cherrypick it in if you want (it's one commit).

I seem to remember that Topology used to be a concrete class, not just an interface, with a PeerSet attached to it? Maybe that's worth revisiting since it looks like that peer set is something both the registrar and external users want access to.

[achingbrain]

I've updated the PR to only do the peer disconnect notifications when protocols are removed on peer:update. After peer:identify we notify every topology for every supported protocol, testing for the transient connection opt-in first.

This is a little easier to reason about as well I think.

[joeltg]

Awesome, this works locally for me! Thanks so much