Update miden-base to Plonky3 (and new miden-crypto, miden-vm) #2213
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
huitseeker
force-pushed
the
huitseeker/update-miden-deps-p3
branch
from
42ad852 to
8768be2
Compare
huitseeker
force-pushed
the
huitseeker/update-miden-deps-p3
branch
6 times, most recently
from
57263c3 to
71fa040
Compare
huitseeker
changed the title
Collaborator
|
Some initial benchmarks (on M4 Pro) (here and here) give the following summarized data The more granular breakdown is as follows The key observation seems to be that P3's commitment times are better than those of Winterfell (except for the hashing times of Blake3), while Winterfell's FRI implementation is more performant the P3's. Since for arithmetic hash functions, commitment costs are the dominant, P3 is slightly faster than Winterfell. |
…stency The P3 migration introduced a critical bug where Word's Ord implementation was inconsistent with its PartialEq implementation in the Goldilocks field. This caused BTreeMap operations to fail - specifically, contains_key() would return false even when keys were present in the map. Root Cause: - StorageMap used BTreeMap<Word, Word> for internal storage - StorageMapDelta used BTreeMap<LexicographicWord, Word> - Word's Ord::cmp returned Ordering::Less for equal values - This violated Rust's trait requirements: if a == b, then a.cmp(b) must be Equal - BTreeMap relies on correct Ord implementation for key lookups The Fix: - Changed StorageMap to use BTreeMap<LexicographicWord, Word> - LexicographicWord has a correct Ord implementation - Updated all methods to wrap/unwrap Word keys with LexicographicWord - Maintained backward compatibility in serialization by converting to Word Test Results: - test_multisig_update_signers_remove_owner now passes - The test correctly removes owners and clears their storage slots Files Changed: - crates/miden-protocol/src/account/storage/map/mod.rs - crates/miden-protocol/src/account/storage/map/partial.rs - crates/miden-protocol/src/account/storage/mod.rs - crates/miden-testing/tests/auth/multisig.rs Note: The mock_chain_serialization test still fails with AccountStorageMode 0b11 error - this is a separate issue to be investigated.
Fixed critical endianness mismatch where serialization used to_be_bytes() but deserialization incorrectly used from_le_bytes(). This caused version byte extraction to read wrong values, resulting in "12 is not a known account ID version" errors during NonFungibleAsset deserialization. Root Cause: - Commit 0bacdd8 refactored deserialization from value.reverse() to from_le_bytes() - Should have used from_be_bytes() to match serialization's to_be_bytes() - Example: bytes [188, 0, 0, 0, 0, 0, 202, 48] = 0xBC00_0000_0000_CA30 (big-endian) - Correct (from_be_bytes): least significant nibble = 0x0 (version 0) - Incorrect (from_le_bytes): reads as 0x30CA_0000_0000_00BC, nibble = 0xC (version 12) The Fix: - Changed from_le_bytes to from_be_bytes in AccountIdPrefixV0::try_from - Also fixed clippy warning in multisig.rs (needless_borrow) Test Results: - All miden-protocol tests pass (225 tests) - All miden-standards tests pass (52 tests) - make check, fmt, fix, clippy all pass Files Changed: - crates/miden-protocol/src/account/account_id/v0/prefix.rs - crates/miden-testing/tests/auth/multisig.rs (clippy fix) - Removed temporary diagnostic test files Note: Documented in beads issue p3-fdr
Updated dependencies: - Plonky3 0.4, p3-miden 0.4 - miden-crypto huitseeker/p3-rebased - miden-vm al-migrate-p3-ver2 - Removed winterfell Fixed LargeSmt API changes (num_leaves/num_entries now return usize). Added async prover path to avoid Tokio runtime nesting. The sync prove() method now calls prove_sync() on non-wasm32 targets. Tests use prove_async(). Fixed wasm32 builds by adding default-features=false to dependencies.
Plonky3 brings in tiny-keccak with CC0-1.0 license. Different Plonky3/miden-vm versions require itertools 0.12/0.13/0.14.
- Add git repositories for miden-vm, crypto, and p3-miden to allowed sources - Allow p3-miden crates from both git and registry during Plonky3 migration - Allow spin v0.9.x legacy version
When building with --no-default-features --all-targets, test modules that use std-only features were failing to compile. This change gates those test modules with #[cfg(all(test, feature = "std"))] to ensure they only compile when the std feature is enabled. Fixes: - account/builder/mod.rs: LazyLock requires std - account/storage/slot/slot_name.rs: ToOwned requires std - account/file.rs: new_falcon512_rpo() requires std feature - block/account_tree/mod.rs: Vec requires std in tests - block/account_tree/partial.rs: Uses tests from parent module - transaction/tx_args.rs: Uses Vec in tests This ensures cargo hack check with --each-feature --all-targets succeeds.
- Gate miden-tx test module that uses new_falcon512_rpo() with std feature - Fix import ordering in miden-protocol/account/file.rs per rustfmt - Allow unused imports in AccountBuilder for dead code elimination cases These changes ensure all feature combinations compile successfully with cargo hack check --each-feature --all-targets.
…ree semantics ## Context The Plonky3 migration includes an updated PartialSmt implementation in miden-crypto that changes the semantics of "tracked" keys in empty trees. ## Semantic Change In the new PartialSmt implementation, a key is considered "tracked" if either: 1. Its merkle path was explicitly added via add_path/add_proof, OR 2. The path from the leaf to the root goes through empty subtrees that are provably empty (they equal the empty subtree root hash). Critically, in a COMPLETELY EMPTY tree (root == EMPTY_ROOT), ALL keys satisfy condition #2 and are therefore implicitly tracked. This is correct cryptographic behavior: in an empty tree, all leaves are provably empty via zero hash computations, so there's no need to explicitly track them. ## Test Update The test `upsert_state_commitments_fails_on_untracked_key` was using `PartialAccountTree::default()` which creates an empty tree with EMPTY_ROOT. Under the new semantics, all keys are implicitly tracked in this tree, so inserting an "untracked" key would succeed rather than fail. The fix: Initialize the partial tree with a non-empty root so that keys are NOT implicitly tracked, allowing us to properly test the UntrackedAccountId error path. ## Related Code See miden-crypto/src/merkle/smt/partial/mod.rs:368-370: ```rust // Empty tree - all leaves implicitly trackable if self.root == Self::EMPTY_ROOT { return Some(SmtLeaf::new_empty(leaf_index)); } ```
…dent ## Root Cause The test was checking for exact ordering of input notes, but the ordering changed due to a fundamental change in how Word::Ord works between Winterfell and P3. **Winterfell Word::Ord**: Used `Felt::inner()` which returns raw Montgomery form u64 **P3 Word::Ord**: Uses `Felt::as_canonical_u64()` which returns canonical (reduced) form Since Nullifier wraps a Word and derives Ord, and input notes are stored in a BTreeMap<Nullifier, ...>, the iteration order depends on Nullifier::Ord which depends on Word::Ord. This change in field element comparison semantics causes different orderings for the same set of nullifiers. The P3 implementation is more correct as it ensures the Ord/PartialEq contract is satisfied (values that are equal via PartialEq must have the same Ord ordering). ## Fix Changed the test to verify **set equality** rather than **order equality**: - Still checks that exactly 2 input notes are present - Still verifies that note1 was correctly erased (created and consumed in batch) - Now uses `contains()` checks instead of exact vector equality - Added clarifying comment explaining why order is not guaranteed ## Related Changes See miden-crypto commit 337ae65 "fix(word): use as_canonical_u64 for Ord to match P3's Goldilocks" which explains the Word::Ord semantic change in detail.
…en 0.4 - Update miden-crypto from huitseeker/p3-rebased to next branch - Update p3-miden-prover from git dependency to released crates.io version 0.4 - Remove p3-miden git repository from cargo-deny allowed sources - Remove skip-tree entries for p3-miden crates in cargo-deny - Fix ExecutionOptions::new call to include core_trace_fragment_size parameter
…rts branch - Updated miden-crypto dependency to bobbin-crate-exports branch - Replaced direct p3-miden-prover dependency with miden-crypto::stark re-export - Updated all miden-vm dependencies to latest al-migrate-p3-ver2 commit - Removed winterfell profile configuration as it's no longer needed - All changes use types and functions re-exported from miden-crypto where possible This eliminates direct dependencies on low-level Plonky3 and p3-miden crates in favor of the higher-level miden-crypto abstractions.
…en 0.4 - Updated miden-crypto dependency from bobbin-crate-exports to next branch - Updated all miden-vm dependencies to latest al-migrate-p3-ver2 commit (82a47b10) - miden-crypto version bumped from 0.20.0 to 0.21.0 - All upstream dependencies now use released Plonky3 v0.4 and p3-miden v0.4 This aligns with upstream changes where miden-crypto next branch now includes the necessary re-exports and all dependencies use released versions.
- Updated miden-crypto dependency to use released version 0.20 from crates.io - Removed 0xmiden/crypto from allowed git sources in deny.toml - Added patch section to force git dependencies to use released miden-crypto 0.20 - This ensures all crates (including miden-vm dependencies) use the same released version All dependencies now use only released versions from crates.io, with the exception of miden-vm which is still on the al-migrate-p3-ver2 branch.
- Updated miden-crypto from 0.20 to 0.20.1 - This version includes enhanced re-exports for STARK components from Plonky3 The codebase already uses miden-crypto re-exports (miden_crypto::stark::prove) and has no direct dependencies on plonky3 or p3-miden crates, so no further changes are needed. The new re-exports in 0.20.1 maintain compatibility while providing better namespace organization.
- Switch all miden-vm crates from branch al-migrate-p3-ver2 to next - Update Cargo.lock to use miden-vm next branch while preserving p3-* dependencies at v0.4.1 (compatible with p3-miden-fri 0.4.0) - Update PrimeField64 imports to use miden_core::field::PrimeField64 (no longer re-exported from miden_core root) - Replace HashFunction::Blake3_192 with Blake3_256 (variant removed)
huitseeker
force-pushed
the
huitseeker/update-miden-deps-p3
branch
from
f01cacd to
2ed755c
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This prefigures the upcoming miden-vm & miden-crypto versions. Working branch made to work out the kinks of :