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.

Sign up for GitHub

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

Jump to bottom

fix(identify): handle partial push messages #4495

Merged
merged 13 commits into from
Sep 24, 2023
Merged

fix(identify): handle partial push messages #4495

merged 13 commits into from
Sep 24, 2023

Conversation

Marcel-G
Copy link
Contributor

@Marcel-G Marcel-G commented Sep 12, 2023 •
edited by thomaseizinger
Loading

Description

According to the spec, all fields within push messages are optional. To handle missing fields, we deserialize push messages into a different struct, patch the previously received, full identify message with it and emit this as the new info.

Previously, we failed to parse the message which causes incompatibility with js-libp2p nodes as they don't send the public key in push messages. See https://github.com/libp2p/js-libp2p/blob/88c47f51f9d67a6261e4ac65c494cd1e6e4ed8dd/packages/libp2p/src/identify/identify.ts#L205-L210.

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • 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

@Marcel-G
fix: make identify public_key optional
b605bf2 
The protocol isn't too decisive on this but js-libp2p does not send publicKey on `identify/push` messages. This results in rust-libp2p ignoring the messages due to failed publicKey parsing.

https://github.com/libp2p/js-libp2p/blob/88c47f51f9d67a6261e4ac65c494cd1e6e4ed8dd/packages/libp2p/src/identify/identify.ts#L205-L210

Note that missing fields should be ignored, as peers may choose to send partial updates containing only the fields whose values have changed.

publicKey is this node's public key (which also gives its node.ID)
 - may not need to be sent, as secure channel implies it has been sent.
 - then again, if we change / disable secure channel, may still want it.
@thomaseizinger
Copy link
Contributor

Is it optional for all usages of identify or only for push? This is a breaking API change so we'll have to hold it until we decide to make another breaking release (or we figure out a way to make it not breaking).

@mxinden
Copy link
Member

mxinden commented Sep 14, 2023

Is it optional for all usages of identify or only for push?

I had to check myself. According to the specification, only for push.

https://github.com/libp2p/specs/tree/master/identify#identifypush

Not sending the public key on each push seems like a premature bandwidth optimization to me. Public keys are small, push events should be rare. Am I missing something?

I am fine moving forward with the proposal as is with the next breaking change. Alternative approach would be to use the last sent public key in case it is missing in the current message.

as peers may choose to send partial updates containing only the fields whose values have changed.

This to me implies that the peer must have sent the non-changed fields in previous messages in order to be able to omit them in upcoming ones.

@mxinden
Copy link
Member

mxinden commented Sep 14, 2023

Nice catch @Marcel-G. Thank you for debugging and reporting!

@thomaseizinger
Copy link
Contributor

Would we be emitting the diff or compute the latest identify info based on the updated data?

The latter would be backwards-compatible with our API so we can ship it immediately :)

@Marcel-G Marcel-G force-pushed the fix/optional-public-key branch from 8177acd to 1fab3ff Compare September 21, 2023 16:29
thomaseizinger
thomaseizinger reviewed Sep 21, 2023
View reviewed changes
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 for making the changes!

I have a few more suggestions to make this consistent.

protocols/identify/src/handler.rs Outdated Show resolved Hide resolved
protocols/identify/src/protocol.rs Outdated Show resolved Hide resolved
protocols/identify/src/protocol.rs Outdated Show resolved Hide resolved
@Marcel-G Marcel-G force-pushed the fix/optional-public-key branch 2 times, most recently from ece63c8 to 88709d6 Compare September 22, 2023 08:19
@Marcel-G Marcel-G changed the title fix: make identify public_key optional fix: handle partial identify/push messages Sep 22, 2023
@Marcel-G Marcel-G force-pushed the fix/optional-public-key branch from 88709d6 to 5449e3d Compare September 23, 2023 10:04
@thomaseizinger
Copy link
Contributor

Please avoid force-pushing! :)

https://github.com/libp2p/rust-libp2p/blob/master/CONTRIBUTING.md#we-squash-merge-pull-requests

thomaseizinger
thomaseizinger reviewed Sep 24, 2023
View reviewed changes
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 for making the changes! There a few more minor things to sort out to make this actually a backwards-compatible change from an API PoV but this is close to being ready. Note that we will also need a changelog entry and a patch-version bump for this :)

protocols/identify/src/handler.rs Outdated Show resolved Hide resolved
protocols/identify/src/handler.rs Outdated Show resolved Hide resolved
protocols/identify/src/handler.rs Outdated Show resolved Hide resolved
protocols/identify/src/handler.rs Outdated Show resolved Hide resolved
protocols/identify/src/protocol.rs Outdated Show resolved Hide resolved
protocols/identify/src/protocol.rs Outdated Show resolved Hide resolved
thomaseizinger
thomaseizinger reviewed Sep 24, 2023
View reviewed changes
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.

Cool, almost ready!

As a last thing, we'll need a changelog entry and a version bump by a patch-version in the Cargo.toml. You'll also have to bump the version of libp2p-identify in the root Cargo.toml.

protocols/identify/src/handler.rs Outdated Show resolved Hide resolved
protocols/identify/src/protocol.rs Outdated Show resolved Hide resolved
protocols/identify/src/protocol.rs Outdated Show resolved Hide resolved
@thomaseizinger thomaseizinger changed the title fix: handle partial identify/push messages fix(identify): handle partial push messages Sep 24, 2023
thomaseizinger
thomaseizinger approved these changes Sep 24, 2023
View reviewed changes
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.

Awesome! Thank you for being so responsive and incorporating all the feedback. I think we've landed at a pretty decent solution :)

@mergify mergify bot merged commit ad45d23 into libp2p:master Sep 24, 2023
@Marcel-G Marcel-G deleted the fix/optional-public-key branch September 24, 2023 12:22
@mxinden
Copy link
Member

mxinden commented Sep 25, 2023

Thank you for the work. I find this a clean fix.

@thomaseizinger thomaseizinger mentioned this pull request Sep 26, 2023
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thomaseizinger thomaseizinger thomaseizinger approved these changes

Assignees
No one assigned
Labels
send-it
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Marcel-G @thomaseizinger @mxinden