Skip to content
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

fix: add tx kernel library with stubs and link it on -l base #277

Merged
merged 7 commits into from
Aug 15, 2024

Conversation

greenhat
Copy link
Contributor

@greenhat greenhat commented Aug 9, 2024

This PR adds miden tx kernel stubs and links them via -l base compiler option

@greenhat
Copy link
Contributor Author

greenhat commented Aug 9, 2024

@bitwalker I'm having second thoughts about the naming. Maybe it's better to be explicit that it's not the real tx kernel, only stubs? miden-tx-kernel-stubs, etc.

@@ -0,0 +1,18 @@
[package]
name = "midenc-tx-kernel"
Copy link
Contributor

Choose a reason for hiding this comment

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

My understanding was that we were going to name wrapper crates like this something to the effect of miden-<name>-sys.
where <name> is something like stdlib, tx_kernel, etc.

While this crate doesn't contain bindings yet, I would expect that whatever crate does contain bindings (or is expected to eventually), would also be responsible for producing the Library (and embedding it in the crate for distribution).

The Miden standard library is a bit weird at the moment, as it is currently maintained in miden-vm, and the miden-stdlib crate does not contain bindings, instead we have -sys crates in this repo which handle that, but the Library is embedded in miden-stdlib. But I wouldn't pay much mind to that - instead, we should define miden-base-sys, and have that define a Library containing the code from miden-base (or our stubs, as the case may be for the moment). We would then link it using -l base.

I'm not sure we need to have separate crates for each subcomponent of miden-base, since it is basically all-or-nothing, but I'm open to reconsidering that aspect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My understanding was that we were going to name wrapper crates like this something to the effect of miden-<name>-sys. where <name> is something like stdlib, tx_kernel, etc.

Yes, but I have not connected the dots and started a new crate. 🤦

While this crate doesn't contain bindings yet, I would expect that whatever crate does contain bindings (or is expected to eventually), would also be responsible for producing the Library (and embedding it in the crate for distribution).

Makes sense. Does it mean that we no longer need to separately specify digests for each function in the bindings? Like in

extern "C" {
#[link_name = "get_id<0x0000000000000000000000000000000000000000000000000000000000000000>"]
pub fn extern_account_get_id() -> AccountId;

The Miden standard library is a bit weird at the moment, as it is currently maintained in miden-vm, and the miden-stdlib crate does not contain bindings, instead we have -sys crates in this repo which handle that, but the Library is embedded in miden-stdlib. But I wouldn't pay much mind to that - instead, we should define miden-base-sys, and have that define a Library containing the code from miden-base (or our stubs, as the case may be for the moment). We would then link it using -l base.

I'm not sure we need to have separate crates for each subcomponent of miden-base, since it is basically all-or-nothing, but I'm open to reconsidering that aspect.

I agree.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense. Does it mean that we no longer need to separately specify digests for each function in the bindings?

My thinking was that the build script for the crate would compile the MAST, write that to OUT_DIR (and then store it in the crate sources using include_bytes!), as well as generate a Rust source file containing the extern function definitions for the procedures in the MAST, using the digests that we can get from the Library struct by visiting each module and procedure, paired with signature metadata that we get initially from something we maintain by hand (maybe JSON? doesn't matter, whatever is most convenient, could also add a dependency on midenc-hir-type and serialize types to/from JSON), but when I am able to land the changes that add type signatures to Miden Assembly, we would derive the signature metadata from that. So essentially we are able to automate the generation of bindings for the most part for -sys crates.

The aspect that would be hand-maintained, would be the high-level Rust crate that depends on the -sys crate - these would provide more native abstractions for the underlying raw procedure bindings.

This gets us the ability to load MAST from a Rust crate in the compiler (or in the test suite, etc.), like we do with miden-stdlib, the ability to generate the bindings is at nice benefit as well. We may want to use a separate process for managing the compilation of the MAST, and simply read the existing MAST to generate the bindings, but for now this will simplify some things.

@@ -41,6 +41,8 @@ pub fn build_masm(
"--verbose".as_ref(),
"-l".as_ref(),
"std".as_ref(),
"-l".as_ref(),
Copy link
Contributor

@bitwalker bitwalker Aug 9, 2024

Choose a reason for hiding this comment

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

A few things:

  1. We should not use miden as the name for any Library, as it is far too general and ambiguous
  2. If we are going to have separate crates for each miden-base subcomponent (such as the TX kernel), we should be more precise with the names, e.g. -l miden_tx_kernel (which should then be the file stem used when writing it to disk). I add the miden_ prefix here because if we have a number of such libraries, they are less "special", so should be namespaced to avoid conflicts
  3. I would prefer to group miden-base libraries together, and link them using -l base. This would be a "special" library like std, which we handle in the compiler directly.
  4. Rather than specifying -l std -l base, we should infer those options in the compiler when --target rollup is specified (since it is implied that those libraries are dependencies). Here, in cargo-miden, we should distinguish between building a crate for the rollup, and building a crate that just targets the base VM, and pass --target to get the default set of libraries for those targets.

I'll add target-derived default libraries today, so you can assume the default target adds -l std, and the rollup target adds both -l std and -l base; but I'll leave it up to you to add handling for -l base in the LinkLibrary type of midenc-session like we do for -l std

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we need one crate for each miden-base subcomponent. I agree that miden is too generic.

Copy link
Contributor

@bitwalker bitwalker left a comment

Choose a reason for hiding this comment

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

A few other suggested tweaks, primarily just to ensure we have usable debug info

midenc-tx-kernel/src/lib.rs Outdated Show resolved Hide resolved
midenc-tx-kernel/src/lib.rs Outdated Show resolved Hide resolved
midenc-tx-kernel/Cargo.toml Outdated Show resolved Hide resolved
@greenhat greenhat marked this pull request as ready for review August 10, 2024 10:38
default = []

# User facing Rust bindings
"bindings" = ["miden-stdlib-sys"]
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can simply make the masl-lib parts of this use #[cfg(not(target_family = "wasm"))], rather than a Cargo feature. This keeps it out of the Wasm builds that would cause this to end up in code processed by the compiler, but otherwise keeps all of the code together.

In the future we may need to add a feature that allows those items to exist in a Wasm build as well (for building the compiler to run in the browser), but we're a way away from that point yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or vice versa, make the bindings conditional on #[cfg(target_family = "wasm")] - that might make more sense, since the bindings only are needed when targeting Miden.

@greenhat greenhat changed the title fix: add tx kernel library with stubs and link it on -l miden fix: add tx kernel library with stubs and link it on -l base Aug 13, 2024
@greenhat greenhat force-pushed the greenhat/tx-kernel-lib-stubs branch from 267c239 to 1fc039c Compare August 13, 2024 09:47
@greenhat
Copy link
Contributor Author

@bitwalker This PR is ready. The only test that fails is get_inputs.

Remove `miden-core` dependency from `midenc-tx-kernel` crate;
load the `CompiledLibrary` from the bytes.
Move the tx kernel to `masm/tx`.
…den-base-sys`

Add `bindings` and `masm-lib` features to the `miden-base-sys`
crate to separate the the user facing Rust bindings from the MASM library
needed only for the compiler.
@greenhat greenhat force-pushed the greenhat/tx-kernel-lib-stubs branch from 1fc039c to b9de0b5 Compare August 14, 2024 10:38
@greenhat
Copy link
Contributor Author

@bitwalker I've rebased the PR and it's ready.

@bitwalker bitwalker merged commit 98fb529 into next Aug 15, 2024
3 of 4 checks passed
@bitwalker bitwalker deleted the greenhat/tx-kernel-lib-stubs branch August 15, 2024 02:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants