Skip to content

chore: migrate to protocol v0.14.0-rc.1#1784

Open
mmagician wants to merge 37 commits intonextfrom
mmagician-claude/update-base-to-beta
Open

chore: migrate to protocol v0.14.0-rc.1#1784
mmagician wants to merge 37 commits intonextfrom
mmagician-claude/update-base-to-beta

Conversation

@mmagician
Copy link
Contributor

@mmagician mmagician commented Mar 11, 2026

Summary

  • Update all miden-protocol dependencies from 0.14.0-alpha.1 to 0.14.0-beta.1
  • Update miden-crypto from 0.19.7 to 0.22
  • Remove miden-air dependency
  • Bump node workspace version to 0.14.0-beta.1

Key API migrations

  • Serialization traits moved to utils::serde submodule
  • Felt::as_int() -> Felt::as_canonical_u64()
  • falcon512_rpo -> falcon512_poseidon2
  • ProvenTransactionBuilder removed, use ProvenTransaction::new() + TxAccountUpdate (refactor: remove ProvenTransactionBuilder in favor of ProvenTransaction::new protocol#2567)
  • 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()
  • SmtStorage trait methods now take &mut self
  • AccountStateForest rewritten to replace removed LargeSmtForest

Test plan

  • cargo check --all-targets passes
  • make lint passes (clippy, formatting, machete)
  • CI passes

🤖 Generated with Claude Code

@mmagician mmagician added the no changelog This PR does not require an entry in the `CHANGELOG.md` file label Mar 11, 2026
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
@mmagician mmagician force-pushed the mmagician-claude/update-base-to-beta branch from b785bde to 922cd86 Compare March 11, 2026 19:46
@mmagician mmagician changed the title chore: migrate to protocol v0.14.0-beta.1 chore: migrate to protocol v0.14.0-beta.1 Mar 11, 2026
@mmagician mmagician force-pushed the mmagician-claude/update-base-to-beta branch from bb9974c to 5320e57 Compare March 12, 2026 10:29
@mmagician
Copy link
Contributor Author

(force pushed coz I had unsigned commits)

claude added 3 commits March 12, 2026 13:39
- 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.
claude added 4 commits March 12, 2026 15:05
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.
Comment on lines +513 to +515
let rebuilt = PublicOutputNote::new(public_note.as_note().clone())
.expect("rebuilding an already-valid public output note should not fail");
OutputNote::Public(rebuilt)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this works but we might want to propagate minify_script to PublicOutputNote to avoid the reconstruction.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minify_script() is called in PublicOutputNote::new() - so, by construction all PublicOutputNotes have minified scripts. But maybe I didn't understand the comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

claude and others added 2 commits March 12, 2026 16:55
- 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>
claude added 2 commits March 23, 2026 10:55
- RpoRandomCoin renamed to RandomCoin
- FungibleAssetDelta::iter() returns (&AssetVaultKey, &i64);
  extract faucet_id via vault_key.faucet_id()
- Assets no longer impl Into<Word>; use to_value_word()/to_key_word()
- TokenSymbol no longer Copy; clone where needed
- AccountComponentMetadata::new() takes supported_types as second arg
- AuthScheme::Falcon512Rpo -> Falcon512Poseidon2
- create_existing_agglayer_faucet() takes MetadataHash parameter

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Update bin/genesis for the rc.1 dep bump:
- miden-agglayer: 0.14.0-beta.4 -> 0.14.0-rc.1
- RpoRandomCoin -> RandomCoin

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@mmagician mmagician force-pushed the mmagician-claude/update-base-to-beta branch from 070bcf7 to 269909a Compare March 23, 2026 10:55
@mmagician mmagician changed the title chore: migrate to protocol v0.14.0-beta.2 chore: migrate to protocol v0.14.0-rc.1 Mar 24, 2026
Copy link
Contributor Author

@mmagician mmagician left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM , I reviewed all the changes in detail

@mmagician mmagician marked this pull request as ready for review March 24, 2026 11:11
@mmagician mmagician requested a review from igamigo March 24, 2026 11:20
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought proving was not going to be async? This isn't correct -- proving is synchronous work.

let result = self.prove(input).instrument(prove_span).await;

if let Err(e) = &result {
tracing::Span::current().set_error(e);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
tracing::Span::current().set_error(e);
prove_span.set_error(e);

Copy link
Collaborator

@igamigo igamigo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@@ -409,18 +409,20 @@ impl AccountStateForest {

let mut entries: Vec<(Word, Word)> = Vec::new();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Feels like this could be better typed (AssetVaultKey and, once it exists, AssetVaultAmount). May also improve the AccountStateForest API

Comment on lines +148 to +149

[patch.crates-io]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

Suggested change
[patch.crates-io]

.commitments
.into_iter()
.map(|(id, commitment)| (account_id_to_smt_key(id), commitment));
.map(|(id, commitment)| (AccountIdKey::from(id).as_word(), commitment));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels a bit weird that we're throwing away the type again

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concerning that this is so much bigger

fn from(symbol: TokenSymbol) -> Self {
// SAFETY: TokenSymbol guarantees valid format, so to_string should not fail
let raw = symbol.to_string().expect("TokenSymbol should always produce valid string");
let raw = symbol.to_string();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SAFETY comment above should be removed

Comment on lines +130 to +132
tokio::runtime::Handle::current()
.block_on(LocalTransactionProver::default().prove(tx.tx_inputs().clone()))
.unwrap()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is allowed? Either way you should just use async since we now have that.

d => panic!("unsupported depth {d}"),
};
KeyBytes::new(index.value(), keep)
KeyBytes::new(index.position(), keep)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this correct? value and position sound like opposites

tx.account_update().account_delta_commitment(),
tx.account_update().details().clone(),
)
.map_err(|e| Status::invalid_argument(e.to_string()))?;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()))?;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

