-
Notifications
You must be signed in to change notification settings - Fork 119
feat(l1): admin_addPeer endpoint #5004
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
mpaulucci
left a comment
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.
nice!
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.
Pull Request Overview
This PR implements the admin_addPeer RPC method to allow manually adding peers to the P2P network. The changes refactor the initialization of the RLPx initiator and peer handler to support this new functionality.
Key changes:
- Added
admin_addPeerRPC endpoint implementation - Refactored
PeerHandlerandSyncManagerdummy methods to be async - Modified
RLPxInitiator::spawnto return a handle for external peer initiation - Updated dependency injection patterns for P2P components in initializers
Reviewed Changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/networking/rpc/admin/peers.rs | Added add_peer function with connection verification loop and helper function |
| crates/networking/rpc/rpc.rs | Registered new admin_addPeer RPC method |
| crates/networking/rpc/admin/mod.rs | Exported add_peer function |
| crates/networking/p2p/peer_handler.rs | Added initiator field and updated constructor |
| crates/networking/p2p/rlpx/initiator.rs | Added Initiate message variant and modified spawn to return handle |
| crates/networking/p2p/sync_manager.rs | Made dummy method async |
| crates/networking/p2p/sync.rs | Made dummy method async |
| cmd/ethrex/initializers.rs | Refactored P2P context and initiator initialization |
| cmd/ethrex/l2/initializers.rs | Refactored P2P context and initiator initialization for L2 |
| tooling/sync/Makefile | Added quotes around JWT secret path |
| crates/networking/rpc/Cargo.toml | Added test-utils feature dependency |
| crates/networking/p2p/Cargo.toml | Added test-utils feature flag |
| .github/workflows/pr-main_l1.yaml | Increased simulation parallelism from 4 to 8 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ElFantasma
left a comment
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.
Left some comments, but it LGTM!
**Motivation** PR #5004 led to changes in `init_l2()` that weren't properly updated, so now when initializing L2 there are 2 Peer Tables. **Description** This pr uses the Peer Table created in the first place to initialize the Peer Handler instead of creating a new one. It also updates the gen server initialization for the gen server dummy.
**Motivation** Include geth's [admin_addPeer](https://geth.ethereum.org/docs/interacting-with-geth/rpc/ns-admin#admin-addpeer) endpoint since we found it necessary for running eip7934 hive tests with the `ethereum/eels/consume-sync ` simulation. Besides Nethermind, Reth, Besu, Erigon and other execution clients implement it. **Description** This pr introduces the RPC endpoint `admin_addPeer`. Since this endpoint needed to access p2p's generic server used to establish connections, the `init_l1` function was restructure so the genserver is saved within the peer_handler struct and can later be accessed from the `RPCApiContext`. In `admin_addPeer` the connection is tried in a period of 10 seconds waiting 100 milliseconds in between each message casted. This is because connections are asynchronous so to check if the connection with the peer was made we need to wait until it's established. --------- Co-authored-by: SDartayet <sofiadartayet@gmail.com> Co-authored-by: SDartayet <44068466+SDartayet@users.noreply.github.com> Co-authored-by: ElFantasma <estebandh@gmail.com>
**Motivation** PR #5004 led to changes in `init_l2()` that weren't properly updated, so now when initializing L2 there are 2 Peer Tables. **Description** This pr uses the Peer Table created in the first place to initialize the Peer Handler instead of creating a new one. It also updates the gen server initialization for the gen server dummy.
Motivation
Include geth's admin_addPeer endpoint since we found it necessary for running eip7934 hive tests with the
ethereum/eels/consume-syncsimulation.Besides Nethermind, Reth, Besu, Erigon and other execution clients implement it.
Description
This pr introduces the RPC endpoint
admin_addPeer. Since this endpoint needed to access p2p's generic server used to establish connections, theinit_l1function was restructure so the genserver is saved within the peer_handler struct and can later be accessed from theRPCApiContext.In
admin_addPeerthe connection is tried in a period of 10 seconds waiting 100 milliseconds in between each message casted. This is because connections are asynchronous so to check if the connection with the peer was made we need to wait until it's established.