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

new version of the stream handler that uses a compact representation per provide #871

Open
Jorropo opened this issue Aug 23, 2023 · 5 comments

Comments

@Jorropo
Copy link
Contributor

Jorropo commented Aug 23, 2023

The current protobuf definition is backward, it allow to define multiple peer id per key and multiple addresses per peer id.
We also enforce that in the server handler.

This means that for 1 announcement we may send a few times more data just in multiaddr. Repeated over millions of CID this is a very wastefull use of resources.

We should create a new version of the stream handler which does not enforce that, instead of maddrs would be magically exchanged OOB (libp2p identify). This will help because it is not hard to have CIDs you host * K / total dht servers to be multiple orders of magnitude over 1, so the protocol should orient itself to batching CIDs or multihashes.
We could even have dedicated stream handler for ADD_PROVIDE and drop protobuf, instead it would be a pure stream of multihashes but this is maybe overkill.

@guillaumemichel
Copy link
Contributor

Yes it makes sense! We don't need to include addresses in the message, because they are already known by the remote peer. Concerning the peer id, we don't need to include it in the message as long as it isn't allowed to advertise provider records for other peers than oneself. Allowing delegated provides is anyway a protocol change, no matter the wire format, because it is blocked by the servers.


I like the idea of having the following protobuf format for the put message:

Method: PUT_PROVIDER
Keys: CID, CID, CID, ...

Both the Reprovide Sweep and fullRT should be able to benefit from it, rather than sending PUT requests back to back.

We could use a new protocol ID (e.g 1.1.0 or 2.0.0) in order to start benefiting from this new format. If we decide to use a new protocol, we should also identify other wire changes (and light protocol changes) that we may want to add (such as mandatory signed peer records). This discussion should certainly continue at libp2p/specs it is about protocol changes.

@2color
Copy link
Contributor

2color commented Sep 3, 2024

The current protobuf definition is backward, it allow to define multiple peer id per key and multiple addresses per peer id.
We also enforce that in the server handler.

Do you mean that you should be able to do an ADD_PROVIDER RPC call without passing your PeerID and maddrs since those can be inferred from the connection?
And that this leads to inefficiencies since you're sending multiple maddrs which are several times the size of just a multihash?

@guillaumemichel
Copy link
Contributor

I think the point was that instead of 1 advertisement (PUT_PROVIDER message) containing multiple PeerIDs/addresses, it should instead contain multiple CIDs.

Having multiple PeerIDs/addresses doesn't make sense atm, because one can only provide for oneself, and the addresses are already known to the DHT server. Providing multiple CIDs takes multiple advertisements, which isn't a good use of the wire capacity (duplicate PeerID/addresses).

@2color
Copy link
Contributor

2color commented Sep 3, 2024

Having multiple PeerIDs/addresses doesn't make sense atm

Does that ever happen though? Or is that just something that the protobuf allows but is ruled out in the code?

@guillaumemichel
Copy link
Contributor

It is something that the protobuf allows, and not used in the code

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

No branches or pull requests

3 participants