0xPolygonMiden / miden-vm

STARK-based virtual machine
MIT License
632 stars 161 forks source link

Implement `MastForest` merging #1534

Closed PhilippGackstatter closed 3 weeks ago

PhilippGackstatter commented 1 month ago

Describe your changes

Closes #1529

Implements MastForest merging.

The implementation is explained in comments and it's hopefully sufficient to understand the PR, which is why I'm not adding the details here again. If any explanation is insufficient I'm happy to expand on it!

Couple of notes and ideas:

Checklist before requesting a review

bobbinth commented 1 month ago

I would like to test the implementation with some more complex "real-world" input, but this is difficult in miden-core without an Assembler. I don't want to move those test into miden-assembly though as that separates the implementation and the tests crate-wise.

I actually think it may be fine to have some tests for merging forests in the miden-assembly crate. It would simplify things a bit and there is not too much downside.

PhilippGackstatter commented 1 month ago

I updated the implementation to use the desired merging behavior for external nodes with decorators present. The refactor to support this turned out to be fairly complex but I think it's alright now. I still need to add documentation in many places, handle some errors and probably some other small things, but conceptually I think it is in a reviewable state now again.

PhilippGackstatter commented 4 weeks ago

I implemented the refactoring now, but there are still some things left to clean up for sure. I added an assertion to most tests now that assert that the "child id < parent id" property holds, so that's nice :tada:. If you have feedback on the approach now, I'd be happy to hear it and the other nits I will address tomorrow.

Also, sorry for the huge code changes, hopefully that was the last one.

PhilippGackstatter commented 4 weeks ago

After adding a test that used the MastForest from the Miden StdLib I encountered a small bug in the iterator logic, but that is fixed now. The PR is now ready for a full review again.