fix account resize test by requesting max tx data size

[tao-stones]

Problem

test_accounts_data_size_and_resize_transactions(): generate random account data size without compute_budget instruction to set tx's max data size. When the randomly generated account size exceeds default 10M limit, transaction will fail due to MaxLoadedAccountsDataSizeExceeded

Summary of Changes

• add compute_budget instruction to set correct tx max data size limit

Fixes #

[tao-stones]

This fixes issues described in #28814

[brooksprumo]

Do you prefer explicitly setting the accounts data size load limit vs the current solution of just using smaller account sizes?

[brooksprumo]

Since this fn is only used in tests, and those tests are not meant to exercise the ComputeBudget's load-account-data-size cap, I think it'd be nicer to not take this parameter here, and instead just call ComputeBudgetInstruction::set_accounts_data_size_limit() with u32::MAX.

[tao-stones]

Initially added the exact tx data size limit as a way to demonstrate the usage of set_accounts_data_size_limit(). But agree this may pollute this test. Replaced it with u32::MAX

[brooksprumo]

nit: Can this be removed?

[tao-stones]

removed

[tao-stones]

perhaps the easiest fix is to redefine

const DEFAULT_LOADED_ACCOUNTS_DATA_LIMIT: u32 = 10_000_000 10 * 1024 * 1024

[brooksprumo]

perhaps the easiest fix is to redefine

const DEFAULT_LOADED_ACCOUNTS_DATA_LIMIT: u32 = 10_000_000 10 * 1024 * 1024

Ahhh, that could the be issue!

Also, does loading the system program count against the account data load limit (even though it lives in the runtime vs on-chain)?

[tao-stones]

does loading the system program count against the account data load limit (even though it lives in the runtime vs on-chain)?

Yea, transaction's account data size is the sum of all accounts being loaded into memory.

[tao-stones]

Do you prefer explicitly setting the accounts data size load limit vs the current solution of just using smaller account sizes?

Calling set_accounts_data_size_limit(u32::MAX) feels more solid test setup to me.

[brooksprumo]

Yea, transaction's account data size is the sum of all accounts being loaded into memory.

@taozhu-chicago So does that mean a transaction that creates a single max-sized account would still fail? (Assuming it does not call ComputeBudgetInstruction::set_accounts_data_size_limit().)

[tao-stones]

So does that mean a transaction that creates a single max-sized account would still fail? (Assuming it does not call ComputeBudgetInstruction::set_accounts_data_size_limit().)

Yea, it needs to call to set limit, otherwise it'd fail -- is this a problem? Is it necessary to exclude builtin programs in count data size towards limit? Think it again, and chat with @Lichtso, builtin should be handled same as any programs wrt data size limit.

[brooksprumo]

Yea, it needs to call to set limit, otherwise it'd fail -- is this a problem? Is it necessary to exclude builtin programs in count data size towards limit?

I guess I was under the impression that the 10 MB default load-limit was calculated so that a single max-sized account could be created without needing to call ComputeBudgetInstruction::set_accounts_data_size_limit().

But that was my assumption. So, if that's not the intent by the default 10 MB limit, that is also OK; and then the default limit does not need to be changed here.

It may be useful to know what the load-limit must be set to in order to create a max-sized account.

[brooksprumo]

I think the changes look good. But, if the the system program counts against the load budget, won't the test still fail if the account data size ends up being the 10 MiB max?

Edit: Nevermind, forgot that the load limit was bumped up to u32::MAX

[tao-stones]

won't the test still fail if the account data size ends up being the 10 MiB max?

Yeah, in early commit (before change to u32::MAX), compute_budget instruction set limit to (10Mib + mock_realloc_account_size + compute_budget_account_size) = 10MiB + 20B + 22B. That's awkward but did make test valid. But these details shouldn't bother the test itself, so u32::MAX is better :)

BTW, the need to loading builtin is discussed separately. Hopefully soon to be able to not loading builtin programs, then the default limit of 10MiB can perfectly load a max sized account.

[brooksprumo]

Lgtm