Conversation
|
cc @bitwalker |
|
Could you add a description or link an issue where we can find information about what the goal of the PR is? It is hard to review the changes or judge alternatives without this information. |
|
@PhilippGackstatter @bobbinth This PR makes it possible for us to debug programs executed by the transaction kernel, which is a big improvement in DX. I'd like to try and get this released as part of the v0.22 release cycle/upcoming testnet - just let me know if you need anything on our end to make that go smoothly. |
|
@djolertrk Looks like a couple lint checks failed |
bobbinth
left a comment
There was a problem hiding this comment.
Thank you! Not a review yet - but a couple of high-level comments:
- It seems like this PR includes a lot of extraneous changes (e.g., from the AggLayer crate). Could we remove them? (these changes will land soon as a part of #2546)
- It would be great to provide a bit more detail about the goal here. After a brief look, my understanding is that we are trying to control how
FastProcessoris instantiated, but I don't yet have full understanding of how this will be used in the compiler. I'm mostly trying to understand if theTransactionProgramFactorypattern is really needed here or if we can do something simpler (e.g., maybe adding a generic type to theTransactionExecutorwould suffice).
Also, I wonder if there may be some overlap with #2293 (though, maybe not).
@bobbinth Thanks for the comments! Yes, it does overlap with the #2546 (the AggLayer changes for example), but I needed those so I can test the feature end to end. But yes, if the PR will land soon, I can just rebase on top of it -- so I am doing it. The immediate consumer here is the debugger, not the compiler. The goal is to let @PhilippGackstatter I've updated PR description as well. |
917ffe3 to
6a5bc19
Compare
|
note: Rebased on top of #2546. |
| stack_inputs: StackInputs, | ||
| advice_inputs: AdviceInputs, | ||
| options: ExecutionOptions, | ||
| ) -> Self::Executor; |
There was a problem hiding this comment.
A couple of questions:
First, why do we need a factory pattern here? Could we use just a single trait - e.g., something like:
pub trait VmProcessor: Sized {
fn new(
stack_inputs: StackInputs,
advice_inputs: AdviceInputs,
options: ExecutionOptions,
) -> Self;
fn execute<H: Host + Send>(
self,
program: &Program,
host: &mut H,
) -> impl FutureMaybeSend<Result<ExecutionOutput, ExecutionError>>;
}Second, why does this need to be defined in the protocol repo? Shouldn't it be in miden-tx as that where transaction execution aspects are handled?
There was a problem hiding this comment.
On the factory point: I don’t think the split is fundamentally required. The real requirement is just to let TransactionExecutor construct and run a custom VM runner from (StackInputs, AdviceInputs, ExecutionOptions). A single trait with new(...) + execute(...) would likely work too. We used a separate factory because the thing passed into with_executor_factory::<...>() is really a constructor hook, and the debugger use case naturally models that as a ZST like DapExecutorFactory. But I’m not strongly attached to that shape if the single-trait version feels cleaner.
On location: the immediate downstream implementer is the debugger, not the compiler. We put the trait in miden-protocol because miden-debug already depends on miden-protocol, and the trait surface itself is just VM/program execution types. That said, the hook is only consumed by miden-tx::TransactionExecutor, so I’d be fine moving it into miden-tx if that seems like the better ownership boundary.
cc @bitwalker
There was a problem hiding this comment.
I agree with the sentiment that the factory pattern seems harder to grok than a simpler trait like VmProcessor, so if it's not really necessary, then conceptually I like the direction @bobbinth proposed.
It would also be nice to think about how we can keep the "default executor" easily accessible, maybe with a simple type DefaultTransactionExecutor = TansactionExecutor<FastProcessor> but maybe there are other/better approaches.
Eventually, it would also be cool if we could make use of the debugger for test execution in miden-testing, so that we can perhaps make use of miden-debugger when debugging a test. Not necessary to integrate in this PR though. I don't know if there are any specific requirements to do this from the testing side but just mentioning this as one use case that would be very useful to enable.
There was a problem hiding this comment.
Eventually, it would also be cool if we could make use of the debugger for test execution in miden-testing, so that we can perhaps make use of miden-debugger when debugging a test. Not necessary to integrate in this PR though. I don't know if there are any specific requirements to do this from the testing side but just mentioning this as one use case that would be very useful to enable.
Yes, this is a nice use case, and we can handle it after this PR (set of PRs in the ecosystem) is merged. Thanks!
There was a problem hiding this comment.
@PhilippGackstatter The compiler in fact does just this (run our tests using the debug executor so we can debug tests with it), but naturally we can't do that with transactions just yet, but ultimately this line of work will enable using the debug executor in more places for that purpose.
There was a problem hiding this comment.
This makes me think: if we put the VmProcessor trait (or equivalent) here, this would create a circular dependency (at least between repos) - i.e., the debugger depends on miden-protocol and miden-protocol would like to use the debugger for tests.
But looking at the definition of VmProcessor - there is nothing inherently related to the protocol. So, maybe we should put this trait in the miden-vm repo? (and the FastProcessor would implement it there too).
There was a problem hiding this comment.
We were driven by the fact that it is all about TX execution -- and the abstraction about it lives in the protocol repo.
There was a problem hiding this comment.
Yes, understood that the driving motivation is debugging of transactions - but my point is that the abstraction is more generally useful and not really tied to the protocol. So, the trait is probably better placed in miden-vm repo.
There was a problem hiding this comment.
My only concern with putting things deeper in the dependency stack is that it slows down the propagation of changes, and the VM isn't really the thing that cares about this trait. If we need something added to it for the debugger or miden-protocol, then we first have to make the change against miden-vm, and then wait for it to bubble all the way up the stack - and if the change is breaking in miden-vm, we'll have to wait a full release cycle, whereas that wouldn't necessarily be the case if it lives in miden-protocol.
It's always possible to push this further down the stack when we actually start using the debug executor in miden-testing and it turns out to be necessary - but if we need to iterate on it at all, it would be beneficial to avoid needing to go through the VM to do so.
There was a problem hiding this comment.
Yes - this makes sense. So, to summarize:
Since we are still iterating on the design, we should keep things as simple as easy to update as possible. To me, this implies:
- Keep the trait in
miden-protocol, but with the eventual intent to move it upstream tomiden-vm. To reduce the number of crates affected, I'd put everything that we think could be upstreamed later on into a self-contained module inmiden-tx. - I would go with a single trait rather than the factory pattern. If the need for a factory pattern arises in the future, we can adjust then.
6a5bc19 to
27c1fea
Compare
|
note: now rebased on top of |
This PR adds a small hook to the transaction execution path so that the program runner used by
midentx::TransactionExecutorcan be swapped out while preserving the existing transaction preparation/execution pipeline.The motivation is debugger support and better developer tooling. We want to be able to debug programs executed under the transaction kernel, instead of only debugging standalone VM programs. In practice, this lets downstream tooling swap in a custom executor that:
while still reusing the existing
TransactionExecutorlogic for transaction preparation, host setup, note handling, foreign account loading, etc.The default behavior is unchanged:
TransactionExecutorstill usesFastProcessorby default.