eip6110: Queue deposit requests and apply them during epoch processing #3818
Conversation
specs/electra/beacon-chain.md
Outdated
| else: | ||
| validator_index = ValidatorIndex(validator_pubkeys.index(deposit.pubkey)) | ||
| validator = state.validators[validator_index] | ||
| # Validator is exiting, do not consume the churn |
There was a problem hiding this comment.
From @mkalinin this line is motivated by
There was a problem hiding this comment.
Why doesn't this condition validator.exit_epoch < FAR_FUTURE_EPOCH match
consensus-specs/specs/electra/beacon-chain.md
Lines 870 to 871 in 43e7344
There was a problem hiding this comment.
The idea is to postpone deposit processing for an exiting validator until it has fully withdrawn. And then do the top-up without affecting WS period.
reading through I think my question is answered. we need it based on this PR |
…process_pending_deposits.py Co-authored-by: Mikhail Kalinin <[email protected]>
…ikhail into deposit-queue
Co-authored-by: Justin Traglia <[email protected]>
specs/electra/beacon-chain.md
Outdated
|
|
||
| ```python | ||
| def get_validator_from_deposit(pubkey: BLSPubkey, withdrawal_credentials: Bytes32, amount: uint64) -> Validator: | ||
| if is_compounding_withdrawal_credential(withdrawal_credentials): |
There was a problem hiding this comment.
Can you use get_max_effective_balance instead? You can create an instance of Validator first and then call get_max_effective_balance.
| withdrawal_credentials=validator.withdrawal_credentials, | ||
| amount=excess_balance, | ||
| signature=bls.G2_POINT_AT_INFINITY, | ||
| slot=GENESIS_SLOT, |
There was a problem hiding this comment.
What is the consequence if I set this to state.slot instead?
There was a problem hiding this comment.
Then the excess balance will wait for when state.slot is finalized before being applied even in the case when there is enough activation churn to let the balance in. The GENESIS_SLOT is used to bypass that finality check as that slot is always finalized
| signature=bls.G2_POINT_AT_INFINITY, | ||
| slot=GENESIS_SLOT, |
There was a problem hiding this comment.
I think we should define meaningful constants for both bls.G2_POINT_AT_INFINITY and GENESIS_SLOT. How about aliasing them as
UNSET_SIGNATUREandUNSET_DEPOSIT_SLOT
By seeing only the point at infinity and the genesis slot, the readers wouldn't know what they mean.
There was a problem hiding this comment.
I have put comments instead of aliasing those two. It is a bit difficult to incorporate the semantics into these constants in both cases
|
After merging in dev, the |
Fixed now (the change of this test from the dev wasn’t applied to this branch)
The corresponding changes were already made, so the PR is ready for review and merging into dev |
tests/core/pyspec/eth2spec/test/electra/epoch_processing/test_apply_pending_deposit.py
Outdated
Show resolved
Hide resolved
| """ | ||
| assert is_post_electra(spec) |
There was a problem hiding this comment.
Might be not as these methods are called from post electra tests only
| - pre-state ('pre') | ||
| - post-state ('post'). | ||
| """ | ||
| assert is_post_electra(spec) |
Description
Introduces majority of things described in #3689 (comment), specifically:
MAX_PENDING_DEPOSITS_PER_EPOCH)(pubkey, index)cache is required.pending_depositsqueue to ensure the deposit will pass through the activation churn.process_pending_depositsby moving deposit applying toapply_pending_depositOther than the above, the logic of the previous
process_pending_balance_depositsfunction remains.Additionally, moves. Removesswitch_to_compounding_validatortoprocess_deposit_requestswitch_to_compounding_validatorfrom the deposit flow in favour of #3918.Points of discussion and open questions
validatorIndexis looked up by thepubkey(process_pending_depositsandapply_pending_depositfunctions), this happens for each pending deposit that is being processed. There is a less readable workaround wherevalidatorIndexis resolved once, but I am not sure if it is necessary because client implementations are likely to use cache in both cases.MAX_PENDING_DEPOSITS_PER_EPOCH_PROCESSINGis set to16, should it be more? The activation churn is limited by256 ETHwhich would mean 256 signature verifications in the worst case. The most optimal scenario of an attack resulting in 256 sig verifications would entail creation of1281-ETH validators (the first deposit would be to create a validator with eth1 creds, the second would be to switch the validator to compounding creds); it seems that employing such an attack is quite discouraging because it requires decent amount of ETH with no substantial gain. Should we even have this limitation if the churn could do the limit?Testing
Many thanks to @james-prysm for helping to write and adjust the tests!
apply_pending_depositschecksprocess_pending_deposittest_process_deposit_requestsscenarious in application toprocess_pending_depositMAX_PENDING_DEPOSITS_PER_EPOCH_PROCESSINGchecktest_process_pending_deposit(is_deposit_applied,is_churn_limit_reachedetc)Link to a more comprehensive test plan: https://hackmd.io/mqV7TaFyTq--vEMcB5msJw
Supersedes #3689