Proposal: Remove Peer Exchange in GossipSub Prune Message

[diegomrsantos]

Introduction

Peer Exchange in GossipSub may introduce more issues than it aims to solve. I propose that we consider removing it.

Current Benefits of Peer Exchange:

• Mesh Recovery: Helps pruned peers find alternative nodes to reestablish a mesh.

Concerns:

• Unverifiable Data: Peer Exchange is the only feature that introduces data that can't be validated in the protocol. A malicious agent could generate fake identities and inject them, leaving no way to verify this data.
• Message Size: We've observed prune messages with sizes up to 9KB and 210 peerIDs, and this size is primarily made up of Peer Exchange data. If signed records were populated, sizes could get substantially larger, raising both efficiency and potential DoS concerns.
• Privacy and Security Risks: Publicizes all connected peers in a global topic, making them easier targets for attacks.
• Suboptimal or Malicious Peer Suggestions: Possible that a malicious actor could flood the network with bad peer suggestions, reducing network quality.
• Limited Applicability for Peer Discovery: Peer Exchange may aid in mesh recovery for pruned peers, but it does not replace the need for a dedicated peer discovery mechanism crucial for forming the initial mesh. As signed peer records containing the node's dialable addresses are currently optional, this feature is even less useful. Finally, waiting to be pruned to receive a list of peers is a slow, unpredictable, and unreliable strategy for bootstrapping a network from a single or small set of peers.

Proposal
I propose that we reconsider the existence of Peer Exchange in GossipSub to reduce potential vulnerabilities and to keep the protocol streamlined and focused on its primary goals.

[vyzo]

It is impossible to bootstrap from a star without PX unfortunately.

Also note that it is clearly stated that this should be limited to trusted entities, like bootstrappers.

[vyzo]

Also note that signed peer records are a MUST.

[vyzo]

Another point I would like to make: PX really is optional and ignorable; it is not even enabled by default. So the risks you suggest are simply not there, unless your systems programmer shoots himself in the foot.

So to summarize my opinion on the matter: not only the risks suggested are not really there, but removing it is going to remove a very important capability from the protocol: the ability to bootstrap from a star.

So here it is: I strongly oppose this motion to remove PX.

[diegomrsantos]

It is impossible to bootstrap from a star without PX unfortunately.

Also note that it is clearly stated that this should be limited to trusted entities, like bootstrappers.

Thank you for your input; it's always great to have diverse perspectives on these matters. A couple of points for clarification and discussion:

Bootstrapping from a Star: I understand your point that Peer Exchange might assist in moving from a star to a mesh topology. However, it's worth noting that Peer Exchange comes into play when a peer is pruned. For pruning to happen, a mesh needs to form in the first place, which inherently requires some form of peer discovery. So, while Peer Exchange might help in recovering or optimizing a mesh, I'm not convinced it can fully substitute for initial peer discovery. It seems to me that the protocol was designed from the outset to rely on ambient peer discovery. Adding Peer Exchange doesn't necessarily alter that fundamental assumption.

Trusted Entities: You mentioned that Peer Exchange should be limited to trusted entities like bootstrappers. I couldn't find this specification in the GossipSub document. Could you point me to where this is stated? Clarifying this could certainly help assess the risk factors associated with Peer Exchange more accurately.

Looking forward to your thoughts on these points!

[diegomrsantos]

Also note that signed peer records are a MUST.

I looked through the GossipSub specification and noticed that in the protobuf message, signedPeerRecord is actually marked as optional. This would suggest that while using signed peer records could enhance the security and integrity of Peer Exchange, it's not strictly a requirement according to the spec. Could you clarify where it's stated that signed peer records are a 'MUST'?

[vyzo]

Also note that signed peer records are a MUST.

I looked through the GossipSub specification and noticed that in the protobuf message, signedPeerRecord is actually marked as optional. This would suggest that while using signed peer records could enhance the security and integrity of Peer Exchange, it's not strictly a requirement according to the spec. Could you clarify where it's stated that signed peer records are a 'MUST'?

It is marked as optional in the protobuf, because the PRUNE doesn't have to carry it.
It is very much the only way to pass peer records, and the implementation only accepts signed peer records.
Perhaps the spec could be cleaner on this, I will be happy to review a pr improving this.

[vyzo]

Trusted Entities: You mentioned that Peer Exchange should be limited to trusted entities like bootstrappers. I couldn't find this specification in the GossipSub document. Could you point me to where this is stated? Clarifying this could certainly help assess the risk factors associated with Peer Exchange more accurately.

We do that by giving them a high application level score. Again, perhaps the spec is not clear enough on this and can be improved; happy to review a pr.

