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 offline signing support to CLI #7104

Merged
merged 4 commits into from Nov 26, 2019
Merged

Add offline signing support to CLI #7104

merged 4 commits into from Nov 26, 2019

Conversation

@jackcmay
Copy link
Contributor

jackcmay commented Nov 22, 2019

Problem

No support for offline signing

Summary of Changes

The following only apply to: pay, delegate-stake, and deactivate-stake

add a --sign-only parameter that will not send a transaction but instead print the blockhash used (could be the nonce) as well as all the PUBKEY=SIGNATURE pairs.
add --blockhash parameter which provides the ability to pass a blockhash to be used rather than querying the network
-add --signer PUBKEY=SIGNATURE parameter that can be used to construct a transaction without requiring keypairs. All the pubkey/signature pairs required by the transaction must be specified.

Fixes #6872

@jackcmay

This comment has been minimized.

Copy link
Contributor Author

jackcmay commented Nov 22, 2019

@mvines @t-nelson First time I've touched the CLI stuff, before I continue on can you give me a quick sanity check that this is heading in the right direction?

cli/src/cli.rs Outdated Show resolved Hide resolved
@t-nelson

This comment has been minimized.

Copy link
Contributor

t-nelson commented Nov 22, 2019

Seem to be on track by my eye!

@mvines

This comment has been minimized.

Copy link
Member

mvines commented Nov 22, 2019

I wouldn't expect the --sign-only argument to accept a nonce. --sign-only is just a flag that says, "output the transaction signature and the blockhash used". Maybe I want to sign-only a non-nonce-based transaction.

On the flip side maybe I want to send a transaction immediately using a nonce instead of recent blockhash.

So I think there's a completely separate argument (that we haven't really discussed yet in #6872 or elsewhere) that activates nonce-support in the CLI

@jackcmay

This comment has been minimized.

Copy link
Contributor Author

jackcmay commented Nov 22, 2019

Do we expect transactions singed offline to be submitted quickly enough to render a recent blockhash valid?

@mvines

This comment has been minimized.

Copy link
Member

mvines commented Nov 22, 2019

For the current custody solution, no. But my point is that I don't we need/should bake in the assumption that --sign-only equals nonce.

@jackcmay

This comment has been minimized.

Copy link
Contributor Author

jackcmay commented Nov 22, 2019

Sounds good, so we can add another option --nonce blah

@mvines

This comment has been minimized.

Copy link
Member

mvines commented Nov 22, 2019

"blah" is not valid base58 :trollface:

@jackcmay jackcmay removed the noCI label Nov 23, 2019
@codecov

This comment has been minimized.

Copy link

codecov bot commented Nov 23, 2019

Codecov Report

Merging #7104 into master will decrease coverage by 5.1%.
The diff coverage is 65.2%.

@@           Coverage Diff            @@
##           master   #7104     +/-   ##
========================================
- Coverage      79%   73.8%   -5.2%     
========================================
  Files         230     230             
  Lines       44576   48165   +3589     
========================================
+ Hits        35236   35574    +338     
- Misses       9340   12591   +3251
@jackcmay jackcmay force-pushed the jackcmay:6872 branch 2 times, most recently from dcab19e to 8e723bd Nov 25, 2019
cli/src/cli.rs Outdated Show resolved Hide resolved
@mvines

This comment has been minimized.

Copy link
Member

mvines commented Nov 25, 2019

Worse PR title ever. 🏅

@jackcmay

This comment has been minimized.

Copy link
Contributor Author

jackcmay commented Nov 25, 2019

@mvines Can't get more straight to the point with the PR title ;-)

@jackcmay jackcmay changed the title 6872 Add offline signing support to CLI Nov 25, 2019
jackcmay added 2 commits Nov 21, 2019
nit
@jackcmay jackcmay force-pushed the jackcmay:6872 branch from 009d9a7 to 80cc659 Nov 25, 2019
@jackcmay jackcmay requested review from jstarry and t-nelson Nov 25, 2019
@@ -50,6 +51,19 @@ pub fn pubkey_of(matches: &ArgMatches<'_>, name: &str) -> Option<Pubkey> {
value_of(matches, name).or_else(|| keypair_of(matches, name).map(|keypair| keypair.pubkey()))
}

