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?
| @@ -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
| let max_effective_balance = | ||
| validator.get_max_effective_balance(spec, state.fork_name_unchecked()); | ||
| validator.effective_balance = std::cmp::min( | ||
| amount.safe_sub(amount.safe_rem(spec.effective_balance_increment)?)?, | ||
| max_effective_balance, |
There was a problem hiding this comment.
This code is unnecessary, amount is hardcoded to 0
There was a problem hiding this comment.
It's not unnecessary (this is logic for setting effective balance which is in the spec), but it IS a bug because amount has been hardcoded to zero. I think this bit of code is getting more and more difficult to manage as we add forks so I've created a PR to simplify this whole piece of code:
it needs a review to ensure we're compliant with the spec across all forks. But if we merge that into this PR it'll simplify this a ton.
There was a problem hiding this comment.
nvm this isn't a bug because the spec set amount to 0 much earlier as well.. but the simplification should still be done.
consensus/state_processing/src/per_block_processing/process_operations.rs
Show resolved
Hide resolved
consensus/state_processing/src/per_block_processing/process_operations.rs
Show resolved
Hide resolved
| // Verify pubkey exists | ||
| let Some(source_index) = state | ||
| .pubkey_cache() | ||
| .get(&consolidation_request.source_pubkey) | ||
| else { | ||
| // source validator doesn't exist | ||
| return Ok(false); | ||
| }; | ||
|
|
||
| let source_validator = state.get_validator(source_index)?; |
There was a problem hiding this comment.
This last line is the only reason this function returns a Result<bool> rather than a bool. But it shouldn't ever return an error if the line above returned a pubkey. But since you need to fetch this validator here anyway, just combine them so this function has no reliance on the pubkey cache:
let source_validator = match state.get_validator(source_index) {
Ok(v) => v,
Err(_) => return false,
};then you can refactor this whole function to just return a bool
| return Ok(false); | ||
| } | ||
| } else { | ||
| // Source doen't have execution withdrawal credentials |
There was a problem hiding this comment.
| // Source doen't have execution withdrawal credentials | |
| // Source doesn't have eth1 withdrawal credentials |
the distinction here is important
| // source validator doesn't exist. This is unreachable as `is_valid_switch_to_compounding_request` | ||
| // will return false in that case. |
There was a problem hiding this comment.
If you implement the function above, the previous function no longer relies on the pubkey cache, so technically this could get hit if the cache is stale.. in which case I think we'd probably wanna build it and check again?
| .map(Address::from_slice) | ||
| }) | ||
| .flatten() | ||
| } |
There was a problem hiding this comment.
i hate that we added this function back just for a single place in the spec.. this is gonna cause a bug at some point due to confusing with get_execution_withdrawal_credential..
| 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.
| ) | ||
| .is_ok() | ||
| { | ||
| let mut validator = Validator { |
There was a problem hiding this comment.
this will also be significantly simplified now that we have add_validator_to_registry() :D
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