[vyzo]

Bootstrapping from a Star: I understand your point that Peer Exchange might assist in moving from a star to a mesh topology. However, it's worth noting that Peer Exchange comes into play when a peer is pruned. For pruning to happen, a mesh needs to form in the first place, which inherently requires some form of peer discovery. So, while Peer Exchange might help in recovering or optimizing a mesh, I'm not convinced it can fully substitute for initial peer discovery. It seems to me that the protocol was designed from the outset to rely on ambient peer discovery. Adding Peer Exchange doesn't necessarily alter that fundamental assumption.

There are pragmatics involved here. If you really care about your bootstrap security (if you are building a blockchain you probably should), then you will have your own bootstrap mechanism and don't need PX -- it can be an aid or be turned off completely.

However, not all applications built on top of gossipsub are blockchains or have similar requirements. For them, it might simply be not possible (or too expensive or they just don't particularly care) to setup a separate bootstrap infrastructure and the protocol would be "broken" for them. So we provide this mechanism to bootstrap easily and build a mesh.

[diegomrsantos]

It is marked as optional in the protobuf, because the PRUNE doesn't have to carry it.
It is very much the only way to pass peer records, and the implementation only accepts signed peer records.
Perhaps the spec could be cleaner on this, I will be happy to review a pr improving this.

I understand that they are always signed when included. However, I'm still a bit uncertain about the utility of a PRUNE message containing only peerID (as we have observed in one implementation) and not the associated signedPeerRecord. In such a scenario, without a signedPeerRecord, wouldn't the pruned peer need to rely on an ambient peer discovery mechanism to establish a connection? The spec seems to suggest as much. This somewhat limits the direct utility of the peer exchange if only a peerID is shared. It might be helpful if we can clarify the specific scenarios where only the peerID would be adequate.

[vyzo]

It is just pragmatics if protobuf; the signed peet record is required when the peer id is present.

…

On Mon, Sep 11, 2023, 3:48 PM diegomrsantos ***@***.***> wrote: It is marked as optional in the protobuf, because the PRUNE doesn't have to carry it. It is very much the only way to pass peer records, and the implementation only accepts signed peer records. Perhaps the spec could be cleaner on this, I will be happy to review a pr improving this. I understand that they are always signed when included. However, I'm still a bit uncertain about the utility of a PRUNE message containing only peerID and not the associated signedPeerRecord. In such a scenario, without a signedPeerRecord, wouldn't the pruned peer need to rely on an ambient peer discovery mechanism to establish a connection? The spec seems to suggest as much. This somewhat limits the direct utility of the peer exchange if only a peerID is shared. It might be helpful if we can clarify the specific scenarios where only the peerID would be adequate. — Reply to this email directly, view it on GitHub <#570 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAI4SXCTNB2W5BAYFRJNW3XZ4CBJANCNFSM6AAAAAA4HYUR74> . You are receiving this because you commented.Message ID: ***@***.***>

[diegomrsantos]

Thanks for the clarification. Given the current structure:

message ControlPrune {
    optional string topicID = 1;
    repeated PeerInfo peers = 2; // gossipsub v1.1 PX
    optional uint64 backoff = 3; // gossipsub v1.1 backoff time (in seconds)
}

message PeerInfo {
    optional bytes peerID = 1;
    optional bytes signedPeerRecord = 2;
}

It might be clearer if both peerID and signedPeerRecord are set as required within the PeerInfo structure when peers contains any entry. Doing so could bring more clarity and ensure that every PeerInfo is complete. What are your thoughts on this?"

[vyzo]

I would prefer to avoid doing that for protobuf compatibility versions; protobuf3 has no required fields and we want to port eventually.

[PedrobyJoao]

However, it's worth noting that Peer Exchange comes into play when a peer is pruned. For pruning to happen, a mesh needs to form in the first place, which inherently requires some form of peer discovery. So, while Peer Exchange might help in recovering or optimizing a mesh, I'm not convinced it can fully substitute for initial peer discovery. It seems to me that the protocol was designed from the outset to rely on ambient peer discovery. Adding Peer Exchange doesn't necessarily alter that fundamental assumption.

I think that is solved setting D=D_lo=D_hi=D_out=0 for bootstrap peers (as recommended by the gossipsub v1.1. specs), then bootstrap peers will be always pruning with normal peers. I tested PX with a star topology and it really worked without any additional discovery mechanisms, check: https://github.com/PedrobyJoao/testing_gossipsubPX

Regarding the signedPeerRecords, mostly agreed. PX is not so useful without them, and they're not shared by default through the identify family of protocols.