Rust native with `neon-rs` #3217

richardpringle · 2025-05-07T19:11:39Z

plonk-wasm: factor out future common crates
plonk-neon: add new crate for nodejs

richardpringle · 2025-05-07T20:11:43Z

I don't quite understand why the benchmarking failed... it passed here:
#3218 (I checked out the exact same code but didn't include plonk-neon). In any case, it looks like it's only kimchi that's being benchmarked, so none of my changes should have effected the benchmark.

I guess I'll try re-running it?

crates/arkworks/src/bigint_256.rs

richardpringle · 2025-05-07T21:06:43Z

crates/arkworks/src/bigint_256.rs

I can back out the changes here if you (dear reviewer) would like, and put them in a separate PR. This is just a cargo clippy fix with clippy::pedantic

crates/arkworks/src/group_affine.rs

crates/arkworks/src/group_projective.rs

richardpringle · 2025-05-07T21:08:08Z

crates/arkworks/src/lib.rs

+pub use group_affine::{WasmGPallas, WasmGVesta};
+pub use group_projective::{WasmPallasGProjective, WasmVestaGProjective};
+pub use pasta_fp::WasmPastaFp;
+pub use pasta_fq::WasmPastaFq;


I'm being a lot more explicit about exports here.

plonk-neon/Cargo.toml

dannywillems · 2025-05-08T08:37:12Z

crates/arkworks/src/bigint_256.rs

@@ -103,12 +106,16 @@ pub fn caml_bigint_256_compare(x: WasmBigInteger256, y: WasmBigInteger256) -> i8
    }
 }

-#[wasm_bindgen]
+// I don't think this is used anywhere


It is required in pasta_bindings.ml on the Mina side, see here. When we compile Mina, we need to have this stub. The print statement might be used in the Caml code in different places.

Ha, your comment made me prove that it's not used, so thank you for that!

In your comment, you're pointing to the ocaml bindings that either use some type of Rust dynamic library or the js here:
https://github.com/MinaProtocol/mina/blob/c2651d8b9711c0bb1cafac9468eff9e5df006045/src/lib/crypto/kimchi_bindings/js/bindings/bigint256.js#L30

plonk-wasm is only used for the js version. The key detail is that the js code is referring to the following:
https://github.com/o1-labs/o1js/blob/4c60cda83949262db1449bc1103b7b46f9d6980a/src/bindings/crypto/bindings/bigint256.ts#L41-L43

In other words, we can probably remove all the public functions in this file. I'm not going to do that now, though.

Furthermore, I don't think this can ever work when targeting wasm32-unknown-unknown because there's no stdout to write to. I'm pretty sure this code will panic no matter what.

plonk-neon/Cargo.toml

crates/arkworks/src/group_affine.rs

Cargo.toml

dannywillems · 2025-05-08T08:43:46Z

crates/arkworks/src/bigint_256.rs

-    let mut serialized_bytes = vec![];
+    // This is the size of the implementation of BigInteger256 at the time of writing
+    // but even if it changes, we will be no worse off than the original code `vec![]`
+    let mut serialized_bytes = Vec::with_capacity(core::mem::size_of::<WasmBigInteger256>());


Should we not use BIGINT256_NUM_BYTES?

In short, no.

The foward-compatible way of checking the capacity is really to use CanonicalSerialize::serialized_size(&x.0), but that iterates over all the u8s, calling core::mem::size_of::<u8>() over and over.

I think it's safe to assume that for all BigInt<const N: usize>, the serialized size will always be core::mem::size_of::<BigInt>().

There's nothing programmatically connecting any of those constants to the actual serialization implementations, I think it's safer to use the knowledge of the implementation.

Thoughts?

