SIMD-0186: Loaded Transaction Data Sizes by 2501babe · Pull Request #6063 · anza-xyz/agave

2501babe · 2025-05-01T18:30:48Z

Problem

simd186 declaratively defines a new transaction data size algorithm that replaces the existing ad hoc but consensus-relevant one. the current algorithm is difficult for new validators to implement correctlt because of certain surprising behaviors, and also routinely undercounts the true size of loaderv3 programs by a substantial margin

Summary of Changes

implement simd186. we do this by changing the existing load_transaction_accounts() to simply branch on the feature gate to the two functions load_transaction_accounts_simd186() or load_transaction_accounts_old(). the former is brand new, the latter is the existing impl unchanged. this entails some code duplication until the feature is activated, but the new function is substantially changed in a way that would be quite difficult to follow if we branched on the feature inline. namely:

we use the LoadedTransactionAccounts struct as an accumulator rather than defining a ton of variables and passing ownership to the struct at the end
we define a new function on the aforementioned struct increase_calculated_data_size() which replaces accumulate_and_check_loaded_account_data_size()
we instantiate program_indices with its proper capacity rather than mapping to it
the accumulation of program_indicies is substantially simplified:
- because we no longer need to count loaders against transaction size, a lot of confusing logic is simply deleted. this was worse when simd186 was initially drafted (loaders were kicked onto account_keys and it was very hard to see that this short-circuited them being counted for every program id) but its behavior still rather unintuitive
- instead of loading the program owner and checking if its owner is owned by NativeLoader, we simply check if the program is owned by PROGRAM_OWNERS and abort if it is not. because enable_transaction_loading_failure_fees is now active plus a fix made by alexander to InvokeContext, the behavior is actually identical and this is strictly an optimization

we also change all transaction_processor.rs and account_loader.rs unit tests to enable all features by default as #6043 did for integration tests. this is substantially safer for implementing new svm features

Feature Gate Issue: https://github.com/anza-xyz/feature-gate-tracker/issues/88

codecov-commenter · 2025-05-01T19:22:11Z

Codecov Report

Attention: Patch coverage is 99.15254% with 5 lines in your changes missing coverage. Please review.

Project coverage is 82.8%. Comparing base (94d70cd) to head (b7f5ed5).
Report is 124 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff            @@
##           master    #6063    +/-   ##
========================================
  Coverage    82.8%    82.8%            
========================================
  Files         843      843            
  Lines      378209   378682   +473     
========================================
+ Hits       313252   313674   +422     
- Misses      64957    65008    +51

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

tao-stones

Did a quick read, load_transaction_accounts_simd186() looks great! Thanks for making the change. Will check out tests next.

runtime/src/bank/tests.rs

svm/src/transaction_processor.rs

runtime/src/bank/tests.rs

svm/src/account_loader.rs

2501babe · 2025-05-30T11:39:21Z

1fff4c0 just a little test improvement

tao-stones

lgtm

2501babe · 2025-05-30T20:06:27Z

@anza-xyz/svm this is ready for review

Lichtso · 2025-05-31T07:50:29Z

svm/src/account_loader.rs

