Skip to content
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
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

HaoranYi
Copy link
Contributor

Add deprecate executable metadata update for bpf loader SIMD

@HaoranYi HaoranYi force-pushed the deprecate_executable_update branch from 682af53 to d421ef8 Compare December 13, 2023 16:56
@HaoranYi HaoranYi changed the title SIMD: deprecate executable metadata update for bpf loader SIMD-0094: deprecate executable metadata update for bpf loader Dec 13, 2023
@HaoranYi HaoranYi force-pushed the deprecate_executable_update branch 2 times, most recently from 0ba43fb to 44d8b94 Compare December 13, 2023 16:59
@HaoranYi HaoranYi force-pushed the deprecate_executable_update branch 2 times, most recently from db6882f to 771e728 Compare December 13, 2023 17:09
@HaoranYi HaoranYi force-pushed the deprecate_executable_update branch from 771e728 to a5ba969 Compare December 13, 2023 17:18
@HaoranYi HaoranYi force-pushed the deprecate_executable_update branch from a5ba969 to 19f7eea Compare December 15, 2023 15:10
@HaoranYi HaoranYi force-pushed the deprecate_executable_update branch from 19f7eea to 82adfa8 Compare December 15, 2023 16:21
@brooksprumo
Copy link
Contributor

Removing myself as a reviewer since I'm not well-versed in the loader details. I defer to @Lichtso and other reviewers.

@brooksprumo brooksprumo removed their request for review February 15, 2024 19:56
@HaoranYi HaoranYi marked this pull request as draft April 8, 2024 20:58
Copy link
Contributor

@Lichtso Lichtso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Superseded by #162

Copy link
Contributor

@Lichtso Lichtso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This SIMD now extends #162 and needs to be be rewritten to address the outstanding points in the beginning of the "Detailed Design" section of SIMD-0162.

@HaoranYi
Copy link
Contributor Author

HaoranYi commented Oct 2, 2024

This SIMD now extends #162 and needs to be be rewritten to address the outstanding points in the beginning of the "Detailed Design" section of SIMD-0162.

Yeah. I pushed a commit to update this SIMD according to the changes in SIMD-0162.

@HaoranYi HaoranYi force-pushed the deprecate_executable_update branch 3 times, most recently from 1dc1974 to 2cde862 Compare October 3, 2024 15:21
@HaoranYi HaoranYi changed the title SIMD-0094: deprecate executable metadata update for bpf loader SIMD-0094: deprecate executable update for bpf loader Oct 3, 2024
@HaoranYi HaoranYi force-pushed the deprecate_executable_update branch from 2cde862 to 8b85018 Compare October 3, 2024 19:08
@HaoranYi HaoranYi force-pushed the deprecate_executable_update branch from 8b85018 to 5151ea8 Compare October 3, 2024 19:29
Comment on lines 51 to 55
- 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`.
Copy link
Contributor

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

Copy link
Contributor Author

@HaoranYi HaoranYi Oct 7, 2024

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.

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

It shouldn't affect Anchor as long as AccountInfo.executable doesn't change (see solana-labs/solana#34425 (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 and executable is a bit confusing. I think using something like <TYPE>.is_executable would help resolve this ambiguity.

Copy link
Contributor Author

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)).

Yeah. updated.

Copy link
Contributor Author

@HaoranYi HaoranYi Oct 7, 2024

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)).

@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 on AccountInfo.executable directly. Is that going to be an issue?

proposals/0094-deprecate-executable-update.md Outdated Show resolved Hide resolved
Copy link
Contributor

@buffalojoec buffalojoec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think things look good from my side, but leaving to @Lichtso for approval. Thanks @HaoranYi!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants