-
Notifications
You must be signed in to change notification settings - Fork 177
SIMD-0186: Clarify program/loader validation #311
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
c81fa42
to
cdab0a4
Compare
cdab0a4
to
5f4dd69
Compare
their program owners, are validated. This process is unrelated to the rest of | ||
this SIMD, but is not defined elsewhere, so we define it here. |
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 think it's fair to say that it is related to this SIMD because now that we don't need to load the owner account to track its size, we are able to make this simplification.
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.
ill reword it, im trying to adhere to the philosophy that the simd document is primary and the solana validators are concrete implementations of the simd. its unrelated in the sense that this simd is exclusively about how sizes are determined so the validation is sort of a tangent
`NativeLoader1111111111111111111111111111111` itself. This special case has been | ||
removed, and `NativeLoader1111111111111111111111111111111` behaves like any | ||
other 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.
I think we should also say that previously the program id's owner was validated by checking if it was equal to the native loader or, if not, was loaded and validated by checking that it was executable and that the owner's owner == native loader.
@jstarry @mjain-jump if i could have your approvals on this, so foundation can merge |
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.
Sorry just a few more small requests
* If [SIMD-0162]( | ||
https://github.com/solana-foundation/solana-improvement-documents/pull/162) | ||
has not been activated, verify the program account is marked executable. When | ||
SIMD-0162 is active, skip this step. |
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.
nit: SIMD-0162 is activated on all clusters so we can remove this now
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.
|
||
The process is as follows; for each instruction's program ID: | ||
|
||
* Verify the account exists. |
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.
Can we add the expected error for this check? (ProgramAccountNotFound
)
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.
https://github.com/solana-foundation/solana-improvement-documents/pull/162) | ||
has not been activated, verify the program account is marked executable. When | ||
SIMD-0162 is active, skip this step. | ||
* Verify the program account's owner is one of: |
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.
Same here, can you add the expected error if this check fails? (InvalidProgramForExecution
)
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.
4afa17a
to
52c4a8e
Compare
the text is straightforward but where this lives and how its added is a bit tricky. we made these changes in the 186 agave impl, but they mirror changes in the 162 agave impl, and neither simd actually describes them. they are highly desirable changes because they drastically simplify the code and there is essentially no difference between them aside from whether the errors happen during execution or during loading
tbh we dont want to rename and rewrite the whole 186 text to expand the scope of the simd to fully describe account loading, so the simplest solutions are to either add it as an independent section (as done here) or make a new short "hey this is how program and loader validation works btw" simd. i think adding the section here is better because these changes are associated with the 186 code (although they reflect changes made in the 162 code) and because it is less likely for someone to miss them, because a new simd would be a random document tied to nothing
these changes were made in 186 because it was a fortuitous place to be able to delete complex code and replace it with simple code. the main concern is just that other validator clients replicate the changes