// Return pubkey/signature pairs for a string of the form pubkey=signature
pub fn pubkeys_sigs_of(matches: &ArgMatches<'_>, name: &str) -> Option<Vec<(Pubkey, Signature)>> {
matches.values_of(name).map(|xs| {

This comment has been minimized.

Copy link
@jstarry

jstarry Nov 26, 2019

Member

nit: xs -> pubkey_sig_pair

This comment has been minimized.

Copy link
@jackcmay

jackcmay Nov 26, 2019

Author Contributor

xs is actually Values, good suggestion though, I changed these names to make them more descriptive.

fn test_pubkeys_sigs_of() {
let key1 = Pubkey::new_rand();
let key2 = Pubkey::new_rand();
let sig1 = Keypair::new().sign_message(&[0u8]);

This comment has been minimized.

Copy link
@jstarry

jstarry Nov 26, 2019

Member

nit: could do Signature::new(&[0u8]) instead

This comment has been minimized.

Copy link
@jackcmay

jackcmay Nov 26, 2019

Author Contributor

These aren't the same things

This comment has been minimized.

Copy link
@jstarry

jstarry Nov 26, 2019

Member

Oh, why not? Isn't it the same Signature? Feel free to disregard then :)

This comment has been minimized.

Copy link
@jackcmay

jackcmay Nov 26, 2019

Author Contributor

new takes a full signature array

|| self.signatures.len() != num_required_signatures
|| self.message.account_keys.len() < num_required_signatures
Comment on lines +262 to +263

This comment has been minimized.

Copy link
@jstarry

jstarry Nov 26, 2019

Member

These checks seem outside of the scope of this method and could lead to confusing errors to the caller of replace_signatures. I think that it's fine to rely on our runtime to reject these errors. Thoughts?

This comment has been minimized.

Copy link
@jackcmay

jackcmay Nov 26, 2019

Author Contributor

Having these checked up front rather then after the transaction is submitted would be very helpful. I was also following the example of get_signing_keypair_positions. Maybe we should generate more descriptive errors?

This comment has been minimized.

Copy link
@jstarry

jstarry Nov 26, 2019

Member

Never mind, I see that by doing this you are avoiding a panic from a bad index. Looks fine 👍

@@ -246,6 +255,26 @@ impl Transaction {
.collect())
}

/// Replace all the signatures and pubkeys
pub fn replace_signatures(&mut self, signers: &[(Pubkey, Signature)]) -> Result<()> {

This comment has been minimized.

Copy link
@jstarry

jstarry Nov 26, 2019

Member

How about moving signature verification here instead of inside a separate method? In order to replace a signature it must be verified first? That way verify can't be forgotten?

.enumerate()
.for_each(|(i, (pubkey, signature))| {
self.signatures[i] = *signature;
self.message.account_keys[i] = *pubkey;

This comment has been minimized.

Copy link
@jstarry

jstarry Nov 26, 2019

Member

I don't think account keys should be overridden. They should be checked for equality. Otherwise the compiled instructions could point to the wrong account key.

This comment has been minimized.

Copy link
@jackcmay

jackcmay Nov 26, 2019

Author Contributor

Where do the account keys come from if the online entity doesn't have the keypairs (only the offline entity does)? Are you thinking the pubkeys should come from a config file?

Couple of thoughts

  • account keys are public, if this is a legitimate concern we should probably make those private
  • The account keys used in the instructions themselves don't come from the config file (private keypairs), they come from parameters passed on the command line (there may be exceptions).
  • If the account keys change then the signatures would have to still match and if they do then its what the signer intended anyway.

This comment has been minimized.

Copy link
@jstarry

jstarry Nov 26, 2019

Member

Hmm good points.

Could account keys come from just the public keys then and when you pass in signers, you don't attempt to sign the transaction?

account keys are public, if this is a legitimate concern we should probably make those private

Yeah, I think I agree

The account keys used in the instructions themselves don't come from the config file (private keypairs), they come from parameters passed on the command line (there may be exceptions).

I'm confused by this. Looks like you are still creating a signed transaction with config here: https://github.com/solana-labs/solana/pull/7104/files#diff-6897771877027eaa520b55b24b6dceafR797. That doesn't look right... Can you help me understand why this makes sense?

If the account keys change then the signatures would have to still match and if they do then its what the signer intended anyway.

Yeah true, but could lead to confusing errors because it wouldn't be obvious that the signature was failing because of mis-ordered signers right?

This comment has been minimized.

Copy link
@jackcmay

jackcmay Nov 26, 2019

Author Contributor

We don't really have a way to determine whether signer ordering is correct since we don't have anything to compare to between what the CLI caller passes in and what is in the transaction.

As for why we create a signed transaction, the instructions themselves don't contain the pubkey, just the indices (unless they are placed into the instruction data but that's a whole different issue) and therefore get replaced appropriately. If we are worried about non-signer keys we could replace all the account keys. A signed transaction matches the source form and order that the pubkey=signature pairs were provided to the offline user when the original was produced. I would rather not generate the transaction differently whether its offline or online. If for some use case we encounter a case where the private key is stored in the instruction data we can deal with that appropriately then.

This comment has been minimized.

Copy link
@jstarry

jstarry Nov 26, 2019

Member

Ah ok! Makes a lot more sense now, thanks for explaining.

@mergify mergify bot dismissed jstarry’s stale review Nov 26, 2019

Pull request has been modified.

@mvines mvines added the v0.21 label Nov 26, 2019
@jackcmay jackcmay merged commit 88cb0c6 into solana-labs:master Nov 26, 2019
13 checks passed
13 checks passed
Rule: v0.21 backport (backport) Backports have been created
Details
Summary 2 rules match and 2 potential rules
Details
buildkite/solana Build #15647 passed (27 minutes, 18 seconds)
Details
buildkite/solana/bench Passed (11 minutes, 57 seconds)
Details
buildkite/solana/checks Passed (1 minute, 49 seconds)
Details
buildkite/solana/coverage Passed (9 minutes, 51 seconds)
Details
buildkite/solana/local-cluster Passed (13 minutes, 26 seconds)
Details
buildkite/solana/move Passed (5 minutes)
Details
buildkite/solana/pipeline-upload Passed (2 seconds)
Details
buildkite/solana/shellcheck Passed (28 seconds)
Details
buildkite/solana/stable Passed (25 minutes, 18 seconds)
Details
buildkite/solana/stable-perf Passed (10 minutes, 3 seconds)
Details
ci-gate Pull Request accepted for CI pipeline
@jackcmay jackcmay deleted the jackcmay:6872 branch Nov 26, 2019
mergify bot pushed a commit that referenced this pull request Nov 26, 2019
(cherry picked from commit 88cb0c6)
solana-grimes added a commit that referenced this pull request Nov 26, 2019
automerge
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.