refactor: swap Goldilocks re-export to the Felt type unified for off-chain and on-chain code#819
refactor: swap Goldilocks re-export to the Felt type unified for off-chain and on-chain code#819
Goldilocks re-export to the Felt type unified for off-chain and on-chain code#819Conversation
greenhat
changed the title
Felt type unified for off-chain and on-chain codeGoldilocks re-export to the Felt type unified for off-chain and on-chain code
greenhat
force-pushed
the
greenhat-unified-felt
branch
from
f38f76c to
bff706b
Compare
make `Word` type to have the WIT-compatible shape
greenhat
force-pushed
the
greenhat-unified-felt
branch
from
bff706b to
ba5aa2b
Compare
greenhat
requested review from
Al-Kindi-0,
bitwalker,
bobbinth and
huitseeker
huitseeker
left a comment
huitseeker
left a comment
There was a problem hiding this comment.
Some concerns with unsafe conversions, and questions inline.
| /// | ||
| /// # Safety | ||
| /// This assumes `(Felt, Felt, Felt, Felt)` has the same memory layout as `[Felt; 4]`. | ||
| fn as_elements_array(&self) -> &[Felt; WORD_SIZE_FELT] { |
There was a problem hiding this comment.
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.
| @@ -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] { | |||
There was a problem hiding this comment.
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.
| @@ -0,0 +1,20 @@ | |||
| //! A unified `Felt` for Miden Rust code. | |||
There was a problem hiding this comment.
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.
| // 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. |
There was a problem hiding this comment.
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].
… `build.yml` To test building for the wasm32+miden (on-chain) target.
2 participants
Close #771
This PR swaps the
p3_goldilocks::Goldilocks as Feltre-export to the wrapperFelt(p3_goldilocks::Goldilocks)type for non-Miden targets. For the Miden targetFelt(f32)is compiled (see explanation in #771).Besides that
Wordtype is changed to have the WIT-compatible shape (structwith a named field).All
Feltmethods and implemented traits are delegating everything to the underlyingGoldilockstype and its trait implementations. So theFeltshould be 100% API equivalent to theGoldilockstype and can be used as a drop-in replacement. This allows us to release it as av0.22.xpatch version so that it can be included in VM v0.21 release. That's why I made this PR against themainbranch.The corresponding PR in the VM repo that updates VM to the version of
miden-cryptofrom this branch is 0xMiden/miden-vm#2649.The corresponding PR in the compiler repo migrating to the
miden-fieldcrate from this PR is 0xMiden/compiler#923TODO:
Wordshape to named fields (a, b, c, d);impl Arbitrary for Felt;