Copy link
Collaborator

@Mirko-von-Leipzig Mirko-von-Leipzig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +50 to -65
miden-large-smt-backend-rocksdb = { path = "crates/large-smt-backend-rocksdb", version = "=0.14.0-beta.4" }
miden-node-block-producer = { path = "crates/block-producer", version = "=0.14.0-beta.4" }
miden-node-db = { path = "crates/db", version = "=0.14.0-beta.4" }
miden-node-grpc-error-macro = { path = "crates/grpc-error-macro", version = "=0.14.0-beta.4" }
miden-node-ntx-builder = { path = "crates/ntx-builder", version = "=0.14.0-beta.4" }
miden-node-proto = { path = "crates/proto", version = "=0.14.0-beta.4" }
miden-node-proto-build = { path = "proto", version = "=0.14.0-beta.4" }
miden-node-rpc = { path = "crates/rpc", version = "=0.14.0-beta.4" }
miden-node-store = { path = "crates/store", version = "=0.14.0-beta.4" }
miden-node-test-macro = { path = "crates/test-macro" }
miden-node-utils = { path = "crates/utils", version = "=0.14.0-alpha.8" }
miden-node-validator = { path = "crates/validator", version = "=0.14.0-alpha.8" }
miden-remote-prover-client = { path = "crates/remote-prover-client", version = "=0.14.0-alpha.8" }
miden-node-utils = { path = "crates/utils", version = "=0.14.0-beta.4" }
miden-node-validator = { path = "crates/validator", version = "=0.14.0-beta.4" }
miden-remote-prover-client = { path = "crates/remote-prover-client", version = "=0.14.0-beta.4" }
# Temporary workaround until <https://github.com/rust-rocksdb/rust-rocksdb/pull/1029>
# is part of `rocksdb-rust` release
miden-node-rocksdb-cxx-linkage-fix = { path = "crates/rocksdb-cxx-linkage-fix", version = "=0.14.0-alpha.8" }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These could now be changed to just 0.14, right?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's this other draft for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no changelog This PR does not require an entry in the `CHANGELOG.md` file

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Replace middle-step serialization of InputNoteCommitment with direct constructor

8 participants