I also was going to just hard-code 32 there with a comment saying that even if the size changes, 32 is not a very large allocation. In the case where the actual size needed is smaller (don't ask me why this would ever be the case), we wouldn't be wasting that much space. In the case where it's larger, we are no worse off than vec![] which is the equivalent of Vec::with_capacity(0).

WasmBigInteger256 could be implemented in such way that the size_of won't give 32 bytes, even though the underlying value is a field element that is 32 bytes. It could be implemented as a pointer, and therefore returns only 8 bytes (or 4 bytes in this case as the WebAssembly runtime is on 32 bits). Or it could have additional value in the structure that makes it bigger than 32 bytes. However, the underlying value requires to be encoded in 32 bytes.

It is true that it is valid, and it won't create any encoding error.

I was mostly making a comment, and was not requesting any change. It is already in the existing codebase.

crates/arkworks/src/bigint_256.rs

plonk-neon/Cargo.toml

dannywillems · 2025-05-08T08:52:57Z

crates/arkworks/src/pasta_fq.rs

+impl wasm_types::FlatVectorElem for WasmPastaFq {
+    const FLATTENED_SIZE: usize = core::mem::size_of::<Fq>();
+    fn flatten(self) -> Vec<u8> {
+        let mut bytes: Vec<u8> = Vec::with_capacity(Self::FLATTENED_SIZE);


Nit/comment: I know it is the original codebase, but it is a bit surprising that it does use the size of the structure on Fp, where it should be unrelated IMO. An encoding is different from its representation, in particular in cryptography where we have different encoding in bytes for the same object (like curve points).

🤔, you don't think the structure and the serialized representation are related? Both Fp and Fq are just BigInt<const N: usize>s with constraints on N. That in turn is just an array of bytes. So they do seem somewhat coupled to me.

But now that I'm thinking about this... why aren't we just using the CanonicalSerialize implementation instead of rolling our own thing that just delegates to CanonicalSerialize? Kinda weird...

Same comment than here. It happened in some codebase that the field element was a pointer, and not the direct value, and therefore was giving a wrong size.

No requested changes. Mostly a comment.

dannywillems · 2025-05-08T08:54:27Z

crates/arkworks/src/lib.rs

@@ -0,0 +1,13 @@
+#![cfg_attr(not(feature = "std"), no_std)]


I would be in favor of adding a top-level description of what the crate is supposed to contain to guide the next people coming into the repository. In addition to that, we are continuously publishing the documentation and it would be useful for everyone to understand the monorepo structure.

So the documentation that was there before didn't describe what arkworks is at all, it just demonstrated that you can convert some OCaml type things into the BigInt types. The example doesn't run with arkworks in its own crate because other types are foreign types.

So I don't think the documentation that was there before was very informative or helpful.

With that said, I'm happy add documentation here. What would you like it to say, "wasm-compatible wrappers around arkworks types and functions"?

IMO, there's no point in duplicating any of the descriptions or documentation of the underlying libraries.

dannywillems · 2025-05-08T08:55:00Z

crates/wasm-types/src/lib.rs

@@ -0,0 +1,5 @@
+#![no_std]


Same comment than here.

Would ///! Shared wasm types suffice? Seems premature to put a description here if I'm being honest.

This crate should effectively be private. It makes sense for the public types to be documented, but there's not much to say about the purpose of this crate other than to put things in here that are shared between other, more public facing crates from the mono-repo.

It might be more helpful for me to rename the crate to shared-js-types or something.

What do you think?

plonk-neon/src/lib.rs

dannywillems · 2025-05-08T08:57:13Z

plonk-neon/src/poseidon.rs

@@ -0,0 +1,57 @@
+use arkworks::{WasmPastaFp, WasmPastaFq};


Can you add a top-level documentation as it has been done for plonk-wasm/src/poseidon.rs, please? It was confusing for some people why we had these primitives.

Is it alrigth if I just put

//! This file defines wrappers for the Poseidon hash functions that are used in //! the [Mina](https://github.com/MinaProtocol/mina) codebase.

All the extra stuff

//! It is a wrapper around the Poseidon implementation in the `mina_poseidon` crate. //! It is required as the native OCaml implementation of Mina does use the Rust //! implementation defined in the crate `mina_poseidon` instead of defining its //! own natively in OCaml for performance reasons. The bindings in OCaml can be //! found in `src/lib/crypto/kimchi_bindings/pasta_fp_poseidon` and //! `src/lib/crypto/kimchi_bindings/pasta_fq_poseidon` in the Mina codebase.

is redundant and impossible to programmatically assure that the code locations stay up to date. We've already seen in this PR that documentation doesn't get kept up to date when referring to other code.

If we do want references to code in mina, they should at the very least be github permalinks.

If we do want references to code in mina, they should at the very least be github permalinks.

Agreed.

Is it alrigth if I just put

//! This file defines wrappers for the Poseidon hash functions that are used in //! the [Mina](https://github.com/MinaProtocol/mina) codebase.

All the extra stuff

//! It is a wrapper around the Poseidon implementation in the `mina_poseidon` crate. //! It is required as the native OCaml implementation of Mina does use the Rust //! implementation defined in the crate `mina_poseidon` instead of defining its //! own natively in OCaml for performance reasons. The bindings in OCaml can be //! found in `src/lib/crypto/kimchi_bindings/pasta_fp_poseidon` and //! `src/lib/crypto/kimchi_bindings/pasta_fq_poseidon` in the Mina codebase.

is redundant and impossible to programmatically assure that the code locations stay up to date. We've already seen in this PR that documentation doesn't get kept up to date when referring to other code.

Yes sure.

richardpringle · 2025-05-08T19:57:20Z

@dannywillems, do you mind if we defer the esthetic changes until after this PR lands? I left a couple of replies asking for your thoughts on semantic changes. Since this PR is quite large, I would prefer to fix all semantic issues, then do a cleanup pass immediately following this.

dannywillems · 2025-05-08T21:06:34Z

@dannywillems, do you mind if we defer the esthetic changes until after this PR lands? I left a couple of replies asking for your thoughts on semantic changes. Since this PR is quite large, I would prefer to fix all semantic issues, then do a cleanup pass immediately following this.

Not at all. I like doing it sequentially.

dannywillems · 2025-05-14T18:58:27Z

It would be nice to also have a way to add a target in the Makefile to build the project and add a job in the CI.

richardpringle · 2025-05-15T16:34:27Z

It would be nice to also have a way to add a target in the Makefile to build the project and add a job in the CI.

100%... I will capture that in an issue. I just wanted to get this code in to make sure that it and plonk-wasm don't deviate

dannywillems

Approving again after master has been merged.

dannywillems · 2025-05-18T22:21:52Z

Rebased and hope it will get merged this time.

richardpringle force-pushed the rust-native-with-neon branch from 63e8d41 to 9d53572 Compare May 7, 2025 19:12

richardpringle force-pushed the rust-native-with-neon branch from 9d53572 to 44fd398 Compare May 7, 2025 20:54

richardpringle requested review from dannywillems and querolita May 7, 2025 21:04

richardpringle marked this pull request as ready for review May 7, 2025 21:04

richardpringle commented May 7, 2025

View reviewed changes

crates/arkworks/src/bigint_256.rs Outdated Show resolved Hide resolved

richardpringle commented May 7, 2025

View reviewed changes

crates/arkworks/src/group_affine.rs Outdated Show resolved Hide resolved

richardpringle commented May 7, 2025

View reviewed changes

crates/arkworks/src/group_projective.rs Outdated Show resolved Hide resolved

richardpringle commented May 7, 2025

View reviewed changes

richardpringle force-pushed the rust-native-with-neon branch from 44fd398 to 0ff4a4f Compare May 7, 2025 21:13

dannywillems reviewed May 7, 2025

View reviewed changes

plonk-neon/Cargo.toml Show resolved Hide resolved

richardpringle force-pushed the rust-native-with-neon branch 5 times, most recently from 58a974c to 9d2c241 Compare May 7, 2025 22:08