Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement error codes for the mtree_verify instructions #1328

Open
wants to merge 4 commits into
base: next
Choose a base branch
from

Conversation

Overcastan
Copy link
Contributor

@Overcastan Overcastan commented May 7, 2024

This PR implements the ability to specify error codes for mtree_verify instruction the same way it is done for assert instructions.

TODO:

  • update changelog

Related issue: #1264

@Overcastan Overcastan force-pushed the andrew-impl-err-codes-for-mtree-verify branch from cb28df9 to 53fe0b4 Compare May 8, 2024 14:36
@Overcastan Overcastan force-pushed the andrew-impl-err-codes-for-mtree-verify branch 2 times, most recently from c53b821 to 69eb6f8 Compare May 31, 2024 11:21
@Overcastan Overcastan force-pushed the andrew-impl-err-codes-for-mtree-verify branch from 69eb6f8 to 41d8342 Compare May 31, 2024 11:30
@Overcastan Overcastan linked an issue May 31, 2024 that may be closed by this pull request
@Overcastan Overcastan marked this pull request as ready for review May 31, 2024 11:57
Copy link
Contributor

@bitwalker bitwalker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Just one small nit, but your call on that one.

@@ -397,7 +397,7 @@ MacroInst: SmallOpsVec = {
#[inline]
Inst: Instruction = {
AdviceInjector,
Assert,
MaybeWithError,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I would name this something a bit more precise, perhaps InstWithErrorCode? Most other instances of Maybe* are macros which can be applied to a non-terminal, but that isn't the case here - the others are more precise about what is being parsed, so less ambiguous at first glance.

Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thank you!

One outstanding thing, probably for a different PR, is figuring out how we can treat MerklePathVerificationFailed error in a similar way as FailedAssertion. Specifically, for FailedAssertion, we have a dedicated host handler which can process assertion errors in a custom way (i.e., by mapping them to user-friendly error messages). Should we do something similar for the MerklePathVerificationFailed?

If we do add a special handler to the Host for MerklePathVerificationFailed - should it cover only mtree_verify instruction, or other Merkle-tree related instructions as well?

As I mentioned, this is probably for a different PR - but I think we should have a good understanding of what we want to do before merging this PR.

@Overcastan
Copy link
Contributor Author

We can add a Host handler for Merkle tree errors exactly the same as we do it for Assert now, with handler like on_merkle_tree_error. I think it makes sense to use it for all Merkle-tree related instructions: for now we have only mtree_verify, but probably other instructions eventually will have error codes as well.

By the way, I noticed that we call on_assert_failed Host function only for Assert(err) operation, but not for U32assert2(err). Formally it is also an assert operation, although it is used in a slightly different way. Should we leave it as it is?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

mtree_verify should also accept err
3 participants