-
Notifications
You must be signed in to change notification settings - Fork 218
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?
SIMD-0319: Remove Accounts is_executable Flag Entirely
#319
Conversation
851726a to
01d3214
Compare
is_executable Flag Entirelyis_executable Flag Entirely
01d3214 to
715ab14
Compare
| Program data accounts (owned by loader-v3) must not be filtered by their | ||
| `is_executable` flag anymore. | ||
|
|
||
| ### Account hash |
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.
If is_executable is not part of the accounts hash anymore, we shouldn't be setting the value on deploys in loader v3 or v4, new core programs/precompiles, or when migrating core programs to bpf, right?
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 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 don't understand your response. Are we not removing the flag from the account hash? You wrote this below:
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 comment
The 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.
And yes, we should not be setting that anymore. I can add it to the SIMD.
| - an account is owned by loader-v1 or loader-v2 | ||
| - an account is owned by loader-v3 and contains the program (proxy) state | ||
| - an account is owned by loader-v4 and has the deployed or finalized status |
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.
Should have a case for when an account is owned by the native loader as well
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.
Do you mean for the loader programs? Because surely not every zero filled account owned by the native loader should be marked as executable.
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 mean I don't really think so either but if we're removing the executable flag we kind of have to, no? Doesn't the same thing apply to loader-v1 and loader-v2? Any account can have those loaders assigned as their owner
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, but I really want to avoid having to do ELF loading, parsing and verification for this. As I said in the loader-v4 SIMD, the easiest and most consistent thing to do is to only check the owner and not the contents. If we check the contents, we can only do useful thing in loader-v3 and v4. Also, keep in mind that the only purpose of this here is not to break existing programs on MNB, otherwise I would just serialize the is_executable flag as always false.
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'm not really following your reply. How is adding native loader to this list of checks any different from having loader-v1 and loader-v2 in the list? We don't check the contents for those either.
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 can add it, it is just a question of what we want to achieve with this. Like I said, my only goal here was not to break existing programs on MNB. I can re-run the validator replay and see if anything changed, but I think nobody cared about the executable flag of loaders.
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'm typically just coming from the perspective of having consistency. In other places we consider an account to be executable if it's owned by one of the 5 loaders and I noticed you didn't include the native loader in the checks here. I don't see any drawback from including native loader here. Your concern about anyone assigning the owner to the native loader also applies to loader v1 or v2 so I don't think it's actually a concern.
|
|
||
| None. | ||
|
|
||
| ## Impact |
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 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 consume_compute_meter in CallerAccount::from_account_info.
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 should be no difference, since the is_executable == true path consumes:
https://github.com/anza-xyz/agave/blob/master/syscalls/src/cpi.rs#L809C9-L817C10
the same as the is_executable == false path:
https://github.com/anza-xyz/agave/blob/master/syscalls/src/cpi.rs#L838C17-L847C20
https://github.com/anza-xyz/agave/blob/master/syscalls/src/cpi.rs#L168C13-L173C16
https://github.com/anza-xyz/agave/blob/master/syscalls/src/cpi.rs#L273C9-L279C12
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.
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 is_executable == true cu consumption
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.
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 comment
The 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?
|
|
||
| ### Account hash | ||
|
|
||
| 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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually a very big change. As long as AccountSharedData has the executable field, we need to populate it.
In order to change the algorithm for calculating an account's hash (i.e. removing executable from the account hash), then we need to recalculate the accounts lt hash. There are a few options here:
- Don't actually remove
executablefrom the account hash. - Do a one-time re-write of the accounts lt hash at the epoch boundary. This will take 2-5 minutes. I think that's a non-starter.
- Add a second accounts lt hash with the new algo, and then at feature activation swap the one used in the bank hash. This is doable, but kind of a pain to do double work; also it's much harder to check/ensure validity of the new accounts lt hash until the feature activation. We did something like this for the first accounts lt hash too, I wouldn't recommend it.
- Add a field to AccountSharedData that indicates the hash algo version used to last calculate its hash. This way we don't need to do double hashing. Note, we wouldn't add new fields, we'd just steal a byte from some other now-obsolete field.
- Related to (1), update all the accounts currently with
executable == truetofalse. Then once we know all the accounts havefalseforexecutable, we can hard code that into the hash calc and remove reading the field, thus allowing us to remove the field afterwards.
My thought is that removing the executable field from AccountSharedData first will make all other changes simpler. However, we do need executable out of AccountHash (now known as AccountLtHash) first. I think (5) may be the simplest option, as (3) and (4) are quite involved. I do think we need (4) eventually though...
| The flag must not be added to the input of the hash function anymore. | ||
| 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 comment
The 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 executable flag including geyser and rpc
| ### Snapshot minimization special case | ||
|
|
||
| Program data accounts (owned by loader-v3) must not be filtered by their | ||
| `is_executable` flag anymore. |
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.
IMO this is an implementation detail of agave. Not necessary to include in this SIMD as it's not relevant to other validator implementations.
|
|
||
| None. | ||
|
|
||
| ## Detailed Design |
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_owner in agave)
|
|
||
| None. | ||
|
|
||
| ## Detailed Design |
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 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 comment
The 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 comment
The 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 comment
The 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.
- Vote program should be replaced as part of the Alpenglow upgrade
- Loader v4 requires a lot of ideating around ELF verification and program cache updates
- ZK-El-Gamal is still in active development
|
|
||
| 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 | ||
| which can abort a transaction. There are other influences the flag has on |
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: grammar
| which can abort a transaction. There are other influences the flag has on | |
| that can abort a transaction. There are other influences the flag has on |
| 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 | ||
| which can abort a transaction. There are other influences the flag has on | ||
| consensus which shall be removed as well. |
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: grammar
| consensus which shall be removed as well. | |
| consensus, which shall be removed as well. |
| ### Account append vector in snapshot format | ||
|
|
||
| When loading existing snapshots the flag must be ignored. When storing | ||
| snapshots the flag should be treated as being always `false`. |
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.
Snapshots are a subset of account storage files in general. We would want to make these changes regardless if a node is generating snapshots or not. Similarly, we'd want this applied to all account storage formats, not just append vecs.
| ### Account append vector in snapshot format | |
| When loading existing snapshots the flag must be ignored. When storing | |
| snapshots the flag should be treated as being always `false`. | |
| ### Account storage file format | |
| When loading accounts the flag must be ignored. When storing | |
| accounts the flag should be treated as being always `false`. |
Note, as long as Account and AccountSharedData have the executable field, we must always populate it. IOW, on load, we'll also need to set the flag to false.
|
|
||
| ### Account hash | ||
|
|
||
| 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 comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually a very big change. As long as AccountSharedData has the executable field, we need to populate it.
In order to change the algorithm for calculating an account's hash (i.e. removing executable from the account hash), then we need to recalculate the accounts lt hash. There are a few options here:
- Don't actually remove
executablefrom the account hash. - Do a one-time re-write of the accounts lt hash at the epoch boundary. This will take 2-5 minutes. I think that's a non-starter.
- Add a second accounts lt hash with the new algo, and then at feature activation swap the one used in the bank hash. This is doable, but kind of a pain to do double work; also it's much harder to check/ensure validity of the new accounts lt hash until the feature activation. We did something like this for the first accounts lt hash too, I wouldn't recommend it.
- Add a field to AccountSharedData that indicates the hash algo version used to last calculate its hash. This way we don't need to do double hashing. Note, we wouldn't add new fields, we'd just steal a byte from some other now-obsolete field.
- Related to (1), update all the accounts currently with
executable == truetofalse. Then once we know all the accounts havefalseforexecutable, we can hard code that into the hash calc and remove reading the field, thus allowing us to remove the field afterwards.
My thought is that removing the executable field from AccountSharedData first will make all other changes simpler. However, we do need executable out of AccountHash (now known as AccountLtHash) first. I think (5) may be the simplest option, as (3) and (4) are quite involved. I do think we need (4) eventually though...
715ab14 to
b8aa4f1
Compare
| 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. |
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.
What if the callee program modifies the caller program account?
| `InstructionError::MissingAccount` anymore but instead not try to update the | ||
| account payload length field (as that is missing if there is no `AccountInfo`). |
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.
What do you mean by "not try to update"?
No description provided.