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.

Sign up for GitHub

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

Jump to bottom

implement part of reth p2p rlpx ping #9762

Merged
merged 6 commits into from
Aug 3, 2024
Merged

implement part of reth p2p rlpx ping #9762

merged 6 commits into from
Aug 3, 2024

Conversation

int88
Copy link
Contributor

@int88 int88 commented Jul 24, 2024

Output of reth:

root@dev:~/re# reth p2p rlpx ping --node enr:-KO4QMne-2ViZIH_qe4P3D7DiXtk3WtEmyWvJ5-epTBPM8U5Nsy1msLffx-v2asiF9v0F7t8MKHjynz49-IQ6yhZfI6GAY6TEHi2g2V0aMfGhPCXvBOAgmlkgnY0gmlwhFf5iVmJc2VjcDI1NmsxoQNwGDtgE0IIvlWTJKr4duH8fWIfrk4NRCWk6uG9jmMILoRzbmFwwIN0Y3CCdl-DdWRwgndp
2024-07-24T12:25:43.885520Z  INFO Initialized tracing, debug log directory: /root/.cache/reth/logs/mainnet
HelloMessage {
    protocol_version: V5,
    client_version: "bor/v1.2.7/linux-amd64/go1.20.14",
    capabilities: [
        Capability {
            name: "eth",
            version: 68,
        },
        Capability {
            name: "snap",
            version: 1,
        },
    ],
    port: 0,
    id: 0x70183b60134208be559324aaf876e1fc7d621fae4e0d4425a4eae1bd8e63082e5b977d6e5c18989f2a0e2d8da72553f3f57f695f729107be2f0a331f78ab27e1,
}

output of devp2p:

root@dev:~/re# devp2p rlpx ping enr:-KO4QMne-2ViZIH_qe4P3D7DiXtk3WtEmyWvJ5-epTBPM8U5Nsy1msLffx-v2asiF9v0F7t8MKHjynz49-IQ6yhZfI6GAY6TEHi2g2V0aMfGhPCXvBOAgmlkgnY0gmlwhFf5iVmJc2VjcDI1NmsxoQNwGDtgE0IIvlWTJKr4duH8fWIfrk4NRCWk6uG9jmMILoRzbmFwwIN0Y3CCdl-DdWRwgndp 
{Version:5 Name:bor/v1.2.7/linux-amd64/go1.20.14 Caps:[eth/68 snap/1] ListenPort:0 ID:[112 24 59 96 19 66 8 190 85 147 36 170 248 118 225 252 125 98 31 174 78 13 68 37 164 234 225 189 142 99 8 46 91 151 125 110 92 24 152 159 42 14 45 141 167 37 83 243 245 127 105 95 114 145 7 190 47 10 51 31 120 171 39 225] Rest:[]}

@int88
Copy link
Contributor Author

int88 commented Jul 24, 2024

@mattsse @emhane PATL 🚀

mattsse
mattsse requested changes Jul 24, 2024
View reviewed changes
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

I like this and this command would be quite useful for testing, for example bootstrapping a private network with trusted peers and pinging the enodes would help making sure every node is reachable.

crates/cli/commands/src/p2p/rlpx.rs Outdated
let key = rng_secret_key();
let enr = node.parse::<Enr>().unwrap();
let enr = EnrCombinedKeyWrapper(enr).into();
let node_record = NodeRecord::try_from(&enr).unwrap();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
let node_record = NodeRecord::try_from(&enr).unwrap();
let node_record = NodeRecord::try_from(&enr)?;

crates/cli/commands/src/p2p/rlpx.rs Outdated
/// ping node
Ping {
/// The node to ping.
#[arg(long, short)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

we can remove this so that this will be compatible with devp2p command

Suggested change
#[arg(long, short)]

emhane
emhane requested changes Jul 24, 2024
View reviewed changes
Copy link
Member

@emhane emhane left a comment

Choose a reason for hiding this comment

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

almost there!

crates/cli/commands/src/p2p/rlpx.rs Outdated
Comment on lines 26 to 27
let enr = node.parse::<Enr>().unwrap();
let enr = EnrCombinedKeyWrapper(enr).into();
Copy link
Member

Choose a reason for hiding this comment

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

if you import the enr::Enr instead of discv5::Enr, you shouldn't need this wrapper

crates/cli/commands/src/p2p/rlpx.rs
Comment on lines +36 to +37
let (_, their_hello) =
UnauthedP2PStream::new(ecies_stream).handshake(hello).await?;

println!("{:#?}", their_hello);
Copy link
Member

Choose a reason for hiding this comment

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

this should follow on to use the established session returned by the handshake (the first index in the tuple), and use that to send a ping

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this should follow on to use the established session returned by the handshake (the first index in the tuple), and use that to send a ping

@mattsse I try to implement this which need to refactor some core code of p2p, it may need some time. Or we could merge this first and I'll implement send ping in another PR :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

cool, let's get this over the line and followup separately

Copy link
Member

Choose a reason for hiding this comment

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

please change the name of the pr then, since it only partially implements the ping command

@mattsse mattsse added the A-cli Related to the reth CLI label Jul 25, 2024
@mattsse
Copy link
Collaborator

mattsse commented Aug 1, 2024

@int88 just checking in, this is almost there, lmk if you don't have capacity for it

@int88 int88 changed the title implement reth p2p rlpx ping implement part of reth p2p rlpx ping Aug 2, 2024
@int88 int88 requested a review from Rjected as a code owner August 2, 2024 06:35
@int88
Copy link
Contributor Author

int88 commented Aug 2, 2024

@mattsse updated!

emhane
emhane reviewed Aug 2, 2024
View reviewed changes
crates/cli/commands/src/p2p/rlpx.rs Outdated
match self.subcommand {
Subcommands::Ping { node } => {
let key = rng_secret_key();
let enr = node.parse::<Enr<secp256k1::SecretKey>>().unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

would be more helpful if this parsed an enode into an enr, rather than the serialised base 64 enr string, could open an issue for addressing this separately though

@mattsse mattsse enabled auto-merge August 3, 2024 04:24
@mattsse mattsse added this pull request to the merge queue Aug 3, 2024
Merged via the queue into paradigmxyz:main with commit 2bfa2de Aug 3, 2024
33 checks passed
martinezjorge pushed a commit to martinezjorge/reth that referenced this pull request Aug 7, 2024
@int88 @mattsse @martinezjorge
implement part of reth p2p rlpx ping (paradigmxyz#9762)
6bbf50a 
Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emhane emhane emhane approved these changes

@mattsse mattsse mattsse approved these changes

@onbjerg onbjerg Awaiting requested review from onbjerg onbjerg is a code owner

@gakonst gakonst Awaiting requested review from gakonst gakonst is a code owner

@Rjected Rjected Awaiting requested review from Rjected Rjected is a code owner

Assignees
No one assigned
Labels
A-cli Related to the reth CLI
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@int88 @mattsse @emhane