[develop] Bindings - add runtime table cfg to prover index creation #79

dannywillems · 2023-07-10T18:41:25Z

See MinaProtocol/mina#13549 for more explanation.
Related to o1-labs/o1js#1020.

Built on top of #78
Also did:

git merge origin/fix-snarkyjs-berkeley
git checkout HEAD compiled MINA_COMMIT
npm run bindings

The result of npm run bindings has been commited in the latest commit.

…ver-index-creation

kimchi/js/bindings.js

dannywillems · 2023-08-02T14:52:13Z

EDIT: it seems happening because of other changes, removed later in this branch. I kept track of it in dannywillems/add-runtime-table-cfg-to-prover-index-creation-backup.

With the latest changes, I have runtime errors with null pointers passed to rust. Trying to fix it. It seems to be related to the finalizer. Digging into it. As we do allocations for the runtime table cfgs, it might come from there.
I have opened multiple other PR which are merged in this branch as it is overlapping with the changes here and good to have here.

See snarkyjs CI.
For instance:

/workspace_root/src/lib/snarkyjs/src/bindings/ocaml/overrides.js:34
    if (err instanceof Error) throw err;
                              ^


Error: null pointer passed to rust
    at module.exports.__wbindgen_throw (/home/runner/work/snarkyjs/snarkyjs/dist/node/bindings/compiled/_node_bindings/plonk_wasm.cjs:8113:11)
    at wasm_bindgen::throw_str::habe968293e902f28 (wasm://wasm/010e0fd6:wasm-function[4697]:0x3bda89)
    at wasm_bindgen::__rt::throw_null::h1d564009f7b78f7c (wasm://wasm/010e0fd6:wasm-function[4698]:0x3bda96)
    at __wbg_wasmfpgatevector_free (wasm://wasm/010e0fd6:wasm-function[3798]:0x3ac3b7)
    at WasmFpGateVector.free (/home/runner/work/snarkyjs/snarkyjs/dist/node/bindings/compiled/_node_bindings/plonk_wasm.cjs:3497:14)
    at joo_global_object (/workspace_root/src/lib/snarkyjs/src/bindings/kimchi/js/bindings.js:588:5)
    at FinalizationRegistry.cleanupSome (<anonymous>)

or

 /workspace_root/src/lib/snarkyjs/src/bindings/ocaml/overrides.js:34
    if (err instanceof Error) throw err;
                              ^


Error: null pointer passed to rust
    at module.exports.__wbindgen_throw (/home/runner/work/snarkyjs/snarkyjs/dist/node/bindings/compiled/_node_bindings/plonk_wasm.cjs:8113:11)
    at wasm_bindgen::throw_str::habe968293e902f28 (wasm://wasm/010e0fd6:wasm-function[4697]:0x3bda89)
    at wasm_bindgen::__rt::throw_null::h1d564009f7b78f7c (wasm://wasm/010e0fd6:wasm-function[4698]:0x3bda96)
    at __wbg_wasmpallasgprojective_free (wasm://wasm/010e0fd6:wasm-function[4465]:0x3bc654)
    at WasmVestaGProjective.free (/home/runner/work/snarkyjs/snarkyjs/dist/node/bindings/compiled/_node_bindings/plonk_wasm.cjs:7795:14)
    at joo_global_object (/workspace_root/src/lib/snarkyjs/src/bindings/kimchi/js/bindings.js:588:5)
    at FinalizationRegistry.cleanupSome (<anonymous>)

mrmr1993

This seems to include unrelated changes (deleting tests etc.). Please revert

kimchi/js/bindings.js

kimchi/wasm/src/plonk_proof.rs

…d runtime table cfg

It also makes the code clearer.

dannywillems · 2023-08-02T17:39:05Z

This seems to include unrelated changes (deleting tests etc.). Please revert

Done

dannywillems · 2023-08-02T18:59:57Z

#104 (and the counterpart #105) should be merged first to solve the conflict.

mrmr1993 · 2023-08-02T20:47:02Z

kimchi/js/bindings.js

+var caml_create_rust_empty_vector = function () {
+  // If this is correct, it should be equivalent than calling
+  // js_class_vector_to_rust_vector with an empty js array
+  return new joo_global_object.Uint32Array(0);


With this comment, IIUC, you want me to remove these two functions? It is equivalent to caml_array_to_rust_vector with the JS array [0] (i.e. the empty caml array).

It was the initial version but I thought introducing helpers would be nice. Removed here.

mrmr1993 · 2023-08-02T20:47:08Z

kimchi/js/bindings.js

+  // ...] is
+  // represented as a JavaScript array [0, v1, v2, ...] where [0] is the runtime
+  // tag
+  return caml_array.length === 1;


See #79 (comment)

mrmr1993 · 2023-08-02T20:50:54Z

kimchi/js/bindings.js

@@ -1219,6 +1235,46 @@ var caml_fq_plonk_gate_to_rust = function (gate) {
  );
 };

+// Provides: caml_fp_lookup_table_to_rust
+// Requires: plonk_wasm, caml_fp_vector_of_rust
+var caml_fp_lookup_table_to_rust = function (caml_lookup_table, mk_class) {


Unused, delete

Done in 96ba0ae. However, it was there because this PR is built on top of others, which introduce it. Will add it back later.

mrmr1993 · 2023-08-02T20:51:02Z

kimchi/js/bindings.js

+
+// Provides: caml_fq_lookup_table_to_rust
+// Requires: plonk_wasm, caml_fq_vector_to_rust
+var caml_fq_lookup_table_to_rust = function (caml_lookup_table, mk_class) {


Unused, delete

See #79 (comment)

mrmr1993 · 2023-08-02T20:54:30Z

kimchi/js/bindings.js

@@ -2129,6 +2251,22 @@ var caml_pasta_fp_proof_of_rust = function (x) {
  return [0, messages, proof, evals, ft_eval1, public_, prev_challenges];
 };

+// Provides: caml_fp_runtime_table_to_rust
+// Requires: plonk_wasm, caml_fp_vector_to_rust
+var caml_fp_runtime_table_to_rust = function (caml_runtime_table, mk_class) {


Unused, delete

As for lookup tables, removed in 6070f67

mrmr1993 · 2023-08-02T20:54:37Z

kimchi/js/bindings.js

@@ -2367,6 +2505,22 @@ var caml_pasta_fq_proof_of_rust = function (x) {
  return [0, messages, proof, evals, ft_eval1, public_, prev_challenges];
 };

+// Provides: caml_fq_runtime_table_to_rust
+// Requires: plonk_wasm, caml_fq_vector_to_rust
+var caml_fq_runtime_table_to_rust = function (caml_runtime_table, mk_class) {


Unused, delete

See #79 (comment)

mrmr1993 · 2023-08-02T21:22:35Z

kimchi/js/bindings.js

@@ -46,6 +46,22 @@ var caml_bigint_256_of_decimal_string = function (s) {
  );
 };

+// Provides: caml_create_rust_empty_vector
+var caml_create_rust_empty_vector = function () {


This name is misleading.

See #79 (comment)

Will be reintroduced later

It is equivalent to caml_array_to_rust_vector with an empty array

dannywillems requested a review from a team as a code owner July 10, 2023 18:41

dannywillems mentioned this pull request Jul 10, 2023

[develop] Bindings - add runtime table cfg to prover index creation o1-labs/o1js#1020

Merged

dannywillems force-pushed the dannywillems/add-runtime-table-cfg-to-prover-index-creation branch from 33be4ed to 7c170df Compare July 11, 2023 08:13

This was referenced Jul 11, 2023

Add lookup table to prover index creation #82

Merged

Expose array lookups to SnarkyJS MinaProtocol/mina#13086

Closed

dannywillems force-pushed the dannywillems/converters-runtime-table-cfg-js branch from 8ca90f8 to f6bbee5 Compare July 11, 2023 15:19

dannywillems force-pushed the dannywillems/add-runtime-table-cfg-to-prover-index-creation branch 2 times, most recently from 29df59b to 586c3bb Compare July 11, 2023 16:19

dannywillems added 9 commits July 13, 2023 13:58

wasm/src/plonk_proof.rs: define wasm runtime tables

af7aac3

kimchi/js/bindings.js: add runtime_tables to proof_create

a257145

bindings.js: remove duplicated definition

c3613c0

kimchi/js/bindings.js: define js/wasm converters for lookup tables

6880ffe

bindings_js_test: add empty runtime table cfg arg

b0b6b24

wasm/index: add runtime table cfgs arg

e9c0cca

bindings.js: add runtime_table_cfgs arg

79bdbd9

kimchi/js/bindings.js: define converters for runtime table cfg

4697a14

Run npm run bindings

aaa0d26

dannywillems force-pushed the dannywillems/add-runtime-table-cfg-to-prover-index-creation branch from 586c3bb to aaa0d26 Compare July 13, 2023 12:46

mitschabaude mentioned this pull request Jul 26, 2023

[develop] Define wasm runtime table #74

Closed

Merge branch 'develop' into dannywillems/add-runtime-table-cfg-to-pro…

7a6cceb

…ver-index-creation

dannywillems changed the base branch from dannywillems/converters-runtime-table-cfg-js to develop July 27, 2023 09:58

dannywillems changed the base branch from develop to dannywillems/converters-runtime-table-cfg-js July 27, 2023 09:59

dannywillems changed the base branch from dannywillems/converters-runtime-table-cfg-js to develop July 27, 2023 10:05

dannywillems mentioned this pull request Jul 27, 2023

[develop] Bindings - add runtime table cfg to prover index creation MinaProtocol/mina#13549

Merged

8 tasks

dannywillems force-pushed the dannywillems/add-runtime-table-cfg-to-prover-index-creation branch from 3dfadad to e8e4d40 Compare July 27, 2023 14:49

dannywillems changed the title ~~Add runtime table cfg to prover index creation~~ [develop] Bindings - add runtime table cfg to prover index creation Jul 27, 2023

mrmr1993 requested changes Jul 27, 2023

View reviewed changes

kimchi/js/bindings.js Outdated Show resolved Hide resolved

dannywillems force-pushed the dannywillems/add-runtime-table-cfg-to-prover-index-creation branch from 4087d49 to 3b8142a Compare August 1, 2023 16:30

mrmr1993 requested changes Aug 2, 2023

View reviewed changes

dannywillems force-pushed the dannywillems/add-runtime-table-cfg-to-prover-index-creation branch from f43e72b to 8462906 Compare August 2, 2023 16:36

mrmr1993 reviewed Aug 2, 2023

View reviewed changes

kimchi/js/bindings.js Outdated Show resolved Hide resolved

kimchi/wasm/src/plonk_proof.rs Outdated Show resolved Hide resolved

dannywillems force-pushed the dannywillems/add-runtime-table-cfg-to-prover-index-creation branch from 8462906 to 9546526 Compare August 2, 2023 16:52

mitschabaude and others added 2 commits August 2, 2023 19:04

update worker spec

88e7090

kimchi/js/bindings.js: fix runtime table cfg field access

bf2a3f7

dannywillems force-pushed the dannywillems/add-runtime-table-cfg-to-prover-index-creation branch from 9546526 to c5871c6 Compare August 2, 2023 17:22

dannywillems added 3 commits August 2, 2023 19:30

kimchi/js/bindings.js: change converters for runtime, lookup table an…

b255ba8

…d runtime table cfg

kimchi/js/bindings.js: unify field access of caml record

927dc1d

It also makes the code clearer.

Run npm run bindings

410a97b

dannywillems force-pushed the dannywillems/add-runtime-table-cfg-to-prover-index-creation branch from c5871c6 to 410a97b Compare August 2, 2023 17:38

mrmr1993 requested changes Aug 2, 2023

View reviewed changes

dannywillems added 3 commits August 2, 2023 23:43

Remove lookup tables converters

96ba0ae

Will be reintroduced later

Remove runtime table converters

6070f67

Will be reintroduced later

Remove caml_is_empty_array_caml_array and caml_create_rust_empty_vector

1d75029

It is equivalent to caml_array_to_rust_vector with an empty array

mrmr1993 approved these changes Aug 3, 2023

View reviewed changes

dannywillems merged commit b84776e into develop Aug 3, 2023
1 check passed

dannywillems deleted the dannywillems/add-runtime-table-cfg-to-prover-index-creation branch August 3, 2023 07:08

This was referenced Aug 3, 2023

Define converters js wasm runtime table #75

Closed

bindings.js: remove duplicated definition #76

Closed

Define converters runtime table cfg js #77

Closed

Converters lookup table js #78

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[develop] Bindings - add runtime table cfg to prover index creation #79

[develop] Bindings - add runtime table cfg to prover index creation #79

dannywillems commented Jul 10, 2023 •

edited

Loading

dannywillems commented Aug 2, 2023 •

edited

Loading

mrmr1993 left a comment

dannywillems commented Aug 2, 2023

dannywillems commented Aug 2, 2023

mrmr1993 Aug 2, 2023

dannywillems Aug 2, 2023

mrmr1993 Aug 2, 2023

dannywillems Aug 2, 2023

mrmr1993 Aug 2, 2023

dannywillems Aug 2, 2023 •

edited

Loading

mrmr1993 Aug 2, 2023

dannywillems Aug 2, 2023

mrmr1993 Aug 2, 2023

dannywillems Aug 2, 2023

mrmr1993 Aug 2, 2023

dannywillems Aug 2, 2023

mrmr1993 Aug 2, 2023

dannywillems Aug 2, 2023

[develop] Bindings - add runtime table cfg to prover index creation #79

[develop] Bindings - add runtime table cfg to prover index creation #79

Conversation

dannywillems commented Jul 10, 2023 • edited Loading

dannywillems commented Aug 2, 2023 • edited Loading

mrmr1993 left a comment

Choose a reason for hiding this comment

dannywillems commented Aug 2, 2023

dannywillems commented Aug 2, 2023

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

dannywillems Aug 2, 2023 • 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

Choose a reason for hiding this comment

dannywillems commented Jul 10, 2023 •

edited

Loading

dannywillems commented Aug 2, 2023 •

edited

Loading

dannywillems Aug 2, 2023 •

edited

Loading