calculate_capitalization uses hash calculation #17443

jeffwashington · 2021-05-24T18:38:06Z

Problem

Summary of Changes

rework calculate_capitalization to use the store scan instead of index scan. Call update_accounts_hash_with_index_option with index=false.
Fixes #

runtime/src/accounts_db.rs

jeffwashington · 2021-05-26T15:25:13Z

@sakridge replacing an index scan with a storage scan will not work if there is account data in the write cache.
The nominal test fails in stable.

Here's where we get where we have cached accounts loaded:

solana/runtime/src/accounts.rs

Lines 623 to 628 in 9a5330b

    
           pub fn calculate_capitalization(&self, ancestors: &Ancestors) -> u64 { 
        
               self.accounts_db.unchecked_scan_accounts( 
        
                   "calculate_capitalization_scan_elapsed", 
        
                   ancestors, 
        
                   |total_capitalization: &mut u64, (_pubkey, loaded_account, _slot)| { 
        
                       let lamports = loaded_account.lamports();

Here's the call that gets us to calculate_capitalization.

solana/ledger/src/blockstore_processor.rs

Line 558 in 179856c

if !bank_forks.root_bank().calculate_and_verify_capitalization() {

   1: solana_runtime::accounts::Accounts::calculate_capitalization
             at /Users/jeffreywashington/dev/s4/runtime/src/accounts.rs:617:9
   2: solana_runtime::bank::Bank::calculate_capitalization
             at /Users/jeffreywashington/dev/s4/runtime/src/bank.rs:4466:9
   3: solana_runtime::bank::Bank::calculate_and_verify_capitalization
             at /Users/jeffreywashington/dev/s4/runtime/src/bank.rs:4472:26
   4: solana_ledger::blockstore_processor::do_process_blockstore_from_root
             at /Users/jeffreywashington/dev/s4/ledger/src/blockstore_processor.rs:557:9
   5: solana_ledger::blockstore_processor::process_blockstore
             at /Users/jeffreywashington/dev/s4/ledger/src/blockstore_processor.rs:410:5

We would presumably need to flush the cache before this call, or we detect there are items in the cache and we fallback to the account scan as before. Or, we do something more complicated.

If we're just worried about the cap scan at startup, there will not be anything in the cache. We could just change that code path.

jeffwashington · 2021-06-02T20:51:14Z

This code path now supports appending the write cache data per slot to the store data per slot.

jeffwashington · 2021-06-02T20:52:23Z

runtime/src/accounts_db.rs

                            let accounts = storage.accounts.accounts(0);
                            accounts.into_iter().for_each(|stored_account| {
                                scan_func(LoadedAccount::Stored(stored_account), &mut retval, slot)
                            });
                        }
                    }
+                    if let Some(cache) = accounts_cache {


@carllin - this is interesting to you. We are using the read cache to augment the data in stores in order to calculate capitalization on banks where we haven't flushed the cache yet.

Ah I see, this is because on startup when we run if !bank_forks.root_bank().calculate_and_verify_capitalization() { from do_process_blockstore_from_root() the cache may not have been flushed yet. This might be a good comment to add to the calculate_capitalization() function for why we pass the use_cache flag. If we do as this comment suggests https://github.com/solana-labs/solana/pull/17443/files#r645215712, then that might also be good self-documentation for that function

I don't understand what you linked to. Do you mean can_cached_slot_be_unflushed?

arg! yeah sorry the links to the comments failed, good thing your intuition is good 😃. Yeah I meant this comment: #17443 (comment) regarding can_cached_slot_be_unflushed.

codecov · 2021-06-03T00:38:42Z

Codecov Report

Merging #17443 (20cd147) into master (708bbcb) will decrease coverage by 0.1%.
The diff coverage is 75.2%.

@@            Coverage Diff            @@
##           master   #17443     +/-   ##
=========================================
- Coverage    82.7%    82.6%   -0.2%     
=========================================
  Files         431      431             
  Lines      120590   121055    +465     
=========================================
+ Hits        99839   100025    +186     
- Misses      20751    21030    +279

runtime/src/accounts_db.rs

jeffwashington · 2021-06-03T17:16:04Z

runtime/src/accounts.rs

+            .accounts_db
+            .update_accounts_hash_with_index_option(false, false, slot, ancestors, None, true)
+            .1;
+        // debug code for the moment


this old code path remains for now to make sure we are getting the right results in all cases. I'll run this against mnb for a while. I am not yet familiar with all the ways calculate_capitalization is called. I'll look into it.

so, to be clear I will not complete this pr with the old code path and the assert present.

jeffwashington · 2021-06-03T17:26:44Z

runtime/src/accounts.rs

+        let cap = self
+            .accounts_db
+            .update_accounts_hash_with_index_option(false, false, slot, ancestors, None, true)
+            .1;


we could also pass a parameter here to prevent actually calculating a hash value since we throw it away.

Also, I need to change this to call a level deeper and not update the hash - but just calculate the hash to maintain the behavior of the function I'm attempting to replace.

runtime/src/accounts_db.rs

runtime/src/accounts.rs

runtime/src/sorted_storages.rs

carllin · 2021-06-04T01:12:40Z

runtime/src/sorted_storages.rs

    }

    // source[i] is in slot slots[i]
    // assumptions:
    // 1. slots vector contains unique slot #s.
    // 2. slots and source are the same len
-    pub fn new_with_slots(source: &'a [SnapshotStorage], slots: &[Slot]) -> Self {
+    pub fn new_with_slots(


nit: maybe rename to new_from_storage_entries?

some callers have only &[SnapshotStorage]. We have to figure out the corresponding slots. Restoring from a snapshot is such a code path. Those callers call 'new'. Tests are another code path.
Other callers can easily generate a corresponding Slot per storage. Those callers call 'new_with_slots'.
We can rename either of the 'new' functions. This is the brief history of the naming.

runtime/src/sorted_storages.rs

runtime/src/accounts_db.rs

carllin · 2021-06-09T08:35:36Z

runtime/src/sorted_storages.rs

        let mut min = Slot::MAX;
        let mut max = Slot::MIN;
+        // none, either, or both of min/max could be specified
+        if let Some(slot) = min_slot {
+            min = std::cmp::min(slot, min);
+            max = std::cmp::max(slot + 1, max);
+        }
+        if let Some(slot) = max_slot_inclusive {
+            min = std::cmp::min(slot, min);
+            max = std::cmp::max(slot + 1, max);
+        }
+


Laid out explicitly like so, this block is easier for me to reason about all the cases:

let (mut min, mut max) = match (min_slot, max_slot_inclusive) { (Some(min_slot), Some(max_slot_inclusive)) => { assert!(max_slot_inclusive >= min_slot); (min_slot, max_slot_inclusive) }, (Some(min_slot), None) => (min_slot, min_slot + 1), (None, Some(max_slot_inclusive)) => (max_slot_inclusive, max_slot_inclusive + 1), (None, None) => (Slot::MAX, Slot::MIN), };

I'd prefer not to add an assert. Also, it seems possible that calculations of min could end up > max given that we have several sources we're merging: global roots, ancestors, current slot of bank?, and the global write cache. We are really looking for the max range that we have to iterate over to check stores or write cache for accounts that we should include in our calculation.

I did rework the function to make it much more clear what is going on. min_slot and max_slot_inclusive are just 2 more 'slot' values run through the min/max calculation, along with all the slots in the iter.

runtime/src/sorted_storages.rs

carllin · 2021-06-09T08:51:33Z

runtime/src/accounts_db.rs

+            let min_root = self.accounts_index.min_root();
+            let storages = SortedStorages::new_with_slots(
+                combined_maps.iter().zip(slots.iter()),
+                min_root,


One thing to note is there can potentially be a large gap between the min root and the first slot in the cache (we keep around 425,000 roots, but we only keep around the latest 10 seconds of roots at startup in the cache).

In fact, it may actually be faster to just iterate through the cache to find the smallest slot, than to iterate over the 400k slots starting from the min root. If we ever need to repeatedly call this function, might be more efficient to either:

Have a separate min for the cache

Cache the starting point for the cache search after performing it the first time.

carllin

A few nits, but otherwise lgtm!

Pull request has been modified.

* calculate_capitalization uses hash calculation * feedback * remove debugging code, clean up slot math (cherry picked from commit d4cc975) # Conflicts: # runtime/src/accounts_db.rs # runtime/src/sorted_storages.rs

* calculate_capitalization uses hash calculation * feedback * remove debugging code, clean up slot math

* calculate_capitalization uses hash calculation * feedback * remove debugging code, clean up slot math Co-authored-by: Jeff Washington (jwash) <75863576+jeffwashington@users.noreply.github.com>

jeffwashington force-pushed the cap4 branch 2 times, most recently from 710b10c to 9d17bb3 Compare May 25, 2021 15:59

jeffwashington commented May 25, 2021

View reviewed changes

runtime/src/accounts_db.rs Show resolved Hide resolved

jeffwashington force-pushed the cap4 branch 3 times, most recently from c480e35 to 4dc62ad Compare May 28, 2021 17:23

brooksprumo self-requested a review May 28, 2021 22:18

jeffwashington force-pushed the cap4 branch 2 times, most recently from 79926ff to d99a6c6 Compare June 2, 2021 20:40

jeffwashington commented Jun 2, 2021

View reviewed changes

jeffwashington force-pushed the cap4 branch from d99a6c6 to e37f004 Compare June 2, 2021 22:45

jeffwashington force-pushed the cap4 branch 2 times, most recently from d0e0365 to b237b0e Compare June 3, 2021 03:29

brooksprumo reviewed Jun 3, 2021

View reviewed changes

runtime/src/accounts_db.rs Outdated Show resolved Hide resolved

jeffwashington marked this pull request as ready for review June 3, 2021 16:48

jeffwashington requested review from sakridge and carllin June 3, 2021 16:50

jeffwashington commented Jun 3, 2021

View reviewed changes