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

Add information about doPX #410

Closed
wants to merge 1 commit into from
Closed

Conversation

TimDaub
Copy link

@TimDaub TimDaub commented Feb 25, 2023

@TimDaub TimDaub requested a review from a team as a code owner February 25, 2023 15:52
Copy link
Contributor

@maschad maschad left a comment

Choose a reason for hiding this comment

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

We actually mentioned in the Libp2p discovery mechanism docs where this is required - I apologize that the docs are bit dated 😓 .

But if you are using a bootstrap node with js-libp2p-gossipsub, you do not necessarily need to use a separate peer discovery mechanism, since the bootstrap node can be used to help your node discover and connect to other peers in the network. The doPx mechanism is to optimize the flow of information in the GossipSub network, by allowing peers to make informed decisions about which messages to forward and which peers to forward them to, so I wouldn't say this is required.

Copy link
Member

@wemeetagain wemeetagain left a comment

Choose a reason for hiding this comment

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

I don't think this is accurate. I do think there's a way to make the docs more helpful, but I'm not sure what exactly to say.

The "PX" feature doesn't enable full connectivity by itself and doesn't necessarily need pubsub-peer-discovery to work. It's just an optional mechanism in gossipsub to have well-connected nodes help poorly-connected nodes to maintain a healthy mesh (by sending them some peers to connect to when they get pruned).

The problem comes because px messages can optionally not include multiaddrs of possible mesh peers and only include peer ids of possible mesh peers (for backwards compatibility sake, since the "signed peer record" spec wasn't yet rolled out when gossipsub v1.1 was released). In that case, when receiving px messages, you'd need a peer discovery configured to hopefully link those possible mesh peers to multiaddrs to dial.

A few points though:

  • It's the receiver of the px messages (not necessarily the one who set doPx = true that may need peer discovery
  • The peer discovery that can or should be used isn't always pubsub-peer-discovery, it depends on the application
  • iirc js-libp2p and go-libp2p's implementations of the identify protocol should include signed peer records, so if that's enabled by all peers, then you shouldn't need additional peer discovery to dial possible mesh peers

@wemeetagain
Copy link
Member

closing this, see the above comment. maybe some libp2p-wide documentation on PX would be helpful to link here.

@wemeetagain wemeetagain closed this Aug 9, 2024
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.

Newly connecting nodes don't interconnect
3 participants