eip6110: Queue deposit requests and apply them during epoch processing

[mkalinin]

Description

Introduces majority of things described in #3689 (comment), specifically:

• Queues full deposit request objects and process them on the epoch processing, rate limits a number of deposit signature verifications per epoch by the max number of deposit requests allowed to be processed (MAX_PENDING_DEPOSITS_PER_EPOCH)
• Finalizes deposit request position in the queue before applying it to the state. The outcome is that no new validator is created before the corresponding deposit is finalized hence no fork aware (pubkey, index) cache is required.
• Eth1 bridge deposits are processed as before this change, the corresponding entry with the entire balance is created in the pending_deposits queue to ensure the deposit will pass through the activation churn.
• No deposit request is processed until Eth1 bridge covers the gap during the transition period. This prevents the scenario when deposit request can easily front-run the Eth1 bridge deposit because of the large follow distance and create validator with the same pubkey but malicious withdrawal credentials.
• Refactors process_pending_deposits by moving deposit applying to apply_pending_deposit

Other than the above, the logic of the previous process_pending_balance_deposits function remains.

Additionally, moves switch_to_compounding_validator to process_deposit_request. Removes switch_to_compounding_validator from the deposit flow in favour of #3918.

Points of discussion and open questions

• Due to refactor that facilitates spec reading there are two places where validatorIndex is looked up by the pubkey (process_pending_deposits and apply_pending_deposit functions), this happens for each pending deposit that is being processed. There is a less readable workaround where validatorIndex is 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_PROCESSING is set to 16, should it be more? The activation churn is limited by 256 ETH which would mean 256 signature verifications in the worst case. The most optimal scenario of an attack resulting in 256 sig verifications would entail creation of 128 1-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?
• This PR does not explore an optimisation with two queues that was discussed previously, it is planned to be a part of a separate PR once this PR gets merged.

Testing

Many thanks to @james-prysm for helping to write and adjust the tests!

• apply_pending_deposits checks
• Deposit transition period checks -- the logic has changed and part of it is in process_pending_deposit
• Repeat test_process_deposit_requests scenarious in application to process_pending_deposit
• Finalized deposit position check
• MAX_PENDING_DEPOSITS_PER_EPOCH_PROCESSING check
• Revisit test_process_pending_deposit (is_deposit_applied, is_churn_limit_reached etc)

Link to a more comprehensive test plan: https://hackmd.io/mqV7TaFyTq--vEMcB5msJw

Supersedes #3689

[dapplion]

From @mkalinin this line is motivated by

• Handle top-ups to exiting/exited validators #3776

[dapplion]

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

	if get_current_epoch(state) <= validator.withdrawable_epoch:
	return False

[mkalinin]

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.

[james-prysm]

Due to refactor that facilitates spec reading there are two places where validatorIndex is looked up by the pubkey (get_activation_churn_consumption and apply_pending_deposit functions), this happens for each pending deposit that is being processed. There is a less readable workaround where validatorIndex is resolved once, but I am not sure if it is necessary because client implementations are likely to use cache in both cases.

does this mean that the cache should continue to be used as opposed to the previous pr that looked for its removal in #3689

reading through I think my question is answered. we need it based on this PR

[james-prysm]

what about

if pubkey not in validator_pubkeys:
  if !s_valid_deposit_signature(pubkey, withdrawal_credentials, amount, signature):
    break
  add_validator_to_registry(state, pubkey, withdrawal_credentials, Gwei(0))
state.pending_deposits.append(PendingDeposit(
                pubkey=pubkey,
                withdrawal_credentials=withdrawal_credentials,
                amount=amount,
                signature=signature,
                slot=GENESIS_SLOT,
            ))

[mkalinin]

not sure if it makes these lines more readable, or there is another intention for this change?

[james-prysm]

oops missed this comment, yeah just readability, as the appending logic is duplicated as far as I could tell

[ralexstokes]

great work!

I think this is the next best step here

I'd also want to see performance numbers if we just rely on the churn limit instead of having an additional constant

and I'd also lean against the next suggested step around managing the queue in a more sophisticated way unless its obvious we need it for performance reasons

[mkalinin]

I'd also want to see performance numbers if we just rely on the churn limit instead of having an additional constant

The most optimal DoS strategy seems to be the following:

1. Create validator with 1 ETH balance and ETH1 creds
2. Top-up the validator with 1 ETH and COMPOUNDING creds

Each of these steps yields signature verification. Current churn limit allows to create 128 validators using this strategy, that is 256 signature verifications at the epoch processing boundary (256ms). The cost of the attack is locked ETH and some amount of ETH required to activate those validators by topping them up with 30 ETH and withdraw them. In the worst case it will take yet another 15 epochs to withdraw all malicious validators. Potential outcome and the cost makes this attack ineffective to my perspective. If the churn limit is increased then the outcome may become worse but the cost will also increase.

[ensi321]

Just wondering in the case of devnet where Electra is already activated at genesis, would this (or other instance of appending PendingDeposit with GENESIS_SLOT to state.pending_deposit) yield an unexpected result?

[mkalinin]

The protocol does not accept a block proposed in the GENESIS_SLOT because of the following check in the process_block_header:

assert block.slot > state.latest_block_header.slot

[ensi321]

Looks like apply_deposit() along with process_deposit() will be abandoned in Electra after all eth1 deposits are processed and will only be used for generating genesis state for devnets.

[mkalinin]

Yes, these functions are going to be deprecated and I think that we will be able/should update the genesis state generation if possible and get rid of these functions

[ensi321]

Can you explain why deposit.index is no longer a reliable way to access validator index but need to use pubkey to fetch it instead?

[mkalinin]

deposit.index is the index of the deposit record in the smart contract, it can’t be used to derive the validator_index anyhow

[james-prysm]

just signalling support , but also waiting for switch_to_compounding piggy backing off consolidations

[ralexstokes]

AIUI we would merge in #3918 which will simplify this PR a bit further

does this align with your understanding @mkalinin ?

[mkalinin]

AIUI we would merge in #3918 which will simplify this PR a bit further

does this align with your understanding @mkalinin ?

yes, the plan is to wait for that PR and then merge this one

[Scutua]

Hello

[mkalinin]

switch_to_compounding_validator is removed from the deposit flow in favour of #3918