Skip to content

Conversation

@martyall
Copy link
Contributor

@martyall martyall commented Apr 24, 2024

NOTE: This PR looks worse than it is -- the diff misrepresents the changes.

Changes

  • factor out createWASMInstance function to build the WASM instance from source code
  • change witness_calculator.builder to check if being passed wasm binary or existing wasm instance

The changes are non-breaking. In fact the code path is exactly the same if you pass in WASM source code to the constructor.

Other Context

I'm working on a project which produces a WASM binary that is compatible with the circom witness calculating binary. The standards for serializing witness files, r1cs, keys etc was documented, and it was easy enough to reverse engineer the necessary API for the witness solver.

However, my WASM binary is compiled via Haskell's GHC WASM backend which requires the wasi_snapshot_preview_1 apis to be included in the instance. This PR allows me to do that with minimal (non-breaking) changes. Here is an example project which does this:

https://github.com/l-adic/factors/blob/main/app/Prover/Prove.js#L62

@martyall martyall force-pushed the add-wasi-import-options branch from e6c6aec to bbfe3ee Compare May 23, 2024 04:20
@martyall martyall force-pushed the add-wasi-import-options branch 3 times, most recently from ce533cf to 968aaa5 Compare July 1, 2024 16:07
@martyall martyall force-pushed the add-wasi-import-options branch from 968aaa5 to 446d42e Compare July 1, 2024 17:06
@martyall martyall changed the title Add additionalWASMImports field to options object Allow for passing/reusing existing WASM instance Jul 1, 2024
@OBrezhniev OBrezhniev self-requested a review July 25, 2024 21:56
Copy link
Member

@OBrezhniev OBrezhniev left a comment

Choose a reason for hiding this comment

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

Snarkjs tests fail with these changes - witness calc is not working. See comments for changes needed to fix this.

instance = await createWASMInstance(codeOrWasm, options);
}

if (typeof instance.exports.getVersion == 'function') {
Copy link
Member

Choose a reason for hiding this comment

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

These checks should be inside createWASMInstance and version object returned together with the instance (new wrapper object for wasm instance + version is needed)

Copy link
Member

Choose a reason for hiding this comment

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

Also variable wc is not declared in this function, but is assigned later in the code AND is used inside createWASMInstance. So maybe it would be easier to move all the rest of the code including instantiation of WitnessCalculatorCircom2/1 to createWASMInstance too (and rename the function to createWitnessCalculatorInstance)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you're totally right, I must have been really sleep deprived when I reworked this 🤦‍♂️

really the only change that's required here is

export default async function builder(code, options) {
    let instance;
    let wc;

    // Only circom 2 implements version lookup through exports in the WASM
    // We default to `1` and update if we see the `getVersion` export (major version)
    // These are updated after the instance is instantiated, assuming the functions are available
    let majorVersion = 1;
    // After Circom 2.0.7, Blaine added exported functions for getting minor and patch versions
    let minorVersion = 0;
    // If we can't lookup the patch version, assume the lowest
    let patchVersion = 0;

    if (code instanceof WebAssembly.Instance) {
        instance = code;
    } else {
        instance = //same definition as before
    }
    
    //same as before

The new diff looks bad because of new indentation, but this is now the only change

@martyall martyall force-pushed the add-wasi-import-options branch 4 times, most recently from b64ee51 to 24224e0 Compare August 2, 2024 15:54
@martyall martyall force-pushed the add-wasi-import-options branch from 24224e0 to d359044 Compare August 2, 2024 16:59
@martyall martyall requested a review from OBrezhniev August 2, 2024 17:06
@OBrezhniev OBrezhniev merged commit 9f670eb into iden3:master Aug 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants