Pruning pending deposits in deposit cache for post-Electra(EIP-6110)
Closed this issue · 10 comments
💎 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.
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.
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.
Thanks for this we haven't had time to review the findings but appreciate it, will be looking into this soon.
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 (https://github.com/prysmaticlabs/prysm/blob/78722239da40bb980c4168b1c194ded95d3fd7ff/beacon-chain/core/electra/deposits.go#L92) when the new electra deposit processing runs for 6110 🤔 wouldn't then that value update?
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.
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.
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.
@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 cacheUltimately 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.
@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.
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
Thanks! It seems much clear. Can I tackle this? I'd first add a function for pruning caches in post-Electra.
Please do so, I have assigned it to you