Electra alpha8 spec updates #6496
Conversation
| @@ -36,7 +36,7 @@ impl DepositRequest { | |||
| pubkey: PublicKeyBytes::empty(), | |||
| withdrawal_credentials: Hash256::ZERO, | |||
| amount: 0, | |||
| signature: Signature::empty(), | |||
| signature: SignatureBytes::empty(), | |||
There was a problem hiding this comment.
Should we change the type to SignatureBytes for the other request types as well for consistency?
There was a problem hiding this comment.
Because we need to allow for invalid bls signature bytes to be sent over the engine api. Otherwise, it becomes a deserialization error when we get the payload from the EL instead of an invalid deposit during processing
| @@ -120,6 +120,9 @@ pub fn initialize_beacon_state_from_eth1<E: EthSpec>( | |||
| let post = upgrade_state_to_electra(&mut state, Epoch::new(0), Epoch::new(0), spec)?; | |||
| state = post; | |||
|
|
|||
| // TODO(electra): do we need to iterate over an empty pending_deposits list here and increase balance? | |||
| // in accordance with `initialize_beacon_state_from_eth1` function from the spec | |||
There was a problem hiding this comment.
No because we have already populated the validators array in line 42 and 45
There was a problem hiding this comment.
was talking about this
# Process deposit balance updates
validator_pubkeys = [v.pubkey for v in state.validators]
for deposit in state.pending_deposits:
validator_index = ValidatorIndex(validator_pubkeys.index(deposit.pubkey))
increase_balance(state, validator_index, deposit.amount)
state.pending_deposits = []Added in b2b728d
consensus/state_processing/src/per_block_processing/process_operations.rs
Outdated
Show resolved
Hide resolved
consensus/state_processing/src/per_block_processing/process_operations.rs
Outdated
Show resolved
Hide resolved
consensus/state_processing/src/per_block_processing/process_operations.rs
Outdated
Show resolved
Hide resolved
consensus/state_processing/src/per_block_processing/process_operations.rs
Outdated
Show resolved
Hide resolved
consensus/state_processing/src/per_block_processing/process_operations.rs
Outdated
Show resolved
Hide resolved
consensus/state_processing/src/per_block_processing/process_operations.rs
Show resolved
Hide resolved
| AsMut::<[u8; 32]>::as_mut(&mut validator.withdrawal_credentials)[0] = | ||
| spec.compounding_withdrawal_prefix_byte; | ||
| AsMut::<[u8; 32]>::as_mut(&mut validator.withdrawal_credentials)[0] = | ||
| spec.compounding_withdrawal_prefix_byte; |
There was a problem hiding this comment.
hmmm we should probably refactor the change_withdrawal_credentials() method on the Validator so that it accepts a prefix.
There was a problem hiding this comment.
This way seems cleaner to me tbh, we are changing it in place instead of creating the entire byte array from scratch
There was a problem hiding this comment.
Can the change_withdrawal_credentials() function not be refactored to also do it in place?
There was a problem hiding this comment.
that function also changes the execution address, not just the prefix. Can do it, but i'm not sure what it adds 🤷♂️
There was a problem hiding this comment.
Gotcha. I guess I'm just arguing for encapsulating it as a function on the validator. Honestly just realizing the naming of this sucks. 0x01 is eth1 credentials (despite wanting to move away from "eth1" and towards "execution") but "execution" now means and both 0x01 and 0x02 😩
I suppose we could fix this in a future PR.. but like what do you think about being explicit:
eth1 -> 0x01 for example has_0x01_withdrawal_credentials
and we could rename change_withdrawal_credentials to change_to_0x01_credentials and add a new function change_to_0x02_credentials which encapsulates what you have above. has_execution_withdrawal_credentials remains the same and is more consistent now.
There was a problem hiding this comment.
Yeah I agree the naming is super confusing, agree with doing the renaming in a future PR
consensus/state_processing/src/per_epoch_processing/single_pass.rs
Outdated
Show resolved
Hide resolved
pawanjay176
force-pushed
the
electra-alpha7
branch
from
5bf6b05
to
da953b8
Compare
Issue Addressed
N/A
Proposed Changes
Implements changes for electra from https://github.com/ethereum/consensus-specs/releases/tag/v1.5.0-alpha.7 and https://github.com/ethereum/consensus-specs/releases/tag/v1.5.0-alpha.8
Other changes from the releases are mostly cosmetic/ do not require changes on our end.
Can be reviewed per commit for each change. However, b28d010 contains bug fixes from all the changes