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): implement message sizes per topic. #5868

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open

feat(gossipsub): implement message sizes per topic. #5868

wants to merge 17 commits into from
+834 −79

Conversation

elijahhampton
Copy link
Contributor

@elijahhampton elijahhampton commented Feb 18, 2025 •
edited
Loading

Description

Fixes: #5745

Notes & open questions

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

@elijahhampton
Implements message sizes/mesh params per topic
fcab4e9
@elijahhampton
Adds documentation to TopicConfigs and TopicMeshConfig structs. Fixes…
8df6a17 
… warnings from clippy, check and fmt. Modifies test cases to use TopicMeshConfig::default() in place of explicit default values
@elijahhampton
Copy link
Contributor Author

@AgeManning I opened this PR in regards to #5745. It still needs some work to ensure consistency and correctness, although, I want to get it in front of the appropriate parties to ensure the PR is in the right direction in regards to the objective.

@dariusc93 dariusc93 requested review from jxs and AgeManning February 18, 2025 05:01
dariusc93
dariusc93 reviewed Feb 18, 2025
View reviewed changes
Copy link
Member

@dariusc93 dariusc93 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 the PR. I did leave a comment but i wonder if we should in a way assign the size to the topic after the behavior is constructed (eg assigning it to TopicHash or via Behaviour::subscribe or through some means) vs providing it in the config, which i believe the former is meant to be intended, though I could be wrong.

AgeManning
AgeManning reviewed Feb 19, 2025
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 imo.

Do we need to clone the config everytime we read it?

Also, I think it would be nice to maintain backwards compatibility. i.e, if users are already using mesh_n() then it works as before, I think it would be nice to add additional config, like mesh_n_topic() which changes just that topic, all others revert to the default or if mesh_n() etc was set.

Nice work.

@elijahhampton
Copy link
Contributor Author

This looks good imo.

Do we need to clone the config everytime we read it?

Also, I think it would be nice to maintain backwards compatibility. i.e, if users are already using mesh_n() then it works as before, I think it would be nice to add additional config, like mesh_n_topic() which changes just that topic, all others revert to the default or if mesh_n() etc was set.

Nice work.

This looks good imo.

Do we need to clone the config everytime we read it?

Also, I think it would be nice to maintain backwards compatibility. i.e, if users are already using mesh_n() then it works as before, I think it would be nice to add additional config, like mesh_n_topic() which changes just that topic, all others revert to the default or if mesh_n() etc was set.

Nice work.

You're right. I will look into removing the unnecessary cloning and the backwards compatibility makes sense. I will try to get this wrapped up this weekend between tomorrow and Sat.

@elijahhampton elijahhampton changed the title Draft: Feat(5745) implement message sizes per topic Feat(5745) implement message sizes per topic Feb 23, 2025
@elijahhampton elijahhampton changed the title Feat(5745) implement message sizes per topic feat(libp2p): implement message sizes per topic. Feb 27, 2025
@jxs jxs changed the title feat(libp2p): implement message sizes per topic. feat(gossipsub): implement message sizes per topic. Feb 27, 2025
@mergify Mergify
Copy link
Contributor

mergify bot commented Feb 27, 2025

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

@elijahhampton
Merge branch 'master' of https://github.com/elijahhampton/rust-libp2p …
091d792 
…into feat(5745)-implement-message-sizes-per-topic
@elijahhampton
Merge branch 'master' into feat(5745)-implement-message-sizes-per-topic
9ca2ab2
@elijahhampton
Adds CHANGELOG entry, removes added ValidationError variant for exist…
1cd6a4d 
…ing TransformFailed variant and fixes warnings from cargo fmt
@elijahhampton
Merge branch 'feat(5745)-implement-message-sizes-per-topic' of https:…
bf23963 
…//github.com/elijahhampton/rust-libp2p into feat(5745)-implement-message-sizes-per-topic
@elijahhampton
Fixes cargo fmt errors under nightly build
173f4e1
@elijahhampton
Merge branch 'master' into feat(5745)-implement-message-sizes-per-topic
b99f9da
@elijahhampton
Resolve merge conflicts with CHANGELOG
596f5db
@elijahhampton
Merge branch 'feat(5745)-implement-message-sizes-per-topic' of https:…
5ffa9ea 
…//github.com/elijahhampton/rust-libp2p into feat(5745)-implement-message-sizes-per-topic
@AgeManning
Copy link
Contributor

Is this ready for a final review?

AgeManning
AgeManning reviewed Mar 17, 2025
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.

Nice! Just left a few tiny comments.

@jxs - I've looked over this, and it seems good to me, but because we touch some sensitive areas, probably worth another set of eyes.

@mergify Mergify
Copy link
Contributor

mergify bot commented Mar 17, 2025

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

jxs
jxs reviewed Mar 18, 2025
View reviewed changes
Copy link
Member

@jxs jxs left a comment

Choose a reason for hiding this comment

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

Hi, and thanks for this!
Left some comments

jxs
jxs reviewed Mar 20, 2025
View reviewed changes
@elijahhampton
Reverts ipfs private example to use default max_transmit_size. Revert…
6a99325 
…s CHANGELOG entry in libp2p. Derives Debug for TopicMeshConfig, adds MessageTooLarge variant to PublishError enum, preserves function order and adds comments in behaviour.rs, removes config from ProtocolConfig.
@elijahhampton
Restores unit test and adds additional unit test to cover config spec…
7bfc18c 
…ific params.
@elijahhampton
Merge branch 'master' of https://github.com/elijahhampton/rust-libp2p …
1a97fd6 
…into feat(5745)-implement-message-sizes-per-topic
@elijahhampton
Cargo fmt resolution
2888abc
@elijahhampton elijahhampton requested review from AgeManning and jxs March 21, 2025 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dariusc93 dariusc93 Awaiting requested review from dariusc93

@AgeManning AgeManning Awaiting requested review from AgeManning

@jxs jxs Awaiting requested review from jxs

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Gossipsub - Implement message sizes per topic
4 participants
@elijahhampton @AgeManning @jxs @dariusc93