Identify: emit useful events after identification #2759
Conversation
| // Conn is the connection we identified. | ||
| Conn network.Conn |
There was a problem hiding this comment.
It makes sense to have this right now, but I'm concerned about the future if/when we have some sort of connection-independent identify. But I guess we have no plans there.
There was a problem hiding this comment.
Don't we want to double down on this connection dependent identify approach? #2693
There was a problem hiding this comment.
In general, yes. I'm just calling this out as a decision.
| ListenAddrs []multiaddr.Multiaddr | ||
|
|
||
| // Protocols is the list of protocols the peer advertised on this connection. | ||
| Protocols []protocol.ID |
There was a problem hiding this comment.
All addrs/protocols, or only the ones the peer told us about?
There was a problem hiding this comment.
Only the one they told us about on this connection
There was a problem hiding this comment.
(what it says in the comment)
| Protocols []protocol.ID | ||
|
|
||
| // SignedPeerRecord is the provided signed peer record of the peer. May be nil. | ||
| SignedPeerRecord *record.Envelope |
There was a problem hiding this comment.
Should we include the other fields?
AgentVersion, ProtocolVersion and as we have the connection, observed addrs might be useful too?
There was a problem hiding this comment.
Yes. And we should stop storing them.
Identify has an event that it emits, but it's not very useful in that it will only tell you a peer has been identified, but not tell you anything about that peer. This design is intended to have the user then lookup peer information somewhere else after they get the event. However, that approach is brittle because it relies on identify itself probing in and updating all the "somewhere elses". This issue is highlight in #2754 and #2355.
Instead, Identify should only concern itself with emitting an event about what it has learned. Other services can subscribe to those events and do their appropriate logic. This helps decouple these services from each other.
Relates to #2754 and #2355
This is a partial fix for #2754. The next step after this is merged is to update GossipSub to make use of these events.