Minor cosmetic cleanups #1525
Minor cosmetic cleanups #1525
Conversation
* remove `BLS_WITHDRAWAL_PREFIX` (it is not used in phase 1, not phase 0) * avoid inline comment (# Validate state root) * simplify header inequality check in `process_proposer_slashing` (using uniqueness of BLS signatures) * add `block = signed_block.message` helper variable for readability * (typo) clarify that the state transition function consumes a signed block (as opposed to a block) * generally make comments more consistent * consistent formatting of container instantiation for `DepositMessage` * avoid using three lines for `rewards[index] += Gwei(max_attester_reward // attestation.inclusion_delay)` * introduce `effective_balance` helper variable for readability, and to avoid multi-line statement * consistent ordering of `MIN_EPOCHS_TO_INACTIVITY_PENALTY` in the time parameters table * (typo) "Dequeued validators for activation up to churn limit" => Dequeue validators * "Save current block as the new latest block" => "Cache current block" (for consistent with `process_slot`) * (typo) "Verify the validator has not yet exited" => "Verify exit has not been initiated" * Use Pythonic `default=` for `max()` call in `initiate_validator_exit`
|
@JustinDrake just to make sure, do you want to switch to targeting on |
I don't mind either way :) (It looks like the If you think it's the best way forward, do feel free to switch the target branch. (Me doing the switch will likely result in a Git mess.) |
|
Nice cleanup, but I think v0.9.4 was the last 0.9 release. So removing the draft status is all that's necessary for review & merge. |
Done :) |
There was a problem hiding this comment.
Looks good. I think it can go in v0.10, but I don't think we are actively maintaining a v010x branch for selection, as phase0 generally just doesn't change. @djrtwo what do you think?
|
|
| @@ -1038,7 +1036,7 @@ def initiate_validator_exit(state: BeaconState, index: ValidatorIndex) -> None: | |||
|
|
|||
| # Compute exit queue epoch | |||
| exit_epochs = [v.exit_epoch for v in state.validators if v.exit_epoch != FAR_FUTURE_EPOCH] | |||
| exit_queue_epoch = max(exit_epochs + [compute_activation_exit_epoch(get_current_epoch(state))]) | |||
| exit_queue_epoch = max(exit_epochs, default=compute_activation_exit_epoch(get_current_epoch(state))) | |||
There was a problem hiding this comment.
Looks like this is a not cosmetic change, but a behavior one.
Say we have exit_epochs=[100, 101, 102] and compute_activation_exit_epoch(get_current_epoch(state))) evaluates 105.
max(exit_epochs + [compute_activation_exit_epoch(get_current_epoch(state))])evaluates105, andmax(exit_epochs, default=compute_activation_exit_epoch(get_current_epoch(state)))evaluates102
The old expression wants a min of max, while the new expression wants a default value when exit_epochs is empty. That results in these behaviors:
- The old one says "' Even' if no one is exiting soon, you can't exit immediately", and
- The new one says "If no one is exiting soon, you can't exit immediately. But if someone is, you can exit with them at the same epoch."
I think the former is what we want.
There was a problem hiding this comment.
Good catch 👍
And also note that exit_epochs may contain already-exited validators, so default will effectively not happen after the first exit. So it looks like it was completely broken :(
process_proposer_slashing(using uniqueness of BLS signatures)block = signed_block.messagehelper variable for readabilityDepositMessagerewards[index] += Gwei(max_attester_reward // attestation.inclusion_delay)effective_balancehelper variable for readability, and to avoid multi-line statementMIN_EPOCHS_TO_INACTIVITY_PENALTYin the time parameters tableprocess_slot)default=formax()call ininitiate_validator_exit