Enable libraries to define event handlers #1647

plafer · 2025-01-30T21:55:11Z

Closes #1584

Also moves the Falcon signature event handling to the standard library.

This is a rough implementation as discussed in #1584, which still requires some cleanup

bobbinth

Looks good! Thank you! Not a review yet, but I left some comments inline.

processor/src/host/event_handling.rs

bobbinth · 2025-01-31T00:50:41Z

stdlib/asm/crypto/dsa/rpo_falcon512.masm

@@ -7,6 +7,8 @@ const.J=77321994752
 const.M=12289
 const.SQUARE_NORM_BOUND=34034726

+const.EVENT_FALCON_SIG_TO_STACK=3419226139


How did we choose this value?

It was chosen randomly, like the other system events. This basically makes collisions with user-defined events improbable.

processor/src/host/mod.rs

when there's already a registered event handler to that id

bobbinth

Left a few small comments after taking a quick look.

core/src/advice/inputs.rs

core/src/advice/mod.rs

core/src/advice/source.rs

stdlib/tests/crypto/falcon.rs

plafer · 2025-02-03T19:24:09Z

The biggest issue with libraries not having to depend on miden-processor is in moving the EventHandler trait, specifically because it uses ProcessState, which is inherently a processor type.

I explored creating an analogous ProcessState trait in core, but ultimately things get messy when we combine Box<dyn EventHandler>, the lifetime parameter of ProcessState, and the fact that traits used with dyn cannot have methods with a generic P: ProcessState parameter. My current conclusion is that the "cleanest" way to move it to core is to make ProcessState trait again, effectively undoing the work done in #1571. The biggest downside of this solution is that we then need to use a generic P: ProcessState everywhere like we had before, which IMO is much less clean that what we have today. And actually it's a bit worst than that, since EventHandler::on_event cannot be generic, so we need to make trait EventHandler<P> generic, and that generic parameter propagates to make a bunch of other traits/types generic over P as well.

So if my understanding is correct, we have to choose between the lesser of 2 evils:

if libraries want to define event handlers, then they have to depend on miden-processor
have ProcessState be a trait again, and all the messiness described above.

Unless I'm missing something, I feel like option (1) is cleaner, especially since not all libraries will need to define event handlers. This also means we can avoid moving all the advice provider types to core if we don't want to.

cc @bobbinth

bobbinth · 2025-02-04T09:17:02Z

So if my understanding is correct, we have to choose between the lesser of 2 evils:

if libraries want to define event handlers, then they have to depend on miden-processor

have ProcessState be a trait again, and all the messiness described above.

Yes, agreed that option 1 is the way to go (for this PR at least). And I actually don't think it is all that "evil". Yes, would have been nice to move this into the core, but since event handlers need to know about processor-specific things, makes sense that stdlib and other similar crates need to depend on the processor.

We could re-evaluate bringing back the ProcessState struct in the future - though, as you mention, this results in another host of issues (including the need for runtime borrow_mut()). Maybe there is a clean way to handle them, but definitely not worth investigating as a part of this PR.

plafer · 2025-02-04T17:10:36Z

Yes, would have been nice to move this into the core, but since event handlers need to know about processor-specific things, makes sense that stdlib and other similar crates need to depend on the processor.

Agreed.

We could re-evaluate bringing back the ProcessState struct in the future

Actually, since we allow stdlib to depend on miden-processor, then no change is needed to ProcessState - as it is right now on this branch.

And therefore the PR is ready for final review.

bobbinth

Looks good! Thank you! Didn't get to do a full review yet - but I left some comments inline.

bobbinth · 2025-02-04T23:27:31Z

test-utils/Cargo.toml

@@ -32,6 +32,7 @@ processor = { package = "miden-processor", path = "../processor", version = "0.1
    "testing",
 ] }
 prover = { package = "miden-prover", path = "../prover", version = "0.13", default-features = false }
+stdlib = { package = "miden-stdlib", path = "../stdlib", version = "0.13", default-features = false }


Maybe not for this PR, but is there a way not to make test-utils dependent on stdlib? Maybe this could be done by adding something like host_libraries: Vec<dyn HostLibrary> field to the Test struct?

Unfortunately, not really, since HostLibrary is not trait object safe (due to get_event_handlers() being generic)

bobbinth · 2025-02-04T23:30:06Z

miden/tests/integration/operations/sys_ops.rs

