[Feature] Return proving/verifying keys from execution methods#1227
[Feature] Return proving/verifying keys from execution methods#1227marshacb wants to merge 1 commit into
Conversation
…atic KeyStore persistence
iamalwaysuncomfortable
left a comment
There was a problem hiding this comment.
Some changes requested, and likely some unification needed with PR #1208. We can use the ProgramImports abstraction you built there to collect the imports and then store them in typescript.
| let cached_keys = if cache { | ||
| let function_name = IdentifierNative::from_str(function).map_err(|err| err.to_string())?; | ||
| let pk = process | ||
| .get_proving_key(program_native.id(), &function_name) | ||
| .map_err(|_| format!("Could not find proving key for {}/{}", program_native.id(), function))?; | ||
| let vk = process | ||
| .get_verifying_key(program_native.id(), &function_name) | ||
| .map_err(|_| format!("Could not find verifying key for {}/{}", program_native.id(), function))?; | ||
| (Some(pk), Some(vk)) | ||
| } else { | ||
| (None, None) | ||
| }; |
There was a problem hiding this comment.
A couple of notes here:
- In the case of imports, sometimes those are synthesized and added to the process, we want to extract those as well.
- Perhaps instead of a new transaction type, we might want to pass an object into an existing method that collects the keys like the one you came up with in which can then be extracted later?
| setKeyStore(keyStore: KeyStore | undefined) { | ||
| this._keyStore = keyStore; |
There was a problem hiding this comment.
There's this implementation of adding the KeyStore to the key provider waiting in a PR: #1221 - let's see if we can merge the approaches tomorrow.
|
|
||
| // Build an execution transaction | ||
| if (options.cacheKeys) { | ||
| const response: ExecutionTransactionResponse = await WasmProgramManager.buildExecutionTransactionWithKeys( |
There was a problem hiding this comment.
In this outer scope, one thing we might consider instead of a different wasm method is passing in the wasm object you defined and at the end, get the pks/vks out of it and then destroy/free it.
| provingKey: ProvingKey; | ||
| verifyingKey: VerifyingKey; |
There was a problem hiding this comment.
Since wasm is limited to 4gb total memory space, we probably want is to take these out of wasm and destroy them so as not to OOM the process. We also want to return the checksums so that people have a future way of verifying the integrity of the keys.
| provingKey: ProvingKey; | |
| verifyingKey: VerifyingKey; | |
| provingKey: Uint8Array; | |
| provingKeyChecksum: string; | |
| verifyingKey: Uint8Array; | |
| verifyingKeyChecksum: string; |
| async buildExecutionTransaction( | ||
| options: ExecuteOptions, | ||
| ): Promise<Transaction> { | ||
| ): Promise<Transaction | ExecutionTransactionResult> { |
There was a problem hiding this comment.
I originally though this signature might be good, but I think it might break a lot of downstream typescript implementors who will have to account for both types in further type lints.
I think instead we want to keep the original signature the same and allow callers to pass in an imports object which gets mutated and callers can extract the imports from there. OR if they have the keyprovider configured, they can extract it from there as well.
| programName: "my_program.aleo", | ||
| functionName: "my_function", | ||
| inputs: ["1u32", "2u32"], | ||
| cacheKeys: true, |
There was a problem hiding this comment.
We might consider changing this to a KeyCache which gets populated with anything not currently existing.
| #[wasm_bindgen] | ||
| pub struct ExecutionTransactionResponse { | ||
| transaction: TransactionNative, | ||
| proving_key: Option<ProvingKeyNative>, | ||
| verifying_key: Option<VerifyingKeyNative>, | ||
| } |
There was a problem hiding this comment.
I think here we want to go in favor of a typescript object instead that gets populated outside the wasm scope. But what we probably want is the ProgramImports object you created in PR #1208 and use that to collect the keys. Then when the execution terminates, we extract the keys out of it into either the KeyProvider or a separate typescript object.
|
covered in #1208 |
Motivation
SnarkVM assumes proving/verifying keys persist in long-running processes, but SDK users typically run ephemeral processes where synthesized keys are discarded after each execution. Since key synthesis is the most expensive part of execution (30s+ for complex functions), this meant users were paying that cost repeatedly with no way to retrieve or reuse keys.
This PR adds
cacheKeys: truetoExecuteOptions, enabling:buildExecutionTransaction()andrun()return the synthesized proving/verifying keys alongside execution resultsKeyStore(e.g.,LocalFileKeyStorefrom [Feature] Add KeyStore interface for persistent Proving and Verifying Key storage #1207)See
docs/design/key-caching.mdfor the full design spec.What changed
program-manager.ts): AddedcacheKeysoption toExecuteOptions,ExecutionTransactionResultreturn type with overloadedbuildExecutionTransaction(), memory + KeyStore caching in bothbuildExecutionTransaction()andrun()keys/provider/memory.ts):AleoKeyProvidernow accepts an optionalKeyStorevia constructor,functionKeys()falls back to KeyStore when memory cache missesexecute.rs,response.rs): NewExecutionTransactionResponsetype andbuildExecutionTransactionWithKeysmethod that extracts proving/verifying keys from the process before it's droppedFollow-up
run()positional parameter fragility — should migrate to an options object (existing tech debt)Test plan
key-provider.test.ts: KeyStore constructor wiring,setKeyStore(), memory → KeyStore fallback chain, memory cache priority, error paths — all passingprogram-manager.test.ts:run()withcacheKeys: trueend-to-end — synthesizes keys via WASM, persists toLocalFileKeyStore, verifies keys on diskbuildExecutionTransactionwithcacheKeys: true— requires a funded account and network access, to be enabled when test infrastructure supports itHow to run