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): implement gossipsub 1.2 beta #5697

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

Conversation

hopinheimer
Copy link

Description

This PR implements gossipsub 1.2 beta bringing changes over from lighthouse
ref PR: sigp/lighthouse#5422

Please include any relevant issues in here, for example:
libp2p/specs#548

@hopinheimer hopinheimer changed the title Implement gossipsub 1.2 beta feat(gossipsub): implement gossipsub 1.2 beta Nov 27, 2024
Copy link
Contributor

mergify bot commented Nov 28, 2024

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

@hopinheimer hopinheimer changed the title feat(gossipsub): implement gossipsub 1.2 beta feat(gossipsub): implement gossipsub 1.2 beta Nov 29, 2024
Copy link
Contributor

@elenaf9 elenaf9 left a comment

Choose a reason for hiding this comment

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

Thanks @hopinheimer!

Couple of comments.
I think the new protocol version also needs to be added to the ProtocolConfig?

protocols/gossipsub/src/rpc.rs Outdated Show resolved Hide resolved
protocols/gossipsub/src/gossip_promises.rs Outdated Show resolved Hide resolved
protocols/gossipsub/src/behaviour.rs Outdated Show resolved Hide resolved
protocols/gossipsub/src/behaviour.rs Outdated Show resolved Hide resolved
protocols/gossipsub/src/behaviour.rs Outdated Show resolved Hide resolved
protocols/gossipsub/src/behaviour.rs Outdated Show resolved Hide resolved
protocols/gossipsub/src/behaviour.rs Outdated Show resolved Hide resolved
protocols/gossipsub/src/metrics.rs Outdated Show resolved Hide resolved
@hopinheimer
Copy link
Author

hopinheimer commented Dec 1, 2024

Hi @elenaf9 thank you for the review, I have made all the requested changes although, there seem to be a test that's failing test_slow_peer_returns_failed_ihave_handling() I debugged and found the events were null. could you guide me to what might be the issue?

@elenaf9
Copy link
Contributor

elenaf9 commented Dec 2, 2024

Hi @elenaf9 thank you for the review, I have made all the requested changes although, there seem to be a test that's failing test_slow_peer_returns_failed_ihave_handling() I debugged and found the events were null. could you guide me to what might be the issue?

I think it has to do something with gossip_promises now not depending on peer_score being activated anymore. Thus, the gossipsub_promises logic is now also there even when peer_score hasn't been activated.

handle_ihave checks if the message id is already in the pending promises, and doesn't send another message to the peer if it is. Thus in the test the second call to gs.handle_ihave is a no-op. Given that the sender to the slow peer's connection handler has capacity of 2, it therefore won't be full yet and not report a SlowPeer event.
Before, this issue didn't come up I think because peer_score (and thus gossip_promises) isn't enabled it the test.

The test can be fixed by using a different MessageId in the second handle_ihave call.


I was actually wondering if it was on purpose that gossip_promises is now always active and doesn't depend on peer_score anymore. @jxs as the author of sigp/lighthouse#5422, could you briefly explain why this decision was made?

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