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

chore(gossipsub): add DEFAULT_MAX_TRANSMIT_SIZE const #5906

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

Conversation

diegomrsantos
Copy link
Contributor

@diegomrsantos diegomrsantos commented Mar 4, 2025

Description

Create DEFAULT_MAX_TRANSMIT_SIZE and use it in the code and docs. The main motivation is to fix the incorrect value mentioned in the docs and prevent it from happening again.

Notes & open questions

No

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

@diegomrsantos diegomrsantos changed the title feat(gossipsub): create DEFAULT_MAX_TRANSMIT_SIZE and use it in the code and docs feat(gossipsub): create DEFAULT_MAX_TRANSMIT_SIZE Mar 4, 2025
@diegomrsantos diegomrsantos changed the title feat(gossipsub): create DEFAULT_MAX_TRANSMIT_SIZE feat(gossipsub): create DEFAULT_MAX_TRANSMIT_SIZE const Mar 4, 2025
@diegomrsantos diegomrsantos changed the title feat(gossipsub): create DEFAULT_MAX_TRANSMIT_SIZE const refactor(gossipsub): create DEFAULT_MAX_TRANSMIT_SIZE const Mar 4, 2025
@jxs jxs changed the title refactor(gossipsub): create DEFAULT_MAX_TRANSMIT_SIZE const chore(gossipsub): add DEFAULT_MAX_TRANSMIT_SIZE const Mar 4, 2025
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 Diego, thanks for this! Left some comments

@@ -136,3 +136,5 @@ pub type Rpc = self::types::Rpc;

pub type IdentTopic = Topic<self::topic::IdentityHash>;
pub type Sha256Topic = Topic<self::topic::Sha256Hash>;

pub use crate::protocol::DEFAULT_MAX_TRANSMIT_SIZE;
Copy link
Member

Choose a reason for hiding this comment

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

do we need it to be pub?

@@ -5,6 +5,8 @@
- Improve `max_messages_per_rpc` consistency by ensuring RPC control messages also adhere to the existing limits.
See [PR 5826](https://github.com/libp2p/rust-libp2p/pull/5826)

- Create `DEFAULT_MAX_TRANSMIT_SIZE` and use it in the code and docs. The main motivation is to fix the incorrect value mentioned in the docs and prevent it from happening again. See [PR 5906](https://github.com/libp2p/rust-libp2p/pull/5906)
Copy link
Member

Choose a reason for hiding this comment

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

this is more like a chore, we don't need to add a CHANGELOG.md entry

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -61,6 +61,8 @@ pub(crate) const FLOODSUB_PROTOCOL: ProtocolId = ProtocolId {
kind: PeerKind::Floodsub,
};

pub const DEFAULT_MAX_TRANSMIT_SIZE: usize = 65536;
Copy link
Member

Choose a reason for hiding this comment

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

same as above, do we need this to be pub?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

and doesn't pub(crate) work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's what I had when it failed 1e72f14

Copy link
Contributor

mergify bot commented Mar 4, 2025

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

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.

2 participants