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

feat(identify)!: report what we pushed to a remote peer #4335

Closed
wants to merge 5 commits into from

Conversation

dhuseby
Copy link
Contributor

@dhuseby dhuseby commented Aug 17, 2023

Description

Starting in #3980, we make more use of the identify push protocol by actively notifying the peer when our locally supported protocols change. We emit an event (Pushed) to the user in that case. This event however does not include what we pushed.

Users might be interested in the internal changes that we push to remote peers. This patch adds the identify Info to the Pushed variant.

Fixes #4332.

Notes & open questions

None

Change checklist

  • I have performed a self-review of my own code
  • Discuss this speculative pull request in the issue
  • I have made corresponding changes to the documentation
  • Fix all of the borked tests
  • I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

@thomaseizinger
Copy link
Contributor

I am not convinced that it is the responsibility of the identify protocol to emit this event. Every protocol has access to this information, why should identify specifically report this in this events? It has nothing to do with identify per-se. As suggested in #4332 (comment), we could extend the Pushed variant with the information that was pushed.

The fact that the user received a Pushed variant tells them that something changed and we actively notified our peers. It may have been an address or the set of protocols. We could also model in more detail what exactly we pushed, i.e. have a "reason" of some sorts like:

Pushed {
	peer: PeerId,
    info: Info,
    trigger: WhatChanged
}

enum WhatChanged {
   LocalProtocols,
   ExternalAddresses
}

This starts to get a bit involved though. As a first step, we could simply include Info and leave it up to the user to diff them. Thoughts?

@dhuseby
Copy link
Contributor Author

dhuseby commented Aug 18, 2023

This starts to get a bit involved though. As a first step, we could simply include Info and leave it up to the user to diff them.

This suggestion makes a lot of sense. Let me change my PR to be this.

@dhuseby dhuseby force-pushed the report-local-protocols-changed branch from e7f6ef2 to a7ae23c Compare August 18, 2023 00:09
@dhuseby dhuseby force-pushed the report-local-protocols-changed branch from b831237 to 4a0012b Compare August 18, 2023 00:42
@dhuseby dhuseby changed the title added behavior event when local protocols change feat!: added behavior event when local protocols change Aug 18, 2023
@mergify
Copy link
Contributor

mergify bot commented Aug 18, 2023

This pull request has merge conflicts. Could you please resolve them @dhuseby? 🙏

@thomaseizinger thomaseizinger changed the title feat!: added behavior event when local protocols change feat(identify)!: report what we pushed to a remote peer Aug 18, 2023
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

Thanks @dhuseby !

A few minor comments :)

protocols/identify/CHANGELOG.md Outdated Show resolved Hide resolved
protocols/identify/CHANGELOG.md Show resolved Hide resolved
Event::IdentificationPushed,
)),
future::Either::Right(()) => {
let local_info = self.build_info();
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it guaranteed that this is actually the info we pushed? What if our state changed since we sent it to the other peer?

I'd prefer if we changed the OutboundUpgrade implementation such that it emits the Info that it sent over the wire so we can carry it forward here.

@thomaseizinger thomaseizinger added this to the v0.53.0 milestone Sep 13, 2023
@thomaseizinger thomaseizinger marked this pull request as draft September 13, 2023 10:06
@thomaseizinger
Copy link
Contributor

Marking this as draft because it is a breaking change.

@dhuseby
Copy link
Contributor Author

dhuseby commented Sep 20, 2023

Closing this broken PR (no force push!) and replacing with #4527

@dhuseby dhuseby closed this Sep 20, 2023
@thomaseizinger thomaseizinger removed this from the v0.53.0 milestone Oct 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants