Skip to content

SIMD-0186: Define LoaderV4 #282

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

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

@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

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