Finalize deposit before applying it to a state

[mkalinin]

Proposal

Extends state.pending_balance_deposits to keep the deposit data and an epoch when a deposit was processed by the consensus layer. The deposit is applied to the state only when that epoch gets finalized and if the activation churn has enough capacity.

Analysis

• Reduces engineering complexity of EIP-6110 implementation as an index of a new validator is associated with the pubkey only after the deposit is finalized, so there is no need to support forks in (pubkey, index) cache implementation.
• Increases beacon state size as each entry of the deposits queue needs to hold 176 bytes of additional data (pubkey, withdrawal credentials and signature) which is a 12 times increase. For example, with 100_000 validators in the activation queue the pending deposits queue will consume ~18MB vs 1.5MB of just pending deposit balances.
• Rate limits signature verifications per epoch by the means of the activation queue, but moves all verification operations to the epoch processing.
• Requires top-ups to be finalized before they can be processed.
• Invalid deposits will waste churn. Considering a high cost of submitting an invalid deposit, the potential effect should be negligible.
• Makes no use for validator.activation_aligibility_epoch which can be repurposed in the future.

[dapplion]

The implementation complexity of the fork-aware pubkey cache has not been as bad as I originally imagined in Lodestar and Lighthouse. Would like to know from other teams too.

[fradamt]

Since deposits are not applied until after finalization, we don't need activation_eligibility_epoch anymore, as its role is replaced by deposit.epoch in a PendingDeposit. Then we can remove is_eligible_for_activation_queue from process_registry_updates, and move its balance check to is_eligible_for_activation:

def is_eligible_for_activation(state: BeaconState, validator: Validator) -> bool:
    """
    Check if ``validator`` is eligible for activation.
    """
    return (
        # Has not yet been activated
        and validator.activation_epoch == FAR_FUTURE_EPOCH
        # Has sufficient effective balance
        and validator.effective_balance >= MIN_ACTIVATION_BALANCE
    )

[fradamt]

What about checking is_valid_deposit_signature here? Unless we specifically want to avoid burning a top-up with incorrect signature

[mkalinin]

There are two things why do signature check as is:

1. Some infra might already have top-ups sent with zero signatures (worth checking tho)
2. Rate limiting a number of deposit signature checks per epoch is really nice, it alleviates the risk of delaying block processing if someone manages to pack a thousand deposits into it

[StefanBratanov]

The implementation complexity of the fork-aware pubkey cache has not been as bad as I originally imagined in Lodestar and Lighthouse. Would like to know from other teams too.

In Teku, we implemented it in a way, where we cache only the finalized indices and for the rest we do an N loop starting from the last finalized index + 1. It could be a problem for chains which are not finalizing for a long time, but otherwise I don't see a big issue with it, but would prefer if this PR makes it into the specs.

[dapplion]

An important consideration w.r.t. the inactivity leak is that this PR prevents top-ups during non-finality. This affects both the ability of leaking validators to maintain their stake share, and non-leaking validators from increasing their share.

[mkalinin]

An important consideration w.r.t. the inactivity leak is that this PR prevents top-ups during non-finality. This affects both the ability of leaking validators to maintain their stake share, and non-leaking validators from increasing their share.

This is an important consideration. Do you think there is any risk related to that?

Honest validators’ stake should remain stable as they keep voting — this is at least a desired property, but i don’t know if in practice this property holds in all cases as there might be not enough block space to accommodate all submitted attestations on chain thus honest validators may leak too. Preventing dishonest validators that aren’t voting from increasing their stake share is not strongly desired but a nice property, imo.

[etan-status]

I like the introduction of the queue, as it makes sure that BLS signature verifications remain bounded (16 / block). This way, the CL hardware requirements don't become a concern on every future gas limit increase.

Finality also simplifies implementations, because it means that validator indices to pubkey mappings are the same across all valid branches. As new validator activations can only happen while finalizing, this shouldn't affect the underlying PoS security.

The one thing that will no longer be possible is topping up a validator while the network is not finalizing / possibly leaking. I don't personally have concerns with that, but it should be documented at least as a semantic change that affects non-finalizing networks.

[etan-status]

2^32 would be the theoretical limit with infinite gas fee and a single block that fills the entire deposit tree

[etan-status]

This should also check for the legacy style deposits import to have caught up with the queue.

Otherwise, deposits from the old style (via eth1_data) and from the new queue can get reordered, possibly messing with decentralized staking pools that rely on a linear deposit order.

[terencechain]

The implementation complexity of the fork-aware pubkey cache has not been as bad as I originally imagined in Lodestar and Lighthouse. Would like to know from other teams too.

Same with Prysm. We implemented it the same way as Teku, as mentioned by @StefanBratanov. I'm indifferent to this, but if the sole purpose is to remove the fork-aware pubkey cache, I don't think it's necessary.

[mkalinin]

Outcomes of the breakout session on the change proposed by this PR:

• Queueing deposits is important to rate limit sig verification operations. There is a desire not only to limit a number of deposits processed per epoch by the churn, but also by a number (the proposed number is 16). The reason for that is that there can be legit blocks with hundreds of deposits in them (e.g. staking pools doing batch depositing) and we want to ensure that processing of such blocks won't be delayed.
• We want to benchmark processing a block with ~1000 deposits in it to understand how big of the problem it could be.
• Finalizing (pubkey, index) might not only matter for client implementations but also for tooling and infrastructure like block explorers. Though, having this cache fork-independent became a weak argument since most of the teams implemented it, it still can prevent potential bugs in clients and reduces complexity.
• To alleviate hash_tree_root complexity of potentially large and slicing queue, we want to explore two queue solution or queue with two pointers solution. The key intuition behind either of such solutions is that any queue is append only and reset to empty, no slicing is possible.
• Deposits should be processed in strict order around the transition to prevent frontrunning. Potential solution is to create new validators induced by Eth1Data poll deposits instantly but add synthesized deposits with the entire balance of that validators to the end of the queue so the balance will hit the churn; and prevent new style deposits from being processed until the Eth1Data poll gap is filled.
• We agreed that we do accept data complexity (potential state growth) and the fact that no top-ups is possible during period of non-finality. The latter even seems a positive thing.

[pawanjay176]

Benchmarked a block with 1000 deposits using lcli.
Attaching the block and pre-state if anyone wants to run it on their clients.
big-deposit-test.zip

[2024-05-17T11:59:49Z INFO  lcli::transition_blocks] Run 0: 841.606125ms                                                                                                                                              
[2024-05-17T11:59:49Z DEBUG lcli::transition_blocks] Build caches: 1.894041ms                                                                                                                                         
[2024-05-17T11:59:49Z DEBUG lcli::transition_blocks] Initial tree hash: 225.958µs                                                                                                                                     
[2024-05-17T11:59:49Z DEBUG lcli::transition_blocks] Slot processing: 5.209µs                                                                                                                                         
[2024-05-17T11:59:49Z DEBUG lcli::transition_blocks] Build all caches (again): 875ns                                                                                                                                  
[2024-05-17T11:59:49Z DEBUG lcli::transition_blocks] Batch verify block signatures: 4.483875ms                                                                                                                        
[2024-05-17T11:59:50Z DEBUG lcli::transition_blocks] Process block: 838.27275ms                                                                                                                                       
[2024-05-17T11:59:50Z DEBUG lcli::transition_blocks] Post-block tree hash: 1.896833ms

(Note: the Batch verify block signatures time doesn't include deposit signatures)
The block was generated on a local kurtosis network by making bulk deposits.

Even on this relatively small network, the block arrived quite late on the node and took quite a bit to process.

May 17 11:56:13.776 DEBG Delayed head block                      set_as_head_time_ms: 2, imported_time_ms: 92, attestable_delay_ms: 6756, available_delay_ms: 6682, execution_time_ms: 3179, blob_delay_ms: unknown, observed_delay_ms: 3502, total_delay_ms: 6776, slot: 36, proposer_index: 125, block_root: 0x20ab5155dff25ded6c704d97028297a8d33af4b171c93b45c1bd7e8df3fec126, service: beacon

The execution layer processing itself took ~3secs. Consensus block processing is interleaved with the execution processing in lighthouse, so overall it took total_delay - observed_delay ~ 3.2s for processing the block.

[mkalinin]

@pawanjay176 thanks a lot for this benchmark!

Even on this relatively small network, the block arrived quite late on the node

I am curious why is that happened? It’s hard to believe that the propagation time took that many especially considering a small network. My guess would be that it wasn’t sent to the wire before fully processed by the proposer’s node, wdyt?

[pawanjay176]

Sorry took a while, was on vacation.

My guess would be that it wasn’t sent to the wire before fully processed by the proposer’s node, wdyt?

Yeah I think that is right, the propagation seems near instant but producing the big block, sending it to the VC across the http api, getting it back and then publishing it finally took longer than for a smaller block.

[etan-status]

Correct to assume that validators can never have an activation_eligibility_epoch > ELECTRA_FORK_EPOCH? e.g., by depositing during the last slot before electra activates.

[mkalinin]

Those validators are added to the pending deposits queue in upgrade_to_electra

[etan-status]

excess_balance? Because MIN_ACTIVATION_BALANCE was already credited.

[etan-status]

For SyncAggregate, G2_POINT_AT_INFINITY is used instead of a zero value when noone signed.

Or, None with EIP-7688 optionals (but I guess zero value or infinity is also fine, as it is not checked anyway).

[etan-status]

Is there an off-by-one here?

When epoch N finalizes, everything up through and including compute_start_slot_at_epoch(N) is finalized.

1. If activations are processed for deposit.epoch <= finalized_checkpoint.epoch, then that would also activate deposits on the non-finalized slots within N (only the initial slot of N is finalized).
2. If activations are processed for deposit.epoch < finalized_checkpoint.epoch, then deposits within the very first slot of N are not processed for another ~6 minutes despite already being finalized.

Currently, it seems we are following the second strategy (deposit.epoch >= state.finalized_checkpoint.epoch). Probably fine to have the extra ~6 minutes wait for deposits within the first slot, but could be avoided, so still keeping the comment for awareness.

[mkalinin]

I personally in favour of strategy (2) because of its simplicity

[potuz]

I thought about this and I changed my mind. The issue is that clients may (and probably will) filter deposits by blocks or db invariants like "finalized", the presence of this off by one creates edge cases that need to be always checked and tested. While it's not likely to create a consensus bug I feel it will be a source of bugs nonetheless

[mkalinin]

If we set deposit.epoch in the queue to get_current_epoch(state) + 1, will that prevent issues that you have in mind? It can also be renamed to deposit.activation_epoch to better reflect the semantics

[etan-status]

I guess storing slot and processing deposits up through deposit_receipt.slot <= compute_start_slot_at_epoch(state.finalized_checkpoint.epoch) is the most straight forward.

[etan-status]

Are withdrawal_credentials necessary to be saved here?

• If yes, there could be a race where a BLSToExecutionChange gets processed while the PendingDeposit is in the queue, changing the withdrawal credentials of the validator while the PendingDeposit retains the stale entry.
• If no, we could just set this to a zero value (or to None with EIP-7688 optionals).

[mkalinin]

It isn’t necessary so I think that setting it to zero value should be fine

[mkalinin]

Closing in favour of #3818

[mkalinin]

Closing in favour of #3818