-
Notifications
You must be signed in to change notification settings - Fork 109
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
SIMD-0094: deprecate executable update for bpf loader #94
Draft
HaoranYi
wants to merge
6
commits into
solana-foundation:main
Choose a base branch
from
HaoranYi:deprecate_executable_update
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+75
−0
Draft
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
82adfa8
Add deprecate executable metadata update simd
HaoranYi 5800ee2
add a paragraph for skipping executable check at runtime
HaoranYi 2b80ded
update based on changes in simd-0162
HaoranYi 5151ea8
more edits
HaoranYi 30fc300
pr feedback
HaoranYi 3babe43
pr feedback
HaoranYi File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,67 @@ | ||
--- | ||
simd: '0094' | ||
title: Deprecate executable update in bpf loader | ||
authors: | ||
- Haoran Yi | ||
category: Standard | ||
type: Core | ||
status: Draft | ||
created: 2023-12-13 | ||
feature: (fill in with feature tracking issues once accepted) | ||
extends: 0162 | ||
--- | ||
|
||
## Summary | ||
|
||
Deprecate executable update during bpf program deployment. | ||
|
||
## Motivation | ||
|
||
Currently, when a program is deployed, the "executable" flag on the account is | ||
set to true for legacy reasons. However, (a) old bpf loaders, such as v1 and v2, | ||
are disabled on mainnet (SIMD-0093); (b) we are going to remove accounts | ||
executable checks for runtime execution (SIMD-0162). (c) the future bpf | ||
loader-v4 no longer need "executable" flag. | ||
|
||
Because of these above reasons, "executable" flag becomes irrelevant and | ||
provides no functional benefit for runtime execution. Therefore, we want to | ||
deprecate its update during bpf program deployment for loader-v3. | ||
|
||
## Alternatives Considered | ||
|
||
None | ||
|
||
## New Terminology | ||
|
||
None | ||
|
||
## Detailed Design | ||
|
||
When the feature - "deprecate executable update" is activated, the bpf loader-V3 | ||
will no longer update *executable* flag to true after program deployment. | ||
|
||
The "executable" flag remains false after program deployment. | ||
|
||
## Impact | ||
|
||
This will affect the following scenarios. | ||
|
||
- program accounts hash after deployment. Since this change is guarded by a | ||
feature, program accounts hash change won't break consensus. | ||
- any dapps that depend on `is_executable` to be true on the serialized | ||
instruction accounts. Existing program deployed on chain will work fine. | ||
However, if the program is redeployed, it may be broken. Before redeployment, | ||
dapps developer will need to check and update the program if it depends on | ||
`is_executable`. | ||
- CPI won't ignore changes made by caller to instruction accounts (Currently CPI | ||
ignores changes made by the caller to instruction accounts which has the | ||
`is_executable` flag set). For correctness, this change will be fine. It just | ||
becomes a bit less efficient with more checks. | ||
buffalojoec marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
## Security Considerations | ||
|
||
None | ||
|
||
## Backwards Compatibility | ||
|
||
Incompatible. |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Possibly a big deal for dApp developers, cc @acheroncrypto
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, it is a bit disruptive. But it is a good step to move forward.
Once this change is done, we can skip serialize "executable" flag at all, which is a good win to serialization time too.
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.
It shouldn't affect Anchor as long as
AccountInfo.executable
doesn't change (see solana-labs/solana#34425 (comment)).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 usage of
is_executable
andexecutable
is a bit confusing. I think using something like<TYPE>.is_executable
would help resolve this ambiguity.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. updated.
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.
@acheroncrypto With this proposal,
AccountInfo.executable
will change. In the original implementation that you mentioned above,AccountInfo.executable
doesn't change because we emulate it at serialization time. But that turns out to be very expensive - 10X increase of serialization. Therefore, in this proposal, we won't emulate executable at serialization time. Hence, dapp can't depend onAccountInfo.executable
directly. Is that going to be an issue?