-
Notifications
You must be signed in to change notification settings - Fork 741
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?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we change the type to SignatureBytes for the other request types as well for consistency?
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.
curious why we did this?
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code is unnecessary, amount is hardcoded to 0
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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