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(gossipsub): apply max_transmit_size to the published message #5642

Merged
merged 4 commits into from
Oct 25, 2024

Conversation

kalabukdima
Copy link
Contributor

Description

When trying to publish a message using gossipsub's publish method,
it should be possible to predict whether it will fit in the limit defined by
the max_transmit_size config option.
If this limit applies to the final protobuf payload, it's not possible to know
that in advance because the size of the added fields is not fixed.

This change makes the limit apply to the passed message size instead of the final wire size.

Notes & open questions

This is a minor version change because it changes the meaning of the existing config option.
However, for the existing clients the limit will only become more permissive, so it shouldn't break anything.

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

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.

LGTM, but we could probably have this be a patch bump instead of a minor since this could be treated more as a fix. Thoughts?

protocols/gossipsub/CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Darius Clark <dariusc93@users.noreply.github.com>
@dariusc93 dariusc93 changed the title feat(gossipsub): apply max_transmit_size to the published message itself feat(gossipsub): apply max_transmit_size to the published message Oct 22, 2024
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.

LGTM! thanks Dzmitry! @dariusc93 no worries, we also have #5595 on the pipeline, so it's allright the CHANGELOG like this

protocols/gossipsub/src/behaviour.rs Outdated Show resolved Hide resolved
@jxs jxs added the send-it label Oct 25, 2024
@mergify mergify bot merged commit c7e8129 into libp2p:master Oct 25, 2024
68 of 69 checks passed
@kalabukdima kalabukdima deleted the gossipsub-msg-limit branch October 30, 2024 17:44
sdbondi added a commit to sdbondi/rust-libp2p that referenced this pull request Nov 1, 2024
* master: (36 commits)
  chore: refactor ping tests (libp2p#5655)
  feat: refactor distributed-key-value-store example (libp2p#5652)
  chore(ci): address clippy beta lints (libp2p#5649)
  feat(gossipsub): apply `max_transmit_size` to the published message (libp2p#5642)
  feat(kad): add `Behavior::find_closest_local_peers()` (libp2p#5645)
  fix(swarm-test): set proper version (libp2p#5648)
  deps(ci): update cargo-semver-checks (libp2p#5647)
  chore: fix typo in comment (libp2p#5643)
  feat: make runtime features optional in swarm-test (libp2p#5551)
  deps: bump Swatinem/rust-cache from 2.7.3 to 2.7.5 (libp2p#5633)
  chore: update igd-next to 0.15.1 (libp2p#5625)
  fix(server): removing dependency on libp2p-lookup (libp2p#5610)
  refactor(examples): use tokio instead of async-std in relay-server (libp2p#5600)
  deps: update metrics example dependencies (libp2p#5617)
  chore: update interop test run condition (libp2p#5611)
  chore(autonat-v2): fix dial_back_to_non_libp2p test (libp2p#5621)
  fix(swarm): don't report `NewExternalAddrCandidate` if already confirmed (libp2p#5582)
  chore(ci): address beta clippy lints (libp2p#5606)
  fix(ci): address cargo-deny advisories (libp2p#5596)
  chore(ci): only run interop tests on commits to master (libp2p#5604)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants