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

Implement beacon db pruner #14687

Merged
merged 30 commits into from
Jan 28, 2025
Merged

Implement beacon db pruner #14687

merged 30 commits into from
Jan 28, 2025

Conversation

dB2510
Copy link
Contributor

@dB2510 dB2510 commented Dec 2, 2024

What type of PR is this?
Feature

What does this PR do? Why is it needed?
This PR implements beacon db pruning based on weak subjectivity period. This is needed to avoid storing historical blocks beyond a point for non-archival nodes.

Which issues(s) does this PR fix?
#8787

Other notes for review
This PR implements a pruner service based on weak subjectivity period to periodically delete blocks and states beyond weak subjectivity period to maintain a constant-size database.

Acknowledgements

  • I have read CONTRIBUTING.md.
  • I have made an appropriate entry to CHANGELOG.md.
  • I have added a description to this PR with sufficient context for reviewers to understand this PR.

Copy link
Member

@prestonvanloon prestonvanloon left a comment

Choose a reason for hiding this comment

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

Just one comment on the interface name.

The implementation looks good to me. Do you plan to plug it in with a feature flag in a follow up PR? Have you tested this?

beacon-chain/db/iface/interface.go Outdated Show resolved Hide resolved
@rkapka
Copy link
Contributor

rkapka commented Dec 13, 2024

No flag to customize the retention period?

@dB2510
Copy link
Contributor Author

dB2510 commented Dec 16, 2024

@rkapka

No flag to customize the retention period?

Will include it in this PR.

@dB2510 dB2510 changed the title Implement weak subjectivity pruner Implement beacon db pruner Dec 23, 2024
@dB2510 dB2510 marked this pull request as ready for review December 24, 2024 10:45
@dB2510 dB2510 requested a review from a team as a code owner December 24, 2024 10:45
cmd/beacon-chain/flags/base.go Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
beacon-chain/db/kv/blocks.go Outdated Show resolved Hide resolved
beacon-chain/db/kv/blocks.go Outdated Show resolved Hide resolved
beacon-chain/db/kv/blocks.go Outdated Show resolved Hide resolved
beacon-chain/db/kv/blocks.go Outdated Show resolved Hide resolved
beacon-chain/db/pruner/pruner.go Show resolved Hide resolved
beacon-chain/db/pruner/pruner.go Show resolved Hide resolved
beacon-chain/db/pruner/pruner.go Outdated Show resolved Hide resolved
beacon-chain/db/pruner/pruner_test.go Outdated Show resolved Hide resolved
beacon-chain/db/pruner/pruner_test.go Outdated Show resolved Hide resolved
@rkapka
Copy link
Contributor

rkapka commented Dec 30, 2024

Additional comment:

How about removing data for each bucket (or at least each major one) in parallel instead of block,state,block,state...? Using a wait group (I think) to make sure we wait for everything before the transaction completes. Before doing that we should check how long it takes to prune in the unhappy case. If it's a 100ms, then no need to optimize, but if it's 10s then it's worth it. The question then becomes what actually is the unhappy case? How many epochs of non-finality can the pruner expect? I am also wondering what happens when the pruner takes more than 12s to complete, as this will kick the next round of pruning before the previous has even completed. But that's probably a very unhappy case lol.

beacon-chain/db/kv/blocks.go Outdated Show resolved Hide resolved
beacon-chain/db/kv/blocks.go Outdated Show resolved Hide resolved
rkapka
rkapka previously approved these changes Jan 9, 2025
Copy link
Contributor

@kasey kasey left a comment

Choose a reason for hiding this comment

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

Overall you did a nice job on this, looking good!

Most of my comments are suggestions or even nitpicks, but I do think we should make the db query improvements - in particular using the slot from the index - before merging this.

CHANGELOG.md Outdated Show resolved Hide resolved
beacon-chain/db/kv/blocks.go Outdated Show resolved Hide resolved
beacon-chain/db/pruner/pruner.go Outdated Show resolved Hide resolved
beacon-chain/db/pruner/pruner.go Outdated Show resolved Hide resolved
beacon-chain/db/pruner/pruner.go Outdated Show resolved Hide resolved
beacon-chain/db/pruner/pruner.go Outdated Show resolved Hide resolved
beacon-chain/sync/backfill/service.go Outdated Show resolved Hide resolved
beacon-chain/sync/backfill/service.go Outdated Show resolved Hide resolved
beacon-chain/sync/backfill/service.go Outdated Show resolved Hide resolved
beacon-chain/db/pruner/pruner.go Show resolved Hide resolved
bkt *bolt.Bucket,
startSlotEncoded, endSlotEncoded, startEpochEncoded, endEpochEncoded, slotStepEncoded interface{},
) ([][]byte, error) {
_, span := trace.StartSpan(ctx, "BeaconDB.blockRootsBySlotRange")
Copy link
Contributor

Choose a reason for hiding this comment

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

Why were these trace spans removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

@dB2510 dB2510 added this pull request to the merge queue Jan 28, 2025
Merged via the queue into develop with commit 4a9c60f Jan 28, 2025
15 checks passed
@dB2510 dB2510 deleted the beacondbpruning branch January 28, 2025 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants