refactor: swap `Goldilocks` re-export to the `Felt` type unified for off-chain and on-chain code by greenhat · Pull Request #819 · 0xMiden/crypto

greenhat · 2026-02-06T14:58:34Z

Close #771

This PR swaps the p3_goldilocks::Goldilocks as Felt re-export to the wrapper Felt(p3_goldilocks::Goldilocks) type for non-Miden targets. For the Miden target Felt(f32) is compiled (see explanation in #771).
Besides that Word type is changed to have the WIT-compatible shape (struct with a named field).

All Felt methods and implemented traits are delegating everything to the underlying Goldilocks type and its trait implementations. So the Felt should be 100% API equivalent to the Goldilocks type and can be used as a drop-in replacement. This allows us to release it as a v0.22.x patch version so that it can be included in VM v0.21 release. That's why I made this PR against the main branch.

The corresponding PR in the VM repo that updates VM to the version of miden-crypto from this branch is 0xMiden/miden-vm#2649.

The corresponding PR in the compiler repo migrating to the miden-field crate from this PR is 0xMiden/compiler#923

TODO:

convert Word shape to named fields (a, b, c, d);
impl Arbitrary for Felt;
convert to proptest in miden-field/src/word/tests.rs;
rebase;
test in VM branch;

make `Word` type to have the WIT-compatible shape

huitseeker

Some concerns with unsafe conversions, and questions inline.

huitseeker · 2026-02-10T00:29:34Z

miden-field/src/word/mod.rs

+    ///
+    /// # Safety
+    /// This assumes `(Felt, Felt, Felt, Felt)` has the same memory layout as `[Felt; 4]`.
+    fn as_elements_array(&self) -> &[Felt; WORD_SIZE_FELT] {


The cast from &(Felt, Felt, Felt, Felt) to &[Felt; 4] indeed assumes identical memory layout, but Rust does not guarantee tuple layout. This could lead to UB if the compiler changes layout.

I'd suggest adding compile-time assertions like const_assert!(size_of::<(Felt, Felt, Felt, Felt)>() == size_of::<[Felt; 4]>()); at least (and a proptest) to verify the assumption of equivalence at build (/test) time.

huitseeker · 2026-02-10T00:29:40Z

miden-field/src/word/mod.rs

@@ -161,14 +208,13 @@ impl Word {
    where
        I: Iterator<Item = &'a Self>,
    {
-        words.flat_map(|d| d.0.iter())
+        words.flat_map(|d| d.as_elements().iter())
    }

    /// Returns all elements of multiple words as a slice.
    pub fn words_as_elements(words: &[Self]) -> &[Felt] {


words_as_elements casts &[Word] to &[Felt] via raw pointer. With #[repr(C, align(16))] on Word, this assumes no padding between elements and specific field ordering. Suggest adding const_assert! checks for size and alignment, or using the safe words_as_elements_iter instead where performance allows.

huitseeker · 2026-02-10T00:29:44Z

miden-field/src/lib.rs

@@ -0,0 +1,20 @@
+//! A unified `Felt` for Miden Rust code.


The new miden-field crate has no tests. Since this is cryptographic code used throughout the codebase, consider adding unit tests for both the native and WASM implementations to ensure correctness of field operations.

I'm not sure adding tests for miden target would be possible here since implementation of these methods would be delegated to compiler intrinsics. IIUC, this would require the whole compiler pipeline to be present. So, maybe a better places for these is in the compiler repo?

For native implementation, I think the main thing we'd be testing is that we didn't mix up delegated function calls. Maybe there is a way to add a couple of tests which would run all (most?) operations on some random elements and make sure we get the same result for Felt and native Goldilocks?

huitseeker · 2026-02-10T00:50:59Z

miden-field/src/word/mod.rs

+    // We cannot define this type as `Word([Felt;4])` since there is no struct tuple support
+    // and fixed array support is not complete in WIT. For the type remapping to work the
+    // bindings are expecting the remapped type to be the same shape as the one generated from
+    // WIT.


Would it not be possible to use:

#[repr(C)] pub struct Word { a: Felt, b: Felt, c: Felt, d: Felt, }

from a WIT PoV, or do you need the tuple?

Because that is layout-compatible with [Felt; 4].

…-field` crate

… `build.yml` To test building for the wasm32+miden (on-chain) target.

bobbinth

Looks good! Thank you! Not a full review yet, but I left some comments inline.

bobbinth · 2026-02-14T23:29:45Z

miden-field/Cargo.toml

+[dev-dependencies]
+rand                          = { default-features = false, version = "0.9" }
+rstest         = { version = "0.26" }


nit: formatting.

bobbinth · 2026-02-15T00:11:40Z

miden-field/Cargo.toml

+keywords.workspace     = true
+license.workspace      = true
+name                   = "miden-field"
+readme                 = "../README.md"


I think we should have a dedicated README file for this crate explaining (briefly) the motivation for it and how it works.

bobbinth · 2026-02-15T00:12:37Z

miden-field/Cargo.toml

+# dependendies for on-chain target
+[dependencies]
+thiserror         = { default-features = false, version = "2.0" }


Not sure if the comment here is correct - I think these dependencies also apply for off chain targets.

I would also move these above line 18.

bobbinth · 2026-02-15T00:15:20Z

miden-field/src/native.rs

+/// A `Felt` backed by Plonky3's Goldilocks field element.
+#[derive(Copy, Clone, Default, serde::Serialize, serde::Deserialize)]
+#[repr(transparent)]
+pub struct Felt(pub Goldilocks);


Does the inner value need to be pub?

bobbinth · 2026-02-15T00:40:44Z

miden-crypto/Cargo.toml

 miden-crypto-derive.workspace = true
+miden-field                   = { path = "../miden-field" }
 miden-serde-utils.workspace   = true


Could we not use miden-field.workspace = true here?

Also, since we now have more than a couple of workspace dependencies, could we move them into a separate section? Similar to how we do it here.

bobbinth · 2026-02-15T00:55:28Z

miden-field/src/word/tests.rs

+pub fn rand_u64() -> u64 {
+    let mut bytes = [0u8; 8];
+    let rng = &mut rand::rng();
+    rng.fill(&mut bytes[..]);
+    let bytes = bytes[..8].try_into().unwrap();
+    u64::from_le_bytes(bytes)
+}


Could this not be just:

pub fn rand_u64() -> u64 { rand::random() }

And if so, maybe we can just use rand::random() instead of this function directly?

bobbinth · 2026-02-15T00:56:47Z

miden-field/src/wasm_miden.rs

I did a very cursory review of this file.

bobbinth · 2026-02-15T01:01:34Z

miden-field/src/native.rs

+            use core::fmt::Write;
+
+            let mut msg = String::new();
+            write!(&mut msg, "value {value} is not a valid felt")
+                .expect("writing to string should not fail");
+            miden_serde_utils::DeserializationError::InvalidValue(msg)


Why not just use format! here?

DeserializationError::InvalidValue(format!( "value {value} is not a valid Goldilocks field element", ))

Also, now that we have these, we can probably get rid of the direct implementations on Goldilocks (in miden-serde-utils/src/lib.rs). Though, this would be a breaking change and should be done in a follow-up PR.

bobbinth · 2026-02-15T01:08:53Z

miden-field/src/native.rs

+    #[inline]
+    fn from_canonical_checked(int: i64) -> Option<Self> {
+        Goldilocks::from_canonical_checked(int).map(From::from)
+    }


Maybe not for this PR, but it would be nice to implement all these methods directly on Felt struct. This way, we won't have to import a bunch of traits every time we want to call one of such methods.

bobbinth · 2026-02-15T01:12:16Z

miden-field/src/word/mod.rs


-const WORD_SIZE_FELT: usize = 4;
-const WORD_SIZE_BYTES: usize = 32;
+pub const WORD_SIZE_FELT: usize = 4;


nit: I'd rename this to WORD_SIZE_FELTS to be consistent with WORD_SIZE_BYTES.

greenhat mentioned this pull request Feb 6, 2026

chore: update miden-crypto (with unified Felt type) 0xMiden/miden-vm#2649

Draft

greenhat changed the title ~~refactor: add Felt type unified for off-chain and on-chain code~~ refactor: swap Goldilocks re-export to the Felt type unified for off-chain and on-chain code Feb 6, 2026

greenhat force-pushed the greenhat-unified-felt branch from f38f76c to bff706b Compare February 9, 2026 07:59

refactor: add Felt type unified for off-chain and on-chain code,

ba5aa2b

make `Word` type to have the WIT-compatible shape

greenhat force-pushed the greenhat-unified-felt branch from bff706b to ba5aa2b Compare February 9, 2026 12:17

greenhat mentioned this pull request Feb 9, 2026

refactor: switch to the miden-field from miden-crypto repo 0xMiden/compiler#923

Draft

2 tasks

greenhat marked this pull request as ready for review February 9, 2026 12:29

greenhat requested review from Al-Kindi-0, bitwalker, bobbinth and huitseeker February 9, 2026 12:39

huitseeker reviewed Feb 10, 2026

View reviewed changes

greenhat marked this pull request as draft February 10, 2026 10:13

greenhat added 4 commits February 13, 2026 14:51

refactor: move Word and derived types from miden-crypto to `miden…

1519870

…-field` crate

chore: add make build_target_miden task and targe-miden CI job in…

2340896

… `build.yml` To test building for the wasm32+miden (on-chain) target.

fix: bring wasm+miden Felt API on par with native

d852c32

fix: build miden-field for wasm+miden target

06d007b

bobbinth reviewed Feb 15, 2026

View reviewed changes

Conversation

greenhat commented Feb 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

huitseeker left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bobbinth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

greenhat commented Feb 6, 2026 •

edited

Loading