libp2p / specs

Technical specifications for the libp2p networking stack
https://libp2p.io
1.56k stars 273 forks source link

Add ping spec #473

Closed MarcoPolo closed 1 year ago

MarcoPolo commented 1 year ago

Nothing new here, just documenting what {js,rust,go}-libp2p do. I checked with nim-libp2p as well, it doesn't have the server loop, but otherwise matches this description (cc @Menduist no action needed, just an friendly fyi).

Menduist commented 1 year ago

I think having no server loop & a 1 / 2 opened streams limit offers a natural protection to make sure there are no more than 32 bytes in flight at any given point (at least in the Server -> Client direction)

On the other hand, I see little point to reusing a stream multiple times (only saves a bit of traffic and RTTs on a non critical protocol)

MarcoPolo commented 1 year ago

On the other hand, I see little point to reusing a stream multiple times (only saves a bit of traffic and RTTs on a non critical protocol)

Maybe you want to get as close as you can to the true network RTT and avoid any cost around stream setup (I don’t think there is much cost with our existing stream muxers, but I can imagine a stream muxer that could have some setup cost per stream.)

Also, I’m not designing the protocol here. I’m simply writing down the existing behavior.

Menduist commented 1 year ago

Maybe you want to get as close as you can to the true network RTT and avoid any cost around stream setup

Yeah, but it's trivial not to take the stream setup into account (just count the time between write & read)

Also, I’m not designing the protocol here. I’m simply writing down the existing behavior.

Yes, and I guess now it's too late to change it for backward compatibility reasons (except if every implementation creates a new stream per request in the client, which I doubt)

(thank you for doing this btw!)

So maybe if we can't have this natural rate limiting, we should add one explicitly in the spec, and agree on a reasonable value. Today if this spec is implemented naively, I could DDoS anyone with asymmetric bandwidth quite easily (since he can receive more than he can send)

MarcoPolo commented 1 year ago

Today if this spec is implemented naively, I could DDoS anyone with asymmetric bandwidth quite easily (since he can receive more than he can send).

I’m not convinced by this argument. As an attacker, I pay 32 bytes to get you to send me 32 bytes. I may as well just tcp syn flood you.

again, this PR is only documenting existing behavior. But I appreciate the engagement :)

Menduist commented 1 year ago

I’m not convinced by this argument. As an attacker, I pay 32 bytes to get you to send me 32 bytes.

You may have symmetric fiber while I have ADSL. Sending 5mbit per second might be cheap for you, but saturate my upload bandwidth. Of course that's also the case with gossipsub (even worse since it adds duplication), but a bit sad to have this on a protocol as simple as ping

If you’re worried about rate limiting a 32byte response

The size of the response doesn't matter that much as long as I can stream them. If I can send you a 1gb blob and you send it back, doesn't matter if it's in 32 bytes pieces or one blob. (and AFAIK most implementations just "bridge" the in and out stream, not checking the 32 bytes)

then you probably want to do something at a higher level (rate limiting all streams and connections for example).

That higher level limits will apply backpressure, which will skew the ping measurements. Having a limit here makes sure that every implementation agrees on safe values that produce better measurements

again, this PR is only documenting existing behavior.

Yes, and this long overdue :) When can merge this and follow up in another PR, but imo, adding limits (of message size, bandwidth usage, number of opened streams, etc) is good habit that we should try to do in every spec (and to be fair, this spec already introduces message size and stream limit, which is better than most of the rest!)

Otherwise, implementations will each invent different limits, which may lead to subtle incompatibilities. So for instance, we'll switch to a server loop in nim-libp2p with limits like I talk about here. And next thing you know, some random dude starts to use the ping protocol in a 100ms loop to measure latencies on eth2, and we are tagged the laggiest one :)

mxinden commented 1 year ago

Following up on the above discussion on explicit limits.