Reclaims more old accounts in `clean` #4044

brooksprumo · 2024-12-10T19:32:36Z

Problem

In clean_accounts(), if there are old storages and old accounts beyond what is expected, we do not always reclaim them. This is because when scanning the candidates during clean, if the account's latest slot in the slot list is not in the uncleaned_roots list, we don't collect reclaims.

This can happen if there are candidates from old/ancient slots, where their newest entry is in a slot that is not being visited by clean. We would find multiple entries for this candidate in the index, yet determine not to collect reclaims.

Summary of Changes

Collect reclaims on candidates that have slot lists greater than one entry, and if they are all cleanable (i.e. rooted).

jeffwashington · 2024-12-10T20:38:10Z

this looks correct. This seems like we can explain now why the brute force method wasn't working, too. And, since we were handling duplicates on the index correctly when we were starting with the in-mem index only. This seems correct.

HaoranYi

Nice find.
looks correct to me.

brooksprumo · 2024-12-10T22:06:31Z

I spun up a mnb node with the disk index disabled to see how it performed. Here's some results:

clean always has a lot of work to do on the first iteration. And we see it removing the old slots:

And since we're cleaning up reclaims, shrink can do a better job now too, purging lots of zero lamport accounts:

HaoranYi

lgtm

accounts-db/src/accounts_db.rs

HaoranYi

If max_clean_root_inclusive is none, we need to follow the old rule, i.e. slots must be in self.uncleaned_roots...

HaoranYi · 2024-12-11T21:19:58Z

Recently, we have seen a few clean related issues with different CLI configuration for validators.

I am trying to think carefully about different ways that clean might be broken when the validator is running with different configurations, i.e. w/o disk index; w/o snapshot generation ...

jeffwashington · 2024-12-12T19:25:17Z

accounts-db/src/accounts_db.rs

+                                            purges_old_accounts_local += 1;
+                                            useless = false;
+                                        }
+                                        // Note, this next if-block is only kept to maintain the


seems like we could get rid of this metric code and stat. or at least make it a fn to simplify the code in this more critical code path to make it easier to understand and verify correct. THis can be done later.

Yep, I agree. I'll save it for later to keep this PR small, to make it simpler to backport.

mergify · 2024-12-12T19:56:23Z

Backports to the stable branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule.

mergify · 2024-12-12T19:56:26Z

Backports to the beta branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule. Exceptions include CI/metrics changes, CLI improvements and documentation updates on a case by case basis.

(cherry picked from commit 3d43824) # Conflicts: # accounts-db/src/accounts_db.rs # accounts-db/src/accounts_db/tests.rs

* Reclaims more old accounts in `clean` (#4044) (cherry picked from commit 3d43824) # Conflicts: # accounts-db/src/accounts_db.rs # accounts-db/src/accounts_db/tests.rs * fixes merge conflicts --------- Co-authored-by: Brooks <[email protected]>

* Reclaims more old accounts in `clean` (#4044) (cherry picked from commit 3d43824) # Conflicts: # accounts-db/src/accounts_db.rs # accounts-db/src/accounts_db/tests.rs * fix merge conflicts --------- Co-authored-by: Brooks <[email protected]>

* v2.0: Reclaims more old accounts in `clean` (backport of anza-xyz#4044) (anza-xyz#4089) * Reclaims more old accounts in `clean` (anza-xyz#4044) (cherry picked from commit 3d43824) # Conflicts: # accounts-db/src/accounts_db.rs # accounts-db/src/accounts_db/tests.rs * fix merge conflicts --------- Co-authored-by: Brooks <[email protected]> * v2.0: Fixes clean_old_storages_with_reclaims tests (backport of anza-xyz#4147) (anza-xyz#4166) * Fixes clean_old_storages_with_reclaims tests (anza-xyz#4147) (cherry picked from commit 4eabeed) # Conflicts: # accounts-db/src/accounts_db/tests.rs * fix merge conflicts --------- Co-authored-by: Brooks <[email protected]> * v2.0: blockstore: mark slot as dead on data shred merkle root conflict (backport of anza-xyz#3970) (anza-xyz#4074) * blockstore: mark slot as dead on data shred merkle root conflict (anza-xyz#3970) (cherry picked from commit 5564a94) # Conflicts: # ledger/src/blockstore.rs * fix conflicts --------- Co-authored-by: Ashwin Sekar <[email protected]> Co-authored-by: Ashwin Sekar <[email protected]> * Bump version to v2.0.22 (anza-xyz#4200) Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * v2.0: hardcode rust version for publish-crate (anza-xyz#4228) * Bump version to v2.0.23 (anza-xyz#4419) Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * v2.0: rolls out chained Merkle shreds to ~21% of mainnet slots (backport of anza-xyz#4431) (anza-xyz#4434) rolls out chained Merkle shreds to ~21% of mainnet slots (anza-xyz#4431) (cherry picked from commit 9d09787) Co-authored-by: behzad nouri <[email protected]> * v2.0: [rpc] Fatal `getSignaturesForAddress()` when Bigtable errors (backport of anza-xyz#3700) (anza-xyz#4442) [rpc] Fatal `getSignaturesForAddress()` when Bigtable errors (anza-xyz#3700) * Unindent code in `get_signatures_for_address` * Add a custom JSON-RPC error to throw when long-term storage (ie. Bigtable) can't be reached * When the `before`/`until` signatures can't be found, throw `SignatureNotFound` instead of `RowNotFound` * Fatal `getSignaturesForAddress` calls when Bigtable must be queried but can't be reached (cherry picked from commit 52f132c) Co-authored-by: Steven Luscher <[email protected]> * v2.0: ci: bump [upload|download]-artifact to v4 (anza-xyz#4501) ci: bump [upload|download]-artifact to v4 * v2.0: ci: hardcode crate publishing version (anza-xyz#4515) ci: hardcode rust version for publish-crate * Bump version to v2.0.24 (anza-xyz#4528) Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * v2.0: fix: reduce max packet receive time during leader window (backport of anza-xyz#2801) (anza-xyz#4544) fix: reduce max packet receive time during leader window (anza-xyz#2801) (cherry picked from commit 20e0df4) Co-authored-by: cavemanloverboy <[email protected]> * v2.0: Scheduler Frequency Fixes (backport of anza-xyz#4545) (anza-xyz#4576) * Change prio_graph_scheduler configurations for 1k maxs, 256 look ahead * Break loop on scanned transaction count * make Hold decision behave same as Consume during receive * receive maximum of 5_000 packets - loose max * receive_completed before process_transactions --------- Co-authored-by: Andrew Fitzgerald <[email protected]> --------- Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Brooks <[email protected]> Co-authored-by: Ashwin Sekar <[email protected]> Co-authored-by: Ashwin Sekar <[email protected]> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Yihau Chen <[email protected]> Co-authored-by: behzad nouri <[email protected]> Co-authored-by: Steven Luscher <[email protected]> Co-authored-by: cavemanloverboy <[email protected]> Co-authored-by: Andrew Fitzgerald <[email protected]>

Reclaims more old accounts in clean

ed25c03

brooksprumo self-assigned this Dec 10, 2024

HaoranYi previously approved these changes Dec 10, 2024

View reviewed changes

brooksprumo marked this pull request as ready for review December 10, 2024 22:08

brooksprumo requested a review from jeffwashington December 10, 2024 22:37

brooksprumo dismissed HaoranYi’s stale review via 00458ec December 11, 2024 16:40

adds test

eb1b7a0

brooksprumo force-pushed the clean/fix-reclaims branch from 00458ec to eb1b7a0 Compare December 11, 2024 16:42

brooksprumo requested a review from HaoranYi December 11, 2024 16:44

HaoranYi approved these changes Dec 11, 2024

View reviewed changes

HaoranYi reviewed Dec 11, 2024

View reviewed changes

accounts-db/src/accounts_db.rs Show resolved Hide resolved

HaoranYi self-requested a review December 11, 2024 20:39

HaoranYi reviewed Dec 11, 2024

View reviewed changes

accounts-db/src/accounts_db.rs Show resolved Hide resolved

HaoranYi requested changes Dec 11, 2024

View reviewed changes

add second test

a3438df

brooksprumo requested a review from HaoranYi December 11, 2024 21:47

HaoranYi approved these changes Dec 11, 2024

View reviewed changes

jeffwashington reviewed Dec 12, 2024

View reviewed changes

brooksprumo merged commit 3d43824 into anza-xyz:master Dec 12, 2024
40 checks passed

brooksprumo deleted the clean/fix-reclaims branch December 12, 2024 19:38

brooksprumo added v2.0 v2.1 Backport to v2.1 branch labels Dec 12, 2024

mergify bot pushed a commit that referenced this pull request Dec 12, 2024

Reclaims more old accounts in clean (#4044)

1102014

(cherry picked from commit 3d43824) # Conflicts: # accounts-db/src/accounts_db.rs # accounts-db/src/accounts_db/tests.rs

mergify bot mentioned this pull request Dec 12, 2024

v2.0: Reclaims more old accounts in clean (backport of #4044) #4089

Merged

mergify bot pushed a commit that referenced this pull request Dec 12, 2024

Reclaims more old accounts in clean (#4044)

431e1c4

(cherry picked from commit 3d43824) # Conflicts: # accounts-db/src/accounts_db.rs # accounts-db/src/accounts_db/tests.rs

mergify bot mentioned this pull request Dec 12, 2024

v2.1: Reclaims more old accounts in clean (backport of #4044) #4090

Merged

jeffwashington mentioned this pull request Dec 12, 2024

Fix accounts index generation at startup when we don't use disk index #4018

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reclaims more old accounts in `clean` #4044

Reclaims more old accounts in `clean` #4044

brooksprumo commented Dec 10, 2024

jeffwashington commented Dec 10, 2024

HaoranYi left a comment

brooksprumo commented Dec 10, 2024

HaoranYi left a comment

HaoranYi left a comment

HaoranYi commented Dec 11, 2024

jeffwashington Dec 12, 2024

brooksprumo Dec 12, 2024

mergify bot commented Dec 12, 2024

mergify bot commented Dec 12, 2024

Reclaims more old accounts in clean #4044

Reclaims more old accounts in clean #4044

Conversation

brooksprumo commented Dec 10, 2024

Problem

Summary of Changes

jeffwashington commented Dec 10, 2024

HaoranYi left a comment

Choose a reason for hiding this comment

brooksprumo commented Dec 10, 2024

HaoranYi left a comment

Choose a reason for hiding this comment

HaoranYi left a comment

Choose a reason for hiding this comment

HaoranYi commented Dec 11, 2024

jeffwashington Dec 12, 2024

Choose a reason for hiding this comment

brooksprumo Dec 12, 2024

Choose a reason for hiding this comment

mergify bot commented Dec 12, 2024

mergify bot commented Dec 12, 2024

Reclaims more old accounts in `clean` #4044

Reclaims more old accounts in `clean` #4044