svm: skip appending loaders to loaded accounts

[2501babe]

Problem

after transaction account loading, all bpf loaders required for instruction program ids are loaded, validated, counted against transaction size, and appended to the accounts list. validating and counting them is necessary according to current consensus rules, but adding them to accounts is not. all bpf loaders are resident in the batch-local program cache by design, otherwise cpi would not work

Summary of Changes

stop appending them to the accounts list. we also take the opportunity to do some minor housekeeping:

• the previous flow was quite obscure: after account loading, it would record the last index of the accounts list, then skip loading the loader if it was found past this index, otherwise validate and append it. this was very easy to misread as loading each loader every time it was used, instead of loading each loader at most once. we use a hashset of validated loader ids now to make this clearer
• dont accumulate a list of found account booleans, just load the program account again. load_account() is idempotent in its return value, and the second call for an existing account comes from the intermediate account cache, instead of accounts-db
• get program ids from the message iterator directly instead of extracting them from the accumulated accounts list
• load the loader with load_account(), instead of bypassing normal account loading and going directly to accounts-db

overall this pr is a minor cleanup with no effect on runtime behavior. what this code actually does will change several times in the future as feature gates are activated, but eventually when simd186 is implemented we expect to be able to delete it entirely

[2501babe]

incidentally i dont know why the vec for program indexes is created with capacity of 2, or why we need vecs for indexes at all. it is possible something is added to the second position inside transaction execution, but i havent checked, and i doubt it. my guess is this used to be used for the loader but was obviated and not fully refactored away when the program cache was added

i dont think its worth messing with right now but when im doing simd186 i will take a look and maybe simplify or eliminate it, since im planning on getting rid of this code block once we dont need to count the loaders against transaction size

[apfitzge]

optimization; PROGRAM_OWNERS is a very small set. I imagine we'd actually spend more time hashing here than we would comparing pubkeys?

We can measure but, we can just directly search PROGRAM_OWNERS, and store the some small stack-space to see if the programs have been seen yet: [false; PROGRAM_OWNERS.len()]

[2501babe]

we need to keep the hashset. the capacity of 4 isnt "there can only be 4 loaders" but "we will typically see 2 or maybe 3 loaders if no one is being annoying on purpose, and if they force us to see 5 loaders they waste like a millisecond of compute so have fun i guess"

the loader validation code is extremely delicate--not because of this pr, but before and after this pr--and cannot safely be changed. this is because:

• the consensus definition of a loader is any executable account owned by NativeLoader, including stake/vote/system. in practice, with current features, this shouldnt actually matter, because these pseudo-loaders never mark accounts they own as executable, which makes the program fail validation before we validate the loader. however if we want to uphold the "svm is 100% simd83-compliant now, and simd83 means batching doesnt affect results" ideal then technically you could exploit the program cache executable bypass to get a pseudo-loader past validation to execution (although it is only a violation of our ideals, since you cant actually do it without simd83 lock relaxation)
• when disable_account_loader_special_case activates this quirk goes away and we can assume "loader" means "contained in PROGRAM_OWNERS
• ...and then when remove_accounts_executable_flag_checks activates this assumption is ruined because system/vote/stake are valid loaders and accounts they own are valid programs (according to account loading but not according to program cache) and these kinds of transactions will successfully load and move to execution
• finally when enable_transaction_loading_failure_fees activates we dont have to care about any of this anymore and can assume again that "loader" means "contained in PROGRAM_OWNERS" as an implementation detail, rather than a consensus constraint, because it doesnt matter how a pseudo-loader transaction fails anymore

and if we changed the technical definition of a loader we would have to introduce a fourth feature gate or backport and cut a new 2.1 declaring previous 2.1 versions invalid

thankfully this is all getting deleted by simd186 anyway

[apfitzge]

github won't let me comment further down - curious if we should add a (debug) assert to make sure that accounts length matches the message's account_keys.len() - we have a few things that now rely on that assumption, which was not true in the past. Slightly concerned maybe someone changes it in future and breaks stuff and we commit some extra appended accounts.

[2501babe]

hmm we could just keep the old way that clamps it to the length of account keys if thats a concern, it doesnt really cost us anything