Skip to content

Conversation

@dannywillems
Copy link
Member

@dannywillems dannywillems commented Apr 8, 2025

Counterpart in Mina: MinaProtocol/mina#16887
Counterpart in o1js: o1-labs/o1js#2128

Reverting some commits introduced in #3149. The PR!3149 seems to contain gentle changes, as it is mostly to appease Clippy.

However, if someone tries to run o1js on top of 3f63052, different errors will happen.

  1. One is described in this comment, which seems to be related to 0154940.
    This PR reverts this commit, after that revert the revert (confirming that there is an actual issue with the change), and again revert the set. I'm keeping this in the history in case we want to debug later.

  2. Another one is having tests hanging indefinitely, without logs.

I initially reverted (a bit arbitraly, but still on the idea that the code semantics might be changed by wasm-bindgen) the following commits:

In addition to that 1c8e9b6, see 78c75a8 is reverted as it impacts this CI.

I ended up re-introducing 00ab947, see 98e9d70 as it really looks inoffensive to me.

I would prefer to start from this set of reverts, and step by step reintroducing them.
Follow-up PR will come.

The reviewer is encouraged to monitor the state of the CI of o1js here, and review the Mina + proof-systems commit used, to convince them that this gives back a clean master branch we can start to build on top of again.

The reviewer can convince themselves there are some issues by looking at logs of the CI of this o1js commit for instance. The four commits on top of it are after these changes.

@dannywillems dannywillems marked this pull request as draft April 8, 2025 11:03
@dannywillems dannywillems marked this pull request as ready for review April 8, 2025 11:16
@dannywillems
Copy link
Member Author

When running npm run test:unit-test when the bindings are generated with the commit: 4ed6523, I'm getting:

bigint unit tests are passing! 🎉
Running ./dist/node/bindings/crypto/bindings/bindings.unit-test.js
Running ./dist/node/bindings/crypto/elliptic-curve.unit-test.js
Running ./dist/node/bindings/crypto/finite-field.unit-test.js
Running ./dist/node/bindings/crypto/glv.unit-test.js
Running ./dist/node/bindings/crypto/poseidon.unit-test.js
poseidon implementation matches the test vectors! 🎉
poseidon hashToGroup implementations match! 🎉
Running ./dist/node/bindings/lib/binable.unit-test.js
Running ./dist/node/lib/mina/v1/account-update-layout.unit-test.js

/workspace_root/src/bindings/ocaml/jsoo_exports/overrides.js:45
    if (err instanceof Error) throw err;
                              ^

Error [RuntimeError]: unreachable
    at plonk_wasm.wasm.rust_panic (wasm://wasm/plonk_wasm.wasm-012b09da:wasm-function[3869]:0x446066)
    at plonk_wasm.wasm.std::panicking::rust_panic_with_hook::h8f3a9206f57ea016 (wasm://wasm/plonk_wasm.wasm-012b09da:wasm-function[2877]:0x424815)
    at plonk_wasm.wasm.std::panicking::begin_panic_handler::{{closure}}::h9cb6abe735a11679 (wasm://wasm/plonk_wasm.wasm-012b09da:wasm-function[3332]:0x43cbb7)
    at plonk_wasm.wasm.std::sys_common::backtrace::__rust_end_short_backtrace::hcd9edd2e461819c9 (wasm://wasm/plonk_wasm.wasm-012b09da:wasm-function[3836]:0x445f8c)
    at plonk_wasm.wasm.rust_begin_unwind (wasm://wasm/plonk_wasm.wasm-012b09da:wasm-function[3550]:0x4435fe)
    at plonk_wasm.wasm.core::panicking::panic_fmt::hda26c7a0d40dd0ac (wasm://wasm/plonk_wasm.wasm-012b09da:wasm-function[3562]:0x443a09)
    at plonk_wasm.wasm.core::panicking::panic_bounds_check::hfe3c396aaac71eee (wasm://wasm/plonk_wasm.wasm-012b09da:wasm-function[3492]:0x442016)
    at plonk_wasm.wasm.rayon_core::registry::ThreadBuilder::run::hc603ea57215f73e2 (wasm://wasm/plonk_wasm.wasm-012b09da:wasm-function[2030]:0x3cfddf)
    at plonk_wasm.wasm.wbg_rayon_start_worker (wasm://wasm/plonk_wasm.wasm-012b09da:wasm-function[1921]:0x3befbf)
    at module.exports.wbg_rayon_start_worker (/home/soc/codes/o1-labs/o1js/dist/node/bindings/compiled/_node_bindings/plonk_wasm.cjs:2741:10)
    at <anonymous> (/home/soc/codes/o1-labs/o1js/src/bindings/js/node/node-backend.js:25:8)
    at ModuleJob.run (node:internal/modules/esm/module_job:218:25)
    at async ModuleLoader.import (node:internal/modules/esm/loader:329:24)
    at async loadESM (node:internal/process/esm_loader:28:7)
    at async handleMainPromise (node:internal/modules/run_main:113:12)
Emitted 'error' event on Worker instance at:
    at [kOnErrorMessage] (node:internal/worker:326:10)
    at [kOnMessage] (node:internal/worker:337:37)
    at MessagePort.<anonymous> (node:internal/worker:232:57)
    at [nodejs.internal.kHybridDispatch] (node:internal/event_target:826:20)
    at exports.emitMessage (node:internal/per_context/messageport:23:28)

Node.js v20.11.1

Gonna leave it on hold, for now.

@dannywillems
Copy link
Member Author

dannywillems commented Apr 9, 2025

pub fn caml_pasta_fp_plonk_index_serialize(index: &WasmPastaFpPlonkIndex) -> String {
let serialized = rmp_serde::to_vec(&index.0).unwrap();
general_purpose::STANDARD.encode(serialized)
base64::encode(serialized)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: it would be better to rebase the branch and remove this "revert revert revert" change together with "revert revert" change earlier in the same PR to avoid confusion...

this looks quite innocent to me btw but we can keep it in the old base64 way for now for clarity.

Copy link
Member Author

Choose a reason for hiding this comment

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

this looks quite innocent to me btw but we can keep it in the old base64 way for now for clarity

Spoiler: it is not innocent.

@dannywillems dannywillems merged commit c3edee7 into master Apr 9, 2025
8 checks passed
@dannywillems dannywillems deleted the dw/tmp-bump-up-again branch April 9, 2025 13:20
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.

3 participants