Migrate to the VM `next` branch (v0.12.0) #1086

Fumuran · 2025-01-20T10:38:56Z

This PR updates the code of the miden-base to be compatible with the next version of the Miden VM.

Fumuran · 2025-01-20T11:43:41Z

During the migration I noticed that our libraries and folders with masm code stopped assembling with strange error: assembler can't find the modules which should be there. I found out that this error appeared after the Assembly: remove duplicate code and make recompute private PR was merged, on the version of the VM before this PR everything works fine. As far as I can see this may be related to the changes where recomputation of the ModuleGraph is happening, or to the way the nested folders are parsed: after I flattened shared folder the modules inside it have managed to compile.

@plafer, @bitwalker could you tell what could have caused this error?

PhilippGackstatter · 2025-01-20T12:54:35Z

To expand on what @Fumuran said our our kernel library is currently structured like this:

 miden-lib/asm/
├──  kernels
│   └──  transaction
│       ├── api.masm
│       ├── lib
│       │   ├── account.masm
│       │   ├── asset.masm
│       │   ├── asset_vault.masm
│       │   ├── constants.masm
│       │   ├── epilogue.masm
│       │   ├── faucet.masm
│       │   ├── memory.masm
│       │   ├── note.masm
│       │   ├── prologue.masm
│       │   └── tx.masm
│       └── main.masm
└──  shared
    └──  util
        ├── account_id.masm
        ├── asset.masm
        └── note.masm

We add the modules in shared to the kernel under the kernel namespace, so that in account.masm we can access them as kernel::util::account_id. We also add them to the miden library as miden::util, not pictured here, which is why they're shared.

In a first step, we assemble the kernel library which works fine. Once that is assembled, we instantiate another assembler with the kernel library to assemble main.masm (I'm not sure if this is actually necessary since assembling main.masm also works without the kernel lib passed as kernel to the assembler, but that's a separate point). The main point here is that, so far, we first added the kernel library (kernels/transaction/lib) with add_modules_from_dir and then the shared modules with add_modules_from_dir:

miden-base/miden-lib/build.rs

Lines 125 to 131 in d678c9e

    
           let assembler = build_assembler(Some(kernel_lib))?; 
        
           // assemble the kernel program and write it the "tx_kernel.masb" file 
        
           let mut main_assembler = assembler.clone(); 
        
           main_assembler.add_modules_from_dir(kernel_namespace.clone(), &source_dir.join("lib"))?; 
        
           // add the shared modules to the kernel lib under the kernel::util namespace 
        
           main_assembler.add_modules_from_dir(kernel_namespace, &shared_path)?;

This previously worked but no longer does after 0xPolygonMiden/miden-vm#1606. Swapping the order makes everything work and since it seems more logical to add the shared modules first anyway, given the dependence of the kernel lib on the shared modules, I pushed a commit that swaps them. Whether that's the intended behavior is a question for the VM team.

miden-lib/src/transaction/errors.rs

PhilippGackstatter · 2025-01-20T13:05:23Z

Cargo.toml

@@ -33,16 +33,16 @@ codegen-units = 1
 lto = true

 [workspace.dependencies]
-assembly = { package = "miden-assembly", version = "0.11", default-features = false }
+assembly = { package = "miden-assembly", git = "https://github.com/0xPolygonMiden/miden-vm.git", branch = "next", default-features = false }


Just to note: We should either pin the dependencies to a specific commit on next to avoid breakage if next is updated, or merge #1059 shortly after this one (which points to the element-addressable-memory branch).

I hope that we will be able to merge the element-addressable memory PR right after this one, but I agree that it makes sense to pin the specific commit

PhilippGackstatter · 2025-01-20T16:29:32Z

miden-tx/src/host/mod.rs

-    fn set_advice<S: ProcessState>(
-        &mut self,
-        process: &S,
-        injector: AdviceInjector,
-    ) -> Result<HostResponse, ExecutionError> {
-        match injector {
-            AdviceInjector::SigToStack { .. } => self.on_signature_requested(process),
-            injector => self.adv_provider.set_advice(process, &injector),
-        }


We had to remove the set_advice and get_advice implementations because they no longer exist on the AdviceProvider trait. We previously used this to call on_signature_requested when the SigToStack injector (now SystemEvent) was provided (e.g. which is what adv.push_sig.rpo_falcon512 results in, like in rpo_falcon512::verify (https://github.com/0xPolygonMiden/miden-vm/blob/b13ed4d67c13d99d750d7b91293a91b0185cb84d/stdlib/asm/crypto/dsa/rpo_falcon512.masm#L599)).

This is failing now in a bunch of tests with AdviceMapKeyNotFound. I assume it's this part where it originates: https://github.com/0xPolygonMiden/miden-vm/blob/8420dcd37078c605f90c87d233eb964ae3468fc9/processor/src/operations/sys_ops/sys_event_handlers.rs#L650-L652.

As I understand it, Miden VM previously delegated signature creation to the AdviceProvider / Host whereas now, in the above code, it requires the public key/secret key key-value pair to be present in the advice map.

We have an account component rpo_falcon512 which users may or may not add to their account code (https://github.com/0xPolygonMiden/miden-base/blob/next/miden-lib/asm/account_components/rpo_falcon_512.masm). If they do, we need to be able to inject a signature into the advice stack. These components are generic and so we likely shouldn't add code in miden-base which injects the public-secret key-pair since that would couple us to this component and wouldn't work for other user-written components.

If the above is true, I'm not sure how we should handle this change. The above code requires the secret key to be mapped into the advice provider. I don't think it is a good idea to expose a secret key to so many parts of code. Typically one would want to minimize exposure of the key to make the attack surface as minimal as possible. In a practical scenario, secret keys will be stored in key stores or secure hardware modules where they won't be accessible, so users need to be able to compute signatures themselves in an impl TransactionAuthenticator, which is invoked by TransactionExecutor to generate a signature, which in turn needs to be able to injected into the advice map.

Not sure if I'm missing something or if that is still possible, but that's my current understanding.

cc @plafer

I think the core of the issue is that we have FalconSigToStack event classified as a system even, and thus it is not "overridable", but we do need to override it in this case. There is an issue in miden-vm to make handling of events more customizable and to reclassify a number of these events as "library" rather than system events. But that'll take some time to implement.

One stop-gap solution could be to just make an exception for the FalconSigToStack event. Then, the default host would call the current method for generating the signature (which relies on the private key being in the advice provider), but our transaction host could do something else for it. I'll make a quick PR with this solution.

I implemented the stop-gap solution in 0xPolygonMiden/miden-vm#1630. @Fumuran - could you try it out and let me know if it works?

bobbinth

Looks good! Thank you. I left a couple of comments inline.

miden-lib/src/transaction/mod.rs

miden-tx/src/host/mod.rs

bobbinth

Looks good! Thank you! I left just one optional comment inline.

@Fumuran - could you update #1084 to work from this branch and make sure everything is working there?

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

miden-lib/build.rs

    // add the shared modules to the kernel lib under the kernel::util namespace
-    main_assembler.add_modules_from_dir(kernel_namespace, &shared_path)?;
+    main_assembler.add_modules_from_dir(kernel_namespace.clone(), &shared_path)?;
+    main_assembler.add_modules_from_dir(kernel_namespace, &source_dir.join("lib"))?;


Should we add a comment here to say that the order is important?

Fumuran · 2025-01-21T09:21:02Z

@bobbinth This PR works on your bobbin-falcon-sig-event branch, so should it be merged first? After that I'll be able to update the element-addressable-memory branch in the VM to make sure that the corresponding branch in the base is working

bobbinth · 2025-01-21T09:23:15Z

@bobbinth This PR works on your bobbin-falcon-sig-event branch, so should it be merged first? After that I'll be able to update the element-addressable-memory branch in the VM to make sure that the corresponding branch in the base is working

Let's not merge anything yet - but you could update #1084 to be based on this PR, no?

Fumuran · 2025-01-21T10:42:52Z

#1084 is updated, although P2ID and P2IDR tests are still failing, because there is no fix of the system event in the element-addressable-memory VM branch. But everything else is working.

bobbinth · 2025-01-21T10:45:07Z

#1084 is updated, although P2ID and P2IDR tests are still failing, because there is no fix of the system event in the element-addressable-memory VM branch. But everything else is working.

Is there a reason to keep #1084 based on next rather than on this branch?

Fumuran · 2025-01-21T10:51:17Z

Ah, my bad, I forgot to do so after rebase. Updated the base branch to be andrew-migrate-to-vm-next.

bobbinth · 2025-01-21T10:53:12Z

Ah, my bad, I forgot to do so after rebase. Updated the base branch to be andrew-migrate-to-vm-next.

Now all the test should be passing, right?

plafer · 2025-01-21T12:52:01Z

Swapping the order makes everything work and since it seems more logical to add the shared modules first anyway, given the dependence of the kernel lib on the shared modules, I pushed a commit that swaps them. Whether that's the intended behavior is a question for the VM team.

I don't think it was an explicit goal but since the fix is simple, I would leave things as-is.

refactor: migrate to the VM next branch

c7844de

chore: Add shared modules before adding kernel lib

f52c818

Fumuran marked this pull request as ready for review January 20, 2025 12:58

Fumuran requested review from PhilippGackstatter and bobbinth January 20, 2025 12:59

Fumuran added the no changelog This PR does not require an entry in the `CHANGELOG.md` file label Jan 20, 2025

PhilippGackstatter reviewed Jan 20, 2025

View reviewed changes

refactor: update dependencies, fix formatting

6b40798

PhilippGackstatter reviewed Jan 20, 2025

View reviewed changes

bobbinth mentioned this pull request Jan 20, 2025

Move handling of FalconSigToStack event to the Host 0xPolygonMiden/miden-vm#1630

Merged

feat: add handling of the FalconSigToStack event to the Host

20c39ad

bobbinth reviewed Jan 21, 2025

View reviewed changes

miden-lib/src/transaction/mod.rs Outdated Show resolved Hide resolved

miden-tx/src/host/mod.rs Outdated Show resolved Hide resolved

PhilippGackstatter added 2 commits January 21, 2025 09:23

feat: Make EVENT_ID_PREFIX an associated constant

2b2c346

chore: Return error if event is an unexpected system event

c4dc749

PhilippGackstatter approved these changes Jan 21, 2025

View reviewed changes

bobbinth approved these changes Jan 21, 2025

View reviewed changes

chore: pointed miden-vm dependencies back to next

3af1b8c

bobbinth merged commit a7c7efe into next Jan 21, 2025
12 checks passed

bobbinth deleted the andrew-migrate-to-vm-next branch January 21, 2025 17:56

plafer mentioned this pull request Jan 29, 2025

Introduce a mechanism of libraries to define event handlers 0xPolygonMiden/miden-vm#1584

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate to the VM `next` branch (v0.12.0) #1086

Migrate to the VM `next` branch (v0.12.0) #1086

Fumuran commented Jan 20, 2025

Fumuran commented Jan 20, 2025 •

edited

Loading

PhilippGackstatter commented Jan 20, 2025 •

edited

Loading

PhilippGackstatter Jan 20, 2025

Fumuran Jan 20, 2025

PhilippGackstatter Jan 20, 2025

bobbinth Jan 20, 2025

bobbinth Jan 20, 2025

bobbinth left a comment

bobbinth left a comment

bobbinth Jan 21, 2025

Fumuran commented Jan 21, 2025

bobbinth commented Jan 21, 2025

Fumuran commented Jan 21, 2025

bobbinth commented Jan 21, 2025

Fumuran commented Jan 21, 2025

bobbinth commented Jan 21, 2025

plafer commented Jan 21, 2025

Migrate to the VM next branch (v0.12.0) #1086

Migrate to the VM next branch (v0.12.0) #1086

Conversation

Fumuran commented Jan 20, 2025

Fumuran commented Jan 20, 2025 • edited Loading

PhilippGackstatter commented Jan 20, 2025 • edited Loading

PhilippGackstatter Jan 20, 2025

Choose a reason for hiding this comment

Fumuran Jan 20, 2025

Choose a reason for hiding this comment

PhilippGackstatter Jan 20, 2025

Choose a reason for hiding this comment

bobbinth Jan 20, 2025

Choose a reason for hiding this comment

bobbinth Jan 20, 2025

Choose a reason for hiding this comment

bobbinth left a comment

Choose a reason for hiding this comment

bobbinth left a comment

Choose a reason for hiding this comment

bobbinth Jan 21, 2025

Choose a reason for hiding this comment

Fumuran commented Jan 21, 2025

bobbinth commented Jan 21, 2025

Fumuran commented Jan 21, 2025

bobbinth commented Jan 21, 2025

Fumuran commented Jan 21, 2025

bobbinth commented Jan 21, 2025

plafer commented Jan 21, 2025

Migrate to the VM `next` branch (v0.12.0) #1086

Migrate to the VM `next` branch (v0.12.0) #1086

Fumuran commented Jan 20, 2025 •

edited

Loading

PhilippGackstatter commented Jan 20, 2025 •

edited

Loading