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

Investigate failing MerkleTreeSimpleTest in zerocashTest. #455

Closed
nathan-at-least opened this issue Nov 21, 2015 · 5 comments
Closed

Investigate failing MerkleTreeSimpleTest in zerocashTest. #455

nathan-at-least opened this issue Nov 21, 2015 · 5 comments
Labels
A-CI Area: Continuous Integration A-testing Area: Tests and testing infrastructure libzcash S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. special to Nathan

Comments

@nathan-at-least
Copy link
Contributor

Description here: Electric-Coin-Company/libzerocash#13 (comment)

@defuse
Copy link
Contributor

defuse commented Nov 23, 2015

Is this the same as #391?

@defuse defuse self-assigned this Nov 23, 2015
@defuse
Copy link
Contributor

defuse commented Nov 23, 2015

I got the test to pass by fixing two bugs in the test:

  • At the bottom, it's manually creating the witness for coin index 3, but comparing against the witness for coin index 1.
  • It's using a depth-16 tree and changing it to a depth-3 tree makes it work.

But I don't exactly understand what the witnesses are supposed to be, so I'm not sure if it's the test that's wrong or if it's the merkle tree implementation that's wrong.

@defuse
Copy link
Contributor

defuse commented Nov 23, 2015

Another problem with the test is that the "christina" witness vector is too big, since it's re-used between the depth-64 and depth-16 tree, and so the getWitness() being tested only fills the first 16 elements of it.

Another clue is that the non-zero witness elements appear at the end of the generated witness, but the test at the bottom expect them to be at the front. So either A: The witness generator is doing the right thing, and the test at the bottom expected the tree to have depth 3 or B: The witness generator is mistakenly padding with zeroes on the front, instead of the back.

@defuse
Copy link
Contributor

defuse commented Nov 23, 2015

Yeah, zeroes should be up front, because there's a bunch of levels you want to ignore and then finally the subtree you want to check. So it's option B. This isn't the same as #391.

@defuse defuse added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 24, 2015
@defuse defuse removed their assignment Nov 24, 2015
@defuse
Copy link
Contributor

defuse commented Nov 24, 2015

There's a pull request for this: Electric-Coin-Company/libzerocash#17

It uncovered the bug in #461.

@defuse defuse closed this as completed Nov 24, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-CI Area: Continuous Integration A-testing Area: Tests and testing infrastructure libzcash S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. special to Nathan
Projects
None yet
Development

No branches or pull requests

2 participants