Skip to content

SIMD-0186: Define LoaderV4#282

Merged
joncinque merged 3 commits intosolana-foundation:mainfrom
2501babe:simd186-updates
Aug 12, 2025
Merged

SIMD-0186: Define LoaderV4#282
joncinque merged 3 commits intosolana-foundation:mainfrom
2501babe:simd186-updates

Conversation

@2501babe
Copy link
Member

@2501babe 2501babe commented May 1, 2025

im drafting the impl of this now (anza-xyz/agave#6063), and loaderv4 is on the horizon, but previously its handling was undefined. my understanding now is it uses a single account for a program so we dont have to do anything special at all

also clarify three things:

  • transactions can actually have multiple lookup tables, so specify we count them all
  • we dont abort loading if a loaderv3 programdata account we load implicitly (ie, it isnt specified in account_keys()) doesnt exist, for instance if the program was closed
  • the size of a new account is 0, not 64

@2501babe 2501babe self-assigned this May 1, 2025
@2501babe 2501babe requested review from Lichtso and apfitzge May 1, 2025 19:10
Comment on lines 112 to +113
There is no special handling for any account owned by the native loader,
LoaderV1, or LoaderV2.

Account size for programs owned by LoaderV4 is left undefined. This SIMD should
be amended to define the required semantics before LoaderV4 is enabled on any
network.
LoaderV1, LoaderV2, or LoaderV4.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Lichtso if you could confirm my understanding is correct

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, only loader-v3 had an indirection. All other loaders (including v4) store the executable directly in the program account.

Copy link
Contributor

@apfitzge apfitzge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Appreciate you adding the clarification on ALT use

Copy link
Contributor

@mjain-jump mjain-jump left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

Comment on lines 104 to +106

If an account does not exist, it has not been loaded, and thus its size is 0.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added another clarification. imo this was already implicitly the rule because this whole simd refers to the size of loaded accounts

@2501babe
Copy link
Member Author

2501babe commented Jun 25, 2025

@jacobcreech could this be approved? the code is already in monorepo, and we might be making some fresh changes after. im not sure what the repo permissions are but assume this requires foundation

@Lichtso
Copy link
Contributor

Lichtso commented Jun 25, 2025

Has anza-xyz/agave#6063 (comment) been added here yet?

@2501babe
Copy link
Member Author

i wanted to land this pr now because the confusion in both anza-xyz/agave#6063 (comment) and anza-xyz/agave#6063 (comment) were because @jstarry was probably looking at the language as it exists in master and i was looking at the language in this pr which i implemented knowing it was approved by both teams

im going to open a separate pr with a sidebar section describing program/loader validation and we can debate if its appropriate to go with that or if it should be its own simd. we cant just add a one-liner because this simd as-is doesnt talk about program/loader validation at all. and the changes in the 186 code essentially move forward changes in the 162 code which are similarly underdescribed

@2501babe
Copy link
Member Author

2501babe commented Jun 26, 2025

adding it in #311

Comment on lines +82 to +84
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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't mind a clarifying comment stating that the 64 bytes for account metadata is not added here. I think as written the SIMD is probably clear enough though.

@2501babe
Copy link
Member Author

@jstarry @mjain-jump if i could have your approvals on this again, so foundation can merge

Copy link
Contributor

@mjain-jump mjain-jump left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me!

Copy link
Contributor

@joncinque joncinque left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've got approval from Anza and Firedancer, merging!

@joncinque joncinque merged commit a57d119 into solana-foundation:main Aug 12, 2025
2 checks passed
@2501babe 2501babe deleted the simd186-updates branch August 13, 2025 09:19
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 12, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants