Add `miden-package` crate with `Package` type to represent a compiled Miden program/library. #1544

greenhat · 2024-10-23T13:23:46Z

This PR adds miden-package crate with Package type ported from the compiler's ad-hoc implementation to represent a compiled Miden program/library.

Please consider reviewing this PR commit-by-commit. I crafted the commits to be focused on a single change and show the evolution of the implementation.

The reasons for the new crate are explained in 0xPolygonMiden/compiler#376

The compiler's PR that uses miden-package from this PR is 0xPolygonMiden/compiler#349

bobbinth · 2025-01-16T09:11:58Z

assembly/src/ast/procedure/name.rs

+        let proc_name =
+            ProcedureName::new_unchecked(Ident::new_unchecked(Span::unknown(Arc::from(str))));


This would allow deserialization of arbitrary strings as procedure names, right? (e.g., strings with control or unprintable characters etc.) Would this not be a problem?

Should we enforce at least some kind of validation (e.g., maybe against the alphabet used on line 371 below)?

Good point! Since it's an unchecked user input, I suppose there is a chance to use it for some sort of attack. I cannot come up with any, but I'd feel safer if we have at least some validation here. This should be a different (more relaxed) validation from the one we use when parsing the MASM source code. @bitwalker What do you think?

I relaxed the validation rules in the Ident (used in LibraryPath) and ProcedureName in new constructors and used them in the deserialization instead of the new_unchecked ones.
The validation on MASM source code parsing is implemented in the lexer and does not use the checked constructors (it calls new_unchecked). So the existing checked constructors are the different (from lexer's) kind of validation to be used everywhere else.

bobbinth · 2025-01-16T09:17:22Z

assembly/src/library/path.rs

+                Ok((ns, rest)) => {
+                    let module_id = Ident::new_unchecked(Span::new(
+                        SourceSpan::default(),
+                        Arc::from(rest.to_string()),
+                    ));
+                    LibraryPath::new_from_components(ns, [module_id])
+                },
+                Err(_) => {
+                    let module_id = Ident::new_unchecked(Span::new(
+                        SourceSpan::default(),
+                        Arc::from(path.to_string()),
+                    ));
+                    LibraryPath::new_from_components(LibraryNamespace::Anon, [module_id])
+                },


Similar comment as above. I think we discussed this previously, but I don't remember why we are skipping validation rather than updating validation rules to support the newly required formats.

AFAIR, the reason was that validation that we have now is for the MASM source code parsing, and technically, we can allow any characters in the identifiers in the compiled code. @bitwalker correct me if I'm wrong.
But from the "validate any user input" point of view, it makes sense to create and use another, more relaxed type of validation here and in ProcedureName (see my comment above).

Done. See my comment above - #1544 (comment)

package/README.md

Co-authored-by: Philippe Laferrière <[email protected]>

bobbinth

Looks good! Thank you! I left some comments inline. The main ones are about:

Potentially adding some validation to library path deserialization.
Using Serializable/Deserializable instead of serde.
Not including functionality that assumes downstream crates (i.e., miden-base).

package/Cargo.toml

package/README.md

bobbinth · 2025-01-17T00:58:23Z

package/Cargo.toml

@@ -0,0 +1,31 @@
+[package]
+name = "miden-package"


I'm wondering if we should make it a bit more specific. Maybe miden-vm-package or miden-code-package (though, not sure if I like these better than just miden-package).

I got so much used to miden-package so I'm biased here. :) In general, if we plan to have another "packages" (account?) giving it a more specific name makes sense.

package/src/dep/mod.rs

Cargo.toml

greenhat · 2025-01-17T11:17:18Z

@bobbinth @plafer Thank you for the review! I've addressed all notes, implemented easy ones, and I am converting this PR to draft to work on:

Switch PackageExport::name to have QualifiedProcedureName type and propagate Arbitrary implementation for proptest;
Remove bitcode and serde based serialization in favor of Serializable/Deserializable;
New validation for ids (LibraryPath, ProcedureName) on deserialization;

instead of `bitcode` and `serde`

Implement `Serializable/Deserializable` and `Arbitrary`(proptest) for `QualifiedProcedureName`. Add serialization roundtrip tests for `LibraryPath` and `ProcedureName`.

bobbinth

Looks good! Thank you! I left some more comments/questions inline.

Besides these, I think the main thing remaining here is potentially adding some validation to library path deserialization - right?

bobbinth · 2025-01-21T08:11:35Z

package/src/lib.rs

+pub use self::{
+    dep::{
+        resolver::{
+            DependencyResolver, LocalResolvedDependency, MemDependencyResolverByDigest,
+            ResolvedDependency,
+        },
+        Dependency, DependencyName,
+    },
+    package::{MastArtifact, Package, PackageExport, PackageManifest},
+};


Question: should we re-export such things as Digest, Program, MastForest etc. from here is that this crate is more "self-contained"?

I'd say "yes". I can imagine miden-package crate being used for some sort of package analysis without relaying on the rest of VM crates.

bobbinth · 2025-01-21T08:13:56Z

package/src/package.rs

+    ///// Parses a package from the provided bytes
+    //pub fn read_from_bytes<B>(bytes: B) -> Result<Self, Report>
+    //where
+    //    B: AsRef<[u8]>,
+    //{
+    //    use alloc::borrow::Cow;
+    //
+    //    let bytes = bytes.as_ref();
+    //
+    //    let bytes = bytes
+    //        .strip_prefix(Self::MAGIC)
+    //        .ok_or_else(|| Report::msg("invalid package: missing header"))?;
+    //    let bytes = bytes.strip_prefix(Self::FORMAT_VERSION).ok_or_else(|| {
+    //        Report::msg(format!(
+    //            "invalid package: incorrect version, expected '1.0', got '{}'",
+    //            bytes.get(0..4).map(String::from_utf8_lossy).unwrap_or(Cow::Borrowed("")),
+    //        ))
+    //    })?;
+    //
+    //    bitcode::deserialize(bytes).map_err(Report::msg)
+    //}
+    //
+    ///// Serializes the package into a byte array
+    //pub fn write_to_bytes(&self) -> Result<Vec<u8>, Report> {
+    //    let mut bytes = Vec::new();
+    //    bytes.extend_from_slice(Self::MAGIC);
+    //    bytes.extend_from_slice(Self::FORMAT_VERSION);
+    //    let mut data = bitcode::serialize(self).map_err(Report::msg)?;
+    //    bytes.append(&mut data);
+    //    Ok(bytes)
+    //}


Is this meant to be commented-out?

Yes, I missed it. Thanks for spotting this!
In my defense, I planned to give this PR a "polish" before marking it ready and asking for another round of reviews. 🙂

bobbinth · 2025-01-21T08:14:24Z

package/src/package.rs

+    pub fn digest(&self) -> Digest {
+        self.mast.digest()
+    }


nit: let's add a doc comment here.

bobbinth · 2025-01-21T08:15:42Z

package/src/tests.rs

Should this test be moved under src/package/serialization?

bobbinth · 2025-01-21T08:16:13Z

package/src/package.rs

Should this file be moved to src/package/mod.rs?

bobbinth · 2025-01-21T08:17:42Z

package/src/package.rs

+pub struct Package {
+    /// Name of the package
+    pub name: String,
+    /// The MAST artifact ([Program] or [Library]) of the package
+    pub mast: MastArtifact,
+    /// The package manifest, containing the set of exported procedures and their signatures,
+    /// if known.
+    pub manifest: PackageManifest,
+}


Probably not for this PR, but where would we put custom metadata? Here, I'm thinking about the storage layout metadata we'd need to instantiate account components.

I'm thinking, either directly in Package or in PackageManifest.

constructors in deserialization.

…ode cleanup

`package/mod.rs`

greenhat · 2025-01-22T06:51:27Z

@bobbinth @plafer I implemented everything and addressed all your notes. Please do another round.

plafer

LGTM!

greenhat changed the title ~~Ass miden-package crate with Package type to represent a compiled Miden program/library.~~ Add miden-package crate with Package type to represent a compiled Miden program/library. Oct 23, 2024

greenhat force-pushed the greenhat/miden-package branch 4 times, most recently from 76a4b7e to 9ff342e Compare October 30, 2024 09:44

greenhat mentioned this pull request Oct 30, 2024

fix: use unchecked constructor for ProcedureName on Library deserialization #1554

Merged

greenhat force-pushed the greenhat/miden-package branch from 9ff342e to 7c9b95b Compare October 30, 2024 10:14

greenhat changed the base branch from next to greenhat/library-deser-uncheck-exports October 30, 2024 10:15

greenhat force-pushed the greenhat/library-deser-uncheck-exports branch from 1ca8581 to 3581727 Compare November 4, 2024 17:37

Base automatically changed from greenhat/library-deser-uncheck-exports to next November 4, 2024 21:37

greenhat force-pushed the greenhat/miden-package branch 7 times, most recently from 41b5f5d to c5ff6f0 Compare November 12, 2024 14:07

greenhat mentioned this pull request Nov 12, 2024

Support ability to specify advice data via MASM #1547

Open

greenhat force-pushed the greenhat/miden-package branch 6 times, most recently from 211f34c to 84ea102 Compare November 19, 2024 08:49

greenhat changed the base branch from next to greenhat/i1547-mastforest-add-advicemap November 19, 2024 08:49

greenhat changed the title ~~Add miden-package crate with Package type to represent a compiled Miden program/library.~~ [2/2] Add miden-package crate with Package type to represent a compiled Miden program/library. Nov 19, 2024

greenhat force-pushed the greenhat/i1547-mastforest-add-advicemap branch from eda4a01 to 006e03a Compare November 19, 2024 14:40

greenhat force-pushed the greenhat/miden-package branch 2 times, most recently from 5ec902e to d6419ca Compare November 19, 2024 15:00

greenhat force-pushed the greenhat/i1547-mastforest-add-advicemap branch from 006e03a to 92f3309 Compare November 20, 2024 07:05

bobbinth reviewed Jan 16, 2025

View reviewed changes

package/README.md Outdated Show resolved Hide resolved

greenhat and others added 8 commits January 16, 2025 12:53

docs: wording

4d031dd

Co-authored-by: Philippe Laferrière <[email protected]>

docs: formatting

15dfc12

Co-authored-by: Philippe Laferrière <[email protected]>

docs: formatting

ae31949

Co-authored-by: Philippe Laferrière <[email protected]>

docs: formatting

2d65242

Co-authored-by: Philippe Laferrière <[email protected]>

docs: wording

cc88636

Co-authored-by: Philippe Laferrière <[email protected]>

docs: wording

86d8974

Co-authored-by: Philippe Laferrière <[email protected]>

chore: fix clippy, formatting, cleanup comments

7e37745

refactor: rename types for resolved dependency for the Package

43e0a83

bobbinth reviewed Jan 17, 2025

View reviewed changes

greenhat added 2 commits January 17, 2025 10:44

refactor: remove SystemLibraryId and use DependencyName(String)

ab26b55

chore: add license in package/README.md and fix docs.rs version in

ad3e722

Cargo.toml

greenhat marked this pull request as draft January 17, 2025 11:17

greenhat added 3 commits January 20, 2025 18:07

refactor: serialize Package with Serializable/Deserializable impls

3cd2d1b

instead of `bitcode` and `serde`

refactor: make PackageExport::name to be QualifiedProcedureName,

72b700e

Implement `Serializable/Deserializable` and `Arbitrary`(proptest) for `QualifiedProcedureName`. Add serialization roundtrip tests for `LibraryPath` and `ProcedureName`.

chore: formatting

a93f3a2

greenhat force-pushed the greenhat/miden-package branch from 915c093 to a93f3a2 Compare January 20, 2025 16:07

chore: fix no_std build

8b60a31

bobbinth reviewed Jan 21, 2025

View reviewed changes

fix: relax Ident and ProcedureName validation, use checked

37fe048

constructors in deserialization.

greenhat force-pushed the greenhat/miden-package branch from 223c95f to 37fe048 Compare January 22, 2025 05:17

greenhat added 2 commits January 22, 2025 08:02

chore: export Digest, MastForest, Program in miden-package, c…

e3fa250

…ode cleanup

refactor: move tests to serialization module, rename package.rs to

a1b44a7

`package/mod.rs`

greenhat mentioned this pull request Jan 22, 2025

chore: update the Miden VM (with updated miden-package crate) 0xPolygonMiden/compiler#376

Draft

greenhat marked this pull request as ready for review January 22, 2025 06:51

plafer approved these changes Jan 22, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `miden-package` crate with `Package` type to represent a compiled Miden program/library. #1544

Add `miden-package` crate with `Package` type to represent a compiled Miden program/library. #1544

greenhat commented Oct 23, 2024 •

edited

Loading

bobbinth Jan 16, 2025

greenhat Jan 17, 2025

greenhat Jan 22, 2025

bobbinth Jan 16, 2025

greenhat Jan 17, 2025

greenhat Jan 22, 2025

bobbinth left a comment

bobbinth Jan 17, 2025

greenhat Jan 17, 2025

greenhat commented Jan 17, 2025

bobbinth left a comment

bobbinth Jan 21, 2025

greenhat Jan 21, 2025

greenhat Jan 22, 2025

bobbinth Jan 21, 2025

greenhat Jan 21, 2025

greenhat Jan 22, 2025

bobbinth Jan 21, 2025

greenhat Jan 22, 2025

bobbinth Jan 21, 2025

greenhat Jan 22, 2025

bobbinth Jan 21, 2025

greenhat Jan 22, 2025

bobbinth Jan 21, 2025

greenhat Jan 21, 2025

greenhat commented Jan 22, 2025

plafer left a comment

		let proc_name =
		ProcedureName::new_unchecked(Ident::new_unchecked(Span::unknown(Arc::from(str))));

Add miden-package crate with Package type to represent a compiled Miden program/library. #1544

Are you sure you want to change the base?

Add miden-package crate with Package type to represent a compiled Miden program/library. #1544

Conversation

greenhat commented Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobbinth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greenhat commented Jan 17, 2025

bobbinth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greenhat commented Jan 22, 2025

plafer left a comment

Choose a reason for hiding this comment

Add `miden-package` crate with `Package` type to represent a compiled Miden program/library. #1544

Add `miden-package` crate with `Package` type to represent a compiled Miden program/library. #1544

greenhat commented Oct 23, 2024 •

edited

Loading