Refactor purge_slots_from_cache_and_store() and handle_reclaims()

[carllin]

Problem

To support the dumping logic in #17269, remove_unrooted_slot()

solana/runtime/src/accounts_db.rs

Line 3230 in a3c0833

pub fn remove_unrooted_slot(&self, remove_slot: Slot) {

needs to for an unrooted slot S:

1. Remove all the accounts index entries for S
2. Remove all the storage entries S

To this end, it would be nice if remove_unrooted_slot() and other purge logic like purge_slots():

solana/runtime/src/accounts_db.rs

Lines 3209 to 3221 in a3c0833

	let non_roots: Vec<&Slot> = slots
	.iter()
	.filter(|slot| !self.accounts_index.is_root(**slot))
	.collect();
	safety_checks_elapsed.stop();
	self.external_purge_slots_stats
	.safety_checks_elapsed
	.fetch_add(safety_checks_elapsed.as_us(), Ordering::Relaxed);
	self.purge_slots_from_cache_and_store(
	true,
	non_roots.into_iter(),
	&self.external_purge_slots_stats,
	);

(called on Bank::drop() for unrooted banks), could share the same core purging logic by. both calling into purge_slots_from_cache_and_store():

solana/runtime/src/accounts_db.rs

Lines 3068 to 3072 in a3c0833

	fn purge_slots_from_cache_and_store<'a>(
	&'a self,
	can_exist_in_cache: bool,
	removed_slots: impl Iterator<Item = &'a Slot>,
	purge_stats: &PurgeStats,

to handle all the associated purging logic.

Currently however purge_slots_from_cache_and_store(), if the slot to be purged is not in the cache, it does not purge the AccountsIndex entries, only the storage entries themselves:

solana/runtime/src/accounts_db.rs

Lines 3098 to 3101 in a3c0833

	// Note this only cleans up the storage entries. The accounts index cleaning
	// (removing from the slot list, decrementing the account ref count), is handled in
	// clean_accounts() -> purge_older_root_entries()
	{

Summary of Changes

1. In order to introduce accounts index purging into purge_slots_from_cache_and_store(slot) (see Problem 1 above), we would ideally like to reuse the purge_exact() -> handle_reclaims() (similar to in clean_accounts) flow to completely remove the slot.
   
   However, the current problem is handle_reclaims() itself calls into purge_slots_from_cache_and_store() to delete the storage entries, via the path handle_reclaims() -> process_dead_slots() -> purge_storage_slots() -> purge_slots_from_cache_and_store(), so we can't currently call handle_reclaims() inside purge_slots_from_cache_and_store().
   
   To fix this first we first note that handle_reclaims() only ever reclaims entries not in the cache. This means if we factor out the storage deletion logic from purge_slots_from_cache_and_store() into a separate purge_slot_storage_entries(), then we can break. this cycle by having both purge_slots_from_cache_and_store() and handle_reclaims() -> purge_storage_slots() call into purge_slot_storage_entries(): https://github.com/solana-labs/solana/compare/master...carllin:RefactorPurge?expand=1#diff-1090394420d51617f3233275c2b65ed706b35b53b115fe65f82c682af8134a6fR3112

2. Now that purge_slots_from_cache_and_store() completely removes the accounts index entry for unrooted banks, we no longer need the logic in Bank::drop() to add the dirty keys: https://github.com/solana-labs/solana/compare/master...carllin:RefactorPurge?expand=1#diff-ed47b4a0198313377e091bb3957bbbc63d937805426d1b2b6de39d0a50d32a0cL5120-L5130
   @brooksprumo

Fixes #

[carllin]

@ryoqun from what I remember, these tests were essentially testing that if we try to load on a purged note, we panic.

This was because the behavior before was we only removed the storage entry in purge_slot() and left the accounts index entry untouched. This meant we would fail to retrieve the account, retry the get from the index, see the same index entry again and panic when we got the same index entry:

solana/runtime/src/accounts_db.rs

Lines 2674 to 2703 in 305d9dd

	if new_slot == slot && new_store_id == store_id {
	// Considering that we're failed to get accessor above and further that
	// the index still returned the same (slot, store_id) tuple, offset must be same
	// too.
	assert!(new_offset == offset);
	// If the entry was missing from the cache, that means it must have been flushed,
	// and the accounts index is always updated before cache flush, so store_id must
	// not indicate being cached at this point.
	assert!(new_store_id != CACHE_VIRTUAL_STORAGE_ID);
	// If this is not a cache entry, then this was a minor fork slot
	// that had its storage entries cleaned up by purge_slots() but hasn't been
	// cleaned yet. That means this must be rpc access and not replay/banking at the
	// very least. Note that purge shouldn't occur even for RPC as caller must hold all
	// of ancestor slots..
	assert!(load_hint == LoadHint::Unspecified);
	// Everything being assert!()-ed, let's panic!() here as it's an error condition
	// after all....
	// That reasoning is based on the fact all of code-path reaching this fn
	// retry_to_get_account_accessor() must outlive the Arc<Bank> (and its all
	// ancestors) over this fn invocation, guaranteeing the prevention of being purged,
	// first of all.
	// For details, see the comment in AccountIndex::do_checked_scan_accounts(),
	// which is referring back here.
	panic!(
	"Bad index entry detected ({}, {}, {}, {}, {:?})",
	pubkey, slot, store_id, offset, load_hint
	);

With the current set of changes where the purge_slot() call above actually removes the account index entry, which means the load thread here will realize the account is deleted and will not retry the account index get, returning None. This causes the load thread to panic here:

solana/runtime/src/accounts_db.rs

Line 10184 in 305d9dd

let loaded_account = db.do_load(&ancestors, &pubkey, None, load_hint).unwrap();

since the pubkey no longer exists in the index.

Essentially it is no longer possible for the panic to happen because the account index entry is removed before the storage entry is removed, so I have removed these tests.

[ryoqun]

@carllin your reasoning sounds good at first glance. let me think on it more thoughly though. btw, another pretty mind-bending pr from you. :)

[ryoqun]

this your comment is finally making sense... what a brain-teasing pr... lol

[ryoqun]

yay! less work (ref #16638) :)

[brooksprumo]

There's a lot of changes in there that I don't fully grok yet, so I'll defer to others on approval. I've added tiny nits, mostly comments because I think they are helpful. I'll revisit this PR and add additional comments if I find anything else.

[carllin]

One thing to note is, this can actually race with clean_accounts(). If clean removes some of the the accounts index entries first, then we wont remove them here, and the handle_reclaims() call here may not remove all the storage entries (clean may have the remaining reclaims from the index necessary to mark the slot as dead).

Luckily right now AccountsBackgroundService runs all Bank::drop() serially with clean_accounts(), but this is the reason the test_store_scan_consistency_unrooted() was failing, which has been fixed in b062124

[ryoqun]

Luckily right now AccountsBackgroundService runs all Bank::drop() serially with clean_accounts()

With the upcoming duplicate slot thing, how about introducing a runtime check to really validate this?

I think we can signal AccountsDb here at the identical timing to setup callbacks for all banks:

solana/core/src/tvu.rs

Lines 237 to 240 in 44831c1

	// Before replay starts, set the callbacks in each of the banks in BankForks
	for bank in bank_forks.read().unwrap().banks().values() {
	bank.set_callback(Some(Box::new(SendDroppedBankCallback::new(
	pruned_banks_sender.clone(),

so that all subsequent purge_slot() must be coming from the ABS thread, not directly from Bank::drop()-ed thread.

[ryoqun]

i mean, I think this property could tend to be broken in the future by other code juggling. :)

[carllin]

@ryoqun, hmm so currently Bank::new_from_parent(), already guarantees that all child banks will have the proper callback.

I think we can signal AccountsDb here at the identical timing to setup callbacks

So something to verify from purge_slot() that the caller is ABS thread, i.e. something like checking the thread id is equal to the ABS thread?

[ryoqun]

So something to verify from purge_slot() that the caller is ABS thread, i.e. something like checking the thread id is equal to the ABS thread?

@carllin Oh, I was imaging more simpler way than thread id like this:

let callback = accounts_db.create_drop_callback(); // internally set `.drop_callback_installed` to `true`.
// Before replay starts, set the callbacks in each of the banks in BankForks 
for bank in bank_forks.read().unwrap().banks().values() { 
    bank.set_callback(callback)
}


impl AccountsDb {
   fn purge_slot(from_abs: bool) {
     if self.drop_callback_installed && !from_abs {
        panic!("bad drop callpath detected; bank.drop must be serialized")
     }
   }
}

impl Drop for Bank {
  fn drop() {
     accounts_db.purge_slot(..., false)
  }
}

impl ABS {
    accounts_db.purge_slot(..., true);
}

[ryoqun]

anyway this is optional nicety.

[codecov]

Codecov Report

Merging #17319 (146b369) into master (662c2aa) will decrease coverage by 0.0%.
The diff coverage is 96.9%.

@@            Coverage Diff            @@
##           master   #17319     +/-   ##
=========================================
- Coverage    82.7%    82.7%   -0.1%     
=========================================
  Files         424      424             
  Lines      118444   118478     +34     
=========================================
+ Hits        97994    98010     +16     
- Misses      20450    20468     +18     

[ryoqun]

so, new callgraph looks like this..

[ryoqun]

memo: purge_slot_storage_entries is the new equivalent fn called in process_dead_slots.

[ryoqun]

here

[ryoqun]

call https://github.com/solana-labs/solana/pull/17319/files#r636980951

[ryoqun]

memo: end of purge_slots_from_cache_and_store in old code.

[ryoqun]

Problem

To support the dumping logic in #17269, remove_unrooted_slot() ....

....

btw, this pr description is finally making sense too. and indeed, it turns out very crisp and consumable explanation. your efforts are well deserved for writing words, not code for human. :)

[brooksprumo]

OK, this is making a lot more sense now; thanks for explaining it in our call! Again, my comments are just around naming and documenting. Everything else is looks good to me!

I also like @ryoqun's suggestion of extracting out the code into a new function like purge_slot_storage().

[ryoqun]

nits: no_purge_stats.

[ryoqun]

memo: so, this is the pivotal code change to cut the circular problem.

[ryoqun]

so, this is the updated call tree:

[ryoqun]

LGTM with nits!

Pull request has been modified.

[ryoqun]

👍

[ryoqun]

lgtm again!