svm: AccountLoader::load_account() refactor

[2501babe]

Problem

the program cache refactor (#6036) requires AccountLoader for loading program accounts in functions that are shared with runtime . the cleanest way to do this is to impl TransactionProcessingCallback for AccountLoader

the trick is that AccountLoader updates its internal store with a mut ref, but TXPCB requires an immut ref previously in this pr we attempted to do this by adding interior mutability in AccountLoader, but this was deemed undesirable

also, it is a bit cumbersome for load_account() to accept a bool everywhere. usually we pass in false and it has no effect, but a true in the wrong place could cause bankhash to break. and finally it is annoying that we inspect accounts everywhere we use AccountLoader when really this only needs to be done once per transaction

Summary of Changes

the elegant solution to all these is to cleanly divide up the kinds of loading possible:

• pub fn load_transaction_account(&mut self, ...): a renamed version of the existing load_account(), which loads from accounts-db or our store, adds to our store if it came from accounts-db, inspects on behalf of bank, and wraps in a LoadedTransactionAccount
• pub fn load_account(&mut self, ...): a new function that loads and stores but does not inspect or wrap
• pub fn get_account_shared_data(&self, ...): the TXPCB trait function, which loads and does not store, inspect, or wrap
• fn do_load(&self, ...): a private helper used by all three, so that the load match is not copy-pasted anywhere, because it is crucial that it behaves predictably across all variant interfaces

despite the added complexity, this is mostly foolproof. as long as you use load_transaction_account() for transaction loading (inspecting any writable accounts before they are written to) it really doesnt matter which function you use anywhere else, they all have the same correctness

also im renaming account_cache to loaded_accounts. this was a very ill-chosen name. the intermediate AccountLoader store a) does not evict, and b) never becomes stale. so its not technically a cache, its just a memoization layer

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 95.20548% with 7 lines in your changes missing coverage. Please review.

Project coverage is 82.9%. Comparing base (cf6946d) to head (28483e0).
Report is 3 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff            @@
##           master    #4680    +/-   ##
========================================
  Coverage    82.8%    82.9%            
========================================
  Files         842      842            
  Lines      377455   377560   +105     
========================================
+ Hits       312895   313044   +149     
+ Misses      64560    64516    -44     

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[2501babe]

note i resolved the previous comment thread because it is no longer relevant, we do not change AccountLoader to use interior mutability anymore. in fact the entire pr has more or less been rewritten, so history is not important and best to think of it as brand new

[apfitzge]

LGTM

[2501babe]

@anza-xyz/svm this is now ready for review

[LucasSte]

Shouldn't this one and the usage below maintain the old behavior (checking the account)?

[2501babe]

the program_id load has already been inspected at this point, when all the account keys for the transaction were loaded. in fact we could just as easily get the account off the accounts vec by index, it just makes the code look a bit nicer to get it from AccountLoader again. the owner_id load below, the transaction isnt using the account for anything, we just want to check the owner of the owner of the program

i dont think anyone has defined anywhere exactly what "inspection" means from the point of view of svm (the caller of the trait), but this is what me and @brooksprumo have arrived at after many discussions. but "inspection" from the point of view of bank (the main implementor of the trait) is very clear though: the first writable inspection must come before the account is modified. future writable inspects are meaningless. all read-only inspects are noops

[brooksprumo]

Yep. For the accounts lattice hash, we need to know the initial state (version) of each account modified in a slot. So that means (1) we don't care about accounts loaded read-only, and (2) we only care about the first time an account is modified in the slot. The Bank will implicitly have the final state (version) available.