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
Tidy Eth2Config generation at runtime #912
Conversation
5937080
to
30bdf7e
Compare
I got the error when run
The error above is being addressed in #878. |
I'm writing tests for |
This PR is ready for review. 😃 |
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.
This looks great, thank you! This is a really gross part of the code base (my least favorite), so special thanks for dealing with that.
I only have a super minor comment and an additional "while you're there" request, if you don't mind? :)
lighthouse/environment/src/lib.rs
Outdated
} | ||
|
||
match sub_cli_args.subcommand() { | ||
("prysm", Some(_)) => { |
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.
Whilst you're here, would you mind deleting this prysm
command? :)
It refers to an old testnet and we have the eth2-testnets spec to handle this now.
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.
Oh, thank you! I'll delete the the codes that related to prysm
. 💪
Btw, please let me know why lighthouse using bootstrap_enr.yaml
, not bootstrap_nodes.txt
that described in the guideline. 🙏💦
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.
Btw, please let me know why lighthouse using bootstrap_enr.yaml, not bootstrap_nodes.txt that described in the guideline.
Good question! As described in this recent comment, multiaddrs (which is what's in bootstrap_nodes.txt
) are not sufficient to connect the nodes, we must use ENR.
Those guidelines were produced at a time before all the clients had implemented discv5 (the service that introduces ENR).
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.
Thank you for the pointer and details! I understand why we must use ENR. 💡
beacon_node/src/config.rs
Outdated
pub fn get_eth2_testnet_config<E: EthSpec>( | ||
testnet_dir: &Option<PathBuf>, | ||
) -> Result<Eth2TestnetConfig<E>> { | ||
let tesnet_config = if let Some(testnet_dir) = testnet_dir { |
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.
This might have just been copy-pasta legacy code, but I think we can do away with the let testnet_config ==
and Ok(testnet_config)
here.
Now the spec is in the eth2-testnets repo
I have updated this PR! Please have another look when you have time. 😌 |
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.
Looks great, thank you! Apologies for the delay on the review, we're in the middle of a big spec update!
I've just triggered CI, this is good to merge once it passes! |
Thank you for your review! |
Issue Addressed
This PR closes #602 🚀