Enable custom network signing #1444

Trivo25 · 2024-02-18T13:04:08Z

This PR enables custom network signing and verification, all across our stack (o1js and mina-signer). This feature unlocks the use of o1js and mina-signer with layer 2s/3s or chains isomorphic to Mina. All while maintaining backwards compatibility

bindings o1-labs/o1js-bindings#247

closes #1149

Trivo25 · 2024-02-18T13:58:43Z

src/mina-signer/src/sign-zkapp-command.ts

@@ -159,18 +160,6 @@ function accountUpdatesToCallForest<A extends { body: { callDepth: number } }>(
  return forest;
 }

-const zkAppBodyPrefix = (network: string) => {


moved this into mina-signer/src/signature.js for shared use with o1js

Trivo25 · 2024-02-18T13:59:05Z

src/mina-signer/src/sign-zkapp-command.unit-test.ts

-  expect(hashTestnet).toEqual(hashSnarkyTestnet.toBigInt());
-  expect(hashMainnet).toEqual(hashSnarkyMainnet.toBigInt());
-});
+test(


cleaned this up a little by utilizing RandomTransaction.networkId

Trivo25 · 2024-02-18T13:59:56Z

src/mina-signer/src/sign-zkapp-command.unit-test.ts

-  let commitment = callForestHash(callForest, 'testnet');
-  expect(commitment).toEqual(FieldConst.toBigint(ocamlCommitments.commitment));
-});
+test(


this now tests all network id types

Trivo25 · 2024-02-18T14:01:54Z

src/mina-signer/src/signature.ts

+  }
+}
+
+const createCustomPrefix = (prefix: string) => {


OCaml uses a * padding to 20 bytes

Trivo25 · 2024-02-18T14:02:30Z

src/mina-signer/src/signature.unit-test.ts

@@ -23,26 +23,29 @@ function checkConsistentSingle(
  msg: Field,
  key: PrivateKey,
  keySnarky: PrivateKeySnarky,
-  pk: PublicKey
+  pk: PublicKey,
+  networkId: string


some cleanup, tests all three network cases now

MartinOndejka

Signature in o1js (not mina-signer) has still hard-coded testnet prefix.

Trivo25 · 2024-02-19T12:20:08Z

Signature in o1js (not mina-signer) has still hard-coded testnet prefix.

good catch!

mitschabaude · 2024-02-19T12:49:25Z

Signature in o1js (not mina-signer) has still hard-coded testnet prefix.

good catch!

The network id was purposefully left out as a parameter in the userland signature methods: the in-SNARK Signature.verify() and its mina-signer counter-part signFields(). These signatures are not designed to be related to any particular network, and are not verified by a Mina node.

We only happen to reuse the same signing functions also used by transaction signatures, so we had to pick some choice of networkId. So we decided to hard-code "testnet".

Unless there is a specific reason to make these signatures network-specific in the PR, I'd be in favor of reverting these changes. Adding the networkId parameter just adds confusion IMO.

CHANGELOG.md

src/lib/signature.ts

src/mina-signer/mina-signer.ts

Trivo25 · 2024-02-19T13:12:09Z

Unless there is a specific reason to make these signatures network-specific in the PR, I'd be in favor of reverting these changes. Adding the networkId parameter just adds confusion IMO.

Hmm given that you can specify a custom message anyway (with a custom nonce), this really can be reverted. But curious to know if @MartinOndejka sees an explicit need for this?

MartinOndejka · 2024-02-19T15:07:33Z

Unless there is a specific reason to make these signatures network-specific in the PR, I'd be in favor of reverting these changes. Adding the networkId parameter just adds confusion IMO.

Hmm given that you can specify a custom message anyway (with a custom nonce), this really can be reverted. But curious to know if @MartinOndejka sees an explicit need for this?

Not at all, I just remember that it was a little confusing a while back when I found out that it has hardcoded testnet.

mitschabaude · 2024-02-19T15:14:10Z

Not at all, I just remember that it was a little confusing a while back when I found out that it has hardcoded testnet.

@Trivo25 maybe it would be good to add a comment to these methods explaining why they hard-code testnet

shimkiv · 2024-02-19T16:49:20Z

src/mina-signer/src/types.ts

@@ -9,7 +9,7 @@ export type Field = number | bigint | string;
 export type PublicKey = string;
 export type PrivateKey = string;
 export type Signature = SignatureJson;
-export type NetworkId = 'mainnet' | 'testnet';
+export type NetworkId = string;


Maybe something like export type NetworkId = 'mainnet' | 'testnet' | string; for better intellisense?

I think TS would just collapse that to string, because 'mainnet' and 'testnet' are contained in string

so after all, we can probably just remove the NetworkId export which caused us to release 2 or 3 new versions in a row 😆

btw, now we also need those runtime checks which I made you remove @shimkiv 😅

damn, I wonder if we should keep the detailed type somehow. Maybe something like

type NetworkId = 'mainnet' | 'testnet' | { custom: string }

@Trivo25 wdyt

shimkiv · 2024-02-20T07:32:28Z

src/mina-signer/src/sign-zkapp-command.unit-test.ts

-  expect(hashTestnet).toEqual(hashSnarkyTestnet.toBigInt());
-  expect(hashMainnet).toEqual(hashSnarkyMainnet.toBigInt());
-});
+test(


@Trivo25
Throughout different tests we check that signatures match like testnet <-> testnet but should we also check that the resulting signatures are indeed different for different networks? Like testnet <-> mainnet should always differ.

Yes, we should! And we do
For example, further down we verify against a different network and expect the verification to fail

expect( verify(sigFieldElements, networkId === 'mainnet' ? 'testnet' : 'mainnet') ).toEqual(false);

but I added a check that the hash doesnt match another networks hash!

Trivo25 · 2024-02-20T09:27:23Z

src/mina-signer/mina-signer.ts

-    if (!options?.network) {
-      throw Error('Invalid Specified Network');
-    }
-    const specifiedNetwork = options.network.toLowerCase();


this is no longer needed since the type now is 'testnet' | 'mainnet' which already is lower case

Trivo25 · 2024-02-20T09:35:40Z

src/mina-signer/src/sign-zkapp-command.unit-test.ts

-  expect(hashTestnet).toEqual(hashSnarkyTestnet.toBigInt());
-  expect(hashMainnet).toEqual(hashSnarkyMainnet.toBigInt());
-});
+test(


but I added a check that the hash doesnt match another networks hash!

Trivo25 · 2024-02-20T09:48:46Z

src/mina-signer/src/signature.unit-test.ts

-  expect(okMainnetTestnet).toEqual(false);
-  expect(okTestnetMainnet).toEqual(false);
-  expect(okMainnetMainnet).toEqual(true);
+  expect(verifyFieldElement(sig, msg, pk, networkId)).toEqual(true);


test is now condensed by using networkId as input

Trivo25 · 2024-02-20T09:49:04Z

src/mina-signer/src/types.ts

-export type NetworkId = 'mainnet' | 'testnet';
+export type NetworkId = 'mainnet' | 'testnet' | { custom: string };
+
+export const NetworkId = {


internal helper to get the plain networkId

nicely done!

mitschabaude

awesome!

Trivo25 added 4 commits February 17, 2024 19:57

add custom network id

04d6085

temp

66f0699

make custom network deriver for signature

d724468

fix types

9081fb8

Trivo25 mentioned this pull request Feb 18, 2024

Enable custom network signing o1-labs/o1js-bindings#247

Merged

Trivo25 added 5 commits February 18, 2024 14:29

refac

44070bd

fix legacy test

f4625c1

bindings

eef6849

refac derivers

02450c4

remove explicit network id declartion

33abba1

Trivo25 commented Feb 18, 2024

View reviewed changes

Trivo25 marked this pull request as ready for review February 18, 2024 14:03

dont use deprecated function

90bb558

Trivo25 changed the title ~~Enable custom network signing~~ Enable custom network signing Feb 18, 2024

MartinOndejka reviewed Feb 19, 2024

View reviewed changes

Trivo25 added 5 commits February 19, 2024 13:22

add optional network parameter to signature

3cb66fb

Merge branch 'main' into custom-network

c35b9a5

changelog

105320d

fix verify

90764e5

typo

2c3a6f2

mitschabaude reviewed Feb 19, 2024

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

src/lib/signature.ts Outdated Show resolved Hide resolved

src/mina-signer/mina-signer.ts Outdated Show resolved Hide resolved

shimkiv reviewed Feb 19, 2024

View reviewed changes

shimkiv reviewed Feb 20, 2024

View reviewed changes

shimkiv mentioned this pull request Feb 20, 2024

Test mainnet configurations for berkeley MinaProtocol/mina#15102

Closed

revert sig type

cd43ef7

Trivo25 added 7 commits February 20, 2024 10:14

revert sig type

1a60fd1

fix build

3c799af

add NetworkID.toString

d311e92

simplify

1685902

cleanup

ecf67e5

fix tests

5840e76

add check against different network hash

5b9bdcf

Trivo25 commented Feb 20, 2024

View reviewed changes

mitschabaude approved these changes Feb 20, 2024

View reviewed changes

Trivo25 merged commit b0fb1c8 into main Feb 20, 2024
13 checks passed

Trivo25 deleted the custom-network branch February 20, 2024 18:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable custom network signing #1444

Enable custom network signing #1444

Trivo25 commented Feb 18, 2024 •

edited

Loading

Trivo25 Feb 18, 2024

Trivo25 Feb 18, 2024

Trivo25 Feb 18, 2024

Trivo25 Feb 18, 2024

Trivo25 Feb 18, 2024

MartinOndejka left a comment

Trivo25 commented Feb 19, 2024

mitschabaude commented Feb 19, 2024 •

edited

Loading

Trivo25 commented Feb 19, 2024

MartinOndejka commented Feb 19, 2024

mitschabaude commented Feb 19, 2024 •

edited

Loading

shimkiv Feb 19, 2024

mitschabaude Feb 19, 2024

mitschabaude Feb 19, 2024 •

edited

Loading

mitschabaude Feb 19, 2024

mitschabaude Feb 19, 2024

shimkiv Feb 20, 2024 •

edited

Loading

Trivo25 Feb 20, 2024

Trivo25 Feb 20, 2024

Trivo25 Feb 20, 2024

Trivo25 Feb 20, 2024

Trivo25 Feb 20, 2024

Trivo25 Feb 20, 2024

mitschabaude Feb 20, 2024

mitschabaude left a comment

Enable custom network signing #1444

Enable custom network signing #1444

Conversation

Trivo25 commented Feb 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MartinOndejka left a comment

Choose a reason for hiding this comment

Trivo25 commented Feb 19, 2024

mitschabaude commented Feb 19, 2024 • edited Loading

Trivo25 commented Feb 19, 2024

MartinOndejka commented Feb 19, 2024

mitschabaude commented Feb 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitschabaude Feb 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shimkiv Feb 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitschabaude left a comment

Choose a reason for hiding this comment

Trivo25 commented Feb 18, 2024 •

edited

Loading

mitschabaude commented Feb 19, 2024 •

edited

Loading

mitschabaude commented Feb 19, 2024 •

edited

Loading

mitschabaude Feb 19, 2024 •

edited

Loading

shimkiv Feb 20, 2024 •

edited

Loading