-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Make eth protocol handler stateful in network #13994
base: main
Are you sure you want to change the base?
Make eth protocol handler stateful in network #13994
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
before we make any significant changes to the existing session code I'd like to explore how this could look like.
we can look at the existing flow for this:
I new pending session is triggered when
a) incoming tcpstream
b) outgoing dial
this should eventually lead to a connection (Sink+Stream)
so we can design this similarly to:
reth/crates/net/network/src/protocol.rs
Lines 18 to 39 in bf20c78
/// A trait that allows to offer additional RLPx-based application-level protocols when establishing | |
/// a peer-to-peer connection. | |
pub trait ProtocolHandler: fmt::Debug + Send + Sync + 'static { | |
/// The type responsible for negotiating the protocol with the remote. | |
type ConnectionHandler: ConnectionHandler; | |
/// Invoked when a new incoming connection from the remote is requested | |
/// | |
/// If protocols for this outgoing should be announced to the remote, return a connection | |
/// handler. | |
fn on_incoming(&self, socket_addr: SocketAddr) -> Option<Self::ConnectionHandler>; | |
/// Invoked when a new outgoing connection to the remote is requested. | |
/// | |
/// If protocols for this outgoing should be announced to the remote, return a connection | |
/// handler. | |
fn on_outgoing( | |
&self, | |
socket_addr: SocketAddr, | |
peer_id: PeerId, | |
) -> Option<Self::ConnectionHandler>; | |
} |
e.g. EthProtocolHandler
with the idea that the SessionManager
stores an instance of this, like it does for:
reth/crates/net/network/src/session/mod.rs
Lines 110 to 111 in bf20c78
/// Additional `RLPx` sub-protocols to be used by the session manager. | |
extra_protocols: RlpxSubProtocols, |
the EthProtocolHandler's input could mimic what you already have for the PendingSession
so we can do:
- incoming tcpstream
- create a new
EthProtocolHandler::ConnectionHandler
handler viaon_incoming
- await the response which is the established connection, similar to:
reth/crates/net/network/src/protocol.rs
Lines 41 to 46 in bf20c78
/// A trait that allows to authenticate a protocol after the `RLPx` connection was established. pub trait ConnectionHandler: Send + Sync + 'static { /// The connection that yields messages to send to the remote. /// /// The connection will be closed when this stream resolves. type Connection: Stream<Item = BytesMut> + Send + 'static;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this likely could almost work
still need to think about type erasing the handler and whether we're missing anything but I think if we change the conn type this should work in theory
crates/net/network/src/config.rs
Outdated
/// The Ethereum protocol handler | ||
pub eth_protocol_handler: Box<dyn DynEthProtocolHandler<N>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for now we can keep the dyn hack, but I think we could even make this an associated type of NetworkPrimitives or a new trait that is also networkprims
peer_id: their_hello.id, | ||
capabilities: Arc::new(Capabilities::from(their_hello.capabilities)), | ||
status: Arc::new(their_status), | ||
conn, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the thing that's still problematic because this is still limited to the concrete connection type that operates on EthMessage
which I think we still want because we expect those variants, but conn here is of a concrete type so we either make this part of the ethhandler trait or we use a boxed stream as the type instead
Closes #13955