Mark obsolete accounts on startup #6885
Conversation
roryharr
force-pushed
the
mark_obsolete_accounts_on_startup
branch
from
28162ec to
49bae50
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6885 +/- ##
=========================================
- Coverage 83.2% 83.2% -0.1%
=========================================
Files 853 853
Lines 377591 377834 +243
=========================================
+ Hits 314527 314723 +196
- Misses 63064 63111 +47 🚀 New features to boost your workflow:
|
|
Used ledger with a panic after finishing verifying the accounts hash and timed loading the same snapshot 3 times with and without the change: | No Change | With Change | Percentage Change (+ is better) It's a very very rough measure, but i don't think there is any real change. |
roryharr
force-pushed
the
mark_obsolete_accounts_on_startup
branch
2 times, most recently
from
a756b52 to
061903d
Compare
| HandleReclaims::ProcessDeadSlots(&stats), | ||
| MarkAccountsObsolete::Yes(max_slot), | ||
| ); | ||
| stats.report("reclaim_duplicates_index_generation", None); |
There was a problem hiding this comment.
Purge stats exist, figured they should at least be reported. I think they also have all the information we would want about what happens when obsolete accounts are marked.
It might make sense in the future to create a new type here, but seems like a future refactor to me.
roryharr
force-pushed
the
mark_obsolete_accounts_on_startup
branch
from
061903d to
6f0d945
Compare
| @@ -682,6 +714,7 @@ mod serde_snapshot_tests { | |||
| current_slot += 1; | |||
| assert_eq!(3, accounts.ref_count_for_pubkey(&pubkey1)); | |||
| accounts.store_for_tests(current_slot, &[(&pubkey1, &zero_lamport_account)]); | |||
| accounts.store_for_tests(current_slot, &[(&pubkey3, &account2)]); | |||
There was a problem hiding this comment.
Need pubkey3 added to the slot, as otherwise the slot will get entirely cleaned much earlier with obsolete accounts marked. This necessitates a shrink down below, but this test is still largely testing the same thing.
| accounts.assert_load_account(current_slot, purged_pubkey2, 0); | ||
| // With accounts marked obsolete, accounts will be marked obsolete when deserializing which | ||
| // will make them not loadable | ||
| if accounts.mark_obsolete_accounts && restore { |
There was a problem hiding this comment.
Needed the restore flag here, as the behaviour is different before and after snapshot restore. On the after snapshot restore flow, the accounts are marked obsolete
| |slot, account_info| { | ||
| // Since the unref makes every account a single ref account, all | ||
| // zero lamport accounts should be tracked as zero_lamport_single_ref | ||
| if account_info.is_zero_lamport() { |
There was a problem hiding this comment.
I looked at doing this in visit_zero_lamport_pubkeys_during_startup but it is more efficient doing it here:
visit_zero_lamport_pubkeys_during_startup would need to walk the slot list to find the maximum slot and then determine whether it is zero lamports or not. clean_pubkey can just return the entry and it can be checked here.
There was a problem hiding this comment.
Pull Request Overview
This PR introduces an experimental obsolete-account tracking feature that marks and cleans out-of-date account entries on startup and during index generation.
- Add a
mark_obsolete_accountsflag toAccountsDbConfigand carry it throughAccountsDb - Integrate conditional obsolete-account skipping in hashing, snapshot verification, and index generation
- Extend
AccountMapEntrywithunref_by_countand add batch unref APIs inAccountsIndex - Update serde-snapshot tests and add new tests for cleaning/unref logic
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| runtime/src/serde_snapshot/tests.rs | Update tests to expect different storage loads based on mark_obsolete_accounts |
| accounts-db/src/accounts_index/account_map_entry.rs | Implement unref_by_count and update unref to support bulk decrement |
| accounts-db/src/accounts_index.rs | Add clean_and_unref_slot_list and clean_and_unref_rooted_entries_by_bin with tests |
| accounts-db/src/accounts_db.rs | Introduce mark_obsolete_accounts config flag and integrate it into hashing, snapshot, and index generation |
Comments suppressed due to low confidence (1)
runtime/src/serde_snapshot/tests.rs:477
- [nitpick] The boolean parameter
restoreis ambiguous in controlling post-restore assertions. Consider renaming it to something more descriptive likeafter_restoreorsimulate_restoreto clarify its purpose.
fn with_chained_zero_lamport_accounts<F>(f: F, restore: bool)
| // to calculate the duplicates lt hash. | ||
| if should_calculate_duplicates_lt_hash && outer_duplicates_lt_hash.is_none() { | ||
| if populate_duplicates_lt_hash && outer_duplicates_lt_hash.is_none() { |
There was a problem hiding this comment.
The final guard uses populate_duplicates_lt_hash instead of should_calculate_duplicates_lt_hash, so when mark_obsolete_accounts is true, you still inject a default duplicates lt hash. Change the condition to use should_calculate_duplicates_lt_hash to honor the obsolete-account flag consistently.
| if populate_duplicates_lt_hash && outer_duplicates_lt_hash.is_none() { | |
| if should_calculate_duplicates_lt_hash && outer_duplicates_lt_hash.is_none() { |
Copilot uses AI. Check for mistakes.
Problem
Summary of Changes
Fixes #