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

Add SetAuthorityChecked instruction to bpf loader #28424

Conversation

0xripleys
Copy link
Contributor

@0xripleys 0xripleys commented Oct 17, 2022

Problem

Transferring upgrade authorities are scary: #27932

Summary of Changes

  • Add SetAuthorityChecked instruction to bpf upgradeable loader

Fixes # #27932
Feature Gate Issue: #28627

@0xripleys 0xripleys changed the title SetAuthorityChecked Add SetAuthorityChecked instruction to bpf loader Oct 17, 2022
@mergify mergify bot added the community Community contribution label Oct 17, 2022
@mergify mergify bot requested a review from a team October 17, 2022 03:30
@0xripleys
Copy link
Contributor Author

0xripleys commented Oct 17, 2022

as for the cli changes, there are a couple ways to do this:

  1. add a set-upgrade-authority-checked command to solana program
  2. modify the existing set-upgrade-authority command to use SetAuthorityChecked, and only use SetAuthority if the user specifies --no-check-new-authority or something like that. I kind of prefer this one bc it's safer.

thoughts?

@jstarry jstarry self-requested a review October 17, 2022 03:53
@jstarry
Copy link
Member

jstarry commented Oct 17, 2022

as for the cli changes, there are a couple ways to do this:

I prefer 2) as well but cli changes should be done in a separate PR

programs/bpf_loader/src/lib.rs Outdated Show resolved Hide resolved
programs/bpf_loader/src/lib.rs Outdated Show resolved Hide resolved
programs/bpf_loader/src/lib.rs Show resolved Hide resolved
transaction-status/src/parse_bpf_loader.rs Outdated Show resolved Hide resolved
programs/bpf_loader/src/lib.rs Show resolved Hide resolved
sdk/program/src/bpf_loader_upgradeable.rs Outdated Show resolved Hide resolved
programs/bpf_loader/src/lib.rs Show resolved Hide resolved
@jstarry jstarry marked this pull request as ready for review October 21, 2022 02:36
@jstarry jstarry added the feature-gate Pull Request adds or modifies a runtime feature gate label Oct 27, 2022
jstarry
jstarry previously approved these changes Oct 29, 2022
@jstarry jstarry added the automerge Merge this Pull Request automatically once CI passes label Oct 29, 2022
@mergify mergify bot dismissed jstarry’s stale review October 29, 2022 02:24

Pull request has been modified.

@mergify
Copy link
Contributor

mergify bot commented Oct 29, 2022

automerge label removed due to a CI failure

@mergify mergify bot removed the automerge Merge this Pull Request automatically once CI passes label Oct 29, 2022
@jstarry
Copy link
Member

jstarry commented Oct 29, 2022

@0xripleys can you please fix up the checks

@0xripleys
Copy link
Contributor Author

0xripleys commented Oct 29, 2022

not done fixing CI yet (trying to run CI locally on my mac but having some issues)

@0xripleys
Copy link
Contributor Author

ok the checks test should pass. i ran the other tests locally as well but ran into what i think are flakes + some mac specific issues. @jstarry do you mind running the CI again?

@jstarry jstarry added the automerge Merge this Pull Request automatically once CI passes label Oct 31, 2022
@0xripleys
Copy link
Contributor Author

@jstarry noob question, do i have to do anything (eg merge master into my branch) to kick off the other CI jobs?

@jstarry jstarry added the CI Pull Request is ready to enter CI label Nov 1, 2022
@solana-grimes solana-grimes removed the CI Pull Request is ready to enter CI label Nov 1, 2022
@jstarry
Copy link
Member

jstarry commented Nov 1, 2022

@jstarry noob question, do i have to do anything (eg merge master into my branch) to kick off the other CI jobs?

Nope, sorry that's my responsibility. In the past the CI label wasn't required, looks like it is now, thanks for the ping on that.

@mergify mergify bot merged commit 5de4dd8 into solana-labs:master Nov 1, 2022
gnapoli23 pushed a commit to gnapoli23/solana that referenced this pull request Dec 16, 2022
* SetAuthorityChecked

* restore old logic for loader

* add more upgrade authority checked test cases

* setBufferAuthority checked tests

* format

* add set_buffer_authority_checked instruction to sdk

* Update transaction-status/src/parse_bpf_loader.rs

Co-authored-by: Justin Starry <[email protected]>

* add is_set_authority_checked function

* fix set_buffer_authority_checked sdk instruction

* feature gate setAuthorityChecked

* add bpf loader tests for setAuthorityChecked ixs

* test that you can set to same authority

* allow set_authority_checked to be called via cpi (if feature is enabled)

* fix ci

* fmt

Co-authored-by: Justin Starry <[email protected]>
Co-authored-by: Justin Starry <[email protected]>
nickfrosty pushed a commit to nickfrosty/solana that referenced this pull request Jan 4, 2023
* SetAuthorityChecked

* restore old logic for loader

* add more upgrade authority checked test cases

* setBufferAuthority checked tests

* format

* add set_buffer_authority_checked instruction to sdk

* Update transaction-status/src/parse_bpf_loader.rs

Co-authored-by: Justin Starry <[email protected]>

* add is_set_authority_checked function

* fix set_buffer_authority_checked sdk instruction

* feature gate setAuthorityChecked

* add bpf loader tests for setAuthorityChecked ixs

* test that you can set to same authority

* allow set_authority_checked to be called via cpi (if feature is enabled)

* fix ci

* fmt

Co-authored-by: Justin Starry <[email protected]>
Co-authored-by: Justin Starry <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Merge this Pull Request automatically once CI passes community Community contribution feature-gate Pull Request adds or modifies a runtime feature gate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants