v2.0: Marks old storages as dirty and uncleaned in clean_accounts() (backport of #3737) #3747
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
Copied from #3702
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, and add to the uncleaned roots list 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/mmaps. There's also the general performance regressions that occur as these old account storage files are unexpectedly kept around forever.
Additional Testing
I started up a node running this PR, and used a snapshot containing over 800k account storage files. The node was quickly able to remove all the old storage files and resume normal behavior.
Here's a graph of the node's count of storages. It starts around 850k and quickly drops to the correct ~432k:

This is an automatic backport of pull request #3737 done by [Mergify](https://mergify.com).