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

feat(gossipsub): remove fast_message_id_fn #4285

Merged
merged 8 commits into from
Oct 20, 2023
Merged

feat(gossipsub): remove fast_message_id_fn #4285

merged 8 commits into from
Oct 20, 2023

Conversation

StemCll
Copy link
Contributor

@StemCll StemCll commented Aug 2, 2023 •
edited
Loading

Description

Resolves #4138.

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

@StemCll StemCll changed the title WIP(gossipsub): remove fast_message_id_fn Chore(gossipsub): remove fast_message_id_fn Aug 2, 2023
@StemCll StemCll force-pushed the feat/gossipsub_remove_fast_message_id_fn branch from dad2f69 to a8365dd Compare August 2, 2023 11:58
@StemCll StemCll changed the title Chore(gossipsub): remove fast_message_id_fn chore(gossipsub): remove fast_message_id_fn Aug 2, 2023
@thomaseizinger thomaseizinger changed the title chore(gossipsub): remove fast_message_id_fn feat(gossipsub): remove fast_message_id_fn Aug 2, 2023
thomaseizinger
thomaseizinger reviewed Aug 2, 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! I've had a first look.

This will be a breaking change so we'll park it for now until we decide to do another round of breaking changes.

protocols/gossipsub/src/types.rs Show resolved Hide resolved
@thomaseizinger thomaseizinger added this to the v0.53.0 milestone Aug 2, 2023
thomaseizinger
thomaseizinger reviewed Aug 3, 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!

Can you write a changelog entry please and bump the minor version of libp2p-gossipsub?

@StemCll StemCll force-pushed the feat/gossipsub_remove_fast_message_id_fn branch from 1276cdc to a0e110d Compare August 5, 2023 15:44
@StemCll StemCll marked this pull request as ready for review August 5, 2023 16:10
@StemCll StemCll force-pushed the feat/gossipsub_remove_fast_message_id_fn branch from 70b971a to 014bcb9 Compare August 5, 2023 16:44
@thomaseizinger
Copy link
Contributor

Can you look into the CI failures? The version bumps seem to be faulty!

@StemCll StemCll force-pushed the feat/gossipsub_remove_fast_message_id_fn branch from 014bcb9 to 877d3f4 Compare August 8, 2023 20:36
@StemCll
Copy link
Contributor Author

StemCll commented Aug 8, 2023

Ah sorry... forgot about the main Cargo file

@mergify
Copy link
Contributor

mergify bot commented Aug 11, 2023

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

@StemCll StemCll force-pushed the feat/gossipsub_remove_fast_message_id_fn branch from 877d3f4 to d26a807 Compare August 12, 2023 13:29
@thomaseizinger
Copy link
Contributor

I am changing this to draft to make it clear that it should not be merged yet because it is a breaking change.

@thomaseizinger thomaseizinger marked this pull request as draft August 13, 2023 11:20
mxinden
mxinden reviewed Aug 16, 2023
View reviewed changes
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.

Thank you @StemCll for the work!

@AgeManning would you mind taking a look. Other then the comment below, this looks good to me.

protocols/gossipsub/src/behaviour.rs Show resolved Hide resolved
AgeManning
AgeManning previously approved these changes Aug 21, 2023
View reviewed changes
Copy link
Contributor

@AgeManning AgeManning left a comment

Choose a reason for hiding this comment

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

This looks good to me. Thanks for the help :)

protocols/gossipsub/src/behaviour.rs Show resolved Hide resolved
@StemCll StemCll force-pushed the feat/gossipsub_remove_fast_message_id_fn branch from d26a807 to 1ec34a4 Compare August 21, 2023 16:45
@StemCll StemCll mentioned this pull request Aug 21, 2023
Closed
@thomaseizinger
Copy link
Contributor

@StemCll Can you deal with the CI failures please?

@StemCll
Copy link
Contributor Author

StemCll commented Sep 21, 2023

Yes, I'll fix it tonight

@StemCll StemCll force-pushed the feat/gossipsub_remove_fast_message_id_fn branch from a6b7b0f to 066822d Compare September 21, 2023 15:02
@StemCll
Copy link
Contributor Author

StemCll commented Sep 21, 2023

@thomaseizinger there was a security issue with quinn-proto crate, thus it's updated with cargo update. The rest should be alright atm.

@StemCll StemCll marked this pull request as ready for review September 21, 2023 20:50
@thomaseizinger
Copy link
Contributor

@thomaseizinger there was a security issue with quinn-proto crate, thus it's updated with cargo update. The rest should be alright atm.

Thanks! We have a separate PR that fixes this up it shouldn't matter as the diff will cancel out :)

thomaseizinger
thomaseizinger previously approved these changes 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.

Thank you!

I am gonna leave this as draft until we actually merge breaking changes.

@thomaseizinger thomaseizinger marked this pull request as draft September 21, 2023 23:16
@mergify
Copy link
Contributor

mergify bot commented Sep 24, 2023

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

@StemCll StemCll force-pushed the feat/gossipsub_remove_fast_message_id_fn branch from 4e19ade to 1ad6e36 Compare October 2, 2023 17:49
@mergify
Copy link
Contributor

mergify bot commented Oct 15, 2023

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

@thomaseizinger thomaseizinger marked this pull request as ready for review October 20, 2023 05:45
@mergify mergify bot dismissed stale reviews from thomaseizinger and AgeManning October 20, 2023 05:45

Approvals have been dismissed because the PR was updated after the send-it label was applied.

@mergify mergify bot merged commit a044b88 into libp2p:master Oct 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mxinden mxinden mxinden left review comments

@thomaseizinger thomaseizinger thomaseizinger approved these changes

@AgeManning AgeManning AgeManning left review comments

Assignees
No one assigned
Labels
send-it
Projects
None yet
Milestone
v0.53.0
Development

Successfully merging this pull request may close these issues.

gossipsub: remove fast_message_id_fn
4 participants
@StemCll @thomaseizinger @mxinden @AgeManning