Closed plafer closed 2 days ago
takes the
Host
object out ofProcess
- This was causing some borrow checker problems with
ProcessState
, when we need a&Process
to create aProcessState
to pass it to aHost
method. The borrow-checker prevents us from doing that, since there's already a&mut Process
reference to get theHost
(i.e.process.host.borrow_mut().host_method(&process_state)
).- This also forced many methods to be generic of a
Host
when they really didn't need to, such asprocessor::trace::finalize_trace()
.
I haven't looked at the code in too much detail yet, but https://github.com/0xPolygonMiden/miden-vm/issues/1100 describes another potential approach to this. Not sure about pros/cons of these approaches yet - but one thing the approach in #1100 tries to avoid is having to pass host
to all internal operation handlers.
I'm not sure how #1100 is a solution to the borrow-checker problem. I see the same problem of needing to do e.g.
// Process::op_emit
fn op_emit(&mut self, event_id) -> ... {
// doesn't work, since `&mut self` takes a mutable reference to the entire `Process` struct
// (including the inner `self.state`), such that `&self.state` is not allowed.
self.host.on_event(&self.state)?;
}
The larger problem IMO stems from the circular dependency
Process -> Host -> AdviceProvider
^ |
|______________________|
where a A -> B
means "A requires reference to B". In the current design, Process
is a master struct that contains everything that is needed to execute the union of all instructions. Therefore, AdviceProvider
methods need a &mut self
to modify the advice provider, while also having a read-only reference to Process
state (which doesn't work with the borrow-checker as described above).
I find it to be a cleaner design to have Process
be only the state that will be encoded into the trace, while the Host
is a separate entity that is the "outside environment" in service of trace generation (i.e. advice provider, MastForest
dependencies, event handlers, and debug/trace handling). i.e. it's a better separation of concerns, and is also borrow-checker friendly.
As an aside, I would also move Decoder.debug_info
into the Host
, so that Process
really only pertains to trace generation. It would also allow us to have Process::execute_decorator(&self, ...)
instead of &mut self
, clearly encoding that decorators do not modify the trace. It would also make Host
more consistent, since it currently is responsible for handling trace events, but not debug infos (which are similar in my mind).
but one thing the approach in https://github.com/0xPolygonMiden/miden-vm/issues/1100 tries to avoid is having to pass host to all internal operation handlers.
Given the above interpretation, I actually like to visually see which method also mutate the Host
, and which don't. For example, we can tell from the signature op_emit(&mut self, event_id, host)
that it mutates the external environment, but op_caller(&mut self)
doesn't.
This is some cleanup as part of #1457.
This PR has 2 commits:
Host
object out ofProcess
ProcessState
, when we need a&Process
to create aProcessState
to pass it to aHost
method. The borrow-checker prevents us from doing that, since there's already a&mut Process
reference to get theHost
(i.e.process.host.borrow_mut().host_method(&process_state)
).Host
when they really didn't need to, such asprocessor::trace::finalize_trace()
.ProcessState
trait into a structProcessState
, where actually there's only ever going to be a single implementation (i.e. the VM'sProcess
). I believe the intention with the trait was to clearly indicate the parts of theProcess
that theAdviceProvider
is allowed to view - the current design achieves this in a simpler way.I checked in
miden-base
, and I believe this change won't cause problems there.