Conversation
Update all miden-protocol dependencies from 0.14.0-alpha.1 to 0.14.0-beta.1 (crates.io). Update miden-crypto from 0.19.7 to 0.22. Bump node workspace version to 0.14.0-beta.1. Key migration changes: - Serialization traits moved to utils::serde submodule - Felt::as_int() -> Felt::as_canonical_u64() - falcon512_rpo -> falcon512_poseidon2 - ProvenTransactionBuilder removed, use ProvenTransaction::new() - OutputNote variants: Full -> Public, Header -> Private - Asset is now key-value (two words instead of one) - MmrProof fields are now methods - account_id_to_smt_key -> AccountIdKey::from() - miden-air removed (replaced by miden-core 0.21) - SmtStorage trait methods now take &mut self - Various other API changes in miden-crypto 0.22
b785bde to
922cd86
Compare
v0.14.0-beta.1
bb9974c to
5320e57
Compare
|
(force pushed coz I had unsigned commits) |
4c74ff6 to
4d53991
Compare
- Use LargeSmt::load() instead of LargeSmt::new() in load_smt to support loading from non-empty RocksDB storage on restart (new() now errors with StorageNotEmpty in miden-crypto 0.22) - Fix prune logic to remove all versions before cutoff while keeping at least one version per lineage to preserve current state - Only pop roots from SmtForest that are no longer referenced by any lineage (prevents removing shared roots) - Fix test helpers tree_id_for_root/tree_id_for_vault_root to use get_root_at_block instead of latest_root - Update all prune test assertions for the new version-tracking model (our AccountStateForest uses explicit version entries rather than LargeSmtForest's deduplicated tree count)
Replace ~130 lines of hand-rolled DER parsing with proper library calls: - Use spki::SubjectPublicKeyInfoRef::from_der() for SPKI public key parsing - Use k256::ecdsa::Signature::from_der() for DER signature decoding This restores the simplicity of the original code before the migration, where PublicKey::from_der() and Signature::from_der() were available directly on the miden-crypto types.
Resolve conflicts in ntx-builder query tests by keeping test_utils imports and fixing TransactionId::new() signature for the beta API.
Remove stale comment about missing re-exports (fixed in miden-crypto 0.22.5) and import from miden_protocol::crypto::merkle::smt to match the convention used on next.
Public and private accounts were created with overlapping seed index ranges, which in the v0.14 beta can produce accounts with duplicate ID prefixes. Use separate non-overlapping index offsets for each storage mode within a block.
The previous approach used low-entropy seeds (index padded with zeros) which could produce AccountId prefix collisions during the hash grinding process. Use a seeded PRNG to generate full 32-byte random seeds per account while maintaining deterministic behavior.
Resolve conflict in state/loader.rs imports: keep RocksDbOptions from next's rocksdb config PR and AccountIdKey from our beta migration.
| let rebuilt = PublicOutputNote::new(public_note.as_note().clone()) | ||
| .expect("rebuilding an already-valid public output note should not fail"); | ||
| OutputNote::Public(rebuilt) |
There was a problem hiding this comment.
this works but we might want to propagate minify_script to PublicOutputNote to avoid the reconstruction.
There was a problem hiding this comment.
minify_script() is called in PublicOutputNote::new() - so, by construction all PublicOutputNotes have minified scripts. But maybe I didn't understand the comment?
There was a problem hiding this comment.
Not quite: I was thinking of mutating PublicOutputNote, something like:
fn strip_output_note_decorators<'a>(
notes: impl Iterator<Item = &'a OutputNote> + 'a,
) -> impl Iterator<Item = OutputNote> + 'a {
notes.map(|note| match note {
OutputNote::Public(public_note) => {
let minified = public_note.clone().minify_script(); // <------ would need this
OutputNote::Public(public_note)
},
OutputNote::Private(header) => OutputNote::Private(header.clone()),
})
}so rather than fully reconstructing PublicOutputNote::new(), we would avoid some of the other checks that new constructor performs. These checks are not expensive at all, but I think it would also be cleaner to "only do the work necessary" to strip the decorators, using the constructor to do it feels somewhat hacky.
There was a problem hiding this comment.
I actually think this function is not needed any more. As I mentioned, PublicOutputNote, by construction should not contain any decorators (these get stripped both on construction and on de-serialization). So, the notes parameter should be guaranteed to have notes with decorator info already stripped.
We should be able to safely remove this function. Let's do it in a follow-up PR or create an issue for this.
- Remove dead `Panic(JoinError)` variant from `NtxError` - Remove unused `get_root`/`set_root` and `ROOT_KEY` from `RocksDbStorage` - Replace verbose `std::iter::empty` with `Vec::new()` in rpc tests - Extract inline fee construction to `test_fee()` variable in store tests Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-authored-by: Claude (Opus) <noreply@anthropic.com>
Co-authored-by: Claude (Opus) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
mmagician
left a comment
There was a problem hiding this comment.
LGTM , I reviewed all the changes in detail
There was a problem hiding this comment.
I thought proving was not going to be async? This isn't correct -- proving is synchronous work.
There was a problem hiding this comment.
For now it is async, but I think in the next release, we'll make it sync again.
| @@ -409,18 +409,20 @@ impl AccountStateForest { | |||
|
|
|||
| let mut entries: Vec<(Word, Word)> = Vec::new(); | |||
There was a problem hiding this comment.
nit: Feels like this could be better typed (AssetVaultKey and, once it exists, AssetVaultAmount). May also improve the AccountStateForest API
| .commitments | ||
| .into_iter() | ||
| .map(|(id, commitment)| (account_id_to_smt_key(id), commitment)); | ||
| .map(|(id, commitment)| (AccountIdKey::from(id).as_word(), commitment)); |
There was a problem hiding this comment.
Feels a bit weird that we're throwing away the type again
There was a problem hiding this comment.
Concerning that this is so much bigger
| tx.account_update().account_delta_commitment(), | ||
| tx.account_update().details().clone(), | ||
| ) | ||
| .map_err(|e| Status::invalid_argument(e.to_string()))?; |
There was a problem hiding this comment.
e.to_string() is insufficient as it only surfaces the top level error
| let stripped_outputs = strip_output_note_decorators(tx.output_notes().iter()); | ||
| builder = builder.add_output_notes(stripped_outputs); | ||
| let rebuilt_tx = builder.build().map_err(|e| Status::invalid_argument(e.to_string()))?; | ||
| .map_err(|e| Status::invalid_argument(e.to_string()))?; |
There was a problem hiding this comment.
I've reviewed insofar as I am able. Not this PRs fault, but I absolutely hate dislike this :/ The churn and subtle changes are more than one can reasonably review and be confident in, in imo, and I wouldn't be surprised if this leads to a bunch of bugs that we'll only discover further down the line.
Co-authored-by: Mirko <48352201+Mirko-von-Leipzig@users.noreply.github.com>
There was a problem hiding this comment.
For now it is async, but I think in the next release, we'll make it sync again.
v0.14.0-rc.1v0.14.0
Summary
0.14.0-alpha.1to0.14.00.19.7to0.220.14.0-beta.1Key API migrations
utils::serdesubmoduleFelt::as_int()->Felt::as_canonical_u64()falcon512_rpo->falcon512_poseidon2ProvenTransactionBuilderremoved, useProvenTransaction::new()+TxAccountUpdate(refactor: removeProvenTransactionBuilderin favor ofProvenTransaction::newprotocol#2567)OutputNotevariants:Full->Public,Header->PrivateAssetis now key-value (two words instead of one)MmrProoffields are now methodsaccount_id_to_smt_key->AccountIdKey::from()SmtStoragetrait methods now take&mut selfAccountStateForestrewritten to replace removedLargeSmtForestTest plan
cargo check --all-targetspassesmake lintpasses (clippy, formatting, machete)🤖 Generated with Claude Code