Skip to content

svm: add loaded_accounts_data_size to FeesOnlyTransaction#7105

Closed
2501babe wants to merge 1 commit into
anza-xyz:masterfrom
2501babe:20250723_186rollback_better
Closed

svm: add loaded_accounts_data_size to FeesOnlyTransaction#7105
2501babe wants to merge 1 commit into
anza-xyz:masterfrom
2501babe:20250723_186rollback_better

Conversation

@2501babe
Copy link
Copy Markdown
Member

@2501babe 2501babe commented Jul 23, 2025

Problem

simd186 mandates that loaded transaction data size include extra bytes for base account size and address lookup table size. we correctly count such bytes when determining loaded size with respect to the loaded size limit, and for the final number on executed transactions. however fee-only transactions only have the orginal account data sizes on RollbackAccounts. this does not affect account loading, but this number is recorded in the cost model and provided via rpc for simulation results

Summary of Changes

add a field loaded_accounts_data_size to FeesOnlyTransaction and use it like the same field on LoadedTransaction in ProcessedTransaction::loaded_accounts_data_size()

this should be backported to 2.3 as simd186 is a 2.3 feature gate

@2501babe 2501babe self-assigned this Jul 23, 2025
@2501babe 2501babe added the v2.3 label Jul 23, 2025
@mergify
Copy link
Copy Markdown

mergify Bot commented Jul 23, 2025

Backports to the beta branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule. Exceptions include CI/metrics changes, CLI improvements and documentation updates on a case by case basis.

@codecov-commenter
Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.2%. Comparing base (092ec6b) to head (09a6e5b).
Report is 32 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #7105   +/-   ##
=======================================
  Coverage    83.2%    83.2%           
=======================================
  Files         853      853           
  Lines      374585   374648   +63     
=======================================
+ Hits       311882   311937   +55     
- Misses      62703    62711    +8     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@2501babe 2501babe marked this pull request as ready for review July 23, 2025 11:29
@2501babe 2501babe requested a review from a team as a code owner July 23, 2025 11:29
@2501babe 2501babe requested a review from jstarry July 23, 2025 11:29
@Lichtso
Copy link
Copy Markdown

Lichtso commented Jul 23, 2025

This adds fees_only_tx.loaded_accounts_data_size but also changes how fees_only_tx.rollback_accounts.data_size() is calculated. Why is that?

@jstarry
Copy link
Copy Markdown

jstarry commented Jul 23, 2025

FeesOnlyTransactions are, by definition, transactions for which we weren't able to be loaded properly for execution. I think 186 should probably be more descriptive on how to handle all the edge cases related to loading failures before we implement anything.

Seems like you settled on just charging for ALT's, fee payer, and nonce in your implementation here, but validators might have loaded many other accounts before the load failure occurred. Should we not charge some cost units for those too? And after solana-foundation/solana-improvement-documents#192 (assuming it's accepted) we will have committed transactions where only partial ALT's were loaded successfully.

@2501babe
Copy link
Copy Markdown
Member Author

This adds fees_only_tx.loaded_accounts_data_size but also changes how fees_only_tx.rollback_accounts.data_size() is calculated. Why is that?

data_size() is currently used externally, i changed it to pub(crate) and it is basically just a helper function in this pr to determine loaded_accounts_data_size. we could just as easily delete it and match on the RollbackAccounts variants directly in load_transaction() though

Seems like you settled on just charging for ALT's, fee payer, and nonce in your implementation here, but validators might have loaded many other accounts before the load failure occurred. Should we not charge some cost units for those too?

my thinking was that pre-simd186 we charge per loaded account for executed transactions, post-simd186 we charge per loaded account + 64 on each + the lookup tables. so if pre-simd186 we change per rollback account, then post-simd186 we should charge per rollback account + 64 on each + the lookup tables

if we need to amend the simd then id prefer to make the above the specified behavior. this has the advantage that the sizes of a LoadedTransaction and a FeesOnlyTransaction are both fully determined by the outcome of loading, whereas if we have special rules for partial loads then the exact point at which an error occurs gets dragged into consensus. the declarative spec of transaction data size becomes pointless because the spec would essentially be the exact computations that agave does in the order it does them

@willhickey
Copy link
Copy Markdown

Please rekey the associated feature gate as part of this PR

@2501babe 2501babe removed the v2.3 label Jul 25, 2025
@2501babe 2501babe marked this pull request as draft July 25, 2025 17:38
@2501babe
Copy link
Copy Markdown
Member Author

we have decided to move forward with 186 as-is and deal with this in a separate amendment. this is a relatively inconsequential edge case and activating the gated code as-written does not change the existing behavior

@2501babe 2501babe closed this Jul 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants