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

Alter config filed name to devnet if it's not populated in file #9949

Merged
merged 4 commits into from
Nov 30, 2021

Conversation

terencechain
Copy link
Member

@terencechain terencechain commented Nov 29, 2021

Today, Prysm ships with an embedded genesis state. When loading custom genesis state with config name unchanged (i.e., mainnet), prysm will by default uses embedded genesis state which conflicts with to be loaded genesis state. (i.e. EMBEDDED_GENESIS_STATE.HTR() != LOADED_GENESIS_STATE.HTR()). This is not ideal, and it's not quite right to assume that whenever config is unchanged, it's always going to use EMBEDDED_GENESIS_STATE. It breaks much of the assumption of starting a new chain from scratch. The assumption that uses EMBEDDED_GENESIS_STATE should be further restricted where deposit contract address and genesis fork version matching

EDIT:
Worked with @nisdas to find a more neutral way to alter mainnet field name in the event if it doesn't exist

credit to @parithosh for pointing this out

@terencechain terencechain requested a review from a team as a code owner November 29, 2021 18:38
@terencechain terencechain self-assigned this Nov 29, 2021
@terencechain terencechain added the Ready For Review A pull request ready for code review label Nov 29, 2021
Copy link
Member

@nisdas nisdas left a comment

Choose a reason for hiding this comment

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

Not sure if this is the right approach, if we want the user to be able to overwrite the embedded state then maybe we should have clearer logs indicating that it is indeed what we are doing. If users are providing a config with mainnet specified then it is reasonable to assume that mainnet is the config that they are using.

@terencechain
Copy link
Member Author

Not sure if this is the right approach, if we want the user to be able to overwrite the embedded state then maybe we should have clearer logs indicating that it is indeed what we are doing. If users are providing a config with mainnet specified then it is reasonable to assume that mainnet is the config that they are using.

It's not users are providing config with mainnet specified, it's users providing config with nonespecified. In this case, it defaults back to mainnet config and loads mainnet genesis state

@terencechain terencechain changed the title Strictly checks to loading embedded genesis state Alter config filed name to devnet if it's not populated in file Nov 30, 2021
@terencechain terencechain merged commit d8aa0f8 into develop Nov 30, 2021
@delete-merged-branch delete-merged-branch bot deleted the stricter-load-genesis-state branch November 30, 2021 03:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready For Review A pull request ready for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants