Pruning pending deposits in deposit cache for post-Electra(EIP-6110)

[syjn99]

💎 Issue

Background

As EIP-6110 is included in the upcoming upgrade(Electra), deposits are directly bridged from EL to CL via execution requests(EIP-7685). Thus, the implementation of EIP-4881 only matters when providing a deposit snapshot via RPC(Let me know if there's other things to consider).

When a block producer builds a block, packDepositsAndAttestations will always return empty deposit list from local when EIP-6110 is fully enabled. Returning early can help for reducing the total building time in this case, so I already worked on this issue at PR #14697.

Description

However, I found we need to tackle with the pending deposits which are managed by DepositCache. (NOTE: This pending deposit is not equal to the new field introduced in Electra) As eth1_deposit_index will be anchored to the certain value, insertFinalizedDeposits will never finalize the included deposits after EIP-6110 applied.

prysm/beacon-chain/blockchain/process_block_helpers.go

Lines 538 to 575 in 30a136f

	func (s *Service) insertFinalizedDeposits(ctx context.Context, fRoot [32]byte) {
	ctx, span := trace.StartSpan(ctx, "blockChain.insertFinalizedDeposits")
	defer span.End()
	startTime := time.Now()
	// Update deposit cache.
	finalizedState, err := s.cfg.StateGen.StateByRoot(ctx, fRoot)
	if err != nil {
	log.WithError(err).Error("could not fetch finalized state")
	return
	}
	// We update the cache up to the last deposit index in the finalized block's state.
	// We can be confident that these deposits will be included in some block
	// because the Eth1 follow distance makes such long-range reorgs extremely unlikely.
	eth1DepositIndex, err := mathutil.Int(finalizedState.Eth1DepositIndex())
	if err != nil {
	log.WithError(err).Error("could not cast eth1 deposit index")
	return
	}
	// The deposit index in the state is always the index of the next deposit
	// to be included(rather than the last one to be processed). This was most likely
	// done as the state cannot represent signed integers.
	finalizedEth1DepIdx := eth1DepositIndex - 1
	if err = s.cfg.DepositCache.InsertFinalizedDeposits(ctx, int64(finalizedEth1DepIdx), common.Hash(finalizedState.Eth1Data().BlockHash),
	0 /* Setting a zero value as we have no access to block height */); err != nil {
	log.WithError(err).Error("could not insert finalized deposits")
	return
	}
	// Deposit proofs are only used during state transition and can be safely removed to save space.
	if err = s.cfg.DepositCache.PruneProofs(ctx, int64(finalizedEth1DepIdx)); err != nil {
	log.WithError(err).Error("could not prune deposit proofs")
	}
	// Prune deposits which have already been finalized, the below method prunes all pending deposits (non-inclusive) up
	// to the provided eth1 deposit index.
	s.cfg.DepositCache.PrunePendingDeposits(ctx, int64(eth1DepositIndex)) // lint:ignore uintcast -- Deposit index should not exceed int64 in your lifetime.
	log.WithField("duration", time.Since(startTime).String()).Debugf("Finalized deposit insertion completed at index %d", finalizedEth1DepIdx)
	}

As a result, pending deposits are never pruned by this logic, leading the heap memory allocation will never be freed. I had investigated this issue for a couple of hours, but it seems there needs a way to notify the "finalized" deposit index to the deposit cache to prune the pending deposits.

prysm/beacon-chain/core/electra/deposits.go

Lines 591 to 614 in 30a136f

	func processDepositRequest(beaconState state.BeaconState, request *enginev1.DepositRequest) (state.BeaconState, error) {
	requestsStartIndex, err := beaconState.DepositRequestsStartIndex()
	if err != nil {
	return nil, errors.Wrap(err, "could not get deposit requests start index")
	}
	if requestsStartIndex == params.BeaconConfig().UnsetDepositRequestsStartIndex {
	if request == nil {
	return nil, errors.New("nil deposit request")
	}
	if err := beaconState.SetDepositRequestsStartIndex(request.Index); err != nil {
	return nil, errors.Wrap(err, "could not set deposit requests start index")
	}
	}
	if err := beaconState.AppendPendingDeposit(&ethpb.PendingDeposit{
	PublicKey: bytesutil.SafeCopyBytes(request.Pubkey),
	WithdrawalCredentials: bytesutil.SafeCopyBytes(request.WithdrawalCredentials),
	Amount: request.Amount,
	Signature: bytesutil.SafeCopyBytes(request.Signature),
	Slot: beaconState.Slot(),
	}); err != nil {
	return nil, errors.Wrap(err, "could not append deposit request")
	}
	return beaconState, nil
	}

My first idea is following. Feedback is welcomed.

1. Add a feed that notifies the last deposit index(and slot) that is included in the state.
2. Execution service subscribes to this feed, and keeps updating the last deposit index(and slot).
3. If the finalized checkpoint is updated, other pruning logic runs.

[james-prysm]

Thanks for this we haven't had time to review the findings but appreciate it, will be looking into this soon.

[james-prysm]

I think i'm misunderstanding something here( will be reaching out to team members who have worked on EIP4881) but eth1_deposit_index should be updated here (

prysm/beacon-chain/core/electra/deposits.go

Line 92 in 7872223

if err := beaconState.SetEth1DepositIndex(beaconState.Eth1DepositIndex() + 1); err != nil {

) when the new electra deposit processing runs for 6110 🤔 wouldn't then that value update?

[syjn99]

https://eips.ethereum.org/EIPS/eip-6110#eth1data-poll-deprecation

In post-Electra, eth1_deposit_index will keep increasing until the legacy process is deprecated. After deprecation(which means state.eth1_deposit_index == state.deposit_requests_start_index), eth1_deposit_index will be fixed forever, as all deposits are originated from execution requests.

[james-prysm]

correct, but we won't need the deposit cache afterwards 🤔 once everything is transitioned to the 6110 process.

I had investigated this issue for a couple of hours, but it seems there needs a way to notify the "finalized" deposit index to the deposit cache to prune the pending deposits.

I was stuck on this comment. I initially thought it was around this transition period that we maybe missed something, but if its post electra it should just be deprecated and eventually removed.

[syjn99]

Indeed, IMO we don't need deposit snapshot except for this API(/eth/v1/beacon/deposit_snapshot). But AFAIK this API is not deprecated in Electra, so I'm afraid that we may maintain depositsnapshot package.