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

fix(gossipsub): Attempt to publish to at least mesh_n peers #5578

Merged
merged 6 commits into from
Sep 3, 2024

Conversation

jxs
Copy link
Member

@jxs jxs commented Aug 29, 2024

Description

With flood published disabled we've noticed that it can be the case that we have connected peers on topics but these peers are not in our mesh (perhaps due to their own mesh requirements). Currently, we fail to publish the message if there are no peers in our mesh.

This PR adjusts this logic to always attempt to publish to at least mesh_n peers. If we have peers that are subscribed to a topic, we will now attempt to publish messages to them (provided they have the required score).

This PR also simplies the peer and respective topics by moving the topic list each peer has subscribed to PeerConnections and removing both peer_topics and topic_peers from the main Behaviour.
Per commit review is suggested.

Copy link
Contributor

@ackintosh ackintosh left a comment

Choose a reason for hiding this comment

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

Looks good to me! I left a small comment. 🙂

protocols/gossipsub/src/behaviour.rs Outdated Show resolved Hide resolved
@jxs jxs changed the title fix(gossipsub): Attempt to publish to at least mesh_n peers, when flood publish is disabled fix(gossipsub): Attempt to publish to at least mesh_n peers Sep 2, 2024
protocols/gossipsub/CHANGELOG.md Outdated Show resolved Hide resolved
protocols/gossipsub/CHANGELOG.md Outdated Show resolved Hide resolved
@jxs jxs added send-it trivial Marks PRs which are considered trivial and don't need approval from another maintainer. labels Sep 3, 2024
@mergify mergify bot merged commit 93169cc into libp2p:master Sep 3, 2024
72 checks passed
TimTinkers pushed a commit to unattended-backpack/rust-libp2p that referenced this pull request Sep 14, 2024
)

## Description

With flood published disabled we've noticed that it can be the case that
we have connected peers on topics but these peers are not in our mesh
(perhaps due to their own mesh requirements). Currently, we fail to
publish the message if there are no peers in our mesh.

This PR adjusts this logic to always attempt to publish to at least
mesh_n peers. If we have peers that are subscribed to a topic, we will
now attempt to publish messages to them (provided they have the required
score).

This PR also simplies the peer and respective topics by moving the topic
list each peer has subscribed to `PeerConnections` and removing both
`peer_topics` and `topic_peers` from the main `Behaviour`.
Per commit review is suggested.

---------

Co-authored-by: Darius Clark <[email protected]>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
send-it trivial Marks PRs which are considered trivial and don't need approval from another maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants