Add a PeriodicPingHandler and a PingListenHandler #574
Conversation
|
I added the possibility to be tolerant to the remote not supporting the ping protocol. |
protocols/ping/src/protocol.rs
Outdated
| Ok(Async::Ready(())) => return Ok(Async::Ready(None)), | ||
| Ok(Async::NotReady) => return Ok(Async::NotReady), | ||
| Err(err) => return Err(err), | ||
| } |
There was a problem hiding this comment.
How about replacing this match with:
try_ready!(self.inner.close());
return Ok(Async::Ready(None))
protocols/ping/src/protocol.rs
Outdated
| Ok(Async::Ready(None)) => { | ||
| // Notify the current task so that we poll again. | ||
| self.needs_close = true; | ||
| task::current().notify(); |
There was a problem hiding this comment.
Instead of going trough the task scheduler in order to close the socket, why not inline the logic here again:
try_ready!(self.inner.close());
return Ok(Async::Ready(None))
protocols/ping/src/protocol.rs
Outdated
| let payload: [u8; 32] = self.rng.sample(Standard); | ||
| debug!("Preparing for ping with payload {:?}", payload); | ||
| self.pings_to_send.push_back((Bytes::from(payload.to_vec()), user_data)); | ||
| task::current().notify(); |
There was a problem hiding this comment.
Maybe add a small comment stating that we wake up the task to have the PingDialer polled as soon as possible? And in addition maybe a Panics section to clarify that ping must better only be called in the context of an executing task?
There was a problem hiding this comment.
Oh. The whole thing is wrong though. I should remove this notification entirely, as it's the job of the user to poll after calling ping().
| /// The ping substreams that were opened by the remote. | ||
| /// Note that we only accept a certain number of substreams, after which we refuse new ones | ||
| /// to avoid being DDoSed. | ||
| ping_in_substreams: ArrayVec<[PingListener<TSubstream>; 8]>, |
There was a problem hiding this comment.
Is this the right place to mitigate DDOS attacks? Silently ignoring the > 8th pings may cause disruptions if the remotes consider this node unresponsive.
There was a problem hiding this comment.
The rational is that eight simultaneous pings is not a normal behaviour.
Our implementation doesn't do that, and the Go/JS neither.
There was a problem hiding this comment.
Actually, the ping handler is interacting with only a single node. So it is impossible to handle a DDOS attack in here. As far as a single malicious node is concerned, would the stream muxer not be the best place to limit the number of simultaneous streams rather than having each ProtocolsHandler be concerned with DOS attacks?
There was a problem hiding this comment.
I don't have a super strong opinion on that, but the problem to me is that differentiating between a DoS attack and a legitimate use can be a bit tricky on the muxer level.
There was a problem hiding this comment.
Yes, but the muxer probably limits the number of streams per connection anyway on a level the system can handle.
|
Ready for re-review. |
* Add ProtocolsHandler trait * Reexport symbols * Add a note about shutting down * Add a PeriodicPingHandler and a PingListenHandler * Fix core doctest * Add tolerating not supported * Fix concerns
…e-handled_node_tasks * upstream/master: Inject event by value in ProtocolsHandler (libp2p#605) Add a PeriodicPingHandler and a PingListenHandler (libp2p#574) Fix stack overflow when printing a SubstreamRef (libp2p#599) Add a peer id generator (libp2p#583) eg. -> e.g.; ie. -> i.e. via repren (libp2p#592)
…rs-to-vecdeque * upstream/master: Inject event by value in ProtocolsHandler (libp2p#605) Add a PeriodicPingHandler and a PingListenHandler (libp2p#574) Fix stack overflow when printing a SubstreamRef (libp2p#599) Add a peer id generator (libp2p#583) eg. -> e.g.; ie. -> i.e. via repren (libp2p#592)
…_swarm * upstream/master: Inject event by value in ProtocolsHandler (libp2p#605) Add a PeriodicPingHandler and a PingListenHandler (libp2p#574) Fix stack overflow when printing a SubstreamRef (libp2p#599) Add a peer id generator (libp2p#583) eg. -> e.g.; ie. -> i.e. via repren (libp2p#592)
…lection_stream * upstream/master: Use vecdeque for fair polling (libp2p#610) Add back the Swarm (libp2p#613) Remove dependency on tokio_current_thread (libp2p#606) Use chashmap from crates (libp2p#615) Fix grammar in core/nodes etc. (libp2p#608) Inject event by value in ProtocolsHandler (libp2p#605) Add a PeriodicPingHandler and a PingListenHandler (libp2p#574) Fix stack overflow when printing a SubstreamRef (libp2p#599) Add a peer id generator (libp2p#583) eg. -> e.g.; ie. -> i.e. via repren (libp2p#592) Add a IdentifyTransport (libp2p#569) Tests for HandledNode (libp2p#546) Some minor fixes reported by clippy (libp2p#600) Add a PeriodicIdentification protocol handler (libp2p#579) Add ProtocolsHandler trait (libp2p#573) Use paritytech/rust-secp256k1 (libp2p#598) Use websocket 0.21.0 (libp2p#597) Reexport multihash from the facade (libp2p#587) Add substrate to the list of projects using libp2p (libp2p#595) Remove spaces before semicolons (libp2p#591)
Based on #573
Renames
ping/src/lib.rstoping/src/protocol.rs, which is why the diff is so large.Adds an implementation of
ProtocolsHandlerthat handles incoming ping substreams, and an implementation ofProtocolsHandlerthat periodically pings the remote and disconnects us if it doesn't respond in time or doesn't support ping.