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-0162: Remove Accounts is_executable Flag Checks #162

Conversation

Lichtso
Copy link
Contributor

@Lichtso Lichtso commented Jul 31, 2024

No description provided.


Error codes of these conditions, which are rarely triggered, will change.

The only consensus relevant change is that it will become possible (for
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this change open doors for user to withdraw funds from program accounts?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, it seems that this change will open doors for modifying data on executable account, and reset executable flag itself on executable account. Are there any concerns about this for consensus? Should we document the impact of these changes in the SIMD?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Users will remain unable to: Withdraw funds from program accounts, reset the is_executable flag, or change the data directly without going through the loaders program managements instructions. That is because for that you need to be the owner, but programs are owned by a loader. Thus, the loader decides what happens to program accounts.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah. Make sense.

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.

Looks good. I'm just curious how buffer accounts will be handled going forward. I'm assuming that it will simply be the header of the account data UpgradeableLoaderState::Buffer, but maybe mentioning how loaders v3 and v4 distinguish between buffer accounts and programs might be useful, even if unchanged, with this flag removed.

flag can be addressed in a subsequent proposal. Thus, the following must remain
unaffected for now:

- Setting of the `is_executable` flag during program deployment
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we specify that this is for both loaders - v3 and v4?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Loader-v4 will get its own SIMD explaining it in full detail.

proposals/0162-remove-accounts-executable-flag-checks.md Outdated Show resolved Hide resolved
proposals/0162-remove-accounts-executable-flag-checks.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.

Nice, looks good!

Copy link
Contributor

@HaoranYi HaoranYi left a comment

Choose a reason for hiding this comment

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

I like it. Lgtm!

With this SIMD, it will help us to move one step closer to SIMD-0094, i.e. we no longer need to worry about "simulating" executable flag for accounts updates during the runtime execution.

Copy link

@topointon-jump topointon-jump left a comment

Choose a reason for hiding this comment

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

It's great to simplify things here!

A bit concerned that it's easy to make mistakes here and unintentionally introduce consensus violations, so there will need to be very thorough testing around the failure cases. We have a lot of tooling which can help here.

@joncinque
Copy link
Contributor

It looks like there's consensus here. If no one objects within the next week, I'll merge this on 25 November.

@joncinque joncinque merged commit 761eef4 into solana-foundation:main Nov 25, 2024
2 checks passed
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