v2.1: Marks old storages as dirty in clean_accounts() (backport of #3702) #3707
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
We do not clean up old storages.
More context: when calculating a full accounts hash, we call
mark_old_slots_as_dirty()
as a way to ensure we do not forget or miss cleaning up really old storages (i.e. ones that are older than an epoch old). But, when we enable skipping rewrites, we don't want to clean up those old storages, as they'll intentionally be treated as ancient append vecs. So insidemark_old_slots_as_dirty()
we conditionally mark old slots as dirty. This is based on the value ofancient_append_vec_offset
, which should be None unless ancient append vecs are enabled.Unfortunately, normal running validators, we end up never marking old slots as dirty, because the ancient append vec offset is always Some. And thus we don't clean up old storages.
Summary of Changes
Mark old storages as dirty in clean_accounts().
We still check if ancient append vecs are enabled, but not with the
ancient_append_vec_offset
. Instead we look at the skipping rewrites feature gate and the cli arg.By moving this marking into clean_accounts(), we also decouple it from accounts hash calculation, which is not necessary anymore. This also removes behavioral differences based on if snapshots are enabled or not.
Justification to Backport
Without this fix, nodes may never clean up old account storage files, leading to eventual crashes due to running out of file descriptors. There's also the general performance regression that occurs as these old account storage files are unexpectedly kept around forever.
This is an automatic backport of pull request #3702 done by [Mergify](https://mergify.com).