Skip to content

feat: Use FastProcessor and TransactionExecutorHost in code tests#1955

Merged
PhilippGackstatter merged 23 commits intonextfrom
pgackst-use-tx-exec-host-in-tests
Oct 10, 2025
Merged

feat: Use FastProcessor and TransactionExecutorHost in code tests#1955
PhilippGackstatter merged 23 commits intonextfrom
pgackst-use-tx-exec-host-in-tests

Conversation

@PhilippGackstatter
Copy link
Copy Markdown
Contributor

@PhilippGackstatter PhilippGackstatter commented Oct 1, 2025

Change the implementation of TransactionContext::execute_code to use the FastProcessor.

This affected mainly:

  • The way we access the stack and memory after executing some arbitrary code. This relies more heavily on an extension trait now to make test code succinct and convenient to write/read.
  • The MockHost was originally intended to be removed and fully replaced by TransactionExecutorHost. That almost worked, but for some tests, the executor host actually does too much. For instance, it handles delta tracking and output note creation which is unnecessary for testing and produces unimportant errors in some tests, so it is standing in the way of testing. For that reason, the MockHost is kept but is just a very thin wrapper around the executor host with the only difference being that it handles a small subset of TransactionEvents. This seems fine because the code is fairly simple and shouldn't be hard to maintain.
  • The LinkMap implementation. It was previously relying on a ProcessState directly but now it needs to work both on a ProcessState and an ExecutionOutput. An abstraction over both was added for that, but long-term this should ideally go away once we change a LinkMap to be a copy of the in-kernel link map, e.g. using a BTreeMap or something similar. See MemoryViewer docs for more details.

Related to the second point, the intention of the issue was to remove everything related to TransactionContextBuilder::enable_lazy_loading, but:

  • enable_lazy_loading specifically will be removed with the PR that closes issue Remove foreign_account_inputs field from the TransactionArgs #1473.
  • The internal flag still will need to continue to exist because we now need the negative version of it, i.e. disable_lazy_loading. There is just one test (test_get_balance_non_fungible_fails) that needs it, but I couldn't think of a better way to work around this. For now, lazy loading is disabled by default, but that will change with another PR and if lazy loading is enabled, the test fails with
Error: error during processing of event with id EventId(3463669078242126216) in on_event handler

Caused by:
    0: provided faucet ID is not valid for fungible assets
    1: faucet id 0xbc0000000000ca300000dd000000ef of type NonFungibleFaucet must be of type FungibleFaucet for fungible assets

Also note that if we do #1919 then no public API would have any enable/disable lazy loading, because only the internal tool would allow for execute_code and only it needs the disable functionality.

Foreign Account lazy loading test

Regarding #1872 (comment), there is at least one test that covers asset or storage map lazy loading for foreign accounts, so while coverage could be better, the overall functionality is tested.

Follow-up

I'll make a follow-up PR to make TransactionContext::execute_code async.

Random note

Long-term I'd like to make CodeExecutor, MockHost and TransactionContext::execute_code completely internal to miden-testing, i.e. only use them as testing tools for miden-base. This makes it easier to evolve them in the future, because currently, we technically always have to keep in mind that we're changing the public API. This should also be fine because they are primarily used to test the transaction kernel, which is something that should not be necessary outside of miden-base.
There is an argument that CodeExecutor might be useful for external users to write unit test for their code, but using Miden VM directly for this is pretty easy as well, as evident by the few lines of code with which CodeExecutor is implemented.
For this reason, some APIs were already made #[cfg(test)].

Copy link
Copy Markdown
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Not a review, but I left a couple of questions/comments inline.

Copy link
Copy Markdown
Contributor

@Fumuran Fumuran left a comment

Choose a reason for hiding this comment

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

Thank you, looks great!

Probably the only nit I wanted to mention is to use a bit more verbose name for the MemoryViewer in the LinkMap, but that was intentionally changed to be succinct 😅

Copy link
Copy Markdown
Collaborator

@mmagician mmagician left a comment

Choose a reason for hiding this comment

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

LGTM ✅

Co-authored-by: Marti <marti@miden.team>
@PhilippGackstatter PhilippGackstatter merged commit f36d050 into next Oct 10, 2025
19 checks passed
@PhilippGackstatter PhilippGackstatter deleted the pgackst-use-tx-exec-host-in-tests branch October 10, 2025 10:32
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.

Enable lazy loading for tests using TransactionContext::execute_code

4 participants