-
Notifications
You must be signed in to change notification settings - Fork 44
wip: upgrade miden VM to 0.17 #659
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: next
Are you sure you want to change the base?
wip: upgrade miden VM to 0.17 #659
Conversation
match self { | ||
Self::Executable(ref prog) => { | ||
if matches!(mode, OutputMode::Binary) { | ||
log::warn!( | ||
"unable to write 'masl' output type for miden_core::Program: skipping.." | ||
); | ||
} | ||
prog.write_to(writer, mode, session) | ||
} | ||
Self::Library(ref lib) => lib.write_to(writer, mode, session), | ||
} | ||
let mut writer = ByteWriterAdapter(&mut writer); | ||
self.write_into(&mut writer); | ||
Ok(()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sidenote: This change is not realted to the VM update itself, the integration tests pass with the previous version as well.
However, shouldn't MastArtifact
's serialization function be called, instead of calling the underlying variant's serialization function. There is a slight difference in serialization, mainly the presence of the initial MagicBytes
when serialized via MastArtifact
.
Here's MastArtifact
's serialization when there's an underlying Library
:
https://github.com/0xMiden/miden-vm/blob/da78ba2ab0db3e092d063e5526c0cd3ddee7163a/package/src/package/serialization.rs#L158-L161
And here's the Library
's:
https://github.com/0xMiden/miden-vm/blob/da78ba2ab0db3e092d063e5526c0cd3ddee7163a/assembly-syntax/src/library/mod.rs#L409-L428
// WARNING: These two are equivalent, shouldn't this be a no-op? | ||
let word = rodata.digest.as_elements(); | ||
let word_value = [word[0], word[1], word[2], word[3]]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I'm not mistaken, WordValue
and Word
are two equivalent structs.
Here's WordValue
's definition: link
And here's Word
's: link, with WORD_SIZE_FELT
being 4: link
Additionally, I believe before this PR (0xMiden/miden-vm@de03313), Word
was used directly.
midenc-debug/src/debug/stacktrace.rs
Outdated
// QUESTION/TODO(fabrio): Should this be A LineNumber and ColumnNumber respectively? | ||
pub line: u32, | ||
pub col: u32, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This commit 0xMiden/miden-vm@62275e9 in version 0.17.1
introduced the LineNumber
and ColumnNumber
structs.
Should these be used here as well?
// Extract and load the advice map from the forest before putting it into the store. | ||
let advice_map = forest.advice_map(); | ||
for (digest, values) in advice_map.iter() { | ||
let key = digest.into(); | ||
self.adv_provider.insert_into_map(key, values.clone()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed this bit since it was also removed in this previous PR: https://github.com/0xMiden/compiler/pull/593/files#diff-4e5cd1ed11ee24544998276064a9b2726f61bf08a7fc9c1490b8f87e45d9125bL53-L61
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
…r::assemble_library_from_dir(path, ns) Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
… underlying Lib/Prog Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
dcdd3e2
to
0f4f4ed
Compare
Note: This PR is still in WIP
While working on
midenup
's tutorial, I tried to create anAccount
with the miden-client's CLI from the generated.masp
that the compiler produced.*The problem I stumbled upon was that the client failed to deserialize the generated
Package
from the.masp
, erroring out with:unsupported version. Got '[0.0.0]', but only '[1.0.0]' is supported
.If I'm not mistaken, this error originates from a mismatch between the client and the compiler VM's version. The compiler is using version 0.15 of the
miden-mast-package
crate (which is a crate present in the VM's repo), while the client is using version 0.17 the VM (indirectly via its usage of version 0.11 of themiden-objects
crate). Particularly, thePackage
serialization format got a major version bump from version 0.15.0 to version 0.17.0So, this PR updates the VM version used by the compiler.
* I added a small experimental-only patch to the miden-client in order to consume the
Package
.PR status:
todo!
).cargo miden build
manages to compile a basic miden repository generated fromcargo miden new
without crashingHere's a list of changes that this PR introduces:
miden-core
no longer has thediagnostics
feature flag, it got removed.miden-core
no longer hasdebug_info
, moved tomiden-debug-types
LibraryPathComponent
got moved fromassembly
toassembly-syntax
miden-lib
got upgraded to version0.11.0
since that's the version that usesmiden-vm
version 0.17.0CompiledLibrary::from_dir
got replaced withmiden_assembly::Assembler::assemble_library_from_dir(path, ns)
miden_processor::Digest
got replaced withmiden_core::Word
Assembler::add_module
were replaced with calls toAssembler::compile_and_link_moduels
.Library::exports
now returns aLibraryExport
rather than aQualifiedProcedureName
QualifiedProcedureName
is a field insideLibraryExports
recover_wasm_cm_interfaces
now returnsBTreeMap<masm::QualifiedProcedureName, LibraryExport>
instead ofBTreeMap<masm::QualifiedProcedureName, MastNodeID>
PushWord
now receives aWordValue
instead ofmiden_core/crypto::Word
.miden-vm
MemAdviceProvider
no longer exists, and got replaced withAdviceProvider
AdviceProvider
struct is now a struct instead of a trait.AdviceProvider
into a struct miden-vm#1904AdviceProvider
into a struct (#1904) miden-vm#1906From<AdviceInputs>
implementation appears to have changed:Host
trait got moved intoBaseHost
.host.rs
,DebuggerHost
implements bothBaseHost
andSyncHost
. Some function implementations are still left astodo!()
'sAsyncHost
be implemented as well?PackageExport
now incluldes funciton signature (updated inassemble.rs
).SourceFile
no longer has apath()
method, now it's construted from like so:Path::new(source_file.uri())
. Seen inbreakpoint.rs
. Question: Should uri.as_path() be used?u32
?miden-objects
to version0.11.0
u32::max
to 536870912 (1 << 29 )array.masm
test changed with the following diff: (this also happened to multiple other tests)Assembler::add_library
got renamed toAssembler::link_dynamic_library
Sidenote for reference: The diff before the
Cargo.lock
files were included was +249 −241.