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

Cache verification key in CLI #158

Closed
mitschabaude opened this issue May 10, 2022 · 17 comments
Closed

Cache verification key in CLI #158

mitschabaude opened this issue May 10, 2022 · 17 comments
Assignees

Comments

@mitschabaude
Copy link
Contributor

When you go through the zk deploy flow multiple times, it's annoying that the veriffication key is rebuilt every time, which takes 1-2 minutes

Would be nice to cache the veriffication key

TBD: how do we handle cache invalidation / enable rebuilding when it's needed. Ideally, we detect when the vk changed from a quick-to-compute hash of the circuit, but that would require more work on the snarkyjs side.

Some ideas for what we could do now:

  • never invalidate the cache, expose an explicit --rebuild option to zk deploy
  • separate compiling into its own command, zk compile, which never uses a cache, and let zk deploy just depend on the output it creates -- that way, rebuilding is always an explicit operation
@jasongitmail
Copy link
Contributor

jasongitmail commented May 25, 2022

In what situations would would a dev would need to generate & deploy the same verification key? Only if deploying once to network A (i.e. Testnet) and again to network B (i.e. Mainnet)? If so, I could see some value but maybe low priority

@jasongitmail
Copy link
Contributor

Actually I found an answer: If an error occurs during deployment (i.e. tx relayer unavailable, etc) then it would be convenient to have that cached for when re-trying again.

@mitschabaude
Copy link
Contributor Author

mitschabaude commented May 26, 2022

Yes, anything where you get off the happy path. For example: Something in your code prevents deploy from working, and you want to iterate on that. Like the error that Serhii reported recently, it probably happened in the Mina.transaction block, which is (has to be) after compiling. Then, with every fix you try, you have to wait 2 minutes to see if it works. This is not a theoretical problem, it was a pain for me and there were users in discord complaining about this too.

There are also cases where you break out of the flow even if no error occurs. For example, you get to the point where you have to set the transaction fee, but you are not sure what fee you should use, so you Ctrl-C and do it again later. These things happen all the time.

@jasongitmail
Copy link
Contributor

That's a good use case. Adding 'to discuss' so we can decide an implementation approach

@mitschabaude
Copy link
Contributor Author

@bkase to write RFC

@bkase
Copy link
Member

bkase commented Jun 6, 2022

Mini RFC addressing this:

how do we handle cache invalidation / enable rebuilding when it's needed. Ideally, we detect when the vk changed from a quick-to-compute hash of the circuit, but that would require more work on the snarkyjs side.

Summary

We already have logic to do this in the OCaml and Rust backends. This RFC proposes we expose some of this logic over the JS bridge and outlines a mechanism by which to do so: Namely, take the OCaml one and plumb it alongside the other compile/proving functions that we've bridged over. Once we have a function that can compute a hash, we can cheaply check if the circuit has changed before doing a recompile. We can store this hash in a file in the build/ directory.

Background

We do already have existing logic for quickly computing a hash of the circuit; however, it's not exposed to the JS-side of SnarkyJS yet.

On the Rust side, we use a SHA-256 hash: https://github.com/o1-labs/proof-systems/blob/master/kimchi/src/circuits/gate.rs#L174

On the OCaml side, we use an MD5 hash: https://github.com/MinaProtocol/mina/blob/6c61262618847f51debf4e59f556da03988c86b0/src/lib/crypto/kimchi_backend/common/plonk_constraint_system.ml#L561

It's unfortunate that there are two different hashes for circuits. Ideally these should be canonicallized, but it's out of scope for this RFC.

Specific Changes

From the perspective of SnarkyJS, zkapp-cli, and any other zkApp developer tooling, the actual representation of this digest doesn't actually matter. What matters is that it's a stable hash that can be compared for equality. As such, we can arbitrarily pick the MD5 or SHA-256 one, and our choice doesn't need to be future-compatible. This format can change later, and it won't break any of our tooling.

Therefore, it makes more sense to use whichever one is easier to wire into SnarkyJS. This seems to be the OCaml representation as the rest of the proving/compilation logic goes through that code path.

In the Mina repo:

At https://github.com/MinaProtocol/mina/blob/b0548428278e4d9f48de87d0e56648337a1527a9/src/lib/pickles/pickles.ml#L557 , we're already computing the constraint system digest as part of the compile logic; we can lift this part of the function into a new function that we stick on the Pickles module, call it Pickles.circuit_digest that returns this digest. Then inside https://github.com/MinaProtocol/mina/blob/b0548428278e4d9f48de87d0e56648337a1527a9/src/lib/snarky_js_bindings/lib/snarky_js_bindings_lib.ml#L1752 we add a new function that just calls the circuit_digest and returns it to JavaScript as a hex string (note: we're already using an Md5.to_hex elsewhere in this file).

In SnarkyJS:

We'll expose a new static method on the SmartContract class in SnarkyJS called digest which uses the JS bridge to call this function and return the digest string.

In zkapp-cli:

We create lazily create files in build/ for each of the smart contracts that we build (currently this compilation logic is in deploy.js). The name of the file is <smart-contract-name>-circuit.digest and the contents is the exact string of the digest. If the file exists the next time we are about to compile the circuit, we first see if the contents match the digest of this circuit we want to build. If so, skip recompilation. If not, rebuild the circuit, and then replace the contents of this file.

We can consider exposing a new command digest for advanced users that prints this digest to stdout in the future.

@jasongitmail
Copy link
Contributor

jasongitmail commented Jun 6, 2022

We could also store the digest values inside of a digests property or similar, within build.json. Feels like a natural location given we store other computed values there already.

@mitschabaude
Copy link
Contributor Author

Very nice @bkase, this approach sounds like it should work and not too hard to implement!

@bkase
Copy link
Member

bkase commented Jun 6, 2022

We could also store the digest values inside of a digests property or similar, within build.json. Feels like a natural location given we store other computed values there already.

The only risk is corruption if there are multiple writes at the same time to this file, but as long as we're careful this should work 👍

@jasongitmail
Copy link
Contributor

true, but should be ok b/c we should gather all digests then write to disk once

@jasongitmail
Copy link
Contributor

@bkase implemented this and Matthew and Gregor approved. Builds and makes artifact. @mitschabaude will help expose in SnarkyJS this week

@jasongitmail jasongitmail changed the title Cache verification key Cache verification key in CLI Jun 27, 2022
@ymekuria
Copy link
Collaborator

ymekuria commented Jul 12, 2022

zkapp-cli caching approach

For every smart contract that we build, we can quickly compute and store the hash of the contract in the build.json file using the SmartContract.digest() method from SnarkyJS. We can store the digest with the property:

"digests": { <contractName>: <digest> }

We can also store the corresponding verification key generated from compiling the smart contract in the build.json file with the property:

"verificationKeys": { <contractName>: <verificationKey> }

First we compute the digest of the current contract here in the generate verification key step of the deploy.js file.

let currentDigest = await zkapp.digest();

We can also read and parse the build.json file here using readJsonSync.

let buildJson = fs.readJsonSync("${DIR}/build/build.json");

If the current digest matches the previous digest string from the build.json file, the contract will not be recompiled and we can use the verification key generated from the previous compilation that is stored in the build.json file.

If the current digest doesn't match the previous stored digest, we replace the old current digest with the new current digest in the build.json file. We then compile the smart contract.

let { verificationKey } = await zkApp.compile(zkAppAddress);

After that, we replace the newly generated verification key with the previously stored verification key in build.json. We update both the the digest and the verification keys first, and write to build.json once with the updated values using writeJsonSync.

Testing

We can add tests to the deploy.tests.js file to validate this implementation and the generate verification key step. We can also manually generate contracts and deploy them from the CLI to test. Cases to test:

  • Multiple contracts in build.json each with digests and verification keys stored.
  • No digest or verification keys in build.json(first time building contract).
  • Verify changing the contract results in a new digest and recompile.

@jasongitmail
Copy link
Contributor

@ymekuria Looks like we can't use build.json after all because it is clobbered each time deploy is run here:

const build = await step('Generate build.json', async () => {
// Identify all instances of SmartContract in the build.
const smartContracts = await findSmartContracts(`${DIR}/build/**/*.js`);
fs.outputJsonSync(
`${DIR}/build/build.json`,
{ smartContracts },
{ spaces: 2 }
);
return { smartContracts };
});

But we could add a build/cache.json file and store the cache-related data there. I'd probably just nest both under the smart contract name:

{
  "<smartContractNameA>": {
    "digest": "abc",
    "verificationKey": "abc",
  },
  "<smartContractnameB>": {
    "digest": "abc",
    "verificationKey": "abc",
  }
}

Note that there is a known limitation of build.json, and this caching code by extension, in that we assume all smart contracts in a project have unique names. It's probably acceptable, but we could warn people if their project contains >1 smart contract with the same name; I'll create an issue to track this.

@ymekuria
Copy link
Collaborator

But we could add a build/cache.json file and store the cache-related data there. I'd probably just nest both under the smart contract name:

Good catch about build.json. I like the idea of adding a cache.json file.

@ymekuria
Copy link
Collaborator

ymekuria commented Jul 13, 2022

@jasongitmail It looks like we can't put cache.json file in the build directory because the directory is emptied every time deploy is run here:

await step('Build project', async () => {
fs.emptyDirSync(`${DIR}/build`); // ensure old artifacts don't remain
await sh('npm run build --silent');
});

We can add the cache.json file to the keys directory or create another directory at the root of the template and add it to gitignore.

@mitschabaude
Copy link
Contributor Author

@ymekuria, I think we should just not empty the build directory in that case :D instead we could delete everything in there except cache.json

FWIW, inside keys feels wrong to me, because the verification "key" isn't a key in the usual sense of the term and putting it there would only contribute further to this confusion :D

@jasongitmail
Copy link
Contributor

The keys dir isn't the right location because this is a file that a dev should never really see.

+1 for changing line 88 to delete all files except the cache.json. I'd suggest reading cache.json into memory, empty the dir, and output cache.json again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants