o1-labs / o1js

TypeScript framework for zk-SNARKs and zkApps
https://docs.minaprotocol.com/en/zkapps/how-to-write-a-zkapp
Apache License 2.0
491 stars 107 forks source link

Allow to call Transaction.sign() several times #1582

Open dfstio opened 4 months ago

dfstio commented 4 months ago

Proposal: make it possible to call .sign several times

tx.sign([zkAppPrivateKey]);
tx.sign([deployer]);

It will allow to call the function that signs the tx without disclosing the privateKey:

tx.sign([zkAppPrivateKey]);
await signWithDeployerKey(tx);

I guess that now the code at the addSignature prevents doing it: https://github.com/o1-labs/o1js/blob/main/src/lib/mina/account-update.ts#L1867

Example:

Specifically, I'm interested now in being able to run the code at zkCloudWorker AWS instance and provide from the zkCloudWorker to running code the possibility to sign the txs without disclosing the privateKey (as even the dust funds will be stolen sooner or later): https://github.com/zkcloudworker/zkcloudworker-lib/blob/main/src/cloud/cloud.ts#L72

We've successfully resolved the challenge to serialize txs to move them between the various environments; now the blocker is to be able to sign the same transaction in the different environments: https://github.com/o1-labs/o1js/issues/1221 https://discord.com/channels/484437221055922177/1228326948078489642

harrysolovay commented 4 months ago

I'm not sure how to reconcile this use case with the changes in #1574. On one hand, the idea of a partial signature seems like a footgun without some type-level tracking (#1570). On the other hand, @dfstio has a clear use case: constructing a transaction signed by two private keys, which live in different environments. My sense is that this should be possible, but not necessarily as the "happy" path. Perhaps we could have some kind of placeholder utility.

const a = tx.prove().sign([knownPk, _("deployerPrivateKey")]) // `Transaction<true, Missing<"deployerPrivateKey">>`

This would make sense given we intend to serialize the transaction a and then hydrate it in a different environment. The hydration step would require the placeheld private key.

mitschabaude commented 4 months ago

well, the signing code is already lying about the types. it puts a dummy signature on the account update if it needs signing but there's no private key

we could very easily enable signing twice by removing the limitation that an already signed tx can't be signed again. the Signed type just needs to get the .sign() method back, and the runtime code needs to replace the signature even if it's no longer "lazy"

dfstio commented 4 months ago

Will it be enough to replace the code

    if (i === -1) {
      // private key is missing, but we are not throwing an error here
      // there is a change signature will be added by the wallet
      // if not, error will be thrown by verifyAccountUpdate
      // while .send() execution
      Authorization.setSignature(accountUpdate, dummySignature());
      return accountUpdate as AccountUpdate & {
        lazyAuthorization: undefined;
      };
    }

with

    if (i === -1) {
      // private key is missing, the sign() should be called one more time later
      return accountUpdate;
    }

at https://github.com/o1-labs/o1js/blob/main/src/lib/mina/account-update.ts#L1867 ?

mitschabaude commented 4 months ago

no @dfstio, I'd prefer to not change that code, because that would break our model of capturing signed status with a type.

I'd rather change this code, such that it doesn't skip signing if a signature (not lazy signature) already exists and the private key matches

if (accountUpdate.lazyAuthorization?.kind !== 'lazy-signature') {
  return accountUpdate as AccountUpdate & { lazyAuthorization?: LazyProof };
}