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] Implement /ipfs/id/push/1.0.0 alongside some refactoring. #1999

Merged
merged 9 commits into from
Mar 18, 2021

Conversation

romanb
Copy link
Contributor

@romanb romanb commented Mar 15, 2021

  • Implement the /ipfs/id/push/1.0.0 protocol, i.e. the ability to actively push information of the local peer to specific remotes.
  • Make the initial delay as well as the recurring delay for the periodic identification requests configurable, introducing IdentifyConfig.
  • Small cleanups and refactorings along the way.

Closes #1136.

Roman S. Borschel and others added 5 commits March 15, 2021 12:27
  * Implement /ipfs/id/push/1.0.0, i.e. the ability to actively
    push information of the local peer to specific remotes.
  * Make the initial delay as well as the recurring delay
    for the periodic identification requests configurable,
    introducing `IdentifyConfig`.
@mxinden
Copy link
Member

mxinden commented Mar 17, 2021

Sorry for the delay here @romanb. I will take a look later today.

Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

Changes look good to me. Thanks for the clean patch.

I wonder whether libp2p-identify should (by default?) automatically initiate pushes to its peers on inject_new_listen_addr, inject_expired_listen_addr and inject_new_external_addr.

If I understand correctly, this would be in line with the specification.

When a peer's basic information changes, for example, because they've obtained a new public listen address, they can use identify/push to inform others about the new information.

protocols/identify/src/identify.rs Outdated Show resolved Hide resolved
protocols/identify/src/identify.rs Outdated Show resolved Hide resolved
@romanb
Copy link
Contributor Author

romanb commented Mar 18, 2021

I wonder whether libp2p-identify should (by default?) automatically initiate pushes to its peers on inject_new_listen_addr, inject_expired_listen_addr and inject_new_external_addr.

I can give that a try in a follow-up PR. It didn't occur to me to do that here. Thanks for the suggestion.

@mxinden
Copy link
Member

mxinden commented Mar 18, 2021

I wonder whether libp2p-identify should (by default?) automatically initiate pushes to its peers on inject_new_listen_addr, inject_expired_listen_addr and inject_new_external_addr.

I can give that a try in a follow-up PR. It didn't occur to me to do that here. Thanks for the suggestion.

Follow up pull request sounds good. Thanks for giving it a shot! Feel free to merge this pull request.

@romanb romanb merged commit 5a45f93 into libp2p:master Mar 18, 2021
@romanb romanb deleted the identify-push branch March 18, 2021 11:47
IdentifyHandlerEvent::Identified(remote_info)));
self.keep_alive = KeepAlive::No;
}
EitherOutput::Second(()) => {}
Copy link
Member

Choose a reason for hiding this comment

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

Something that came to my mind: How about emitting an event along the lines of IdentifyEvent::Pushed once a push succeeded? I would expect this to be useful for people tracking these events in a monitoring system like Prometheus. This event would serve a similar purpose to the RequestSent event in libp2p-request-response:

/// A response to an inbound request has been sent.
///
/// When this event is received, the response has been flushed on
/// the underlying transport connection.
ResponseSent {
/// The peer to whom the response was sent.
peer: PeerId,
/// The ID of the inbound request whose response was sent.
request_id: RequestId,
},

In case you think this is useful @romanb I can prepare a pull request.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good to me.

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.

Implement identify/push
2 participants