-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Holesky support #12821
Holesky support #12821
Conversation
dfcf88b
to
5d98a67
Compare
Copy config/params from deneb-integration
70b808a
to
797e486
Compare
if errors.Is(err, stategen.ErrNoGenesisBlock) { | ||
log.Errorf("No genesis block/state is found. Prysm only provides a mainnet genesis "+ | ||
"state bundled in the application. You must provide the --%s or --%s flag to load "+ | ||
"a genesis block/state for this network.", "genesis-state", "genesis-beacon-api-url") |
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.
I see you used variable interpolation for the flag names - did you intend to replace these with direct flag var .Name references?
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.
I tried this and got hit with a circular dep. I would have to refactor / move those flags to another package so I just hard coded it in the laziest way. I could remove the templating if that's your feedback here.
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.
Makes sense, in that case, yes the templating seems to introduce a smidge of confusion for no discernible benefit, but I don't feel strongly about it.
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.
Some nitpicks but otherwise LGTM
@@ -154,6 +161,10 @@ func applyPraterFeatureFlags(ctx *cli.Context) { | |||
func applySepoliaFeatureFlags(ctx *cli.Context) { | |||
} | |||
|
|||
// Insert feature flags within the function to be enabled for Holesky testnet. | |||
func applyHoleskyFeatureFlags(ctx *cli.Context) { |
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.
i think deepsource is complaining these parameters are not used
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.
I think it's fine. These are essentially reserved functions for future use anyway
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 good to me
@prestonvanloon will you be updating the prysm docs? if not, we should get an owner on that task?
@terencechain -- yes, i will update docs as well. #12724 as assigned to me and we have a task item there. |
What type of PR is this?
Feature
What does this PR do? Why is it needed?
Adds holesky config support for the upcoming holesky testnet.
The holesky genesis state is far too large to bundle inside of the prysm distribution binary (200Mb) so it must be loaded from disk or checkpoint sync. This is existing behavior for other testnets, but I felt it would be helpful to provide a meaningful failure message when the user attempts to run prysm on a non-mainnet network.
Which issues(s) does this PR fix?
#12724
Other notes for review
Example of running Prysm without a genesis state