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

feat(network): add rpc messages and codec #460

Closed
wants to merge 1 commit into from
Closed

Conversation

dancoombs
Copy link
Collaborator

Starts #248

Proposed Changes

  • Starts the rundler-network crate
  • Defines all of the request/response methods from the spec
  • Defines all of the supported libp2p protocols.
    • Note: some of this is dead code at the moment, but will be used in an upcoming PR. Made sense to combine the definitions in this PR.
  • Implements Tokio Decoder and Encoder traits for both Inbound (requests from peers) and Outbound (requests too peers) directions.

Notes:

  • Some of the TODOs left in the code are the result of ambiguous or missing wording in the spec. I'll be following up with fixes once discussed with the community.
  • There are also some TODOs around the structure of the RPC messages that should be worked out.

Next up:

  • Use these messages to create the RPC libp2p behavior.
  • Peer discovery and management

@codecov
Copy link

codecov bot commented Oct 10, 2023

Codecov Report

Merging #460 (c43c845) into main (440c096) will increase coverage by 2.31%.
The diff coverage is 89.61%.

Impacted file tree graph

Files Coverage Δ
crates/network/src/rpc/handler/snappy.rs 96.80% <96.80%> (ø)
crates/network/src/rpc/handler/codec.rs 94.92% <94.92%> (ø)
crates/network/src/rpc/handler/serde.rs 95.64% <95.64%> (ø)
crates/network/src/rpc/message.rs 56.60% <56.60%> (ø)
crates/network/src/rpc/protocol.rs 46.15% <46.15%> (ø)
Flag Coverage Δ
unit-tests 54.51% <89.61%> (+2.31%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
rundler binary 0.00% <ø> (ø)
builder 45.04% <ø> (ø)
dev 0.00% <ø> (ø)
pool 59.54% <ø> (ø)
provider 1.33% <ø> (ø)
rpc 18.26% <ø> (ø)
sim 82.61% <ø> (ø)
tasks ∅ <ø> (∅)
types 93.56% <ø> (ø)
utils 7.20% <ø> (ø)
network 89.61% <89.61%> (∅)

ProtocolSchema::GoodbyeV1 => "goodbye/1",
ProtocolSchema::PingV1 => "ping/1",
ProtocolSchema::MetadataV1 => "metadata/1",
ProtocolSchema::PooledUserOpHashesV1 => "pooled_user_op_hashes/1",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might be an inconsistency in the spec, but the protocol id in the spec adds an "s" and calls it pooled_user_ops_hashes

https://github.com/eth-infinitism/bundler-spec/blob/main/p2p-specs/p2p-interface.md#pooleduserophashes

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good eye, I have this on my list of changes to request to make in the spec. The protocol name should match the message name, which doesn't have the "s".

pub(crate) struct Metadata {
/// Metadata sequence number
pub(crate) seq_number: u64,
// TODO: spec has an invalid field here, add if needed
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this referring to mempool_nets? Why is it invalid?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is another thing I wanted to discuss with the spec writers, I don't believe the mempool nets field is valid as defined. There currently is no definition for a "mempool id gossip subnets". We use the gossip topic to signify different domains for mempool ids.

Copy link
Collaborator

@0xfourzerofour 0xfourzerofour left a comment

Choose a reason for hiding this comment

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

looks good from what I can see!


#[test]
fn test_encode_decode() {
let mut codec = LengthPrefixedSnappyCodec::new(0, 1024);
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: reuse of the same codec definition

ProtocolSchema::StatusV1 => "status/1",
ProtocolSchema::GoodbyeV1 => "goodbye/1",
ProtocolSchema::PingV1 => "ping/1",
ProtocolSchema::MetadataV1 => "metadata/1",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know the /1 endpoints are in the spec but I hate it haha. I like the /v1/method endpoint style. Just a rant, don't mind me

@dancoombs dancoombs changed the title feat(p2p): add rpc messages and codec feat(network): add rpc messages and codec Oct 17, 2023
@github-actions github-actions bot added the stale label Nov 16, 2023
@github-actions github-actions bot closed this Nov 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants