cli: program upgrade with offline signing (--sign-only mode) #33860

norwnd · 2023-10-25T12:10:27Z

Problem

Resolves #23975.

Summary of Changes

This PR implements offline signing capabilities for solana program upgrade command (solana program-v4 deploy is out of scope of this PR, solana program deploy works as before and doesn't support offline signing) through --sign-only and some helper flags. See rendered docs this PR adds for how it is supposed to work.

Notable things:

as you can see in tutorial docs, user needs to pass quite a lot of params between commands involved in offline signing (--sign-only mode), it could potentially be improved by serialising everything into 1 (or maybe 2-3) params - but I think it's better to leave this possible improvement out of this PR (and maybe consider adding to program-v4 later on)
durable nonces aren't yet supported (probably better do it in a separate PR)
changing program authority once it's been set to offline signer isn't implemented in this PR

cli/src/program.rs

norwnd · 2023-10-25T13:09:54Z

sdk/src/signer/mod.rs

-pub trait Signer {
+pub trait Signer: Debug {


Not sure what's the best approach in Rust to be able to println!( a Trait at runtime, but this is the simplest one I got working, is it okay to keep Debug constraint here (or should I remove it) ?

The downside - seems like I have to derive Debug for each implementation (including test mocks), didn't find a way to do it in more re-usable manner.

The error compiler complains about looks like this:

error[E0521]: borrowed data escapes outside of function --> cli/src/program.rs:1869:23 | 1858 | program_signers: Option<&[&dyn Signer]>, | --------------- - let's call the lifetime of this reference `'1` | | | `program_signers` is a reference that is only valid in the function body ... 1869 | println!("signers {:?}", program_signers); | ^^^^ | | | `program_signers` escapes the function body here | argument requires that `'1` must outlive `'static` | = note: this error originates in the macro `$crate::format_args_nl` which comes from the expansion of the macro `println` (in Nightly builds, run with -Z macro-backtrace for more info)

or

error[E0521]: borrowed data escapes outside of function --> cli/src/program.rs:1869:23 | 1858 | program_signers: Option<&[&dyn Signer]>, | --------------- - let's call the lifetime of this reference `'1` | | | `program_signers` is a reference that is only valid in the function body ... 1869 | println!("signers {:?}", &program_signers); | ^^^^ | | | `program_signers` escapes the function body here | argument requires that `'1` must outlive `'static` | = note: this error originates in the macro `$crate::format_args_nl` which comes from the expansion of the macro `println` (in Nightly builds, run with -Z macro-backtrace for more info)

Let's revert this change: Signer has pubkey() in its implementation, which is all that we should ever print anyway, so the previous implementation gives more flexibility.

I used it to debug some things, wasn't sure whether it's better to leave it out or not,

but removed now (see latest PR version)

sdk/src/signer/mod.rs

docs/src/cli/deploy-a-program.md

norwnd · 2023-10-26T21:27:53Z

Wrote some tests, and found rough edges ... maybe hold on proper review for now, I'll push another commit soon to address those.

norwnd · 2023-10-28T13:38:17Z

Wrote some tests, and found rough edges ... maybe hold on proper review for now, I'll push another commit soon to address those.

Finished tests, applied adjustments as needed and squashed everything into single commit, keeping PR in draft until all of these are added,

but expecting to get feedback on the overall approach first,

cc @joncinque

norwnd · 2023-10-28T13:39:49Z

clap-utils/src/keypair.rs

+    /// loaded. If multiple equivalent (same pub key) signers are provided - only
+    /// one of those will be returned in the result, such that NullSigner(s)
+    /// always get lower priority.
    ///
    /// The returned value includes all of the `bulk_signers` that were not
-    /// `None`, and maybe the default signer, if it was loaded.
+    /// `None`, and maybe the default signer (if it was loaded). There is no
+    /// guarantees on resulting signers ordering.


This is needed to allow specifying same signer seamlessly multiple times (with some of those "duplicates" being NullSigner(s)). One case I found this to be helpful in is doing something like this:

solana program deploy --sign-only --fee-payer <OFFLINE_SIGNER_PUB_KEY> --program <PROGRAM_SIGNER_PUB_KEY> --upgrade-authority <OFFLINE_SIGNER> --buffer <BUFFER_PUB_KEY> --blockhash <VALUE> --max-len <VALUE> --min-rent-balance <VALUE>

where <OFFLINE_SIGNER> can have same pubkey as <OFFLINE_SIGNER_PUB_KEY> (and even perhaps <PROGRAM_SIGNER_PUB_KEY>).

Also, I'm guessing ALL the changes I've introduced in clap-utils must be copied 1-to-1 into clap-v3-utils ?

Sorry, I'm not understanding the motivation for this. Why would someone specify the same signer different ways?

Unless I'm missing something, I think these changes should be in a separate PR so we can discuss there, since they don't seem to be needed for offline signing of program upgrades.

Essentially, this is a prerequisite work for implementing this "refactoring": #33860 (comment)

One reason (closely-related to this PR) is the use-case of specifying both --upgrade-authority <OFFLINE_SIGNER_PUB_KEY> and --signer <OFFLINE_SIGNER_SIGNATURE> when doing solana program upgrade:

solana program upgrade --fee-payer /Users/norwnd/.config/solana/id.json --program-id BgY6WizXe2Hfrq71TjjvV2ntjiMX3tjHKAARvg47SJUC --upgrade-authority GuKy3G22F4avoPKGtpqBEuoQYLEWaCRmncGGc5Q7JoPL --buffer C1o2XLFAU6EqZuMPVfW9C2gFEtBxMuXaNkEYLyNtsbiU --blockhash qyof8GDHGrVwUqtiDAFfit9MY62Xub2i1cEAnVXpQ62 --signer GuKy3G22F4avoPKGtpqBEuoQYLEWaCRmncGGc5Q7JoPL=Qonons7tm1oRxR2akT4cVu9HyXQjah2DxAzS8Ht7RwU3Gx1Nk2SUTv9X9LMa8mYFrPSRZbpYDsNVPG5BNNLGsh7

since OFFLINE_SIGNER_PUB_KEY will convert to NullSigner we need to differentiate between these 2 signers (preferring the most "complete" of these 2). Which is why I didn't create a separate PR for it - but can potentially move these changes into "a separate PR with preliminary changes" this PR depends on, if that helps with anything.

To sum up, I'm using the power of NullSigner to avoid unnecessary ifs and duplicate code:

I'm using signer_of_or_null_signer to treat pubkey arguments as any other signer (through NullSigner)

generate_unique_signers will choose the best signer for me (preferring real signers over NullSigner(s))

fn process_program_deploy, fn process_program_upgrade will take only signer and work with it (corresponding pubkey, instead of being a 2nd "duplicate" argument, is abstracted away behind Signer interface)

Alternative way to address this (along the lines of what's done in current master) would be to try parse --upgrade-authority as both signer and pubkey in ("upgrade", Some(matches)) => { and doing some if-checks to decide which should be used, plus we'll need more if-checks in fn process_program_deploy and fn process_program_upgrade (which I removed as mentioned here). Same for --fee-payer.

In the CLI, this is all done for you with signer_of. If you follow the code to signer_from_path_with_config, you'll see that if a pubkey is provided, the function will then check the --signer args to see if a signature is provided, and use that signature for any transaction.

For example, see how process_transfer is using the different signers:

solana/cli/src/wallet.rs

Line 871 in 2c033e0

pub fn process_transfer(

So the code is all in there for you already!

Right, what you describe here solves it specifically for --signer argument,

I've described here why we might want to solve it for other parameters too, might be a bit of a stretch on my part, wdyt ?

Changed to get rid of this NullSigner-based approach I've suggested above (see latest PR version).

CriesofCarrots · 2023-11-25T17:14:53Z

@norwnd , are you still working on this? Happy to re-open if so.

norwnd · 2023-11-26T07:24:32Z

Hi @CriesofCarrots! I'm happy to proceed if you or @joncinque can provide some feedback on this PR,

it's in Draft because there are couple of minor things I still need to add, but it's ready for review otherwise (I'd rather wait for general-approach review first though).

joncinque · 2023-11-28T12:42:25Z

Hi @norwnd -- I sincerely apologize for taking such a long time to get to this. I really appreciate the effort that you've put into this.

Looking at how many changes this requires, I'm leaning towards a slightly different way to implement it, through a totally new command called upgrade which only does the upgrade instruction.

Currently, solana program deploy does way too much work to cover many different situations. As you found out, since it works for initial deployments and upgrades, it has many possible branches to figure out. This forced you to take into account too many situations.

So rather than mess with solana program deploy, let's keep that functionality as it exists currently, and add solana program upgrade, which only sends or signs the UpgradeableLoaderInstruction::Upgrade instruction. What do you think?

norwnd · 2023-11-28T15:15:09Z

Hey @joncinque, no worries,

So rather than mess with solana program deploy, let's keep that functionality as it exists currently, and add solana program upgrade, which only sends or signs the UpgradeableLoaderInstruction::Upgrade instruction. What do you think?

I think we can move the upgrade functionality into a separate command to get cleaner result - I'll get to it in a day or two to try it out. Just want to note, --sign-only feature still complicates solana program deploy either way, so pls let me know if there is anything else we can/should do about it.

Update:

I just sketched out a "very rough diff" for introducing solana program upgrade would look like (assuming, solana program deploy still preserves an ability to upgrade program - for backward compatibility) - 72ad73f - and as it turns out, the improvement is rather marginal. Considering long-term code maintenance, I'd prefer to add it (since it does somewhat simplify complex some conditions I had to add in process_program_deploy func) even at the cost of some code duplication (which we can maybe further de-dup if we want).

It doesn't quite address the overall complexity of --sign-only though. I did consider splitting solana program deploy in two (current version, and --sign-only version) - but that's seems quite arbitrary and splitting by deploy/upgrade is more fitting imo (if we prefer splitting it at all).

joncinque · 2023-11-29T00:26:01Z

It doesn't quite address the overall complexity of --sign-only though. I did consider splitting solana program deploy in two (current version, and --sign-only version) - but that's seems quite arbitrary and splitting by deploy/upgrade is more fitting imo (if we prefer splitting it at all).

Ah sorry, there was a misunderstanding. For solana program upgrade, it should only send one transaction, which contains the UpgradeableLoaderInstruction::Upgrade, and that's it. It assumes that the buffer is already prepared for the upgrade.

The interface would be something like:

solana program upgrade <PROGRAM_ID> <BUFFER_ADDRESS> [--upgrade-authority <SIGNER>] [--fee-payer <SIGNER>] [--sign-only]  [--blockhash <BLOCKHASH>] [--nonce <NONCE_PUBKEY>] [--nonce-authority <SIGNER>] [--signer <PUBKEY>=<SIGNATURE> ...]

Or, in the enum, this would give:

    Upgrade {
        program_pubkey: Pubkey,
        buffer_pubkey: Pubkey,
        upgrade_authority_signer_index: SignerIndex,
        fee_payer: SignerIndex,
        // ... plus all the sign-only stuff ...
        sign_only: bool,
        dump_transaction_message: bool,
        blockhash_query: BlockhashQuery,
        nonce_account: Option<Pubkey>,
        nonce_authority: SignerIndex,
    },

Does that make more sense?

norwnd · 2023-12-11T11:09:56Z

Squashed and rebased, proceeding to resolve other review-comments still left from previous reviews (I'll ping when it's ready for another look).

norwnd · 2023-12-11T17:25:16Z

Hey @joncinque, I think I've addressed all the pending comments, pending your feedback now. Appreciate thorough reviews!

joncinque

Looks really great! We're down to just tiny little changes, then this will be ready to merge. Thanks for all of your patience and edits

cli/src/program.rs

docs/src/cli/deploy-a-program.md

joncinque · 2023-12-12T00:37:12Z

Oh also, it looks like the docs just got refactored, so you'll have to make the doc change in docs/src/cli/examples/deploy-a-program.md instead.

norwnd · 2023-12-13T15:57:17Z

Oh also, it looks like the docs just got refactored, so you'll have to make the doc change in docs/src/cli/examples/deploy-a-program.md instead.

Thanks for the heads-up! This last push ^ was a rebase, it moved my stuff into appropriate file, but seems like there are path-dependent links that broke (I'll push a follow-up commit(s) to fix that).

joncinque · 2023-12-13T16:24:01Z

All the changes look great to me! There's a failure in the sanity checks in CI due to some trailing whitespace https://buildkite.com/solana-labs/solana/builds/105554#018c63f9-526c-4080-bafc-d27ec92899d5, so once that's sorted out and CI passes, we'll merge this in!

joncinque · 2023-12-13T17:45:59Z

sdk/src/signer/mod.rs

Can you revert the changes here?

Hm, that's creeped from a sloppy rebase, fixed in f3e466f

codecov · 2023-12-13T18:29:55Z

Codecov Report

Merging #33860 (f3e466f) into master (501458a) will decrease coverage by 0.1%.
Report is 7 commits behind head on master.
The diff coverage is 14.7%.

Additional details and impacted files

@@            Coverage Diff            @@
##           master   #33860     +/-   ##
=========================================
- Coverage    81.9%    81.8%   -0.1%     
=========================================
  Files         819      819             
  Lines      220931   221019     +88     
=========================================
- Hits       180954   180950      -4     
- Misses      39977    40069     +92

joncinque

This is finally ready to go, thanks so much for your time, effort, and understanding!

mergify bot added community Community contribution need:merge-assist labels Oct 25, 2023

mergify bot requested a review from a team October 25, 2023 12:11

norwnd changed the title ~~program: --sign-only (offline signing)~~ cli: program deploy with offline signing (--sign-only mode) Oct 25, 2023

norwnd commented Oct 25, 2023

View reviewed changes

cli/src/program.rs Outdated Show resolved Hide resolved

norwnd commented Oct 25, 2023

View reviewed changes

cli/src/program.rs Outdated Show resolved Hide resolved

norwnd commented Oct 25, 2023

View reviewed changes

sdk/src/signer/mod.rs Outdated Show resolved Hide resolved

norwnd force-pushed the program-sign-only branch from 423bd08 to 2c57502 Compare October 25, 2023 13:14

norwnd commented Oct 25, 2023

View reviewed changes

sdk/src/signer/mod.rs Outdated Show resolved Hide resolved

norwnd force-pushed the program-sign-only branch from 2c57502 to b0c6879 Compare October 25, 2023 13:31

norwnd mentioned this pull request Oct 25, 2023

RFC: Add --sign-only flag to solana program deploy #23975

Closed

ryoqun requested a review from joncinque October 26, 2023 05:18

norwnd commented Oct 26, 2023

View reviewed changes

docs/src/cli/deploy-a-program.md Outdated Show resolved Hide resolved

norwnd force-pushed the program-sign-only branch 2 times, most recently from 0ac8a3d to fd5af67 Compare October 28, 2023 13:37

norwnd commented Oct 28, 2023

View reviewed changes

github-actions bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Nov 16, 2023

github-actions bot closed this Nov 24, 2023

joncinque reopened this Nov 28, 2023

norwnd force-pushed the program-sign-only branch 2 times, most recently from d6eee19 to 72ad73f Compare November 28, 2023 16:16

github-actions bot removed the stale [bot only] Added to stale content; results in auto-close after a week. label Nov 29, 2023

norwnd force-pushed the program-sign-only branch from 63e6e02 to 54c34f4 Compare December 11, 2023 10:11

norwnd changed the title ~~cli: program deploy with offline signing (--sign-only mode)~~ cli: program upgrade with offline signing (--sign-only mode) Dec 11, 2023

norwnd force-pushed the program-sign-only branch from 54c34f4 to d5c9aff Compare December 11, 2023 11:09

norwnd force-pushed the program-sign-only branch from d5c9aff to bcf977d Compare December 11, 2023 11:11

joncinque reviewed Dec 12, 2023

View reviewed changes

norwnd added 5 commits December 13, 2023 15:45

cli: program upgrade with offline signing (--sign-only mode)

468626a

ditch universal NullSigner approach as overly complex

1608729

rearrange some code to make it more coherent, fix typos

edc9de1

apply clippy suggestions to simplify code

d71d785

review pass

621297f

norwnd force-pushed the program-sign-only branch from 887683e to 621297f Compare December 13, 2023 15:46

adjust brokens (due to rebase onto refactored structures) doc links

4fc03c3

joncinque added the CI Pull Request is ready to enter CI label Dec 13, 2023

solana-grimes removed the CI Pull Request is ready to enter CI label Dec 13, 2023

fix linter complaint in doc formatting

da9074f

joncinque added the CI Pull Request is ready to enter CI label Dec 13, 2023

solana-grimes removed the CI Pull Request is ready to enter CI label Dec 13, 2023

joncinque reviewed Dec 13, 2023

View reviewed changes

remove rebase artifacts

f3e466f

joncinque added the CI Pull Request is ready to enter CI label Dec 14, 2023

solana-grimes removed the CI Pull Request is ready to enter CI label Dec 14, 2023

joncinque approved these changes Dec 14, 2023

View reviewed changes

joncinque merged commit 0be3b6e into solana-labs:master Dec 14, 2023
47 checks passed

willhickey mentioned this pull request Mar 28, 2024

v1.18 commits - please ignore anza-xyz/agave#475

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cli: program upgrade with offline signing (--sign-only mode) #33860

cli: program upgrade with offline signing (--sign-only mode) #33860

norwnd commented Oct 25, 2023 •

edited

Loading

norwnd Oct 25, 2023 •

edited

Loading

joncinque Dec 1, 2023

norwnd Dec 11, 2023

norwnd commented Oct 26, 2023

norwnd commented Oct 28, 2023

norwnd Oct 28, 2023 •

edited

Loading

joncinque Dec 1, 2023

norwnd Dec 2, 2023 •

edited

Loading

joncinque Dec 4, 2023

norwnd Dec 6, 2023 •

edited

Loading

norwnd Dec 11, 2023

CriesofCarrots commented Nov 25, 2023

norwnd commented Nov 26, 2023 •

edited

Loading

joncinque commented Nov 28, 2023

norwnd commented Nov 28, 2023 •

edited

Loading

joncinque commented Nov 29, 2023

norwnd commented Dec 11, 2023

norwnd commented Dec 11, 2023

joncinque left a comment

joncinque commented Dec 12, 2023 •

edited

Loading

norwnd commented Dec 13, 2023

joncinque commented Dec 13, 2023 •

edited

Loading

joncinque Dec 13, 2023

norwnd Dec 14, 2023

codecov bot commented Dec 13, 2023 •

edited

Loading

joncinque left a comment

cli: program upgrade with offline signing (--sign-only mode) #33860

cli: program upgrade with offline signing (--sign-only mode) #33860

Conversation

norwnd commented Oct 25, 2023 • edited Loading

Problem

Summary of Changes

norwnd Oct 25, 2023 • edited Loading

Choose a reason for hiding this comment

joncinque Dec 1, 2023

Choose a reason for hiding this comment

norwnd Dec 11, 2023

Choose a reason for hiding this comment

norwnd commented Oct 26, 2023

norwnd commented Oct 28, 2023

norwnd Oct 28, 2023 • edited Loading

Choose a reason for hiding this comment

joncinque Dec 1, 2023

Choose a reason for hiding this comment

norwnd Dec 2, 2023 • edited Loading

Choose a reason for hiding this comment

joncinque Dec 4, 2023

Choose a reason for hiding this comment

norwnd Dec 6, 2023 • edited Loading

Choose a reason for hiding this comment

norwnd Dec 11, 2023

Choose a reason for hiding this comment

CriesofCarrots commented Nov 25, 2023

norwnd commented Nov 26, 2023 • edited Loading

joncinque commented Nov 28, 2023

norwnd commented Nov 28, 2023 • edited Loading

joncinque commented Nov 29, 2023

norwnd commented Dec 11, 2023

norwnd commented Dec 11, 2023

joncinque left a comment

Choose a reason for hiding this comment

joncinque commented Dec 12, 2023 • edited Loading

norwnd commented Dec 13, 2023

joncinque commented Dec 13, 2023 • edited Loading

joncinque Dec 13, 2023

Choose a reason for hiding this comment

norwnd Dec 14, 2023

Choose a reason for hiding this comment

codecov bot commented Dec 13, 2023 • edited Loading

Codecov Report

joncinque left a comment

Choose a reason for hiding this comment

norwnd commented Oct 25, 2023 •

edited

Loading

norwnd Oct 25, 2023 •

edited

Loading

norwnd Oct 28, 2023 •

edited

Loading

norwnd Dec 2, 2023 •

edited

Loading

norwnd Dec 6, 2023 •

edited

Loading

norwnd commented Nov 26, 2023 •

edited

Loading

norwnd commented Nov 28, 2023 •

edited

Loading

joncinque commented Dec 12, 2023 •

edited

Loading

joncinque commented Dec 13, 2023 •

edited

Loading

codecov bot commented Dec 13, 2023 •

edited

Loading