-
Notifications
You must be signed in to change notification settings - Fork 177
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
base: main
Are you sure you want to change the base?
Conversation
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. |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this 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
There was a problem hiding this 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!
|
||
If an account does not exist, it has not been loaded, and thus its size is 0. | ||
|
There was a problem hiding this comment.
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
@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 |
Has anza-xyz/agave#6063 (comment) been added here yet? |
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 |
adding it in #311 |
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. |
There was a problem hiding this comment.
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.
@jstarry @mjain-jump if i could have your approvals on this again, so foundation can merge |
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:
account_keys()
) doesnt exist, for instance if the program was closed