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

Fix initialize_beacon_state_from_eth1 previous_version #2634

Merged
merged 2 commits into from
Sep 30, 2021

Conversation

hwwhww
Copy link
Contributor

@hwwhww hwwhww commented Sep 28, 2021

Thank @tbenr for finding this error!

@djrtwo
Copy link
Contributor

djrtwo commented Sep 28, 2021

On second thought, I’m not convinced what correct behavior for this versioning is.

  • compute_domain() in process_deposit is “fork agnostic” and is always based on GENESIS_FORK_VERSION
  • There is no prior fork when initializing a chain so the use of ALTAIR does not immediately seem correct as there is no Altair history
  • The primary usecase of previous_version is to verify signatures from the previous fork, but there is no previous history prior to the genesis in this case. The only prior history is kind of the deposits that use GENESIS_FORK_VERSION.

Because of the above, I’d argue to either initialize it to GENESIS_FORK_VERSION because deposits use this OR just initialize it to MERGE_FORK_VERSION which I think is a bit more semantically correct when looking at how the state in initialized in phase 0.

The altair analogue of this function uses GENESIS_FORK_VERSION so double MERGE_FORK_VERSION isn’t consistent unless we change the function in altair as well.

@tbenr
Copy link
Contributor

tbenr commented Sep 29, 2021

my two cents (just because I raised the problem).
the second option seems preferable to me:

current_version=MERGE_FORK_VERSION,  # [Modified in Merge]
previous_version=current_version

I was also exploring the idea about introducing a non-configurable constant NO_FORK_VERSION=Version('0x00000000'). Using this should maintain phase0-MainNet compatibility but I guess brakes pahse0-devnets.

@hwwhww
Copy link
Contributor Author

hwwhww commented Sep 29, 2021

My institution was making it more like a mainnet real case, but you convinced me that MERGE_FORK_VERSION is better for the testing context.

If possible, I'd like to also change Altair initialize_beacon_state_from_eth1 to using previous_version=ALTAIR_FORK_VERSION.

Note: it has nothing to do with the mainnet consensus, just for testing.

@hwwhww hwwhww removed the Bellatrix CL+EL Merge label Sep 30, 2021
@hwwhww
Copy link
Contributor Author

hwwhww commented Sep 30, 2021

@djrtwo @tbenr

I changed it to previous_version=current_version.

@djrtwo
Copy link
Contributor

djrtwo commented Sep 30, 2021

Agreed that it's reasonable to adjust the altair function as well.

This affects not only testing, but also testnets/networks that deploy their genesis from altair logic. I don't think we have any such nets today, so it's okay to introduce this change

@djrtwo djrtwo merged commit f221674 into dev Sep 30, 2021
@djrtwo djrtwo deleted the fix-initialize-merge-state branch September 30, 2021 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants