-
Notifications
You must be signed in to change notification settings - Fork 519
network: disable pubsub PX for hybrid relays #6385
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
network: disable pubsub PX for hybrid relays #6385
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6385 +/- ##
==========================================
- Coverage 50.68% 50.50% -0.18%
==========================================
Files 661 654 -7
Lines 110682 110631 -51
==========================================
- Hits 56097 55877 -220
- Misses 51724 51887 +163
- Partials 2861 2867 +6 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR disables pubsub PX (peer exchange) and adds peer filtering functionality for hybrid relays to prevent P2P/pubsub communication with other public relays. The implementation introduces configurable pubsub options through a new network configuration system.
Key changes:
- Refactors pubsub initialization to accept flexible options instead of hardcoded parameters
- Introduces peer filtering capabilities to restrict pubsub communication based on peer roles
- Creates specialized mesh creator for hybrid relays with filtered pubsub configuration
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| network/p2pNetwork.go | Updates P2P network initialization to use configurable pubsub options from mesh creators |
| network/p2p/pubsub.go | Refactors makePubSub function to accept variadic pubsub options instead of single metrics tracer |
| network/p2p/peerstore/peerstore.go | Adds HasRole method and RoleChecker interface for peer role validation |
| network/p2p/p2p_test.go | Adds comprehensive tests for new pubsub option functions and mock implementations |
| network/p2p/p2p.go | Implements PubSubOption functions for disabling PX, setting tracers, and peer filtering |
| network/msgp_gen.go | Updates generated variable names (likely from code generation) |
| network/mesh.go | Extends mesh creators with makeConfig method and adds filtered pubsub creator |
| network/hybridNetwork.go | Updates hybrid network to use specialized mesh creators for different network types |
Comments suppressed due to low confidence (1)
network/p2p/p2p.go:244
- The pubsubCtx field is being removed but it's unclear if this context was being used elsewhere. Ensure that any code depending on this field has been properly updated to use the main context instead.
pubsub: ps,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like all the new interfaces, types and filters are wired up correctly, but it would be nice if there was an E2E test or a unit test that started up multiple hybridNetwork relay instances (perhaps with some mocking?) and them some hybridNetwork and/or p2pNetwork non-relay instances too, to show that this was working as expected, once all the role stuff is set?
gmalouf
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to me.
Summary
Disable pubsub PX and add PeerFilter to prevent hybrid relays to make p2p/pubsub communication to other public relays.
Test Plan
manual cluster test s1 - no relay to relay p2p conns.