feat: Use FastProcessor and TransactionExecutorHost in code tests#1955
Merged
PhilippGackstatter merged 23 commits intonextfrom Oct 10, 2025
Merged
feat: Use FastProcessor and TransactionExecutorHost in code tests#1955PhilippGackstatter merged 23 commits intonextfrom
FastProcessor and TransactionExecutorHost in code tests#1955PhilippGackstatter merged 23 commits intonextfrom
Conversation
bobbinth
reviewed
Oct 1, 2025
Contributor
bobbinth
left a comment
There was a problem hiding this comment.
Not a review, but I left a couple of questions/comments inline.
bobbinth
reviewed
Oct 8, 2025
Fumuran
approved these changes
Oct 8, 2025
Contributor
Fumuran
left a comment
There was a problem hiding this comment.
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 😅
Co-authored-by: Marti <marti@miden.team>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Change the implementation of
TransactionContext::execute_codeto use theFastProcessor.This affected mainly:
MockHostwas originally intended to be removed and fully replaced byTransactionExecutorHost. 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, theMockHostis kept but is just a very thin wrapper around the executor host with the only difference being that it handles a small subset ofTransactionEvents. This seems fine because the code is fairly simple and shouldn't be hard to maintain.LinkMapimplementation. It was previously relying on aProcessStatedirectly but now it needs to work both on aProcessStateand anExecutionOutput. An abstraction over both was added for that, but long-term this should ideally go away once we change aLinkMapto be a copy of the in-kernel link map, e.g. using aBTreeMapor something similar. SeeMemoryViewerdocs 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_loadingspecifically will be removed with the PR that closes issue Removeforeign_account_inputsfield from theTransactionArgs#1473.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 withAlso 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_codeand 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_codeasync.Random note
Long-term I'd like to make
CodeExecutor,MockHostandTransactionContext::execute_codecompletely internal tomiden-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
CodeExecutormight 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 whichCodeExecutoris implemented.For this reason, some APIs were already made
#[cfg(test)].