"solana program set-upgrade-authority" uses SetAuthorityChecked instruction by default #29190

0xripleys · 2022-12-09T23:26:23Z

Problem

Accompanying CLI change for #28424
(Original issue: #27932)

Summary of Changes

solana program set-upgrade-authority requires the new authority to sign by default. this behavior can be overwritten by the flag --no-sign-with-new-authority

Added a basic unit test and also did some manual testing, note that the feature gate for SetAuthorityChecked doesn't seem to have been enabled yet.

# provide new authority as pubkey only, the cli fails and complains about no signer
cargo run program set-upgrade-authority  GxSxDvJb7BEe32r93btUXgU41JEVYRr1YgHwZg94fHSk --new-upgrade-authority Gmwr4u7qyjtzbjwmk4XrWEWyn6aA6rWhAA6BUoFNdikG -u d

Error: Dynamic program error: missing signature for supplied pubkey: Gmwr4u7qyjtzbjwmk4XrWEWyn6aA6rWhAA6BUoFNdikG

# provide new authority as keypair, the transaction lands on devnet (but fails as expected since feature gate is disabled)
# https://solscan.io/tx/iKdbztjZXfQAhHa8eBPZfr5sMArVEdqRAFY14aNAvMogpY2q8Mtijd6BGMEUkyHgnceCCJmo76TXdxu3fdKSdup?cluster=devnet
cargo run program set-upgrade-authority  GxSxDvJb7BEe32r93btUXgU41JEVYRr1YgHwZg94fHSk --new-upgrade-authority test_1.json -u d
Error: Setting authority failed: Error processing Instruction 0: invalid instruction data

jstarry

Looks great! Just a few nits

jstarry · 2022-12-10T03:43:34Z

cli/src/program.rs

@@ -288,6 +293,13 @@ impl ProgramSubCommands for App<'_, '_> {
                                .conflicts_with("new_upgrade_authority")
                                .help("The program will not be upgradeable")
                        )
+                        .arg(


The new_upgrade_authority arg above (can't comment directly) should be updated to match how the upgrade_authority arg is defined

jstarry · 2022-12-10T03:44:31Z

cli/src/program.rs

+                        .arg(
+                            Arg::with_name("no_sign_with_new_authority")
+                                .long("no-sign-with-new-authority")
+                                .conflicts_with("final")


final already conflicts with new_upgrade_authority so I think it's more clear to define this as requiring new_upgrade_authority to be present.

jstarry · 2022-12-10T03:46:07Z

cli/src/program.rs

+                                .long("no-sign-with-new-authority")
+                                .conflicts_with("final")
+                                .takes_value(false)
+                                .help("Set this flag if you don't want the new authority to sign the set-upgrade-authority transaction."),


Can you explain more that the default behavior requires the new authority to sign to prevent mistakes in setting the upgrade authority? You can also warn that by opting out of this default behavior, users must be confident that they are setting the correct authority. Furthermore, if they intentionally are setting a new authority that can never sign, they should be instructed to use final instead.

jstarry · 2022-12-10T03:53:44Z

cli/src/program.rs

@@ -288,6 +293,13 @@ impl ProgramSubCommands for App<'_, '_> {
                                .conflicts_with("new_upgrade_authority")
                                .help("The program will not be upgradeable")
                        )
+                        .arg(
+                            Arg::with_name("no_sign_with_new_authority")
+                                .long("no-sign-with-new-authority")


How about skip-new-upgrade-authority-signer-check?

jstarry · 2022-12-10T03:57:47Z

cli/src/program.rs

+                        program_pubkey,
+                        upgrade_authority_index: signer_info
+                            .index_of(upgrade_authority_pubkey)
+                            .unwrap(),


Can you use expect here and say "upgrade authority is missing from signers" or something to that effect? Ditto for new_upgrade_authority unwrap below

jstarry · 2022-12-10T04:01:32Z

cli/src/program.rs

+            &tx,
+            config.commitment,
+            RpcSendTransactionConfig {
+                skip_preflight: true,


I see that other bpf loader transactions use true here but I don't see any reason for that. It'd be better to fail the transaction in preflight if the action isn't authorized or valid for some reason.

mergify · 2022-12-12T06:53:52Z

automerge label removed due to a CI failure

0xripleys · 2022-12-12T16:13:00Z

@jstarry build failure seems unrelated to my PR? unclear.

error: unused attribute
--
  | --> runtime/src/bank.rs:14958:5
  | \|
  | 14958 \|     #[ignore] // this test only works when not using the write cache
  | \|     ^^^^^^^^^ help: remove this attribute
  | \|
  | = note: `-D unused-attributes` implied by `-D warnings`
  | note: attribute also specified here
  | --> runtime/src/bank.rs:14956:5
  | \|
  | 14956 \|     #[ignore]
  | \|     ^^^^^^^^^

error: could not compile `solana-runtime` due to previous error

jstarry · 2022-12-13T01:31:39Z

@0xripleys please merge master into your branch, the fix is in master already

Pull request has been modified.

0xripleys · 2022-12-13T17:06:35Z

done

mergify · 2022-12-15T07:11:05Z

automerge label removed due to a CI failure

0xripleys · 2022-12-16T04:38:11Z

seemed like another flake to me so i just updated the branch. lmk if there's something else i should do

jstarry · 2022-12-16T05:22:13Z

Thanks, but most times there's no need to update the branch because most of the CI failures just require retrying an individual job. @yihau do you mind helping make sure this PR gets through CI? I don't have buildkite access at the moment

yihau · 2022-12-16T05:25:49Z

sure. will keep my eyes on this 👁️

…uction by default (solana-labs#29190) * set-upgrade-authority uses checked instruction by default * remove print statements * PR fixes

set-upgrade-authority uses checked instruction by default

ddf00b2

0xripleys changed the title ~~set-upgrade-authority uses checked instruction by default~~ "solana program set-upgrade-authority" uses SetAuthorityChecked instruction by default Dec 9, 2022

mergify bot added the community Community contribution label Dec 9, 2022

mergify bot requested a review from a team December 9, 2022 23:26

0xripleys added 2 commits December 9, 2022 18:29

remove print statements

7b7892b

Merge remote-tracking branch 'upstream/master'

14ffe23

0xripleys marked this pull request as ready for review December 9, 2022 23:37

jstarry reviewed Dec 10, 2022

View reviewed changes

PR fixes

62cbd67

jstarry added the CI Pull Request is ready to enter CI label Dec 12, 2022

solana-grimes removed the CI Pull Request is ready to enter CI label Dec 12, 2022

jstarry previously approved these changes Dec 12, 2022

View reviewed changes

jstarry added the automerge Merge this Pull Request automatically once CI passes label Dec 12, 2022

mergify bot removed the automerge Merge this Pull Request automatically once CI passes label Dec 12, 2022

Merge remote-tracking branch 'upstream/master'

fef4293

jstarry added the CI Pull Request is ready to enter CI label Dec 15, 2022

solana-grimes removed the CI Pull Request is ready to enter CI label Dec 15, 2022

jstarry added the automerge Merge this Pull Request automatically once CI passes label Dec 15, 2022

mergify bot removed the automerge Merge this Pull Request automatically once CI passes label Dec 15, 2022

Merge branch 'master' into master

f23891c

jstarry added the CI Pull Request is ready to enter CI label Dec 16, 2022

solana-grimes removed the CI Pull Request is ready to enter CI label Dec 16, 2022

jstarry added the automerge Merge this Pull Request automatically once CI passes label Dec 16, 2022

mergify bot merged commit ec06cf7 into solana-labs:master Dec 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

"solana program set-upgrade-authority" uses SetAuthorityChecked instruction by default #29190

"solana program set-upgrade-authority" uses SetAuthorityChecked instruction by default #29190

0xripleys commented Dec 9, 2022 •

edited

Loading

jstarry left a comment

jstarry Dec 10, 2022

jstarry Dec 10, 2022

jstarry Dec 10, 2022

jstarry Dec 10, 2022

jstarry Dec 10, 2022

jstarry Dec 10, 2022

mergify bot commented Dec 12, 2022

0xripleys commented Dec 12, 2022

jstarry commented Dec 13, 2022

0xripleys commented Dec 13, 2022

mergify bot commented Dec 15, 2022

0xripleys commented Dec 16, 2022

jstarry commented Dec 16, 2022

yihau commented Dec 16, 2022

"solana program set-upgrade-authority" uses SetAuthorityChecked instruction by default #29190

"solana program set-upgrade-authority" uses SetAuthorityChecked instruction by default #29190

Conversation

0xripleys commented Dec 9, 2022 • edited Loading

Problem

Summary of Changes

jstarry left a comment

Choose a reason for hiding this comment

jstarry Dec 10, 2022

Choose a reason for hiding this comment

jstarry Dec 10, 2022

Choose a reason for hiding this comment

jstarry Dec 10, 2022

Choose a reason for hiding this comment

jstarry Dec 10, 2022

Choose a reason for hiding this comment

jstarry Dec 10, 2022

Choose a reason for hiding this comment

jstarry Dec 10, 2022

Choose a reason for hiding this comment

mergify bot commented Dec 12, 2022

0xripleys commented Dec 12, 2022

jstarry commented Dec 13, 2022

0xripleys commented Dec 13, 2022

mergify bot commented Dec 15, 2022

0xripleys commented Dec 16, 2022

jstarry commented Dec 16, 2022

yihau commented Dec 16, 2022

0xripleys commented Dec 9, 2022 •

edited

Loading