Cap accounts data a transaction can load by its requested compute-unit

[tao-stones]

Problem

Data bytes loaded by transaction have cost in compute-units, defined in cost model limits. Amount of data a transaction loads should not exceed its requested compute-unit limit.

Summary of Changes

• fail loading transaction with error MaxAccountsDataSizeExceeded if its accounts data size exceed requested CU permits.

Feature Gate Issue #27839

[tao-stones]

it may be valuable to say "loaded" somewhere in the error name/description.

Added "Loaded"

[tao-stones]

@jstarry @brooksprumo I was side-tracked from it; It has been refreshed, addressed all previous comments. Can you guys take another look? Thanks!

[jstarry]

Sorry I didn't look at this before merge. My main concern is that when loading executable accounts we redundantly load bpf loader a bunch of times and if a program is called multiple times its accounts are loaded multiple times. It would have been nice to clean that up first because now that implementation is tied to consensus because it could lead to a transaction being rejected

[jstarry]

@Lichtso should be aware of this change too because of how it impacts accounts loaded for transactions

[tao-stones]

Is it straightforward to remove redundant bpf loader loading? Happy to clean that up first if can get some pointers (not very familiar with that part of code).

Prepared revert PR in case want to clean it up first, or can do it before feature activation without reverting.

[brooksprumo]

@taozhu-chicago I just put up a PR to work around a test hitting MaxLoadedAccountsDataSizeExceeded when I was not expecting it to: https://github.com/solana-labs/solana/pull/28814

I was expecting that it should be allowed to load one max-sized account (or at least near max), but I saw errors. I imagine this is due to the size of the program itself being loaded? Can you help me figure out what's happening?

[tao-stones]

@taozhu-chicago I just put up a PR to work around a test hitting MaxLoadedAccountsDataSizeExceeded when I was not expecting it to: #28814

I was expecting that it should be allowed to load one max-sized account (or at least near max), but I saw errors. I imagine this is due to the size of the program itself being loaded? Can you help me figure out what's happening?

This is due to the transaction didn't set to request max account data size, so default 10M is used. When the randomly generated account_data_size exceeds 10M, it'd fail. I'll put out a PR to add set_accounts_data_size_limit ix to test transaction later today.

https://github.com/solana-labs/solana/pull/28826

[tao-stones]

My main concern is that when loading executable accounts we redundantly load bpf loader a bunch of times and if a program is called multiple times its accounts are loaded multiple times.

Still reading code and trying to wrapping my head around it. But perhaps to track accounted account keys to de-dup? But feel there are still holes.... (continue looking)

[brooksprumo]

This is due to the transaction didn't set to request max account data size, so default 10M is used. When the randomly generated account_data_size exceeds 10M, it'd fail.

@taozhu-chicago But the test does not request more than 10M. That's why I was/am confused. Does loading the system program count against the account data load limit (even though it lives in the runtime vs on-chain)?

[tao-stones]

well, it does actually at

            let account_size = rng.gen_range(
                1,
                MAX_PERMITTED_DATA_LENGTH as usize - MAX_PERMITTED_DATA_INCREASE,
            );

and

pub const MAX_PERMITTED_DATA_LENGTH: u64 = 10 * 1024 * 1024;
pub const MAX_PERMITTED_DATA_INCREASE: usize = 1_024 * 10;

the account_size could be > 10,000,000.

(can move this to #28826 to keep this PR chats clean)

[jstarry]

I also think that the max is too small.. it should allow at least one max sized account to be loaded in my opinion.

[brooksprumo]

I also think that the max is too small.. it should allow at least one max sized account to be loaded in my opinion.

The max is currently set to 100 MB https://github.com/solana-labs/solana/blob/c17d5946840594f3226cc422d1ba4112a7f27134/program-runtime/src/compute_budget.rs#L37

The default is 10 MB https://github.com/solana-labs/solana/blob/c17d5946840594f3226cc422d1ba4112a7f27134/program-runtime/src/compute_budget.rs#L28

If you're saying that the default should allow one max-sized account to be loaded, then I agree; that was my expectation as well.

[jstarry]

Yes, sorry, I was referring to the default value. I don't think it's a good idea to backport this PR. Especially while some of these details are ironed out.

[tao-stones]

yeah agree, I'll halt backporting. (backporting to 1.14 has just automerged. https://github.com/solana-labs/solana/pull/28861 to revert it)

WRT default limit, it equals to max account size rn, simplest transaction is able to just load one max sized account. However @brooksprumo found in a unit test where a mock realloc instruction is added to builtin, the corresponding builtin account eats few bytes (eg the number of bytes of its name), therefore would prevent loading the max sized account.

Is it OK to update accounts.load_executable_accounts()to exclude builtins from being loaded?

[tao-stones]

Also, can we move further discussion to:

• proper default limit: https://github.com/solana-labs/solana/issues/28863
• not to load builtin programs: https://github.com/solana-labs/solana/issues/28864

[Lichtso]

@taozhu-chicago This has not been activated on any network and is in 1.15 only, right?

Because, I think I have found a way remove the duplication of loaded accounts in load_executable_accounts() and would like to refactor that. That would then affect the way they are counted and thus this PR.

[tao-stones]

@taozhu-chicago This has not been activated on any network and is in 1.15 only, right?

Because, I think I have found a way remove the duplication of loaded accounts in load_executable_accounts() and would like to refactor that. That would then affect the way they are counted and thus this PR.

oops, sorry didn't see your comment till now. But rather late than never. Yes it was only in master, and #29373 had reverted it. #29743 re-aaplies a simplified version on top of your changes.