Refactoring: Move KeyedAccounts to InvokeContext

[Lichtso]

Problem

keyed_accounts are currently passed as a parameter everywhere, but they need to be managed in one place so that their allocation scheme can be changed.

Summary of Changes

Collects all parametric occurrences and the construction of keyed_accounts and puts them into InvokeContext.

Progress and Open Issues

next_keyed_account() vs. keyed_account_at_index()

The replacement of next_keyed_account by keyed_account_at_index is necessary in order to drop references of keyed_accounts and retrieve them after function calls which involve passing invoke_context. The reference inside the iterator of keyed_accounts would lead to a collision of borrows otherwise. Ideas of alternative approaches are welcome!

Pro

Indices are better because they:

• Can be explicated later (e.g. as enums or consts).
• Make it possible to move parts of the code around more freely as it would not depend on the order of .next().
• Enable leaving gaps in case a specific slot is not needed any longer.

Contra

Quoting @garious:

In early versions of this code, we indexed the accounts as you're proposing now. We generally viewed that code as unreadable (so directly opposed to your "indices are better because..." arguments). As I recall, the indexing also caused more fights with the borrow checker and so we'd end up passing the whole accounts list into low-level functions when they only required references to one or two accounts.
The first step to improving readability was to replace all the hardcoded indexes with constants. After that, we did a big refactor to replace all the indexing with the parsing style you see today (next_keyed_account()), at which point we were finally able to pass individual accounts to lower-level functions.

Neutral

Quoting @jackcmay:

@garious I could go either way on the iter vs index debate. Both have access functions that provide sufficient protection. Also, our instruction definitions specify the accounts numerically, so it kinda lends itself to index-based accessing as well. We might also want to provide some kind of (maybe generated) accessor functions along with the instruction definitions to index a specific account of an instruction. Doing this by index also eliminates the need to convert to an iter to grab some accounts, and then back to a slice to pass off to other instructions. One place where it isn't as nice is in instructions where there is a group of accounts that will be iterated through but the length may not be known, like list of multisig acounts.

The tale of MessageProcessor::process_cross_program_instruction

The main problem is getting the KeyedAccounts into the InvokeContext through the stack push. This is hard as there are two references inside the KeyedAccount struct:

• key: &'a Pubkey
• account: &'a RefCell<AccountSharedData>

Both give the KeyedAccount struct the lifetime 'a. Now, in oder to push the new KeyedAccounts into the InvokeContext we need to prove to the borrow checker that the new KeyedAccounts will live as long as InvokeContext does. We can't just recycle the existing KeyedAccounts, as they also have two other fields is_signer and is_writable which can be different at every invocation level.

Lifetime of KeyedAccounts

Because the InvokeContext is a trait we can not alias the lifetime of its implementation with the push method directly. Instead the constructor of the KeyedAccounts has to be called from inside the specific implementations of the trait so that the two lifetimes correctly alias.

Lifetime of Account keys

The keys can not be borrowed from Vec<PreAccount> as the Vec is owned by self inside of ThisInvokeContext. Instead they must be borrowed from message.account_keys like it is done in create_keyed_accounts.

Lifetime of Account RefCells

While the keys of the KeyedAccounts can be borrowed from the existing ones, the Accounts are created on the Rust stack at these three locations:

• BPF syscall
• message_processor
• program-test

So we have to unsafe { transmute } the lifetime to get them into the InvokeContext. This is possible as we know that the stack frame will be popped again before the original reference goes out of scope.

Otherwise, if we instead correctly bind the existing Accounts to the new KeyedAccounts verify_and_update will sound alarm as the inner instruction supposedly changed the Accounts of the outer one.

Future Work (for subsequent PRs)

• Map Accounts into the BPF VM using MemoryRegions
• Apply the KeyedAccounts is_writable flag to make some readonly
• Remove the PreAccounts from the InvokeContext as the Accounts are readonly anyways
• Remove the unsafe { transmute } which was introduced in this PR

[codecov]

Codecov Report

Merging #15410 (0689763) into master (b06e93f) will decrease coverage by 0.0%.
The diff coverage is 93.3%.

@@            Coverage Diff            @@
##           master   #15410     +/-   ##
=========================================
- Coverage    83.0%    83.0%   -0.1%     
=========================================
  Files         415      415             
  Lines      114701   114852    +151     
=========================================
+ Hits        95265    95375    +110     
- Misses      19436    19477     +41     

[mvines]

(I'm mailed Travis CI support about the missing status here, no need to block this PR on them)