+    }
+
+    for (program_id, instruction) in message.program_instructions_iter() {
+        if native_loader::check_id(program_id) {


I think we can use this opportunity to get rid of this as well. It is a relict from when we had chained loaders. This removal would have to be mentioned in the SIMD.

which simd? i dont think its relevant to 186, which only talks about loaded sizes and not anything about the loader checks themselves. it could be relevant to 162 though

my view of the new message.program_instructions_iter() loop is that, since we no longer need to load the loaders to count their program data sizes, the new code is a simple refactor/optimization. the new PROGRAM_OWNERS check is the same as the block in process_executable_chain() which is activated by remove_accounts_executable_flag_checks, so since enable_transaction_loading_failure_fees is already enabled, the result is the same and it just happens earlier

likewise if we remove the native loader check here, we fail in the same manner as process_executable_chain() with remove_accounts_executable_flag_checks but earlier (because the account doesnt exist, and it cant be funded due to write-lock demotion, so loading it to check its owner fails)

edit: instead of relying on it never existing we can just hardcode it as invalid for execution. ultimately its all the same, transaction pays fees and fails

Isn't this if here redundant and will run into !native_loader::check_id(owner_id) && !PROGRAM_OWNERS.contains(owner_id) with the same error being thrown anyway?

in theory it should always hit the else branch of

let Some(program_account) = account_loader.load_account(program_id) else { error_metrics.account_not_found += 1; return Err(TransactionError::ProgramAccountNotFound); };

but i think that error would be confusing and it depends on write-lock demotion preventing anyone from ever adding lamports to the address (in which case it would hit the one you mentioned)

That is kinda my point. Just remove the handling of this special case all together, the entire if block.

I'm not a fan of making this change here. Transactions that invoke the native loader go from being executed and depleting cu's (pre-PR) to being fee-only and not depleting cu's (post-PR). This is a consensus breaking change and btw it's still not included in the SIMD anywhere.

It needs to be documented in the SIMD for sure as it is a protocol change. But I think it is not consensus breaking because the entire code path load_transaction_accounts_simd186 is feature gated, right?

Yeah, needs to be documented so that other validator implementations make the same changes.

Lichtso · 2025-05-31T07:52:51Z

svm/src/account_loader.rs

+
+        loaded_transaction_accounts
+            .program_indices
+            .push(vec![instruction.program_id_index as IndexOfAccount]);


If we remove the native_loader::check_id(program_id) special case above, then we also don't need a Vec around the program index here anymore.

we can do this as part of the feature gate removal, for now the old code still needs the vec

Yes, exactly.

implement a new version of `load_transaction_accounts()` which conforms to the simd186 transaction account data size algorithm also simplify accumulation of `program_indicies` and program owner checks; the program owner logic is functionally equivalent to the checks inside `InvokeContext` added by `remove_accounts_executable_flag_checks`, so it is an optimization rather than a new spec

jstarry · 2025-06-23T13:39:19Z

svm/src/account_loader.rs

+
+// Per SIMD-0186, resolved address lookup tables are assigned a base size of 8248
+// bytes: 8192 bytes for the maximum table size plus 56 bytes for metadata.
+const ADDRESS_LOOKUP_TABLE_BASE_SIZE: usize = 8248;


Shouldn't this include the TRANSACTION_ACCOUNT_BASE_SIZE as well?

2. Each loaded account's size is defined as the byte length of its data prior to transaction execution plus 64 bytes to account for metadata. 3. There is an additional flat 8248 byte cost for each address lookup table used by a transaction, accounting for the 8192 bytes for the maximum size of such a table plus 56 bytes for metadata.

i interpret point 3 to unambiguously mean no because it makes no reference to point 2 and the 8248 cost is described as "flat." its hard for me to read "additional" as meaning anything but "added to the transaction total" because the base size is described as being added to the real byte length of a loaded account

im fine adding another 64 here but would definitely amend the simd, i think this rightly implements the current wording we arrived at

jstarry · 2025-06-23T13:50:31Z

svm/src/account_loader.rs


        account.map(|account| LoadedTransactionAccount {
-            loaded_size: account.data().len(),
+            loaded_size: base_account_size.saturating_add(account.data().len()),


Should we be adding the base account size to accounts that didn't exist before transaction execution as well? From the SIMD it's not super clear to me what the answer should be. Are we limiting the amount of IO reads or are we limiting the amount of memory used to track accounts when processing a transaction? If it's the former then we probably shouldn't include the base account size, but if it's the latter we should I think.

i debated whether this was ambiguous in the simd or not when implementing it, and decided it wasnt because everything talks about "loaded account data size" and new accounts are not "loaded." but since you also raised it i think its worth adding a clarification

correction: i debated whether this was ambiguous and decided that it was and already added the clarification to my open simd186 clarifications pr which foundation still needs to merge 😅 solana-foundation/solana-improvement-documents#282 (comment)

jstarry · 2025-06-23T19:53:25Z

svm/src/account_loader.rs

+    }
+
+    for (program_id, instruction) in message.program_instructions_iter() {
+        if native_loader::check_id(program_id) {


I'm not a fan of making this change here. Transactions that invoke the native loader go from being executed and depleting cu's (pre-PR) to being fee-only and not depleting cu's (post-PR). This is a consensus breaking change and btw it's still not included in the SIMD anywhere.

2501babe self-assigned this May 1, 2025

2501babe mentioned this pull request May 1, 2025

SIMD-0186: Define LoaderV4 solana-foundation/solana-improvement-documents#282

Merged

2501babe force-pushed the 20250501_simd186 branch 4 times, most recently from 208001e to b579871 Compare May 9, 2025 18:41

2501babe added 10 commits May 13, 2025 06:01

FIRST COMMIT basic impl

15f5371

wire it together

8f70b55

add the loaderv3 magic

e016a91

cleanups

cb4957b

add feature gate, fix some tests and mark others

25a0254

fix realloc integration test

91600c7

fix account loader tests

97aaee0

fix transaction processor tests

fd12e4d

note to self

9936c1d

add coverage to runtime tests

09b243e

2501babe force-pushed the 20250501_simd186 branch 4 times, most recently from 678faef to 635d9d3 Compare May 13, 2025 13:18

add test, cleanup

d7d7dc2

2501babe force-pushed the 20250501_simd186 branch from 635d9d3 to d7d7dc2 Compare May 13, 2025 13:26

2501babe added the feature-gate label May 13, 2025

2501babe marked this pull request as ready for review May 13, 2025 17:11

2501babe requested a review from a team as a code owner May 13, 2025 17:11

2501babe requested a review from apfitzge May 13, 2025 17:11

2501babe requested a review from tao-stones May 22, 2025 01:13

tao-stones reviewed May 23, 2025

View reviewed changes

tao-stones reviewed May 28, 2025

View reviewed changes

cover new accounts in test

1fff4c0

tao-stones previously approved these changes May 30, 2025

View reviewed changes

Lichtso reviewed May 31, 2025

View reviewed changes

2501babe dismissed tao-stones’s stale review via 5357ba1 June 2, 2025 06:30

make usage of native loader error early

34e58c9

2501babe force-pushed the 20250501_simd186 branch from 5357ba1 to 34e58c9 Compare June 2, 2025 09:00

remove native loader special case

b7f5ed5

Lichtso approved these changes Jun 4, 2025

View reviewed changes

2501babe merged commit 53861e5 into anza-xyz:master Jun 4, 2025
47 checks passed

2501babe deleted the 20250501_simd186 branch June 4, 2025 09:46

2501babe mentioned this pull request Jun 4, 2025

rpc: fix simulated loaded size tests #6412

Merged

jstarry reviewed Jun 23, 2025

View reviewed changes

Conversation

2501babe commented May 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Summary of Changes

Uh oh!

codecov-commenter commented May 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tao-stones left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

2501babe commented May 30, 2025

Uh oh!

tao-stones left a comment

Choose a reason for hiding this comment

Uh oh!

2501babe commented May 30, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

2501babe Jun 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

2501babe Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

2501babe commented May 1, 2025 •

edited

Loading

codecov-commenter commented May 1, 2025 •

edited

Loading

2501babe Jun 2, 2025 •

edited

Loading

2501babe Jun 25, 2025 •

edited

Loading