Index loaders / executable accounts #19469
Conversation
Lichtso
force-pushed
the
refactor/merge_executables_into_accounts
branch
4 times, most recently
from
537ffc8 to
ba5b5ad
Compare
Codecov Report
@@ Coverage Diff @@
## master #19469 +/- ##
=======================================
Coverage 82.6% 82.6%
=======================================
Files 471 471
Lines 132557 132549 -8
=======================================
+ Hits 109509 109522 +13
+ Misses 23048 23027 -21 |
Lichtso
force-pushed
the
refactor/merge_executables_into_accounts
branch
4 times, most recently
from
49f8c9c to
d5b4a3d
Compare
Lichtso
force-pushed
the
refactor/merge_executables_into_accounts
branch
2 times, most recently
from
375d1b5 to
5eb2697
Compare
now referenced by transaction wide index into "accounts".
as it is already in MessageProcessor.
Lichtso
force-pushed
the
refactor/merge_executables_into_accounts
branch
from
5eb2697 to
f042892
Compare
|
@CriesofCarrots While resolving merge conflicts I touched some of your recent changes. A quick assessment if the three tests (see last commit) are still correct suffices. |
|
@Lichtso I don't understand what problem this change fixed. Can you explain? |
| .map(|instruction| { | ||
| self.load_executable_accounts( | ||
| ancestors, | ||
| &mut accounts, |
There was a problem hiding this comment.
Why do we need to add executable accounts to accounts here? Couldn't we add them above?
There was a problem hiding this comment.
That is a preparation for the next step:
Currently we leave gaps for the executable accounts (so that the indices stay correct) and then load them for each instruction. Thus, they are loaded duplicate if multiple instructions use the same executable. And that could be avoided here, once we can make sure that accounts used as programs are not writable. (see #19629)
There was a problem hiding this comment.
Ah yeah that is tricky. Someone could update program and then call it later within the same transaction, thanks for the explanation
You are right, the PR description was not particularly insightful. |
|
Ah yeah that makes sense. I like that all accounts can be indexed in the same way but I think it would be nice to have a wrapper type which keeps account inputs and account deps separate internally but implements the index trait. This way the code is more straightforward and no one has to remember what is actually in the accounts vec |
* Appends loaders / executable_accounts to accounts in transaction loading. * Adds indices to loaders / executable_accounts. * Moves MessageProcessor::create_keyed_accounts() into InvokeContext::push(). * Removes "executable_accounts", now referenced by transaction wide index into "accounts". * Removes create_pre_accounts() from InstructionProcessor, as it is already in MessageProcessor. * Collect program account indices directly in load_executable_accounts().
This reverts commit 24b26d6.
Problem
Continuation of #17898 which unified
account_depsandaccounts.Loaders / executable accounts are still separate and need to be unified with the normal accounts.
Then, all three kinds of accounts can be indexed transaction wide,
which is necessary for inter-operation between the current ABI and the new one (#19191).
Left for another PR:
Fill executable account gaps with the real deal directly instead of concatenating them at the end.
That requires them to be read-only and might need a feature gate.
Summary of Changes
loadersbyexecutable_indices.loadersat the end of the normalaccountsin transaction loading.executable_indicesdirectly inload_executable_accounts().MessageProcessor::create_keyed_accounts()intoInvokeContext::push(), which is required for resolving theexecutable_indices.