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

mpverify: don't panic when verification fails #1230

Merged
merged 2 commits into from
Feb 1, 2024
Merged

Conversation

plafer
Copy link
Contributor

@plafer plafer commented Feb 1, 2024

Converts a panic!() into an error when Merkle Path verification fails.

@plafer plafer requested a review from bobbinth February 1, 2024 21:59
Comment on lines 89 to 90
assert_eq!(root, computed_root, "inconsistent Merkle tree root");
if root == computed_root {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably remove assert_eq from here and the the condition for the if statement should be root != computed_root - right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, the comment should be updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops - and it should have been !=. Fixed

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!

@plafer plafer merged commit 0a4fa7a into next Feb 1, 2024
15 checks passed
@plafer plafer deleted the plafer-fix-mpverify branch February 1, 2024 22:20
bobbinth pushed a commit that referenced this pull request Feb 26, 2024
* Replace `panic!()` with `Err()`

* fix
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.

2 participants