Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Identify: emit useful events after identification #2759

Merged
merged 4 commits into from
Apr 16, 2024

Conversation

MarcoPolo
Copy link
Collaborator

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.

@MarcoPolo MarcoPolo requested a review from sukunrt April 5, 2024 03:07
Comment on lines +16 to +17
// Conn is the connection we identified.
Conn network.Conn
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we want to double down on this connection dependent identify approach? #2693

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, yes. I'm just calling this out as a decision.

Comment on lines +20 to +23
ListenAddrs []multiaddr.Multiaddr

// Protocols is the list of protocols the peer advertised on this connection.
Protocols []protocol.ID
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All addrs/protocols, or only the ones the peer told us about?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only the one they told us about on this connection

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we include the other fields?
AgentVersion, ProtocolVersion and as we have the connection, observed addrs might be useful too?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. And we should stop storing them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed. Thanks

@MarcoPolo MarcoPolo merged commit be32b5b into master Apr 16, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants