Refactor Process and ProcessState

[plafer]

This is some cleanup as part of #1457.

This PR has 2 commits:

1. takes the Host object out of Process
   • This was causing some borrow checker problems with ProcessState, when we need a &Process to create a ProcessState to pass it to a Host method. The borrow-checker prevents us from doing that, since there's already a &mut Process reference to get the Host (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 as processor::trace::finalize_trace().
2. Converts the ProcessState trait into a struct
   • This is cleaner IMO, where all advice provider methods were generic over a ProcessState, where actually there's only ever going to be a single implementation (i.e. the VM's Process). I believe the intention with the trait was to clearly indicate the parts of the Process that the AdviceProvider 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.

[bobbinth]

takes the Host object out of Process

• This was causing some borrow checker problems with ProcessState, when we need a &Process to create a ProcessState to pass it to a Host method. The borrow-checker prevents us from doing that, since there's already a &mut Process reference to get the Host (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 as processor::trace::finalize_trace().

I haven't looked at the code in too much detail yet, but #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.

[plafer]

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 #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.

[bobbinth]

Looks good! Thank you! I left a few comments inline - they are pretty minor but applicable to quite a few places.

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.

Agreed. I just find it a bit annoying that we have to pass &mut host everywhere (which decreases code readability a bit). But also, don't have a better solution at this point.

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).

Makes sense. Let's do it in a separate PR though.

[bobbinth]

All looks good! Thank you!