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 ping spec #473

Merged
merged 4 commits into from
Nov 15, 2022
Merged

Add ping spec #473

merged 4 commits into from
Nov 15, 2022

Conversation

MarcoPolo
Copy link
Contributor

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).

Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

🥳 A ping protocol specification.

Can you add an entry to the root README.md?

@Menduist
Copy link
Contributor

Menduist commented Nov 5, 2022

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
Copy link
Contributor Author

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
Copy link
Contributor

Menduist commented Nov 5, 2022

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)

peer. The server SHOULD accept at most 2 streams per peer since cross stream
behavior is not linearizable for client and server. In other words, the client
closing stream A and then opening stream B, might be perceived by the server as
the client opening stream B and then closing stream A.
Copy link
Contributor

@Menduist Menduist Nov 5, 2022

Choose a reason for hiding this comment

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

So for instance

The server SHOULD wait 200 milliseconds after a ping before accepting another ping a stream.
The client MUST wait 400 milliseconds after a reply to ping the server again on a stream.

(or make it global instead of per stream, idk)

200ms of margin to allow for jitter at the networking & scheduler level. This is less elegant than just one request per stream, but what can we do. If your ping suddently goes from 500s to 100ms (for instance if the cpu is clogged up), you will have an erroneous value for one ping.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why? This isn’t DOS mitigation. If you’re worried about rate limiting a 32byte response, then you probably want to do something at a higher level (rate limiting all streams and connections for example).

@MarcoPolo
Copy link
Contributor Author

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
Copy link
Contributor

Menduist commented Nov 5, 2022

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 :)

@MarcoPolo MarcoPolo requested a review from mxinden November 14, 2022 15:48
Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

Thank you Marco for picking this up!

@mxinden
Copy link
Member

mxinden commented Nov 15, 2022

Following up on the above discussion on explicit limits.

  • By intuition I would expect that no matter which limits we choose, we will choose the wrong ones. I.e. while a 100ms wait might be good in some scenarios, it might be too much or too little in others.
  • Instead of defining specific limits, I think implementations should mitigate the above attacks through prioritization. E.g. prioritize the ping protocol below other protocols. Spread prioritization evenly across connections, thus not allowing one to starve the other. ...

Co-authored-by: Max Inden <mail@max-inden.de>
Comment on lines +26 to +27
server SHOULD loop and echo the next payload. The client SHOULD close the write
side of the stream after sending the last payload, and the server SHOULD finish
Copy link
Member

@tomaka tomaka Dec 29, 2023

Choose a reason for hiding this comment

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

Why is it "SHOULD" rather than "MUST"?

If the server is allowed to not respond to pings (which is implied by the usage of "SHOULD"), then the protocol is pretty useless.

Of course, the client should be aware of the fact that the server can in practice do something else than this text says, but this is off-topic for a specification. The word "MUST" implicitly means "MUST ... in order to comply to this specification".
The possibility that you might be connected to a peer that doesn't conform to the specification doesn't need to be taken into account in the context of the specification itself, otherwise you've got a snake biting its tail.

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.

5 participants