Improve P2PStream buffering

paradigmxyz / reth

Modular, contributor-friendly and blazing-fast implementation of the Ethereum protocol, in Rust

https://reth.rs/

Apache License 2.0

3.99k stars 1.22k forks source link

Improve P2PStream buffering #12202

Open mattsse opened 3 weeks ago

mattsse commented 3 weeks ago

Describe the feature

we currently have a buffer:

https://github.com/paradigmxyz/reth/blob/ff9a42ae8fbf441f4fb9ca8dcea84c765b284721/crates/net/eth-wire/src/p2pstream.rs#L244-L245

TODO

remove buffer
update how pings are processed, because if we now go through the Sink API, then this must be stored as options that we optionally need to keep around
if we modify the poll logic in p2pstream then we also need to manually add a flush call at the callsite https://github.com/paradigmxyz/reth/blob/ff9a42ae8fbf441f4fb9ca8dcea84c765b284721/crates/net/network/src/session/active.rs#L569-L569

also ptal at the Sink docs if unfamiliar with the Sink API

Additional context

No response

MatteoMer commented 2 weeks ago

hey! Would love to work on this 🫡

jenpaff commented 2 weeks ago

hey! Would love to work on this 🫡

assigned !

MatteoMer commented 2 weeks ago

I made a draft for the issue #12417

But @mattsse I've still got few questions 😄

In my PR I only managed the case where there's one ping that needs to be saved (waiting to be sent). Is there any real case scenario where multiple pings would need to be saved? For example a ping and pong? Maybe it should be an array of saved pings?
It was not described in the PR description, but I also needed to handle disconnect, but unfortunately, I don't think I managed to do it 1/1 to the original version. Since we don't have access to the buffer anymore, we cannot clear the messages that are waiting to be sent. Maybe it's already handled by the inner Sink? Right now I'm just sending a disconnect message, and in poll_flush, treating it as a priority. But let me know what's the best way 🫡

mattsse commented 2 weeks ago

In my PR I only managed the case where there's one ping that needs to be saved (waiting to be sent). Is there any real case scenario where multiple pings would need to be saved?

we can ignore this case and only consider one pending ping/pong at a time

Right now I'm just sending a disconnect message, and in poll_flush, treating it as a priority.

that makes sense and fits how flush is supposed to work