Upgrade VM, crypto, and rand dependencies#3146
Conversation
huitseeker
requested review from
PhilippGackstatter,
mmagician and
partylikeits1983
| @@ -1 +1,4 @@ | |||
| #![cfg_attr(not(feature = "concurrent"), no_std)] | |||
There was a problem hiding this comment.
Do we need to run benchmarks for no_std or why is this needed? I assume this has something to do with all dependencies being made optional as well?
There was a problem hiding this comment.
The issue is Cargo feature unification. The transaction benchmark uses the concurrent proving path. Since 0xMiden/crypto#1073, miden-lifted-stark/concurrent is part of the default crypto stack. In crypto 0.27, miden-crypto/concurrent enables std. On the protocol side, miden-tx/concurrent and miden-processor/concurrent also require std.
That works for the benchmark, but it must not leak into cargo build --no-default-features --target wasm32-unknown-unknown --workspace --lib. So the feature gate keeps normal benchmark builds as the default through default = ["concurrent"], while the no-default library build does not enable the std only benchmark stack.
| use miden::core::word | ||
| use agglayer::common::utils | ||
| use ::miden::protocol::asset::FUNGIBLE_ASSET_MAX_AMOUNT | ||
| use {FUNGIBLE_ASSET_MAX_AMOUNT} from ::miden::protocol::asset |
There was a problem hiding this comment.
I think call sites would be clearer if we could import just the module and then reference items from a module as module::item, e.g. push.asset::FUNGIBLE_ASSET_MAX_AMOUNT. I thought this would be implemented as part of 0xMiden/miden-vm#2643 (comment), but it seems like it wasn't. Is that planned as a follow-up? cc @bitwalker
Main reason I'm asking is because I'm not a big fan of this syntax and that is totally subjective. But if we had the ability to do the above, then there would be no need to use this syntax anywhere.
There was a problem hiding this comment.
The exact form push.asset::FUNGIBLE_ASSET_MAX_AMOUNT is rejected as an invalid identifier so far. In any case, I would handle the module import style in a later MASM cleanup, once the assembler accepts it.
There was a problem hiding this comment.
It seems like a large part of the changes are related to packages and the way we assemble MASM. Does this not result in many conflicts with #2896? That workstream currently has 4 open PRs. So it seems it would make sense to do this migration after that issue was resolved, wdyt?
There was a problem hiding this comment.
Yes, this PR is already doing part of issue #2896. The VM 0.24 upgrade removes the old shape of several Library and Program APIs, so this PR has to move some code to Package and MastForest. That is visible in account component code, note scripts, transaction scripts, and the build scripts.
The part this PR does not do is the project file migration. It adds no miden-project.toml files, and it does not move protocol or standards assembly to the declarative project layout from #3094 and #3107.
I think this leaves us with an ordering choice. If we want the dependency upgrade now, this PR can land first and the project file PRs can rebase over it. If we want less churn in the package migration work, this PR can wait and I can rebase it after #3094 and #3107.
2 participants
This updates Miden VM to v0.24, Miden Crypto to v0.27, and rand to v0.10.
The VM upgrade moved package debug info out of MastForest. This PR keeps that debug info attached through scripts, account code, and MAST forest stores by carrying
PackageDebugInfoinLoadedMastForest.This also updates MASM imports and build helpers for the new assembly package API.
Closes #3064