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

AccountsHashVerifier purges old bank snapshots #31519

Merged
merged 1 commit into from
May 12, 2023

Conversation

brooksprumo
Copy link
Contributor

@brooksprumo brooksprumo commented May 5, 2023

Problem

We retain more bank snapshots than we need, longer than we need.

Building on #31511, what if a validator turns off generating snapshots? In that case, the node will still take bank snapshots since they are needed for accounts hash verification. Only snapshots intended for archival are turned off (this is the majority though).

In this scenario, we may not clean up old bank snapshots.

This is/will especially be true once AccountsBackgroundService is not in charge of cleaning up old bank snapshots.

In that case, since AccountsHashVerifier is the last one to consume the bank snapshot, it should be the one to clean them up as well.

Summary of Changes

AccountsHashVerifier will purge bank snapshots older than the one that was just processed IFF snapshot (archival) is disabled.

@brooksprumo brooksprumo self-assigned this May 5, 2023
@brooksprumo brooksprumo added the noCI Suppress CI on this Pull Request label May 9, 2023
@brooksprumo brooksprumo force-pushed the snap/purge-in-ahv branch 3 times, most recently from 0c16677 to a9a7a76 Compare May 12, 2023 17:27
@brooksprumo brooksprumo added CI Pull Request is ready to enter CI and removed noCI Suppress CI on this Pull Request labels May 12, 2023
@solana-grimes solana-grimes removed the CI Pull Request is ready to enter CI label May 12, 2023
@codecov
Copy link

codecov bot commented May 12, 2023

Codecov Report

Merging #31519 (c17312a) into master (8e5e66f) will decrease coverage by 0.1%.
The diff coverage is 57.1%.

@@            Coverage Diff            @@
##           master   #31519     +/-   ##
=========================================
- Coverage    81.3%    81.2%   -0.1%     
=========================================
  Files         733      733             
  Lines      206693   206698      +5     
=========================================
- Hits       168064   168033     -31     
- Misses      38629    38665     +36     

@brooksprumo brooksprumo marked this pull request as ready for review May 12, 2023 20:28
@brooksprumo brooksprumo requested a review from apfitzge May 12, 2023 20:28
Copy link
Contributor

@apfitzge apfitzge left a comment

Choose a reason for hiding this comment

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

lgtm - change seems reasonable; if we aren't dooing archiving, we don't need these snapshots except for accounts hashing or fast-boot.

@brooksprumo brooksprumo merged commit 962650e into solana-labs:master May 12, 2023
@brooksprumo brooksprumo deleted the snap/purge-in-ahv branch May 12, 2023 21:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants