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(l1): initiate rlpx connections from discv4 #936

Merged
merged 23 commits into from
Oct 23, 2024

Conversation

ElFantasma
Copy link
Contributor

Motivation

DiscV4 protocol discovers new peers, and after deciding it is a valid peer, a TCP connection is established and RLPx protocol starts

Description

Now, when receiving a DiscV4 Pong message and after evaluating the peer, a RLPx connection is created and established. If handshake fails it removes the peer from the Kademlia table.

Closes #837.

Also removed some hard-coded testing code.

Once the listen loop is built (#840) there may be other conditions where a peer has to be discarded. (eg. when the other peer sends a Disconnect message). After #840 is completed we may create some more issues on this regard.

@ElFantasma ElFantasma requested a review from a team as a code owner October 22, 2024 15:29
@ElFantasma ElFantasma linked an issue Oct 22, 2024 that may be closed by this pull request
Copy link
Contributor

@fkrause98 fkrause98 left a comment

Choose a reason for hiding this comment

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

Left some comments, is there a proper way I can test this yet?

crates/networking/p2p/net.rs Outdated Show resolved Hide resolved
crates/networking/p2p/net.rs Show resolved Hide resolved
crates/networking/p2p/net.rs Show resolved Hide resolved
crates/networking/p2p/rlpx/connection.rs Show resolved Hide resolved
maximopalopoli and others added 15 commits October 23, 2024 15:47
**Motivation**

Change lint flag to only cover levm crate.

**Description**

This is done by removing `--workspace` flag when calling cargo clippy.

<!-- Link to issues: Resolves #111, Resolves #222 -->

Closes #876
**Motivation**

<!-- Why does this pull request exist? What are its goals? -->

**Description**

<!-- A clear and concise general description of the changes this PR
introduces -->

<!-- Link to issues: Resolves #111, Resolves #222 -->
**Motivation**

We want to use [risc zero](https://risczero.com/) as proving system, we
need to build ethereum_rust's crates to accomplish it.

**Description**

Replace `size_of` with `core::mem::size_of` in order to build the crate
for the desired target.
**Motivation**

Provide proper attribution to the Geth code for networking.

**Description**
While implementing the discovery protocol, we've been inspired by `geth`
code. This pr adds clearer and more explicit references to the relevant
sections of the Geth code.

Closes None
**Motivation**

After walking through the README as someone new to the project
everything worked as expected except for the Hive test runs.

**Description**

This PR adds golang to the `.tool-versions` (commented, given that it's
not needed for the rest of the node execution) and a small prereq
section to the hive readme about installing the plugin and uncommenting
it in case of wanting to run the hive tests locally.

Also I added a couple of services to the kurtosis `network-params.yml`,
`el_forkmon` works as a monitor of the canonical chain and checks all
clients are synced between themselves and the amount of bad blocks
they've processed. On the other hand `tx|blob_spammer` adds more
sustance to the run, it mainly affects other nodes given that we don't
have p2p already implemented but will be picked up then.
**Motivation**

<!-- Why does this pull request exist? What are its goals? -->

To separate the localnet run from the `ethereum_rust_l2` CLI. The CLI
should be used as a developer tool to improve the developer experience
and for making more serious deployments (like deploying the stack on
testnet).

**Description**

<!-- A clear and concise general description of the changes this PR
introduces -->

- Now, to run a localnet you just need to change your current dir to the
`l2` crate and run `make`.
- Adds a `help` command to the Makefile and a description for every
command.

---------

Co-authored-by: Federico Borello <[email protected]>
**Motivation**

This PR goal is to add validations in transact that are not done in
upper layers

**Description**

The validations are based in [yellow
paper's](https://ethereum.github.io/yellowpaper/paper.pdf) transaction
execution section (the 6th one). At the moment there is a lack of tests
of the Cancun fork.
Also, removes some constants about transactions intrinsic gas costs,
because we don't do those calculations in this layer.

<!-- Link to issues: Resolves #111, Resolves #222 -->

Closes #900
**Motivation**

<!-- Why does this pull request exist? What are its goals? -->
Some tools like Metamask send big `u64` numbers as `id` in RPC requests
that breaks the parser.

**Description**

<!-- A clear and concise general description of the changes this PR
introduces -->
Change the `RpcRequestId::Number` type from `i32` to `u64`.

<!-- Link to issues: Resolves #111, Resolves #222 -->
**Motivation**
Revival of #818

Adds the GetReceipts request and Receipts response for the eth protocol

**Description**
- Adds GetReceipts and Receipts with encode/decode capabilities
- rustic test with UDP sockets sending the data
- Implements get_all_receipts_by_hash for storage

Closes #386
Closes #387
**Motivation**

The wrong EIP was set for the account abstraction milestone

<!-- Why does this pull request exist? What are its goals? -->

**Description**

<!-- A clear and concise general description of the changes this PR
introduces -->

<!-- Link to issues: Resolves #111, Resolves #222 -->
**Description**
- Moved blocks P2P encoding/decoding into it's own module like with
receipts and transactions
- Removed useless tests
**Motivation**

Implement the issue "Move opcodes to their corresponding folder" #863 

**Description**

Moved the opcodes from block.rs in environment.rs accordings to the list
commented at the beginning of each file.
The modification was verified by running the tests via 'make test'.

Resolves #863

Closes #863
**Motivation**

<!-- Why does this pull request exist? What are its goals? -->
`send`, `deploy` and `call` commands are very useful commands that are
always needed. `calldata` command allows to use this commands in an
easier way.

**Description**

<!-- A clear and concise general description of the changes this PR
introduces -->
Create some commands in the CLI that implement this methods, with
different flags to customize the requests.

<!-- Link to issues: Resolves #111, Resolves #222 -->
**Motivation**

Clippy was throwing some warnings, applying it suggestions.

**Description**

Command:
```sh
cargo clippy --all-targets --all-features --workspace -- -D warnings
```

Output to Fix:
```
warning: it looks like you're manually copying between slices
  --> cmd/ethereum_rust_l2/src/commands/test.rs:77:5
   |
77 | /     for i in 0..64 {
78 | |         buffer[i] = public_key[i + 1];
79 | |     }
   | |_____^ help: try replacing the loop by: `buffer.copy_from_slice(&public_key[1..(64 + 1)]);`
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#manual_memcpy
   = note: `#[warn(clippy::manual_memcpy)]` on by default

warning: unnecessary `if let` since only the `Ok` variant of the iterator element is used
   --> cmd/ethereum_rust_l2/src/commands/test.rs:133:21
    |
133 |                       for line in lines {
    |                       ^           ----- help: try: `lines.flatten()`
    |  _____________________|
    | |
134 | |                         if let Ok(pk) = line {
135 | |                             let thread = tokio::spawn(transfer_from(
136 | |                                 pk,
...   |
144 | |                         }
145 | |                     }
    | |_____________________^
    |
help: ...and remove the `if let` statement in the for loop
   --> cmd/ethereum_rust_l2/src/commands/test.rs:134:25
    |
134 | /                         if let Ok(pk) = line {
135 | |                             let thread = tokio::spawn(transfer_from(
136 | |                                 pk,
137 | |                                 to_address.clone(),
...   |
143 | |                             threads.push(thread);
144 | |                         }
    | |_________________________^
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#manual_flatten
    = note: `#[warn(clippy::manual_flatten)]` on by default

warning: using `clone` on type `H160` which implements the `Copy` trait
   --> cmd/ethereum_rust_l2/src/commands/test.rs:137:33
    |
137 | ...                   to_address.clone(),
    |                       ^^^^^^^^^^^^^^^^^^ help: try removing the `clone` call: `to_address`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_copy
    = note: `#[warn(clippy::clone_on_copy)]` on by default
```
@ElFantasma
Copy link
Contributor Author

Left some comments, is there a proper way I can test this yet?

I'm testing it locally setting up a geth node and connecting it with ours. Eventually this will be tested on hive tests that test the eth capability (couldn't find tests for p2p capability)

Copy link
Contributor

@fkrause98 fkrause98 left a comment

Choose a reason for hiding this comment

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

LGTM,
My biggest concert is proper error handling, but we'll address it in future issues.

@ElFantasma ElFantasma added this pull request to the merge queue Oct 23, 2024
Merged via the queue into main with commit 943a395 Oct 23, 2024
10 checks passed
@ElFantasma ElFantasma deleted the 837-initiate-rlpx-connections-from-discv4 branch October 23, 2024 20:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Initiate RLPx connections from discv4