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

Mina-signer: in-SNARK verifiable Signatures #710

Merged
merged 10 commits into from
Feb 10, 2023
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

- `Transaction.fromJSON` to recover transaction object from JSON https://github.com/o1-labs/snarkyjs/pull/705

### Changed

- BREAKING CHANGE: Modify signature algorithm used by `Signature.{create,verify}` to be compatible with mina-signer https://github.com/o1-labs/snarkyjs/pull/710
- Signatures created with mina-signer's `client.signFields()` can now be verified inside a SNARK!
- Breaks existing deployed smart contracts which use `Signature.verify()`

## [0.8.0](https://github.com/o1-labs/snarkyjs/compare/d880bd6e...c5a36207)

### Added
Expand Down
6 changes: 5 additions & 1 deletion src/lib/account_update.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1877,7 +1877,11 @@ function signJsonTransaction(
accountUpdate.authorization.proof === null
) {
zkappCommand = JSON.parse(
Ledger.signAccountUpdate(JSON.stringify(zkappCommand), privateKey, i)
Ledger.signOtherAccountUpdate(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unrelated fix

JSON.stringify(zkappCommand),
privateKey,
i
)
);
}
}
Expand Down
7 changes: 7 additions & 0 deletions src/lib/core.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ import { bytesToBigInt } from '../js_crypto/bigint-helpers.js';
import { defineBinable } from '../provable/binable.js';
import { sizeInBits } from '../provable/field-bigint.js';
import { Bool, Field, Scalar, Group } from '../snarky.js';
import { Scalar as ScalarBigint } from '../provable/curve-bigint.js';
import { mod } from '../js_crypto/finite_field.js';

export { Field, Bool, Scalar, Group };

Expand Down Expand Up @@ -72,3 +74,8 @@ That means it can't be called in a @method or similar environment, and there's n
highBit: Bool(x >> lowBitSize === 1n),
};
};

Scalar.fromBigInt = function (scalar: bigint) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing helper which was useful here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same question as elsewhere, I'm curious how a dev would know not to use these functions if they want them to create constraints? My guess is that we're warning them that going to bigint or from bigint is not constraining anything?

Copy link
Member Author

@mitschabaude mitschabaude Feb 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For methods like these, which take a JS value and produce constant field elements, I think no warning is needed -- it's a valid and useful way to introduce constants into the circuit.

Going the other way -- from field elements to JS value -- is a very common mistake, but throws an error as soon as you try to compile the contract. That's because extracting values, under the hood, always works like this:

match x with
  | Constant x -> x
  | x -> As_prover.read_var x

so during compile time you'll hit an error for the As_prover.read_var x case. The constant case is fine - no need to prove how constants are computed, since the prover can't change them

scalar = mod(scalar, ScalarBigint.modulus);
return Scalar.fromJSON(scalar.toString());
};
74 changes: 66 additions & 8 deletions src/lib/signature.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,12 @@
import { Group, Field, Bool, Scalar, Ledger, Circuit } from '../snarky.js';
import { Group, Field, Bool, Scalar, Ledger } from '../snarky.js';
import { prop, CircuitValue, AnyConstructor } from './circuit_value.js';
import { Poseidon } from './hash.js';
import { hashWithPrefix } from './hash.js';
import {
deriveNonce,
Signature as SignatureBigint,
} from '../mina-signer/src/signature.js';
import { Scalar as ScalarBigint } from '../provable/curve-bigint.js';
import { prefixes } from '../js_crypto/constants.js';

// external API
export { PrivateKey, PublicKey, Signature };
Expand Down Expand Up @@ -192,12 +198,23 @@ class Signature extends CircuitValue {
static create(privKey: PrivateKey, msg: Field[]): Signature {
const publicKey = PublicKey.fromPrivateKey(privKey).toGroup();
const d = privKey.s;
const kPrime = Scalar.random();
const kPrime = Scalar.fromBigInt(
deriveNonce(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

crypto change: instead of a random nonce, we now use the same deterministic nonce derivation as in mina-signer (and elsewhere in our protocol)

{ fields: msg.map((f) => f.toBigInt()) },
{ x: publicKey.x.toBigInt(), y: publicKey.y.toBigInt() },
BigInt(d.toJSON()),
'testnet'
)
);
let { x: r, y: ry } = Group.generator.scale(kPrime);
const k = ry.toBits()[0].toBoolean() ? kPrime.neg() : kPrime;
const e = Scalar.fromBits(
Poseidon.hash(msg.concat([publicKey.x, publicKey.y, r])).toBits()
let h = hashWithPrefix(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

crypto change: instead of plain hashing with Poseidon, we use a hash prefix

prefixes.signatureTestnet,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I chose to hard-code the signature to 'testnet', since the distinction between testnet and mainnet for signatures which aren't about transactions seemed not useful to me and just adds noise IMO

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is actually still very important if these signatures are going to be used in zkApps -- the same security vulnerability applies: If I sign something on the testnet, it could be replayed on mainnet and my funds can be drained if I use the same keypairs/nonce.

Copy link
Member Author

@mitschabaude mitschabaude Jan 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point! It opens a can of worms though. When signing transactions, the signature verifier is the Mina node, which has a well-defined network - it's either a mainnet node or a testnet node, so it knows which one it should use for verifying.
However, a smart contract currently doesn't have that. Do you think the network type should be hard-coded into the smart contract / circuit somehow? Should we leave it as a constant for the developer to define? (But then we're pushing to them the problem of switching this flag when deploying / compiling to either network).

I think, if we go this route, the network type should definitely be a constant in the circuit and not a Bool that the developer can make variable. Because if it can be variable, it's too easy a mistake to just make it a private user input which doesn't constrain the proofs to be valid for the respective network.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I suppose it’s much less dangerous than a transaction so I think it’s not worth the complexity cost of making these signatures distinct, but we should probably run this by more people. I’ll approve but I’m going to send a link to one of our slack channels and let’s let it bake overnight if anyone disagrees before we land it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

am I understanding this right that the constant doesn't matter here because it's what the zkapp will verify and so you could pretty much use anything?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mimoo yes, doesn't matter here in the sense that these signatures won't be sent to either network. we are completely in user-land: signatures are inputs to proofs created locally, where they are verified in-snark.

I think Brandon highlights an important problem: we don't provide replay protection for the user here, especially given that signatures are deterministic (nonce is derived from the message and private key). so, we currently leave it up to the application to figure out how to prevent that.

I think its too deep and broad a problem to solve in the context of this PR, but I created an issue so we don't forget to discuss this: #720

msg.concat([publicKey.x, publicKey.y, r])
);
// TODO: Scalar.fromBits interprets the input as a "shifted scalar"
// therefore we have to unshift e before using it
let e = unshift(Scalar.fromBits(h.toBits()));
Comment on lines +215 to +217
Copy link
Member Author

@mitschabaude mitschabaude Jan 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

crypto change: So this seems like a bug in Scalar.fromBits, which is somewhat hard to resolve in that function directly, since this function would need to unshift the scalar in a SNARK-compatible way. It seemed easier to unshift out of the snark (here) and do the unshifting at the level of curve points when we're in the snark (see verify below)

const s = e.mul(d).add(k);
return new Signature(r, s);
}
Expand All @@ -208,10 +225,51 @@ class Signature extends CircuitValue {
*/
verify(publicKey: PublicKey, msg: Field[]): Bool {
const point = publicKey.toGroup();
let e = Scalar.fromBits(
Poseidon.hash(msg.concat([point.x, point.y, this.r])).toBits()
let h = hashWithPrefix(
prefixes.signatureTestnet,
msg.concat([point.x, point.y, this.r])
);
let r = point.scale(e).neg().add(Group.generator.scale(this.s));
// TODO: Scalar.fromBits interprets the input as a "shifted scalar"
// therefore we have to use scaleShifted which is very inefficient
let e = Scalar.fromBits(h.toBits());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this going to create constraints?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah sure! toBits does the proper in-snark unpacking of a field element, and fromBits is a no-op, since scalars are already represented as an array of bits (unfortunately, the bits represent a shifted scalar :/)

let r = scaleShifted(point, e).neg().add(Group.generator.scale(this.s));
return Bool.and(r.x.equals(this.r), r.y.toBits()[0].equals(false));
}

/**
* Decodes a base58 encoded signature into a {@link Signature}.
*/
static fromBase58(signatureBase58: string) {
let { r, s } = SignatureBigint.fromBase58(signatureBase58);
return Signature.fromObject({
r: Field(r),
s: Scalar.fromJSON(s.toString()),
});
}
/**
* Encodes a {@link Signature} in base58 format.
*/
toBase58() {
let r = this.r.toBigInt();
let s = BigInt(this.s.toJSON());
return SignatureBigint.toBase58({ r, s });
}
}

// performs scalar multiplication s*G assuming that instead of s, we got s' = 2s + 1 + 2^255
// cost: 2x scale by constant, 1x scale by variable
function scaleShifted(point: Group, shiftedScalar: Scalar) {
let oneHalfGroup = point.scale(Scalar.fromBigInt(oneHalf));
let shiftGroup = oneHalfGroup.scale(Scalar.fromBigInt(shift));
return oneHalfGroup.scale(shiftedScalar).sub(shiftGroup);
Comment on lines +259 to +264
Copy link
Member Author

@mitschabaude mitschabaude Jan 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

crypto change: this is how we undo the scalar shift inside the SNARK

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not just do

point.scale((shiftedScalar.sub(shift).mul(inv_2))

Copy link
Contributor

@mimoo mimoo Feb 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh I think I see, the scalars are in the wrong field :D that sucks

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep exactly :D we don't have the foreign field arithmetic to do sub and mul on the scalar, inside the snark

}
// returns s, assuming that instead of s, we got s' = 2s + 1 + 2^255
// (only works out of snark)
function unshift(shiftedScalar: Scalar) {
return shiftedScalar
.sub(Scalar.fromBigInt(shift))
.mul(Scalar.fromBigInt(oneHalf));
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why does this only work out of snark? Also, isn't this a problem if I want to sign within a snark? (since it's used in the signature algo)

Copy link
Member Author

@mitschabaude mitschabaude Feb 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only works out of snark because sub and mul are not available on variable scalars. not sure how efficient they would be to implement in-snark, compared to the extra scaling I have to do instead. do you know?

the signing algorithm was already (I think, deliberately) written to not work inside a snark (for example, it used a constant random field element for the nonce 😱 ; now I'm deriving the nonce as a blake2b hash. tbf, the nonce could be a prover witness). the above will throw an error in the snark.

Inherently, signing in the snark is not as useful because then you have to provide your private key as input -- but that usually sits in some guarded place like your wallet. so, I think it's fine to only support signing outside + verifying inside which achieves the same and plays well with wallets


let shift = ScalarBigint(1n + 2n ** 255n);
let oneHalf = ScalarBigint.inverse(2n)!;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't you do let oneHalf = Scalar.fromBigInt(ScalarBigint.inverse(2n));?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no.. because Scalar.fromBigInt needs the JSOO / Wasm bundle to be loaded, and that's not the case at the top level. (only after await isReady)

45 changes: 44 additions & 1 deletion src/mina-signer/MinaSigner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import {
publicKeyToHex,
rosettaTransactionToSignedCommand,
} from './src/rosetta.js';
import { sign, Signature, verify } from './src/signature.js';

export { Client as default };

Expand Down Expand Up @@ -96,6 +97,48 @@ class Client {
return PublicKey.toBase58(publicKey);
}

/**
* Signs an arbitrary list of field elements in a SNARK-compatible way.
* The resulting signature can be verified in SnarkyJS as follows:
* ```ts
* // sign field elements with mina-signer
* let signed = client.signFieldElements(fields, privateKey);
*
* // read signature in snarkyjs and verify
* let signature = Signature.fromBase58(signed.signature);
* let isValid: Bool = signature.verify(publicKey, fields.map(Field));
* ```
*
* @param fields An arbitrary list of field elements
* @param privateKey The private key used for signing
* @returns The signed field elements
*/
signFields(fields: bigint[], privateKey: Json.PrivateKey): Signed<bigint[]> {
let privateKey_ = PrivateKey.fromBase58(privateKey);
let signature = sign({ fields }, privateKey_, 'testnet');
Comment on lines +116 to +118
Copy link
Member Author

@mitschabaude mitschabaude Jan 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

crypto change: very straight-forward new function in mina-signer to sign an arbitrary list of field elements. it uses the existing sign and hard-codes the network to 'testnet' since this is not a transaction signature

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it makes sense to hardcode the chain id to 'zkapp' or something else then?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it does! Do you think we need actual domain separation between testnet signatures and signatures meant for zkapps? Or is it more about better naming, so that 'zkapp' is an alias for 'testnet'? (it wouldn't be hard to add the real domain separation fwiw)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry for late reply. In general I would say we should use domain separate as much as possible, because it's cheap/free : ) it's possible that it's not exploitable, but still why not distinguish it

return {
signature: Signature.toBase58(signature),
publicKey: PublicKey.toBase58(PrivateKey.toPublicKey(privateKey_)),
data: fields,
};
}

/**
* Verifies a signature created by {@link signFields}.
*
* @param signedFields The signed field elements
* @returns True if the `signedFields` contains a valid signature matching
* the fields and publicKey.
*/
verifyFields({ data, signature, publicKey }: Signed<bigint[]>) {
return verify(
Signature.fromBase58(signature),
{ fields: data },
PublicKey.fromBase58(publicKey),
'testnet'
);
}

/**
* Signs an arbitrary message
*
Expand All @@ -114,7 +157,7 @@ class Client {
}

/**
* Verifies that a signature matches a message.
* Verifies a signature created by {@link signMessage}.
*
* @param signedMessage A signed message
* @returns True if the `signedMessage` contains a valid signature matching
Expand Down
1 change: 1 addition & 0 deletions src/mina-signer/src/signature.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ export {
NetworkId,
signLegacy,
verifyLegacy,
deriveNonce,
};

const networkIdMainnet = 0x01n;
Expand Down
49 changes: 49 additions & 0 deletions src/mina-signer/tests/verify-in-snark.unit-test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
import { Field, isReady, shutdown } from '../../snarky.js';
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this file tests that the new signatures are really compatible between mina-signer and snarkyjs, and also checks that they can be verified inside a SNARK

import { ZkProgram } from '../../lib/proof_system.js';
import Client from '../MinaSigner.js';
import { PrivateKey, Signature } from '../../lib/signature.js';
import { provablePure } from '../../lib/circuit_value.js';
import { expect } from 'expect';

let fields = [10n, 20n, 30n, 340817401n, 2091283n, 1n, 0n];
let privateKey = 'EKENaWFuAiqktsnWmxq8zaoR8bSgVdscsghJE5tV6hPoNm8qBKWM';

// sign with mina-signer
let client = new Client({ network: 'mainnet' });
let signed = client.signFields(fields, privateKey);

// verify with mina-signer
let ok = client.verifyFields(signed);
expect(ok).toEqual(true);

// sign with snarkyjs and check that we get the same signature
await isReady;
let fieldsSnarky = fields.map(Field);
let privateKeySnarky = PrivateKey.fromBase58(privateKey);
let signatureSnarky = Signature.create(privateKeySnarky, fieldsSnarky);
expect(signatureSnarky.toBase58()).toEqual(signed.signature);

// verify out-of-snark with snarkyjs
let publicKey = privateKeySnarky.toPublicKey();
let signature = Signature.fromBase58(signed.signature);
signature.verify(publicKey, fieldsSnarky).assertTrue();

// verify in-snark with snarkyjs
const MyProgram = ZkProgram({
publicInput: provablePure(null),
methods: {
verifySignature: {
privateInputs: [Signature],
method(_: null, signature: Signature) {
signature.verify(publicKey, fieldsSnarky).assertTrue();
},
},
},
});

await MyProgram.compile();
let proof = await MyProgram.verifySignature(null, signature);
ok = await MyProgram.verify(proof);
expect(ok).toEqual(true);

shutdown();
7 changes: 6 additions & 1 deletion src/snarky.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -990,6 +990,11 @@ declare class Scalar {
* This operation does NOT affect the circuit and can't be used to prove anything about the string representation of the Scalar.
*/
static fromJSON(x: string | number | boolean): Scalar;
/**
* Create a constant {@link Scalar} from a bigint.
* If the bigint is too large, it is reduced modulo the scalar field order.
*/
static fromBigInt(s: bigint): Scalar;
static check(x: Scalar): void;
}

Expand Down Expand Up @@ -1274,7 +1279,7 @@ declare class Ledger {
/**
* Signs an account update.
*/
static signAccountUpdate(
static signOtherAccountUpdate(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why did this name change?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an unrelated fix, the name was actually signOtherAccountUpdate in the ocaml bindings, so it didn't work before

txJson: string,
privateKey: { s: Scalar },
i: number
Expand Down