-
Notifications
You must be signed in to change notification settings - Fork 47
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
Add BatchMerkleProof + PoPow Interlink vector verification #538
Conversation
Pull Request Test Coverage Report for Build 2235024812
💛 - Coveralls |
Sounds great! Good call! I appreciate you did benchmarks! |
4ed32ef
to
64a5053
Compare
64a5053
to
3fd3c75
Compare
…_proofs, move pack_interlinks to NipopowAlgos
…erialization code
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.
Looking great! Just a few minor comments.
c801a60
to
bee6ca5
Compare
@greenhat Hi, do you happen to know why the Github workflow isn't running proptests for BatchMerkleProof? I noticed that they aren't included in the tests. While running proptests, I believe I've found a bug that leads to a stack overflow in both the Rust and Scala implementations of BatchMerkleProof::valid, it should be simple enough to fix however (If a/e are empty, BatchMerkleProof::validate will keep being called, a misbehaving node could send a bad proof for a header and cause a stack oveflow). I've attached an example that replicates the issue in Scala val batch = BatchMerkleProof(Seq(), Seq((Digest32 @@ Array.fill[Byte](32)(0),MerkleProof.LeftSide)))
batch.valid(Digest32 @@ Array.fill[Byte](32)(0)) |
Maybe because of
Great catch! Could you please make an issue in scrypto repo? Thanks. |
…tion The old code for validating indices would cause issues if a tree has an odd number of nodes, and a user requests the last node (empty node)
Figured out how to get proptests to run by default (added the feature in ergo-lib dev-dependencies). Here's the scrypto issue input-output-hk/scrypto#96. I've fixed it in the Rust version |
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.
Looking great! Thank you!
Closes #536