Introduce MastForestStore
#1359
Conversation
There was a problem hiding this comment.
Thank you! Looks good. I left some comments inline. The biggest missing pieces are:
- As far as I can tell, the assembler does not currently output any
Externalnodes. We should output external nodes at least forcall.0x...instructions and potentially forsyscallinstructions as well. - I think the way the
MastForestis currently being built by the assembler would result in incorrect set of roots and also would contain duplicate nodes. So, we may want to introduce a concept ofMastForestBuilderto address this.- We should also improve tests to make sure
MastForests are being built by the assembler correctly.
- We should also improve tests to make sure
- We don't have a mechanism for loading Kernel forests into the VM processor - but maybe that's something we leave for the next PR.
There was a problem hiding this comment.
Looks good! Thank you! I left a few more comment inline - but all of them should be pretty straight forward.
Would be great to get a review from @bitwalker though - especially for the assembler-specific changes.
For the next PR, we should start working on the package format and MAST forest serialization. I'll write a more detailed comment in the issue discussion.
|
Oh - and let's update the changelog. |
|
Sorry I haven't had a chance to review this yet, it's on the docket for tomorrow though, so bear with me! |
There was a problem hiding this comment.
All looks good! Thank you!
@bitwalker - could you take a quick look? Would be great to merge this in the next day or so.
There was a problem hiding this comment.
I'm mostly done with my review, and am requesting changes, or at least discussion/clarification on a few points, but I'm still skimming a few remaining files outside of the assembler, but I'll add any comments to those as one-offs.
My primary concerns are two-fold:
- The ability to assemble libraries, not just executables
- The ability to
execprocedures in libraries which are not part of the current translation unit (i.e. they were compiled as part of another library, not the current library/program).
As far as the compiler is concerned, those are blocking issues that cannot be left unresolved. They are also practical issues for packaging (and consequently, how to reference code across packages).
| /// # Errors | ||
| /// | ||
| /// Returns an error if parsing or compilation of the specified program fails. | ||
| pub fn assemble(self, source: impl Compile) -> Result<MastForest, Report> { |
There was a problem hiding this comment.
It seems the Assembler was changed so that it is no longer possible to assemble a library, only programs. The compiler, for example, is going to primarily be assembling libraries, not programs - particularly for the rollup, where accounts are libraries, and note scripts are not executables but libraries dynamically invoked from the "real" executable, the transaction kernel.
How does one obtain a MastForest for a library from the Assembler? I took a look around, but it looks to me like it isn't possible.
| self.compile_body(then_blk.iter(), context, None, mast_forest)?; | ||
| let else_blk = | ||
| self.compile_body(else_blk.iter(), context, None, mast_forest)?; | ||
| self.compile_body(then_blk.iter(), context, None, mast_forest_builder)?; |
There was a problem hiding this comment.
Looks like there was a merge conflict here - the old version in the diff is actually newer than the "new" version in the diff: we no longer treat the else block specially here (either block could be empty now, and we rely on compile_body to fill empty bodies with a single nop, if any have made it to the assembler).
assembly/src/errors.rs
Outdated
| @@ -104,6 +104,15 @@ pub enum AssemblyError { | |||
| source_file: Option<Arc<SourceFile>>, | |||
| callee: RpoDigest, | |||
| }, | |||
| #[error("invalid exec: exec'd procedures must be available during compilation, but '{callee}' is not")] | |||
There was a problem hiding this comment.
This is not supposed to be an error - exec does not require the procedure to be available, and cannot require that.
There was a problem hiding this comment.
As discussed offline, removed
| // effect of inlining it | ||
| InvokeKind::Exec => callee_id, | ||
| // For `call`, we just use the corresponding CALL block | ||
| InvokeKind::Exec => { |
There was a problem hiding this comment.
I'm confused how we have ended up back here again - exec and call are identical in every respect, with the sole exception of their execution context semantics. Why are we treating exec differently than call here?
There was a problem hiding this comment.
As discussed offline, this was updated to act similarly to call: 25fe82f
| root_digest: external_node.digest(), | ||
| }, | ||
| )?; | ||
| let root_id = mast_forest.find_procedure_root(external_node.digest()).ok_or( |
There was a problem hiding this comment.
I would think that an External node could be any MAST node, not necessarily a procedure root. In particular, this would make it impossible to hide portions of a program - you would have to factor out all of the parts you want to hide into procedures, which sort of defeats the point. That was in fact what Proxy was used for previously, and I've somewhat lost the thread on what our plans are for the use cases it supported.
Maybe this is a fine restriction to have for now, but we should probably make a note here that the implicit assumption here that External nodes are always procedures is an artifact of other implementation choices, and not of the representation itself. In particular, the notion that exec is restricted to invoking procedures is a fiction - if you specify the MAST root of a non-procedure node as the callee of exec, that is totally valid, because the semantics are that the referenced MAST is treated as inlined at the callsite. This is in fact the only current means by which someone can hide portions of their program in Miden Assembly today.
Anyway, I'm bringing this up, because I know we made various claims about what the assembler does and does not produce when we were discussing Proxy, but I think what got lost in that discussion was what the prior representation made possible, that I'm not sure is being preserved by the changes made as part of the MAST refactoring. That may very well be deliberate, but it is not documented AFAICT.
Follow-up to #1349.
Partially closes #1226
Introduces the
MastForestStore(referred to elsewhere asObjectStore).Note that since packages aren't implemented yet, we still require programs to be assembled directly with the libraries they depend on. Hence, the current use of
MastForestStoreis limited to supportedcall.0x...anddyncalls to procedures assembled as part of a separate compilation unit.