Stateful execution checker #1269

Fumuran · 2025-04-03T14:26:48Z

This PR implements a new procedure for the TransactionExecutor which will check the ability for the transaction and the provided notes to be successfully executed (and consumed).

This new procedure utilizes the same events which is used by the benchmark to keep track of the executed and failing notes.

For the well known notes (P2ID, P2IDR and SWAP) we additionally check that the account ID provided as the note input matches the ID of the target account.

TODO:

Write the docs about how to use the checker.

Corresponding issue: #831

PhilippGackstatter

Looks good to me!

Generally, I think this PR implements a middle-ground between checking well known notes manually and attempting notes execution. I think we should lean in one or the other direction.

I think the manual check for well known notes adds maintenance burden to keep the logic of the those notes in sync between MASM and this checker. Right now, we don't gain much because we still have to execute the well known notes regardless of the manual check. Basically, the check, afaiu, only avoids execution if we can already tell from the manual check that they won't succeed, so only the unhappy path is faster.

So overall I think we have two options:

Check the well known notes fully manually, so that we don't have to execute them in the VM. This justifies the maintenance burden more because we don't have to execute well known notes.
Remove the special well-known note checks and always execute all notes.

My preference is more the latter, because I prefer general solutions over specific ones and due to the well known note checks that we have to keep in sync not only in the checker, but we'd ideally also have to make sure this is tested so that if they fall out of sync we're somehow notified. I guess my main question is, with the fast VM processor, is always executing a big slowdown?

Pinging @bobbinth because I think he might lean more towards option 1.

PhilippGackstatter · 2025-04-12T21:00:11Z

crates/miden-lib/asm/kernels/transaction/main.masm

-    # use `push.* drop` instructions before `trace` to make sure that MAST root will be unique
-    push.91683607 drop


Let's also close #1094 once this PR is merged.

crates/miden-tx/src/host/tx_progress.rs

crates/miden-tx/src/executor/mod.rs

PhilippGackstatter · 2025-04-12T23:34:53Z

crates/miden-tx/src/executor/notes_checker.rs

+    /// provided input notes. If so, assert that the note inputs are correct.
+    ///
+    /// Returns [NoteAccountCompatibility::No] if at least one note has incorrect inputs.
+    pub fn check_note_inputs(&self) -> (NoteAccountCompatibility, Option<NoteId>) {


Does this have to be a public API, like is it used in the client or somewhere? If not, then I would make it private.

One way or another, it would be nice to encode this guarantee that if NoteAccountCompatibility::No then the Option is Some, None otherwise, because that is not clear without looking at the actual code.

The easiest is to just return ExecutionCheckResult directly from this function, but it's not really correct to return Success variant from here, because we can't really say that it's a success just yet.

Maybe we can just wrap this in a struct like:

enum NoteInputsCheck { Maybe, No { failed_note_id: NoteId } }

and then convert this back into a NoteAccountCompatibility in check_notes_consumability.

Does this have to be a public API, like is it used in the client or somewhere? If not, then I would make it private.

I assumed that potentially the user may want to run this light check only, without executing the scripts, so I left this to be public. But not sure how helpful it would be, especially assuming your head comment (maybe I'll even remove this check entirely).

crates/miden-tx/src/executor/notes_checker.rs

Fumuran · 2025-04-14T10:00:42Z

@PhilippGackstatter Thank you for the comprehensive review!

Check the well known notes fully manually, so that we don't have to execute them in the VM. This justifies the maintenance burden more because we don't have to execute well known notes.

This approach will work only for P2ID and P2IDR notes, but not for the SWAP note. To perform a static check for it we have to have the output notes (to make sure that the income + current balance >= expense), which will be constructed only after transaction execution (see 0xMiden/miden-client#470 (comment)). So probably the second option is the only one which allows us to make sure that all notes will be consumed.

PhilippGackstatter

Thank you for the updates!

This approach will work only for P2ID and P2IDR notes, but not for the SWAP note.

Ah, that's right. The SWAP note has dependencies on the rest of the transactions, so it cannot be looked at in isolation.

This to me suggests that we should always execute SWAP notes and avoid implementing any checks in Rust on it, mainly because I think:

the note inputs check in itself is not very useful, because there is no incentive for anyone to create a note of any type whose note inputs are incorrect, because that note would never be consumable anyway. And so we will still have to always execute such checked notes in the success case, and it should be very rare to run into the "note inputs incorrect" case.
adding a check if there is enough of the requested asset in the account is not 100% safe either, because of the above-mentioned dependencies. If we consume a SWAP note where the requested asset amount is 100, and the account initially has just 50, then we'd fail this check. But there could be a P2ID note executed before the SWAP note that adds 50 to the account, which would make it succeed. So this would result in a false negative, i.e. the check would say it cannot be consumed, but it can in fact be consumed.

For P2ID(R) notes, it may still make sense to have the full logic implemented in Rust, because:

While we can only skip execution if there are only P2ID(R) notes in the transaction (or any other future note types that we can implement static checks for), this should still happen quite frequently in practice. For example, if I send an asset to someone else, I will most likely do it using a P2* variant and the receiver will likely claim it in a single transaction where the only input is that note. In those cases, we can completely skip execution, which should result in a significant enough performance benefit to be worth the reimplementation of the logic.

Just to note it: If there is a mix of P2ID(R) notes and other note types in the transaction, we will end up executing in any case in the happy path. In the unhappy path, we will potentially return early before execution, if some of the P2ID(R) logic fails.

So to summarize, my suggestion is to:

Remove any Rust checks for SWAP notes and always execute them.
Implement the full P2ID(R) note logic in Rust to skip execution entirely, if the transaction consumes notes of only that type.

PhilippGackstatter · 2025-04-16T10:31:46Z

crates/miden-tx/src/executor/notes_checker.rs

+    /// Checks whether there are "well known" notes (`P2ID`, `P2IDR` and `SWAP`) in the list of the
+    /// provided input notes. If so, assert that the note inputs are correct.
+    ///
+    /// Returns [NoteAccountCompatibility::No] if at least one note has incorrect inputs.


Looking at this again, I wonder if this should have the same behavior as check_notes_consumability, i.e. return NoteInputsCheck::No with a list of notes that were fine up until that point? I think that'd be more consistent overall, so that the NotesChecker uses a best-effort strategy on all checks.

(This applies only if we keep this in the first place).

PhilippGackstatter · 2025-04-16T10:39:37Z

crates/miden-tx/src/tests/mod.rs

+    assert!(if let NoteAccountExecution::Failure {
+        failed_note_id,
+        successful_notes,
+        error: Some(e),
+    } = execution_check_result
+    {
+        assert_eq!(failed_note_id, failing_note_2.id());
+        assert_eq!(successful_notes, vec![input_note_ids[0]]);
+        assert!(matches!(e, TransactionExecutorError::TransactionProgramExecutionFailed { .. }));
+        true
+    } else {
+        false
+    });


Nit: This could be written a bit more concisely with assert_matches! but it's also fine as-is.

PhilippGackstatter · 2025-04-16T10:42:27Z

crates/miden-tx/src/executor/mod.rs

+    /// - If the transaction host can not be created from the provided values.
+    /// - If the execution of the provided program fails on the stage other than note execution.
+    #[maybe_async]
+    pub(crate) fn try_notes_execution(


As far as I understand, there is no way for us to drive the VM in a stateful way, right? What I mean is this:

Even more sophisticated approach (but not sure how much complexity it would add), would be create a "stateful" executor such that we could execute notes incrementally. That is, execute note a first, then execute note b, if execution of note b fails, we could execute note c, and if that works, we'd know that executing notes a and c together will work fine.
#831 (comment)

If b fails, we'd have to continue the execution from the state after a was executed but using c. This would require new VM APIs I guess.

This would require new VM APIs I guess.

Yes, for now it won't really work. We should be able to somehow store and load VM state to be able to execute notes one by one: store the state after prologue, then store it after each successful note execution and load the previous state if the execution failed. Not sure though how this can be implemented

…consumability

Fumuran · 2025-04-17T20:32:17Z

Probably the only big thing left is documentation. Not sure though where to place: we check notes against some account, so I can put the paragraph in the Account section or in the Note section of the Protocol documentation, but Note probably would be better.

Also not sure how can we put it: AFAICS we use an upper-level descriptions in the docs, but this check (both stateless and stateful) involves understanding of how it is implemented. Should we just mention that consumability of the input notes against some account could be checked before the execution, without going into the details of how exactly it could be done?

bobbinth

Looks good! Thank you. I left a few comments inline. I think the biggest thing is to update this PR once #1229 is merged.

So overall I think we have two options:

Check the well known notes fully manually, so that we don't have to execute them in the VM. This justifies the maintenance burden more because we don't have to execute well known notes.

Remove the special well-known note checks and always execute all notes.

My preference is more the latter, because I prefer general solutions over specific ones and due to the well known note checks that we have to keep in sync not only in the checker, but we'd ideally also have to make sure this is tested so that if they fall out of sync we're somehow notified. I guess my main question is, with the fast VM processor, is always executing a big slowdown?

I'd probably keep the fast path for now. I'd like to see if we can evolve to be more modular/customizable - but if that doesn't work out, we can scrap it.

Probably the only big thing left is documentation. Not sure though where to place: we check notes against some account, so I can put the paragraph in the Account section or in the Note section of the Protocol documentation, but Note probably would be better.

I'm actually not sure we need to document it in mdBook. Let's skip this for now.

crates/miden-lib/src/note/well_known_note.rs

crates/miden-tx/src/executor/mod.rs

crates/miden-tx/src/executor/notes_checker.rs

…bility

PhilippGackstatter

Looks good to me!

The main comment is the one about the account ID validity which I think we should address. The rest is optional.

PhilippGackstatter · 2025-04-24T10:38:45Z

crates/miden-lib/src/note/well_known_note.rs

                }

-                Self::check_input_account_id(note_inputs, target_account_id)
+                let recall_hight: Result<u32, _> = note_inputs[2].try_into();


Suggested change

let recall_hight: Result<u32, _> = note_inputs[2].try_into();

let recall_height: Result<u32, _> = note_inputs[2].try_into();

I think this still needs to be addressed.

crates/miden-lib/src/note/well_known_note.rs

PhilippGackstatter · 2025-04-24T10:55:43Z

crates/miden-lib/src/note/well_known_note.rs

+                match recall_hight {
+                    // invalid input value: the block number does not fit in u32
+                    Err(_) => NoteAccountCompatibility::No,
+                    Ok(recall_hight) => {


This could be slightly easier to read with a let-else:

let Ok(recall_height) = recall_height else { return NoteAccountCompatibility::No; }; // ...

PhilippGackstatter · 2025-04-24T10:59:12Z

crates/miden-lib/src/note/well_known_note.rs

        let inputs_account_id = AccountId::try_from([account_id_felts[1], account_id_felts[0]])
            .expect("invalid account ID felts");


I don't think we can assume that the felts are valid account ID felts. By accident or malice, a note creator could put incorrect values into the note inputs. So in the error case we should return NoteAccountCompatibility::No instead of panicking.

PhilippGackstatter · 2025-04-24T12:24:48Z

crates/miden-tx/src/executor/notes_checker.rs

+/// The check is performed using the [NoteConsumptionChecker::check_notes_consumability] procedure.
+/// Essentially runs the transaction to make sure that provided input notes could be consumed by the
+/// account.
+pub struct NoteConsumptionChecker<'a>(pub &'a TransactionExecutor);


Should we make the field private and add a constructor instead?

crates/miden-tx/src/executor/notes_checker.rs

bobbinth

Looks good! Thank you! I left a few small comments inline - but it also seems like several of @PhilippGackstatter's comments still need to be addressed.

crates/miden-lib/src/note/well_known_note.rs

bobbinth · 2025-04-26T04:21:18Z

crates/miden-lib/src/note/well_known_note.rs

                }

-                Self::check_input_account_id(note_inputs, target_account_id)
+                let recall_hight: Result<u32, _> = note_inputs[2].try_into();


I think this still needs to be addressed.

crates/miden-tx/src/executor/mod.rs

bobbinth

Thank you! I left a few more comments inline.

crates/miden-lib/src/note/well_known_note.rs

crates/miden-tx/src/executor/notes_checker.rs

PhilippGackstatter · 2025-04-28T08:59:49Z

crates/miden-tx/src/executor/notes_checker.rs

+                    // we need to execute the transaction if compatibility is unclear
+                    NoteAccountCompatibility::Maybe => break,


WellKnownNote::check_note_inputs is guaranteed to return Yes or No in this scenario because we never run it on a SWAP note.

However, I don't know if it's a good idea to turn this into an unreachable!, because we might add more well-known notes in the future and then this subtle assumption could break, unless we add a continue for notes that could return Maybe like we have for SWAP now.

As of right now, we should never run into this case though. Defensively, I would change the logic so that if it happens we continue instead of break. Since we do not add this note to the successful notes, the length check later will fail and so we will execute in the VM anyway, right? That way, we do not abort too early if there are still P2ID(R) notes to be checked and it should be correct albeit not optimal, even if we forget to update the code when a new well known note is added in the future.

Would that work? If so, let's add an explanation of this as a comment in code.

I would change the logic so that if it happens we continue instead of break

Right, I agree, I just missed this break when I was refactoring. In that case this should be equivalent to the
if let WellKnownNote::SWAP = well_known_note { continue }; branch, which leads to the tx execution.

Would that work? If so, let's add an explanation of this as a comment in code.

Yep, I'll add the explanation

…ction

bobbinth

Looks good! Thank you! I added a couple of minor comments inline. Once these are addressed, we are good to merge.

crates/miden-lib/src/note/well_known_note.rs

Fumuran force-pushed the andrew-statefull-check branch from b86dd5f to 11a6537 Compare April 4, 2025 18:29

Fumuran marked this pull request as ready for review April 4, 2025 18:43

Fumuran requested review from PhilippGackstatter and bobbinth April 4, 2025 18:43

Fumuran force-pushed the andrew-statefull-check branch from 446cf71 to 2e22184 Compare April 11, 2025 10:06

PhilippGackstatter reviewed Apr 12, 2025

View reviewed changes

Fumuran added 9 commits April 15, 2025 00:14

feat: initial working state

c811ac0

refactor: change traces to emits

e44f599

refactor: update checker returning value

6164c52

test: slightly improve the test

59d2922

chore: update changelog and comments

2d892c2

refactor: lift requirement for Account in check_note_inputs

c2c0fac

feat: impl NotesChecker and move check procs there

2b9f5f4

chore: export NoteChecker

9a6b0e6

refactor: update names, add docs, add error to NoteAccountExecution

4ab2230

Fumuran force-pushed the andrew-statefull-check branch from 2e22184 to 4ab2230 Compare April 14, 2025 21:16

docs: update doc comments

2b5bbf5

Fumuran requested a review from PhilippGackstatter April 15, 2025 12:36

PhilippGackstatter reviewed Apr 16, 2025

View reviewed changes

Fumuran added 2 commits April 17, 2025 22:44

refactor: remove check_note_inputs proc, update logic of check_notes_…

63e6229

…consumability

chore: update doc comments

c3a7817

Fumuran force-pushed the andrew-statefull-check branch from bb65a68 to c3a7817 Compare April 17, 2025 20:15

Fumuran requested a review from PhilippGackstatter April 17, 2025 20:32

bobbinth reviewed Apr 20, 2025

View reviewed changes

Fumuran added the no docs label Apr 20, 2025

Fumuran added 2 commits April 22, 2025 13:57

refactor: add recall block hight check, add Yes to NoteAccountCompati…

d060511

…bility

Merge branch 'next' into andrew-statefull-check

44c316c

Fumuran requested a review from bobbinth April 22, 2025 16:01

chore: fix diff check, fix typo

864b04a

Fumuran force-pushed the andrew-statefull-check branch from abf43bc to 864b04a Compare April 22, 2025 20:06

PhilippGackstatter approved these changes Apr 24, 2025

View reviewed changes

bobbinth reviewed Apr 26, 2025

View reviewed changes

Fumuran added 3 commits April 26, 2025 21:36

refactor: update note checking logic, small improvements

6c275ed

Merge branch 'next' into andrew-statefull-check

91fc158

chore: fix diff check

4416a62

Fumuran requested a review from bobbinth April 26, 2025 19:18

bobbinth reviewed Apr 26, 2025

View reviewed changes

crates/miden-lib/src/note/well_known_note.rs Outdated Show resolved Hide resolved

crates/miden-lib/src/note/well_known_note.rs Outdated Show resolved Hide resolved

crates/miden-tx/src/executor/notes_checker.rs Show resolved Hide resolved

PhilippGackstatter reviewed Apr 28, 2025

View reviewed changes

refactor: update P2IDR check logic, rework check_input_account_id fun…

fd6c1c1

…ction

Fumuran requested a review from bobbinth May 3, 2025 21:15

bobbinth approved these changes May 3, 2025

View reviewed changes

crates/miden-lib/src/note/well_known_note.rs Outdated Show resolved Hide resolved

crates/miden-lib/src/note/well_known_note.rs Outdated Show resolved Hide resolved

refactor: add constants for note input nums, update doc comments

e0963f9

bobbinth merged commit 3ffa3ac into next May 5, 2025
16 checks passed

bobbinth deleted the andrew-statefull-check branch May 5, 2025 19:30

bobbinth mentioned this pull request May 5, 2025

Implement ability to determine if a note/tx script cannot be executed against a given account #831

Closed

tomyrd mentioned this pull request May 6, 2025

Enhance Note Consumption: Skip Invalid Notes and Consume Valid Ones from the List 0xMiden/miden-client#679

Closed

bobbinth mentioned this pull request May 19, 2025

trace events handling #1094

Closed

		# use `push.* drop` instructions before `trace` to make sure that MAST root will be unique
		push.91683607 drop

	let recall_hight: Result<u32, _> = note_inputs[2].try_into();
	let recall_height: Result<u32, _> = note_inputs[2].try_into();

		let inputs_account_id = AccountId::try_from([account_id_felts[1], account_id_felts[0]])
		.expect("invalid account ID felts");

		// we need to execute the transaction if compatibility is unclear
		NoteAccountCompatibility::Maybe => break,

Stateful execution checker #1269

Stateful execution checker #1269

Uh oh!

Conversation

Fumuran commented Apr 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PhilippGackstatter left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Fumuran commented Apr 14, 2025

Uh oh!

PhilippGackstatter left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fumuran commented Apr 17, 2025

Uh oh!

bobbinth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

PhilippGackstatter left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bobbinth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bobbinth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

PhilippGackstatter Apr 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fumuran commented Apr 3, 2025 •

edited

Loading

PhilippGackstatter Apr 28, 2025 •

edited

Loading