-
Notifications
You must be signed in to change notification settings - Fork 982
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
eip7251: Switch to compounding when consolidating with source==target #3918
Conversation
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.
Clean simplification! Much better to signal the switch without requiring the 1 ETH deposit.
Co-authored-by: Lion - dapplion <[email protected]>
specs/electra/beacon-chain.md
Outdated
# Initiate source validator exit and append pending consolidation | ||
source_validator.exit_epoch = compute_consolidation_epoch_and_update_churn( | ||
state.validators[source_index].exit_epoch = compute_consolidation_epoch_and_update_churn( |
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 think in Python variable is assigned to a reference. So if you modify a variable that is a reference to an element in an array, the array element will be updated as well.
I don't see why we would need to replace source_validator
with state.validators[source_index]
explicitly when updating. Unless I am missing something.
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.
Yes, you’re right, and the reason for doing so is the bug somewhere in one of the components that spec relies on, likely in remerklable. Consider the following test:
@with_electra_and_later
@spec_state_test
def test_state_changes_overwritten(spec, state):
validator_0 = state.validators[0]
validator_1 = state.validators[1]
validator_0.exit_epoch = spec.Epoch(0)
validator_1.exit_epoch = spec.Epoch(1)
assert state.validators[1].exit_epoch == spec.Epoch(1)
assert state.validators[0].exit_epoch == spec.Epoch(0)
The second assert fails while it must not. This problem requires a separate work, and I decided to use a workaround for this spec change. Filed an issue #3925
tests/core/pyspec/eth2spec/test/electra/block_processing/test_process_consolidation_request.py
Outdated
Show resolved
Hide resolved
Co-authored-by: Alex Stokes <[email protected]>
I’ve attempted to isolate the switch to compounding flow from the main consolidation flow, the reason for that is slightly different checks considering that source equals target, especially the switch request should be valid even when there is not enough consolidation churn and pending consolidation queue is full. This entailed update to the spec tests as well but left the consolidation flow pretty much the same as it was before. IMHO, the spec became more readable and easier to reason about. Sorry for almost the last minute change to this request. |
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.
generally looks good! I like uncoupling the "switch to compounding" flow from the deposit flow as much as we can
will also want to review in the context of the other refactors to the core pectra EIPs we are looking at
love 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.
LGTM
tests/core/pyspec/eth2spec/test/electra/block_processing/test_process_consolidation_request.py
Outdated
Show resolved
Hide resolved
tests/core/pyspec/eth2spec/test/electra/block_processing/test_process_consolidation_request.py
Outdated
Show resolved
Hide resolved
tests/core/pyspec/eth2spec/test/electra/block_processing/test_process_consolidation_request.py
Outdated
Show resolved
Hide resolved
Co-authored-by: Justin Traglia <[email protected]>
tests/core/pyspec/eth2spec/test/electra/block_processing/test_process_consolidation_request.py
Outdated
Show resolved
Hide resolved
@@ -1470,6 +1505,10 @@ def process_consolidation_request( | |||
source_index=source_index, | |||
target_index=target_index | |||
)) | |||
|
|||
# Churn any target excess active balance of target and raise its max |
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.
can we keep this in process_pending_consolidations
?
so drop this addition here, and then keep the deletion up above
# Churn any target excess active balance of target and raise its max
switch_to_compounding_validator(state, pending_consolidation.target_index)
seems easier to reason about if the effects are all in one place
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 like the idea of switching to compounding in one place. So, the consolidation request has an additional semantics that takes an effect disregarding whether it is a genuine consolidation or merely a switch request. The switch also adds pending balance to the activation queue which creates additional dependency between two process epoch routines: process_pending_consolidations
and process_pending_balance_deposits
, having no additional dependency is cleaner.
# Verify that source != target, so a consolidation cannot be used as an exit. | ||
if source_index == target_index: | ||
return | ||
|
||
# Verify source withdrawal credentials | ||
has_correct_credential = has_execution_withdrawal_credential(source_validator) | ||
is_correct_source_address = ( |
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.
lets do this later but I think we can also factor out a helper like is_valid_consolidation_request
Co-authored-by: Alex Stokes <[email protected]> Co-authored-by: Justin Traglia <[email protected]>
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.
Sounds much better
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.
Sorry for the late review.
I think the conditions in is_valid_switch_to_compounding_request
and process_consolidation_request
can be refactored, but I also don't want to add six more helpers. 😅
The test cases generally look great! I don't want to block the release, so we can merge it now and follow up with post-merge issue & PR.
This PR implements @ralexstokes’s idea of switching validator to compounding credentials via consolidation request rather than depositing with the updated withdrawal credentials. The pure switch happens when a consolidation request has
source_index == target_index
. Advantages of this approach:Additionally, this PR moves switching to compounding from
process_pending_consolidations
toprocess_consolidation_request
which is cleaner and simpler to reason about asprocess_pending_consolidations
doesn’t append topending_deposit_balances
queue anymore. Also the switch happens instantly instead of waiting for consolidation queue to be processed.