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.
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
Electra alpha8 spec updates #6496
base: unstable
Are you sure you want to change the base?
Electra alpha8 spec updates #6496
Changes from 19 commits
3c24e2b
3fa7790
3d595e6
0a4e6be
c0cac0e
b28d010
c8dfe7d
0dd215c
061be34
37c7658
9f85074
707338d
b2b728d
21da047
2aaf9b5
27ce1a0
da953b8
6c56dc4
def2498
c5c9aca
6d10456
e49689b
140d4cf
da33bd6
0af6b0f
1af232b
21591a3
d5ec058
5a3f5df
ff687ed
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
What is this doing? How can we be iterating over an empty list -- doesn't this just not do anything?
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.
My understanding was this applies pending_deposits to the state during initialization if the state contains pending_deposits. Here's the part of the spec
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.
Ohh right, I was confused because we are calling
upgrade_state_to_electra
immediately prior to this, which sets thepending_deposits
to the empty list:lighthouse/consensus/state_processing/src/genesis.rs
Line 120 in 6d10456
lighthouse/consensus/state_processing/src/upgrade/electra.rs
Line 165 in 6d10456
I think we might need to move the
apply_deposit
logic after upgrading to Electra, so that thepending_deposits
list gets populated. Currently it happens here:lighthouse/consensus/state_processing/src/genesis.rs
Lines 36 to 43 in 6d10456
Or maybe we just delete the whole
initialize_beacon_state_from_eth1
function and say we don't support starting from eth1 genesis any more 😅It's interesting that the EF tests for
initialize_beacon_state_from_eth1
already pass. Maybe processing the deposits with the pre-Electra logic is actually equivalent, and we could just delete the code here that loops overpending_deposits
?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.
Yeah i'm okay with that. It seemed redundant to me initially but i just implemented it for completeness.
I'm fine with removing 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.
Removed in d5ec058