feat: add bootnode support and basic discv5 support #38
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
KolbyML
force-pushed
the
connect-to-eth2
branch
3 times, most recently
from
fb19cc9 to
6cd7bac
Compare
KolbyML
requested review from
unnawut,
jihoonsong,
hopinheimer,
varun-doshi,
syjn99 and
Kayden-ML
KolbyML
changed the title
KolbyML
force-pushed
the
connect-to-eth2
branch
from
6cd7bac to
29f9d44
Compare
hopinheimer
reviewed
Feb 10, 2025
| @@ -64,16 +74,18 @@ impl Discovery { | |||
| } | |||
|
|
|||
| let _ = discv5.add_enr(bootnode_enr).map_err(|e| { | |||
| println!("Discv5 service failed. Error: {:?}", e); | |||
| info!("Discv5 service failed. Error: {:?}", e); | |||
jihoonsong
approved these changes
Feb 10, 2025
| @@ -64,16 +74,18 @@ impl Discovery { | |||
| } | |||
|
|
|||
| let _ = discv5.add_enr(bootnode_enr).map_err(|e| { | |||
| println!("Discv5 service failed. Error: {:?}", e); | |||
| info!("Discv5 service failed. Error: {:?}", e); | |||
There was a problem hiding this comment.
Suggested change
| info!("Discv5 service failed. Error: {:?}", e); | |
| info!("Failed to add bootnode to DHT {e:?}"); |
Same here.
| let enr_local = convert_to_enr(local_key)?; | ||
| let enr = Enr::builder().build(&enr_local).unwrap(); | ||
| let node_local_id = enr.node_id(); | ||
|
|
||
| let mut discv5 = Discv5::new(enr, enr_local, config.discv5_config.clone()) | ||
| .map_err(|e| format!("Discv5 service failed. Error: {:?}", e))?; | ||
| .map_err(|e| anyhow!("Discv5 service failed. Error: {:?}", e))?; |
There was a problem hiding this comment.
Suggested change
| .map_err(|e| anyhow!("Discv5 service failed. Error: {:?}", e))?; | |
| .map_err(|e| anyhow!("Failed to create discv5 {e:?}"))?; |
Can we have consistent and more descriptive error message something like this?
crates/networking/p2p/src/network.rs
Outdated
|
|
||
| #[derive(NetworkBehaviour)] | ||
| pub(crate) struct ReamBehaviour { | ||
| pub identify: identify::Behaviour, | ||
|
|
||
| /// The discovery domain: discv5 | ||
| pub discovery: Discovery, | ||
|
|
||
| pub connection_registry: libp2p::connection_limits::Behaviour, |
There was a problem hiding this comment.
Suggested change
| pub connection_registry: libp2p::connection_limits::Behaviour, | |
| pub connection_registry: connection_limits::Behaviour, |
what do you mean refactor it? |
|
I see some room for improvements. Could be just trimming or refactor depending on rest of the implementation. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #91
Followup PR's
In my next PR I am going to be implementing basic Req/Resp for the Req/Resp domain of the consensus p2p-interface spec https://github.com/ethereum/consensus-specs/blob/dev/specs/phase0/p2p-interface.md#the-reqresp-domain
There is still work to do like finding subnets, and setting a key-value in the enr to specify the node is an eth2 node