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.

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

Prune pending deposits from the deposit cache post-Electra #14829

Merged
merged 16 commits into from
Feb 7, 2025

Conversation

syjn99
Copy link
Contributor

@syjn99 syjn99 commented Jan 27, 2025

What type of PR is this?

Feature

What does this PR do? Why is it needed?

After conversations with @james-prysm and @nisdas at #14698, this will be a first step to mitigate an memory effect after post-Electra.
This PR adds DepositPruner, which prunes all pending deposits and proofs after Electra(when legacy eth1 polling is deprecated). a7eae66 moves some functions in deposit_fetcher.go into deposit_pruner.go, as it seems more suitable. deposit_pruner.go might contain pruning whole deposits, but I thought that it could be handled at other PRs.

I did some experiment on local devnet. I spammed deposit transactions at those devnets, and here's the metrics:

image

Normally, after post-Electra, pending deposits are not pruned at all, it leads pending deposits to infinitely grow like the panel shows.

image

When this PR applied, pending deposits are pruned(Red line) at every finalization event. (You can see the config file for kurtosis at this gist.) depositsnapshot doesn't affect the consensus after post-Electra at all.

Which issues(s) does this PR fix?

Resolves #14698

Other notes for review

Adding DepositPruner could be quite controversial, so feedbacks are welcomed.

Acknowledgements

@james-prysm james-prysm added the Electra electra hardfork label Jan 28, 2025
@syjn99 syjn99 mentioned this pull request Jan 29, 2025
3 tasks
Rationale:
As deposit_fetcher.go contains all pruning logics, it would be better to separate its interest into fetcher/inserter/pruner.
@syjn99 syjn99 changed the title [WIP] Prune deposit cache in post-Electra Prune deposit cache in post-Electra Jan 29, 2025
@syjn99 syjn99 marked this pull request as ready for review January 29, 2025 12:47
@syjn99 syjn99 requested a review from a team as a code owner January 29, 2025 12:47
@syjn99 syjn99 changed the title Prune deposit cache in post-Electra Prune pending deposits at deposit cache in post-Electra Jan 29, 2025
@syjn99 syjn99 changed the title Prune pending deposits at deposit cache in post-Electra Prune pending deposits from the deposit cache post-Electra Jan 29, 2025
@james-prysm james-prysm requested a review from nisdas January 29, 2025 15:44
@syjn99 syjn99 marked this pull request as draft January 30, 2025 15:17
@james-prysm
Copy link
Contributor

Electra will be looking to release in the next month , we would like to get this into the release depending on your findings with testing. we mostlikely will be releasing in testnet without this feature yet.

@syjn99
Copy link
Contributor Author

syjn99 commented Feb 1, 2025

@james-prysm Yes, I agree to the point. Then I'll keep this PR in draft mode.

@james-prysm
Copy link
Contributor

james-prysm commented Feb 2, 2025

please let us know if you think this will be ready in the next few weeks for mainnet 🙏 thank you for the work

@syjn99
Copy link
Contributor Author

syjn99 commented Feb 3, 2025

@james-prysm
Thanks! Observing a Grafana dashboard with two metrics would be feasible.

  • beacondb_pending_deposits_eip4881
  • beacondb_all_deposits_eip4881

@james-prysm
Copy link
Contributor

@syjn99 we're going to try to get this into the release for testnet actually. I'll see if I can pick up where you left off here if nishant had any comments. he'll review tomorrow and we'll get this merged. please let us know if there are any concerns.

@james-prysm
Copy link
Contributor

@syjn99 are you able to let me push changes directly to your branch? otherwise I can create a fork of your branch I think or start a new one with acknowledgment to you

@syjn99
Copy link
Contributor Author

syjn99 commented Feb 6, 2025

@james-prysm Just pushed a commit that applies all reviews from team!

@syjn99 syjn99 requested review from james-prysm and nisdas February 6, 2025 16:08
@syjn99 syjn99 marked this pull request as ready for review February 6, 2025 16:09
@james-prysm
Copy link
Contributor

james-prysm commented Feb 6, 2025

@syjn99 looks like unit tests are failing Test_executePostFinalizationTasks

I think that's a test I added in the past, it's an electra state 😓 so I guess it shouldn't have that log anymore

@syjn99
Copy link
Contributor Author

syjn99 commented Feb 6, 2025

@james-prysm How about adding test cases, and separating them into pre-Electra and post-Electra?

@james-prysm
Copy link
Contributor

james-prysm commented Feb 6, 2025

I think this is silly... there's a bug I think, because it's a fake state eth1depositindex of 0 is the same as the requests start index of 0

I don't think that's possible in any other state but probably a bug with this test not using realistic values.

// DepositRequestsStarted determines if the deposit requests have started.
func DepositRequestsStarted(beaconState state.BeaconState) bool {
	if beaconState.Version() < version.Electra {
		return false
	}

	requestsStartIndex, err := beaconState.DepositRequestsStartIndex()
	if err != nil {
		return false
	}
       
	return beaconState.Eth1DepositIndex() == requestsStartIndex
}

})
t.Run("deposit requests started", func(t *testing.T) {
require.NoError(t, headState.SetEth1DepositIndex(1))
require.NoError(t, headState.SetDepositRequestsStartIndex(1))
Copy link
Contributor

@james-prysm james-prysm Feb 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe this is still too much copy paste, just wanted to fix it

@james-prysm
Copy link
Contributor

james-prysm commented Feb 7, 2025

@syjn99 Thanks for your work on this ! it was a great catch, we'll get this merged

@james-prysm james-prysm enabled auto-merge February 7, 2025 04:03
@james-prysm james-prysm added this pull request to the merge queue Feb 7, 2025
Merged via the queue into prysmaticlabs:develop with commit 557c5be Feb 7, 2025
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Electra electra hardfork
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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