0xPolygonMiden / miden-vm

STARK-based virtual machine
MIT License
615 stars 152 forks source link

mtree_verify should also accept err #1264

Closed hackaugusto closed 2 months ago

hackaugusto commented 5 months ago

The mtree_verify instruction, like the assert instructions, will fail the execution if its criteria fails. This should also support an error code.

Fumuran commented 5 months ago

I'm not sure I understand the purpose of adding an error codes to the mtree_verify. We decided to add error codes to the assert instructions because they were returning just a FailedAssertion error and a cycle number without additional data, so it was difficult to find out what exactly caused an error. But in case of mtree_verify we have a specific error which contains necessary data about Merkle Tree and a value being checked. I think I just missing something, @hackaugusto can you describe in more detail why this is necessary?

bobbinth commented 5 months ago

I think the main rationale is that mtree_verify acts kind of like an assert instruction. That is, if a Merkle path cannot be verified, the execution fails. These failures can happen in different contexts (e.g., asset is not present in account vault) and it would be helpful to let users specify their own error codes.

hackaugusto commented 5 months ago

The motivation for the issue was the usage of mtree_verify in miden-base. There are two uses and having an error will make it clearer which one caused an issue (also for other errors)

In general, I think every instruction that can fail should have an error to make debugging easier. My ideal situation would be to have error code to every instruction that can fail. So that when I get a failure in a test, I can just search for the error code and find exactly the instruction that failed.

bobbinth commented 2 months ago

Closed by #1328.