-
Notifications
You must be signed in to change notification settings - Fork 82
feat(l1): only use negotiated capabilities #3047
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
Conversation
…com:lambdaclass/ethrex into refactor/capability-struct-instead-of-tuple
…com:lambdaclass/ethrex into refactor/capability-struct-instead-of-tuple
…hrex into mecha/implement-protocols
crates/networking/p2p/rlpx/frame.rs
Outdated
message.encode(&mut frame_data)?; | ||
message.encode( | ||
&mut frame_data, | ||
&self.p2p_protocol, |
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.
why are we passing this here?
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.
The parameter p2p_protocol
(along with eth_protocol
and snap_protocol
) is used inside Message::encode
to encode the message header.
However the header could be encoded just before calling Message::encode
, which would allow us to remove the p2p_protocol
and snap_protocol
parameters, it could be something like this (we still need to pass eth_protocol
to encode the message according to the right eth
capability)
// Header encoding
(message.code() + message.offset(
&self.p2p_protocol,
&self.eth_protocol,
&self.snap_protocol)?
).encode(&mut frame_data);
message.encode(&mut frame_data, &self.eth_protocol)
What do you think? do you prefer doing the header encoding of Message::encode
?
Closing since this hasn't been worked on in some time. But we will use this PR as reference for when we work on this again. |
Motivation
We need a way to ensure what version of a protocol we're using when communicating with a peer.
Description
Important
After merging this PR, we can merge this other PR from the hive repository, which adds updates the devp2p simulator to include the latest test suite.
When the connection is established with the peer we agree in the p2p protocol version (as default n 5). This happens after the handshake is done.
Then we exchange the hello messages, where we coordinate with the peer which protocols we're using. Once we agree on them, we store them in the
Capabilities
struct (which is shared with theRLPxCodec
behind anArc<Mutex<>>
.The codec has the responsability to generate the messages to send the peers and decode the messages we receive. Now, the
p2p
,eth
andsnap
protocols agreed with the peers are stored in theCapabilities
struct. So when we need to encode/decode we know which version of the protocol has to be used.Note
TO DO:
The
Capabilities
struct is accessed from theRLPxCodec
in theencode
anddecode
functions.However the
encode
anddecode
functions are sync, while accessing theCapabilities
isasync
(since we need to lock the mutex withself.capabilities.lock().await
).The problem is that, even though the functions are sync, they are executed within an
async
context, so we cannot useblock_on
(we actually have tried that and the thread just crashes), so we need to find a way to fix this.The logic of the code offset has been improved to support differents version of the same protocol. For example, in the
eth/69
version a new message is added, so the offset of the snap suite changes depending of which version of the protocol we are using.To allow the messages from different versions to be more differentiated, the
Status
andReceipts
enums were removed and now the structsStatus68
,Status69
,Receipts68
andReceipts69
are used directly (each of them implementing theRLPxMessage
trait).Much of the code introduced in the PR #2860 was removed. Only the encode/decode of status and receipt was left. The file struct created has also been deleted.
Closes #3032