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(net): add discv4 crate #113

Merged
merged 38 commits into from Oct 25, 2022
Merged

feat(net): add discv4 crate #113

merged 38 commits into from Oct 25, 2022

Conversation

mattsse
Copy link
Collaborator

@mattsse mattsse commented Oct 21, 2022

Add discv4 support.

This merges parts of https://github.com/vorot93/discv4 and parts of https://github.com/sigp/discv5 (kademlia) and also applies reth rlp encoding.

TODO

  • queries
  • message encoding/decoding from udp packets

@mattsse mattsse added C-enhancement New feature or request A-devp2p Related to the Ethereum P2P protocol labels Oct 21, 2022
@mattsse
Copy link
Collaborator Author

mattsse commented Oct 21, 2022

currently blocked by pending ethers-corre bump with breaking changes.

@mattsse
Copy link
Collaborator Author

mattsse commented Oct 23, 2022

Update:

reimplemented the entire protocol

TODO

  • tests
  • additional docs

@codecov
Copy link

codecov bot commented Oct 23, 2022

Codecov Report

Merging #113 (9815b13) into main (2b67e75) will decrease coverage by 3.72%.
The diff coverage is 50.61%.

❗ Current head 9815b13 differs from pull request most recent head d261944. Consider uploading reports for the commit d261944 to get more accurate results

@@            Coverage Diff             @@
##             main     #113      +/-   ##
==========================================
- Coverage   72.50%   68.78%   -3.73%     
==========================================
  Files         150      146       -4     
  Lines       10642    10575      -67     
==========================================
- Hits         7716     7274     -442     
- Misses       2926     3301     +375     
Impacted Files Coverage Δ
crates/net/discv4/src/bootnodes.rs 0.00% <0.00%> (ø)
crates/net/discv4/src/error.rs 0.00% <0.00%> (ø)
crates/primitives/src/lib.rs 100.00% <ø> (+88.88%) ⬆️
crates/net/discv4/src/lib.rs 23.89% <23.89%> (ø)
crates/net/discv4/src/config.rs 24.13% <24.13%> (ø)
crates/net/discv4/src/node.rs 75.86% <75.86%> (ø)
crates/net/discv4/src/proto.rs 92.82% <92.82%> (ø)
crates/interfaces/src/db/mod.rs 63.88% <0.00%> (-22.96%) ⬇️
crates/db/src/kv/cursor.rs 27.71% <0.00%> (-15.15%) ⬇️
crates/interfaces/src/db/models/blocks.rs 84.61% <0.00%> (-7.23%) ⬇️
... and 17 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@mattsse
Copy link
Collaborator Author

mattsse commented Oct 24, 2022

the self lookup step is a bit hacky atm, there's a bug in discv5's kad table implementation that prevents self lookups, opened a PR here: sigp/discv5#142

Copy link
Member

@gakonst gakonst left a comment

Choose a reason for hiding this comment

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

love it. a bunch of comments / but you can merge and address separately.

crates/net/discv4/README.md Show resolved Hide resolved
crates/net/discv4/src/config.rs Outdated Show resolved Hide resolved
crates/net/discv4/src/kbucket/bucket.rs Outdated Show resolved Hide resolved
crates/net/discv4/src/kbucket/bucket.rs Outdated Show resolved Hide resolved
crates/net/discv4/src/kbucket/entry.rs Outdated Show resolved Hide resolved
crates/net/discv4/src/lib.rs Outdated Show resolved Hide resolved

#[tokio::test(flavor = "multi_thread")]
#[traced_test]
#[ignore]
Copy link
Member

Choose a reason for hiding this comment

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

is this so we don't get banned? i'd maybe feature gate it

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 ignored since this connects with live nodes, started writing a mock service that allows us to test messages.

Copy link
Member

@gakonst gakonst Oct 25, 2022

Choose a reason for hiding this comment

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

sweet - let's also keep in mind a integration test runner which spawns geth nodes and peers with them (let's just copy-paste geth.rs from ethers-core and add options for p2p config)

Copy link
Collaborator Author

@mattsse mattsse Oct 25, 2022

Choose a reason for hiding this comment

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

will take this on and track

Comment on lines +166 to +169
#[test]
fn test_noderecord_codec_ipv4() {
let mut rng = thread_rng();
for _ in 0..100 {
Copy link
Member

Choose a reason for hiding this comment

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

wshould be replaced with proptest or test-fuzz, but let's do separately cc @joshieDo , lets track

Comment on lines +94 to +98
let signature: RecoverableSignature = SECP256K1.sign_ecdsa_recoverable(
&secp256k1::Message::from_slice(keccak256(&payload).as_ref())
.expect("is correct MESSAGE_SIZE; qed"),
secret_key,
);
Copy link
Member

Choose a reason for hiding this comment

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

lets track in an issue about maybe using k256 down the line if performance improves

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

wanted to do this, but the other p2p stuff also relies on secp256k currently, so we should migrate this in one step. will track

Comment on lines +386 to +393
#[cfg(test)]
mod tests {
use super::*;
use crate::SAFE_MAX_DATAGRAM_NEIGHBOUR_RECORDS;
use bytes::BytesMut;
use rand::{thread_rng, Rng, RngCore};

fn rng_endpoint(rng: &mut impl Rng) -> NodeEndpoint {
Copy link
Member

Choose a reason for hiding this comment

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

should also be done w fuzzer @joshieDo

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In essence this is an Arbitrary impl for NodeEndpoint since all values are randomly generated.

Copy link
Collaborator

@joshieDo joshieDo Oct 25, 2022

Choose a reason for hiding this comment

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

test-fuzz does not require Arbitrary, only requires:

@mattsse mattsse requested a review from gakonst October 25, 2022 11:17
Comment on lines +135 to +154
/// # use std::io;
/// use std::net::SocketAddr;
/// use std::str::FromStr;
/// use rand::thread_rng;
/// use secp256k1::SECP256K1;
/// use reth_discv4::{Discv4, Discv4Config, NodeId, NodeRecord};
/// # async fn t() -> io::Result<()> {
/// // generate a (random) keypair
/// let mut rng = thread_rng();
/// let (secret_key, pk) = SECP256K1.generate_keypair(&mut rng);
/// let id = NodeId::from_slice(&pk.serialize_uncompressed()[1..]);
///
/// let socket = SocketAddr::from_str("0.0.0.0:0").unwrap();
/// let local_enr = NodeRecord {
/// address: socket.ip(),
/// tcp_port: socket.port(),
/// udp_port: socket.port(),
/// id,
/// };
/// let config = Discv4Config::default();
Copy link
Member

Choose a reason for hiding this comment

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

sick

@mattsse mattsse merged commit ce64fef into main Oct 25, 2022
@mattsse mattsse deleted the matt/discv4 branch October 25, 2022 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-devp2p Related to the Ethereum P2P protocol C-enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants