-
Couldn't load subscription status.
- Fork 220
SIMD-0319: Remove Accounts is_executable Flag Entirely
#319
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,96 @@ | ||
| --- | ||
| simd: '0319' | ||
| title: Remove Accounts `is_executable` Flag Entirely | ||
| authors: | ||
| - Alexander Meißner | ||
| category: Standard | ||
| type: Core | ||
| status: Review | ||
| created: 2025-03-18 | ||
| feature: TBD | ||
| extends: 0162 | ||
| --- | ||
|
|
||
| ## Summary | ||
|
|
||
| Remove the accounts `is_executable` flag from the protocol entirely. | ||
|
|
||
| ## Motivation | ||
|
|
||
| See SIMD-0162 for the reasons why the `is_executable` flag is unnecessary | ||
| protocol complexity. That SIMD however only removes all checks of the flag | ||
| that can abort a transaction. There are other influences the flag has on | ||
| consensus, which shall be removed as well. | ||
|
|
||
| ## New Terminology | ||
|
|
||
| None. | ||
|
|
||
| ## Detailed Design | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's also some code that verifies that the target bpf program account is executable in the core to bpf migration code path. This should be detailed as well. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. I think it is easiest to make this SIMD dependent on all of the core programs to be migrated first. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, that's a good point. From chatting with @buffalojoec it sounds like all the builtins that we are planning to migrate have already been migrated. So feel free to leave out the migration code path details. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are others we hopefully plan to migrate, but their timelines are unknown.
|
||
|
|
||
| The following changes in consensus relevant behavior must occur with the | ||
| activation of the feature TBD: | ||
|
|
||
| ### Account storage file format | ||
|
|
||
| When loading accounts the flag must be ignored. When storing accounts the | ||
| flag should be treated as being always `false`. | ||
|
|
||
| ### Snapshot minimization special case | ||
|
|
||
| Program data accounts (owned by loader-v3) must not be filtered by their | ||
| `is_executable` flag anymore. | ||
|
Comment on lines
+39
to
+42
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO this is an implementation detail of agave. Not necessary to include in this SIMD as it's not relevant to other validator implementations. |
||
|
|
||
| ### Account hash | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand your response. Are we not removing the flag from the account hash? You wrote this below:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah sorry, misunderstood you initial comment. You were talking about the post SIMD state, not the pre SIMD state. |
||
|
|
||
| The flag must not be added to the input of the hash function anymore. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. cc @brooksprumo to comment on how this migration will work when the feature for this SIMD is activated at an epoch boundary. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is actually a very big change. As long as In order to change the algorithm for calculating an account's hash (i.e. removing
My thought is that removing the |
||
| Note that this is different from hashing it as always `false`. | ||
|
|
||
| ### VM serialization | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Might want to expand this section to cover all potential reads of the |
||
|
|
||
| ABI v2 will simply not have the flag from the start, however ABI v0 and v1 must | ||
| change their serialization of the flag to be `0u8`. | ||
|
|
||
| ### RPC and Geyser | ||
|
|
||
| Similarly, all other external interfaces must also always return `false` for | ||
| the `is_executable` flag. | ||
|
|
||
| ### Loader Management Instructions | ||
|
|
||
| (Re)deployments in the loader programs must stop setting the `is_executable` | ||
| flag. | ||
|
|
||
| ### CPI special case | ||
|
|
||
| Currently CPI ignores changes made by the caller to instruction accounts which | ||
| have the flag set, meaning even requesting write access to a program account | ||
| throws no error. Instead the flag must now be ignored, meaning all changes made | ||
| by the caller to instruction accounts are treated equally, regardless of the | ||
| `is_executable` flag. | ||
|
Comment on lines
+66
to
+70
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What if the callee program modifies the caller program account? |
||
|
|
||
| Because programs did not have to supply `AccountInfo`s for accounts with the | ||
| `is_executable` flag set so far, they would be missing and abort the | ||
| transaction. Thus, in case a `AccountInfo` is missing, it must not throw | ||
| `InstructionError::MissingAccount` anymore but instead not try to update the | ||
| account payload length field (as that is missing if there is no `AccountInfo`). | ||
|
Comment on lines
+75
to
+76
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you mean by "not try to update"? |
||
|
|
||
| CU consumption must remain unchanged, that is: CUs will continue to be charged | ||
| either way. | ||
|
|
||
| ## Alternatives Considered | ||
|
|
||
| None. | ||
|
|
||
| ## Impact | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The CU usage for CPIs that pass executable accounts will increase as well, potentially by quite a lot if an executable account size is quite large (ie the token program). Maybe we don't need to charge CU's if direct mapping is enabled? Note that I'm referring to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There should be no difference, since the the same as the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh thanks for pointing that out. If we are going to remove the special case for executable accounts in CPI then we should probably remove that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, that is what removing the special case entails, it follows the normal path. Or were you suggesting we change the special case so that program accounts don't charge CUs? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah sorry I misunderstood the code, yeah I think this won't be impactful given that we were already charging CU's for executable accounts. Thanks! Not necessary but do you mind adding some more detail to the CPI special case section explaining that as before, executable accounts will be charged CU's according to length, but will now have their changes copied into and out of the SVM just like other accounts? |
||
|
|
||
| The changes to the snapshots and account hashes should be irrelevant. The | ||
| changes to the VM serialization should be mostly identical to the existing | ||
| behavior. The changes to the CPI special case will technically allow for a new | ||
| failure mode, when a caller attempts to give write access to a program | ||
| account to a callee, but this case does not seem to occur in currently deployed | ||
| dApps. | ||
|
|
||
| ## Security Considerations | ||
|
|
||
| None. | ||
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.
There's some code that adds recreates precompile accounts at startup and at epoch boundaries if the precompile account doesn't have the executable flag set. This should be detailed too (in
add_precompiled_account_with_ownerin agave)