@@ -79,6 +79,6 @@ fn assert_eq_fail() {

 #[test]
 fn emit() {


Question: why did we need to change this?

I just changed the event that is emitted to be 1 instead of 42, since I installed a NoopEventHandler to event 1 by default in Test::execute(), but not 42

bobbinth · 2025-02-04T23:31:34Z

stdlib/src/lib.rs

+use processor::{EventHandler, HostLibrary, ProcessState};
+use vm_core::{AdviceProvider, AdviceProviderError, AdviceSource, Felt, Word};


AdviceProvider, AdviceProviderError, AdviceSource, Felt, Word should all be re-exported from the processor, right? I'd probably import them from there and then get rid of the vm_core dependency.

stdlib/src/lib.rs

bobbinth · 2025-02-05T10:25:24Z

stdlib/src/lib.rs

+/// A trait for signing messages using the Falcon signature scheme.
+///
+/// This trait is used by [FalconSigToStackEventHandler] to sign messages using the Falcon signature
+/// scheme.
+///
+/// It is recommended to use [dsa::falcon_sign] to implement this trait once the private key has
+/// been fetched from a user-defined location.
+pub trait FalconSigner<A>: Send + Sync {
+    /// Signs the message using the Falcon signature scheme, and returns the signature as a
+    /// `Vec<Felt>`.
+    fn sign_message(
+        &self,
+        pub_key: Word,
+        msg: Word,
+        advice_provider: &A,
+    ) -> Result<Vec<Felt>, Box<dyn Error + Send + Sync + 'static>>
+    where
+        A: AdviceProvider;
+}


We are including the advice provider into the interface so that we could read the private key from it, right? I'm wondering if we could avoid this if we convert this trait into a closure - e.g., something like:

type FalconSigner = Box<dyn Fn(Word, Word) -> Vec<Felt>>;

And then in the FalconSigToStackEventHandler::on_event() we'd just invoke this closure capturing the private key we get from the advice provider.

But this would force implementations to getting the private key from the advice provider, which is what we're trying to avoid (e.g. in miden-base we get it from somewhere else).

I also abstracted away the signing part to give the option to e.g. sign with a hardware device - and exposed dsa::falcon_sign() for convenience otherwise.

But this would force implementations to getting the private key from the advice provider,

No, the idea here is for the closure to take only 2 parameters: a public key and a message (the two words). The private key would be captured by the closure from the context. We are doing something similar in the client - though there, it is a bit more complex because the closures need to be async.

bobbinth · 2025-02-05T10:37:02Z

prover/src/lib.rs

 pub use processor::{
-    crypto, math, utils, AdviceInputs, Digest, ExecutionError, Host, InputError, MemAdviceProvider,
-    StackInputs, StackOutputs, Word,
+    crypto, math, utils, Digest, ExecutionError, Host, InputError, MemAdviceProvider, StackInputs,
+    StackOutputs, Word,
 };


I would still re-export AdviceInputs from here.

Now `StdLibrary` is generic over `Inputs`.

plafer added 4 commits January 30, 2025 11:38

chore: make MemMastForestStore Send + Sync

7c43884

chore: make DefaultHost generic over trace and debug handling

92822df

feat: add event handling infrastructure to DefaultHost

3461ec5

chore(assembly): remove adv.push_sig

a8b877d

bobbinth reviewed Jan 31, 2025

View reviewed changes

plafer added 4 commits January 31, 2025 09:49

feat: move falcon signature handling to stdlib

d3ed6c4

chore: move most AdviceProvider types to core

8a2cb5a

chore: merge DefaultTraceHandler and DefaultDebugHandler

58f2385

chore: make register_event_handler() return an error

608bc41

when there's already a registered event handler to that id

plafer force-pushed the plafer-1584-event-handlers branch from 4c2a284 to 608bc41 Compare January 31, 2025 19:14

bobbinth reviewed Jan 31, 2025

View reviewed changes

core/src/advice/inputs.rs Outdated Show resolved Hide resolved

core/src/advice/mod.rs Outdated Show resolved Hide resolved

core/src/advice/source.rs Outdated Show resolved Hide resolved

stdlib/tests/crypto/falcon.rs Outdated Show resolved Hide resolved

plafer added 8 commits February 3, 2025 15:21

fix tests

f448840

PR reviews

1716652

address TODOs

4b27efd

Merge branch 'next' into plafer-1584-event-handlers

745293e

changelog

5ae9926

fix no-std

cefc26e

fix StdLibrary Falcon signing interface

07ddff8

fix docs

6c0ed5f

plafer marked this pull request as ready for review February 3, 2025 21:41

fix docs

17c4c49

plafer mentioned this pull request Feb 4, 2025

Move hardcoded advice injectors to stdlib #1652

Open

bobbinth reviewed Feb 5, 2025

View reviewed changes

address PR feedback

cbd8dd9

plafer requested a review from bobbinth February 5, 2025 12:22

plafer added 2 commits February 6, 2025 10:35

fix: fix DefaultHost constructors

8b9ef7e

fix: make FalconSigner::sign_message(&mut self)

22d8303

fix: Make HostLibrary generic over Inputs

9c62b2f

plafer mentioned this pull request Feb 6, 2025

Update with latest event handlers PR (miden-vm) 0xPolygonMiden/miden-base#1127

Draft

plafer added 4 commits February 7, 2025 09:41

fix: FalconSigner is no longer generic over AdviceProvider

5e92370

fix: fix StdLibrary constructors

4517759

Now `StdLibrary` is generic over `Inputs`.

fix: expose EventHandlerRegistry

f903fcb

address TODO

2a2677a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable libraries to define event handlers #1647

Enable libraries to define event handlers #1647

plafer commented Jan 30, 2025 •

edited

Loading

bobbinth left a comment

bobbinth Jan 31, 2025

plafer Jan 31, 2025

bobbinth left a comment

plafer commented Feb 3, 2025

bobbinth commented Feb 4, 2025

plafer commented Feb 4, 2025

bobbinth left a comment

bobbinth Feb 4, 2025 •

edited

Loading

plafer Feb 5, 2025

bobbinth Feb 4, 2025

plafer Feb 5, 2025

bobbinth Feb 4, 2025

plafer Feb 5, 2025

bobbinth Feb 5, 2025

plafer Feb 5, 2025

bobbinth Feb 5, 2025

bobbinth Feb 5, 2025

plafer Feb 5, 2025

		use processor::{EventHandler, HostLibrary, ProcessState};
		use vm_core::{AdviceProvider, AdviceProviderError, AdviceSource, Felt, Word};

Enable libraries to define event handlers #1647

Are you sure you want to change the base?

Enable libraries to define event handlers #1647

Conversation

plafer commented Jan 30, 2025 • edited Loading

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

bobbinth left a comment

Choose a reason for hiding this comment

plafer commented Feb 3, 2025

bobbinth commented Feb 4, 2025

plafer commented Feb 4, 2025

bobbinth left a comment

Choose a reason for hiding this comment

bobbinth Feb 4, 2025 • 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

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

plafer commented Jan 30, 2025 •

edited

Loading

bobbinth Feb 4, 2025 •

edited

Loading