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(discv5): add crate for interfacing reth network and sigp/discv5 #7336

Merged
merged 80 commits into from
Apr 2, 2024

Conversation

emhane
Copy link
Member

@emhane emhane commented Mar 26, 2024

  • Makes sigp/discv5 usable in reth network by implementing minimal interface HandleDiscovery.
  • Parses enodes into multiaddresses (used by CL) that can be added into sigp/enr via public api.
  • Exposes sigp/discv5 api for filtering queries with custom filters, to narrow down lookup queries. this is necessary since mostly eth2 peers are found on discv5 rn. populating kbuckets with eth2 nodes will lead to almost inability to find EL peers if kbuckets become full, since by Kademlia, kbuckets favour long lived connections - this means it will keep the first nodes it puts in its buckets if those nodes stay online.

Copy link
Member

@Rjected Rjected left a comment

Choose a reason for hiding this comment

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

I'm still a bit confused by the downgrade logic, why do we have to include discv4 in this discv5 logic? It seems to make everything more complex, and I'm not sure what the benefit is

crates/net/discv5/src/config.rs Outdated Show resolved Hide resolved
crates/net/discv5/src/config.rs Outdated Show resolved Hide resolved
crates/net/discv5/src/config.rs Outdated Show resolved Hide resolved
crates/net/discv5/src/enr.rs Outdated Show resolved Hide resolved
crates/net/discv5/src/config.rs Outdated Show resolved Hide resolved
crates/net/discv5/src/config.rs Outdated Show resolved Hide resolved
crates/net/discv5/src/metrics.rs Outdated Show resolved Hide resolved
/// L1 CL
const ETH2: &'static [u8] = b"eth2";
/// Optimism
const OPSTACK: &'static [u8] = b"opstack";
Copy link
Member

Choose a reason for hiding this comment

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

so opstack actually includes this key in enrs?

Copy link
Member Author

Choose a reason for hiding this comment

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

some yeah, for example

2024-03-22T11:29:19.139714Z TRACE net::discovery::discv5: discovered peer on opstack fork_id=None enr=Enr { id: Some("v4"), seq: 1708293705211, NodeId: 0x8aa3a341cc388b03310b4b13f641722dd2bf0d6a992c1a9bf984489674ef4876, signature: "98ac9fdef036164e4566b84efea47b3f6944f96d8bbef0026830d27346f4253e4731288dddbe4af11dfd5481cfcb6ebc71a56c83dd931a547e7da0a1b75b6a21", IpV4 UDP Socket: Some(78.153.130.198:9222), IpV6 UDP Socket: None, IpV4 TCP Socket: Some(78.153.130.198:9222), IpV6 TCP Socket: None, Other Pairs: [("opstack", "85f1dbda0300"), ("secp256k1", "a1031b9dda693d646e1dabff64e3140c65afeb9c16c6847d40ce8f8936e6b347d496")] }
2024-03-22T11:29:19.140715Z TRACE net::swarm: adding new peer to network peer_id="0x1b9d…4d41" socket_addr=78.153.130.198:9222 fork_id=None

not 100% sure if its fork id, or smthg else?

Copy link
Member

Choose a reason for hiding this comment

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

hmmm, yeah probably a forkid / forkhash

Copy link
Member Author

Choose a reason for hiding this comment

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

but maybe only for base?

crates/net/discv5/src/config.rs Outdated Show resolved Hide resolved
@emhane
Copy link
Member Author

emhane commented Mar 26, 2024

blocked by #7328, missing TryFrom<Enr> impl

@emhane emhane added S-blocked This cannot more forward until something else changes A-networking Related to networking in general A-discv5 Related to discv5 discovery labels Mar 26, 2024
@emhane emhane requested a review from Rjected March 26, 2024 23:33
@emhane emhane requested a review from Rjected April 1, 2024 20:58
@emhane emhane requested a review from mattsse April 2, 2024 12:00
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.

few more nits

crates/net/discv5/Cargo.toml Outdated Show resolved Hide resolved
crates/net/discv5/src/enr.rs Outdated Show resolved Hide resolved
crates/net/discv5/src/filter.rs Outdated Show resolved Hide resolved
crates/net/discv5/src/lib.rs Outdated Show resolved Hide resolved
crates/net/discv5/src/lib.rs Outdated Show resolved Hide resolved
@emhane
Copy link
Member Author

emhane commented Apr 2, 2024

the uint, why are we defining new uint types?

because I copied a module from sigp/discv5, because I didn't want to invent smthg new to test the code, i wanted to use code that is used in production and is heavily audited. it's just a dev dep so I'm hoping it doesn't disturb you.

then this function should be pub crate

related to the point above, if libraries would just make functions public, code would seldom have to be copy pasted. I'm relecetuant to restrict visibility since I don't know in what way users may want to use reth, and want it to be as fun as possible for them. an inaccessible function doesn't spark joy.

they get all sent to the same service.
doing this on separate tasks is unnecessary.

yes, but they only get sent when the preceding query finishes if not spawned on separate threads. I can poll the futures concurrently on the same thread if you prefer.

@Rjected
Copy link
Member

Rjected commented Apr 2, 2024

the uint, why are we defining new uint types?

because I copied a module from sigp/discv5, because I didn't want to invent smthg new to test the code, i wanted to use code that is used in production and is heavily audited. it's just a dev dep so I'm hoping it doesn't disturb you.

let's remove the construct_uint call and use our U256, because Key logic is more important wrt testing imo

@emhane
Copy link
Member Author

emhane commented Apr 2, 2024

the uint, why are we defining new uint types?

because I copied a module from sigp/discv5, because I didn't want to invent smthg new to test the code, i wanted to use code that is used in production and is heavily audited. it's just a dev dep so I'm hoping it doesn't disturb you.

let's remove the construct_uint call and use our U256, because Key logic is more important wrt testing imo

alright sure, how do I do this with our type (need to re-write the copied sigp module so I can run the test)?

let a = U256::from(self.hash.as_slice());

@emhane emhane requested a review from mattsse April 2, 2024 16:02
@Rjected
Copy link
Member

Rjected commented Apr 2, 2024

the uint, why are we defining new uint types?

because I copied a module from sigp/discv5, because I didn't want to invent smthg new to test the code, i wanted to use code that is used in production and is heavily audited. it's just a dev dep so I'm hoping it doesn't disturb you.

let's remove the construct_uint call and use our U256, because Key logic is more important wrt testing imo

alright sure, how do I do this with our type (need to re-write the copied sigp module so I can run the test)?

let a = U256::from(self.hash.as_slice());

let's use U256::from_be_slice

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 think this is a great standalone service and this should give us everything we need for reth-network

lgtm, pending @Rjected

Copy link
Member

@Rjected Rjected left a comment

Choose a reason for hiding this comment

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

LGTM! this should unblock a lot of things

@emhane emhane added this pull request to the merge queue Apr 2, 2024
Merged via the queue into main with commit ebc4bc8 Apr 2, 2024
28 checks passed
@emhane emhane deleted the emhane/discv5 branch April 2, 2024 19:52
Ruteri pushed a commit to Ruteri/reth that referenced this pull request Apr 17, 2024
…aradigmxyz#7336)

Co-authored-by: Dan Cline <6798349+Rjected@users.noreply.github.com>
Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>
Ruteri pushed a commit to Ruteri/reth that referenced this pull request Apr 17, 2024
…aradigmxyz#7336)

Co-authored-by: Dan Cline <6798349+Rjected@users.noreply.github.com>
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
Labels
A-discv5 Related to discv5 discovery A-networking Related to networking in general
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants