Peer Exchange protobuf improvements

[fryorcraken]

Optional Response/Query

https://github.com/vacp2p/waku/blob/6a89c52065f6e15ef8545f632b93ceda71bf7424/waku/peer_exchange/v2alpha1/peer_exchange.proto#L20

In PeerExchangeRPC, query and response should be optional

1. This aligns with other request/response protobuf: https://github.com/vacp2p/waku/blob/6a89c52065f6e15ef8545f632b93ceda71bf7424/waku/lightpush/v2beta1/lightpush.proto#L19 https://github.com/vacp2p/waku/blob/6a89c52065f6e15ef8545f632b93ceda71bf7424/waku/store/v2beta4/store.proto#L50
2. When doing a query, it is expected for the response to be undefined.

message PeerExchangeRPC {
  PeerExchangeQuery query = 1;
  PeerExchangeResponse response = 2;
}

Pascal Casing

Other messages uses Pascal Casing, I would recommend to do the same: PeerExchangeRpc

Request Id

Other messages uses a Request Id. More of an RFC change. Now that the nwaku implementation does not force a client to mount the protocol, it might not be needed.

Cc @alrevuelta @kaiserd @danisharora099

[kaiserd]

Thanks :). I added a milestone issue, consolidating tasks towards the new peer exchange design: vacp2p/research#188
I made this issue part of the task list.

(It is in icebox for now, because SeM focus is on secure scaling for now.)

[kaiserd]

Regarding the request ID:
It should not be necessary for the proposed req/resp version.

Regarding the numPeers field:
as discussed before, it should be moved to a uint32 both in the RFC as well as in the nwaku implementation.
We could also remove the PeerExchangeQuery and interpret the establishment of a px connection as the request.
numPeers could be d (the gossipsub out-degree).

[fryorcraken]

numPeers could be d (the gossipsub out-degree).

Seeing how many failure we are currently seeing, most likely we will want more than d. Maybe something related to the observed failure rate after we have some curation method in place (e.g. 50% failure -> 2*d).

We could also remove the PeerExchangeQuery and interpret the establishment of a px connection as the request.

I think this would re-introduce the previous fixed issue. Because in this case, the light client would need to mount the peer exchange protocol and then, for every connection, it would get unsolicited ENRs via peer exchange. I think it could open to a Sybil attack or we'll have to put in place mitigations such as "only accept N ENRs from a given node".

Using PeerExchangeQuery seems more KISS.

[kaiserd]

50% failure -> 2*d

Yes, we should test larger ds, but this is a trade-off to load on discv5.
(The trade-off to load could be traded for a trade-off to anonymity, reusing px peers more often).

Because in this case, the light client would need to mount the peer exchange protocol

I would not suggest to leave the connection open and wait for replies later.
It would be like a req/resp.
Establishing the connection would trigger the service node to reply once.
The light client should not have to mount the protocol.

for every connection, it would get unsolicited ENRs via peer exchange

By establishing the connection, the requesting node would solicit a single response from the service node and then close the connection.

Using PeerExchangeQuery seems more KISS.

Let's keep it that way :). Just a possibility to further simplify, if needed/desired.

[alrevuelta]

Seeing how many failure we are currently seeing, most likely we will want more than d. Maybe something related to the observed failure rate after we have some curation method in place (e.g. 50% failure -> 2*d).

Related: waku-org/nwaku#1521