Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Closed
syjn99 opened this issue Dec 8, 2024 · 10 comments · Fixed by #14829
Closed

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

syjn99 opened this issue Dec 8, 2024 · 10 comments · Fixed by #14829
Assignees
@syjn99 @james-prysm
Labels
Electra electra hardfork

Comments

@syjn99
Copy link
Contributor

syjn99 commented Dec 8, 2024

💎 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.
@syjn99 syjn99 mentioned this issue Dec 8, 2024
Merged
3 tasks
@syjn99 syjn99 changed the title Pruning pending deposits in deposit snapshot for post-Electra(EIP-6110) Pruning pending deposits in deposit cache for post-Electra(EIP-6110) Dec 8, 2024
@james-prysm james-prysm added the Electra electra hardfork label Jan 6, 2025
@james-prysm james-prysm self-assigned this Jan 6, 2025
@james-prysm
Copy link
Contributor

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

@james-prysm
Copy link
Contributor

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
Copy link
Contributor Author

syjn99 commented Jan 23, 2025

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
Copy link
Contributor

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
Copy link
Contributor Author

syjn99 commented Jan 24, 2025

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.

@nisdas
Copy link
Member

nisdas commented Jan 24, 2025

@syjn99 If you are interested to tackle this what we can simply do is to check if the

if  state.Eth1DepositIndex >= state.DepositRequestsStartIndex {

// Prune all deposit proofs in cache
// Prune all pending deposits in cache

Ultimately we should also pause deposit log processing once this threshold is hit as that is what leads to all the pending deposits being stored in our cache. But it requires more thought on how to disable that cleanly as its tied up with many things in our execution service.

My first idea is following. Feedback is welcomed.

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

We ultimately want to remove all the deposit log processing logic so there isn't any benefit to adding in a feed here as it will utimately also be removed after electra.

@syjn99
Copy link
Contributor Author

syjn99 commented Jan 24, 2025

@nisdas Thank you for your comment. I wonder whether depositsnapshot should be fully deleted or not. As I mentioned at the previous comment, the deposit_snapshot API(/eth/v1/beacon/deposit_snapshot) uses the depositsnapshot package, and there is no discussion about how to deal with this API among core devs. (Actually I raised this issue at discord, but it seems trivial compared to other issues in Pectra)

Do we have to return error(like "not supported") if it is post-Electra? If not, what number do we have to return for deposit_count? Does deposit_count should include the deposits from EL requests?

Also, as you mentioned, there are some parts like rpc packacge that relies on the execution service, so removing the service could be a heavy work.

I'm glad to tackle this issue in any way. A starting point of this issue is to add a new function for pruning all things in cache, like insertFinalizedDeposits does.

prysm/beacon-chain/blockchain/process_block_helpers.go

Lines 553 to 592 in 5d6a406

// inserts finalized deposits into our finalized deposit trie, needs to be
// called in the background
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)
}

@nisdas
Copy link
Member

nisdas commented Jan 24, 2025

The deposit snaphot api will be deprecated after electra is transitioned. Like most of the other things such as eth1data voting and deposit log processing it would be deprecated the same way. Clients could still be theoretically support providing snapshots even though it is no longer needed. We can leave it on for now

@syjn99
Copy link
Contributor Author

syjn99 commented Jan 24, 2025 •
edited
Loading

Thanks! It seems much clear. Can I tackle this? I'd first add a function for pruning caches in post-Electra.

@nisdas
Copy link
Member

nisdas commented Jan 24, 2025

Please do so, I have assigned it to you

@syjn99 syjn99 mentioned this issue Jan 27, 2025
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@syjn99 syjn99

@james-prysm james-prysm

Labels
Electra electra hardfork
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Prune pending deposits from the deposit cache post-Electra syjn99/prysm
3 participants
@nisdas @syjn99 @james-prysm