-
Notifications
You must be signed in to change notification settings - Fork 46
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
fix: incorrect proof hash count causing panic #28
Conversation
Hi, would it be possible to also include tests into this PR showcasing what was fixed? |
The problem fixed is a bug, you can refer to #20, I also encountered a similar problem. I don't know what kind of test is appropriate to build, it seems to just fix the original bug: use the old code, the merkle proof bug will appear, use the new code, the merkle proof will work normally
|
@witter-deland Thanks for the work! Yeah, please just include one simple test that passes an incorrect number of hashes, and check that this error is returned. Thank you very much! |
It's done : )
|
@witter-deland Thank you! Merged. Going to publish a new patch version soon |
Version 1.4.1 published, thank you! |
let extracted_root = proof.root( | ||
&indices_to_prove, | ||
&leaves_to_prove, | ||
test_data.leaf_values.len() + 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@witter-deland, could you pls look into this test again.
the +1
shouldn't be here iiuc. When I remove it, this test fails.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 means incorrect proof hash count, why did you remove it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i see, sry i was confused for a second. I hope you don't mind if a change the test to actually remove the hash #31
On second thought, maybe hold off fixing it @witter-deland for now. This PR doesn't seem to address the root cause of the panic. Im working on a PR which addresses it properly. It will subsume this fix. |
nvm, i think this PR's fix covers all cases. thanks. |
fix #20