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

Update IncrementalMerkleTree test vectors to use valid commitments #3585

Merged
merged 1 commit into from Nov 15, 2018

Conversation

@str4d
Copy link
Contributor

str4d commented Oct 10, 2018

The original commitments were SHA256 outputs, and some were outside the
scalar field. This didn't affect the Merkle hash, which drops the high
bit from each commitment, but it does affect the creation of the Merkle
path in Rust, which requires path nodes to be valid scalars.

Here, we explicitly drop the high bit of all test vector commitments,
as well as reducing the two that remain outside the field. The test
vectors still pass, and can now also be used in the Rust implementation.

Update IncrementalMerkleTree test vectors to use valid commitments
The original commitments were SHA256 outputs, and some were outside the
scalar field. This didn't affect the Merkle hash, which drops the high
bit from each commitment, but it does affect the creation of the Merkle
path in Rust, which requires path nodes to be valid scalars.

Here, we explicitly drop the high bit of all test vector commitments,
as well as reducing the two that remain outside the field. The test
vectors still pass, and can now also be used in the Rust implementation.

@str4d str4d requested review from daira and ebfull Oct 10, 2018

@str4d str4d added this to Review Backlog in Zcashd Team Oct 10, 2018

@daira

daira approved these changes Oct 11, 2018

Copy link
Contributor

daira left a comment

ACK. I checked manually that merkle_commitments_sapling.json was changed as described (I didn't check the other files).

@bitcartel bitcartel moved this from Review Backlog to In Review in Zcashd Team Oct 18, 2018

@mdr0id mdr0id requested a review from Eirik0 Nov 8, 2018

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Nov 14, 2018

@str4d I can see the high bit being dropped. Which two were outside the field and how were they reduced?

@str4d

This comment has been minimized.

Copy link
Contributor

str4d commented Nov 14, 2018

The two that remained outside the field after dropping the high bit were f607dd230ada93d14f4de1d9008a5e64a59af87c2e4f64a5f9e55e0cd44867f8 and 7c1dbdb260441b89a08ba411d5f8406e81abd9dc85382f307999fdf77d8fcac8.

IIRC what I logically did for every entry was:

  • Drop the high bit.
  • Reduce modulo r_J by loading the big-endian hex representation into Python as a decimal value, reducing with the % operator (using the decimal r_J value from here), and then converting back to hex.

Obviously reducing modulo r_J leaves the value unchanged if it's already in the field.

@str4d str4d added this to the v2.0.2 milestone Nov 14, 2018

@bitcartel bitcartel self-requested a review Nov 14, 2018

@bitcartel
Copy link
Contributor

bitcartel left a comment

utACK

@bitcartel
Copy link
Contributor

bitcartel left a comment

utACK

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Nov 14, 2018

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Nov 14, 2018

📌 Commit 9aab383 has been approved by bitcartel

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Nov 14, 2018

⌛️ Testing commit 9aab383 with merge bd75f62...

zkbot added a commit that referenced this pull request Nov 14, 2018

Auto merge of #3585 - str4d:merkle-tree-test-vectors, r=bitcartel
Update IncrementalMerkleTree test vectors to use valid commitments

The original commitments were SHA256 outputs, and some were outside the
scalar field. This didn't affect the Merkle hash, which drops the high
bit from each commitment, but it does affect the creation of the Merkle
path in Rust, which requires path nodes to be valid scalars.

Here, we explicitly drop the high bit of all test vector commitments,
as well as reducing the two that remain outside the field. The test
vectors still pass, and can now also be used in the Rust implementation.

@bitcartel bitcartel moved this from In Review to Merge Queue in Zcashd Team Nov 14, 2018

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Nov 15, 2018

💔 Test failed - pr-merge

@str4d

This comment has been minimized.

Copy link
Contributor

str4d commented Nov 15, 2018

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Nov 15, 2018

⌛️ Testing commit 9aab383 with merge bc81c12...

zkbot added a commit that referenced this pull request Nov 15, 2018

Auto merge of #3585 - str4d:merkle-tree-test-vectors, r=bitcartel
Update IncrementalMerkleTree test vectors to use valid commitments

The original commitments were SHA256 outputs, and some were outside the
scalar field. This didn't affect the Merkle hash, which drops the high
bit from each commitment, but it does affect the creation of the Merkle
path in Rust, which requires path nodes to be valid scalars.

Here, we explicitly drop the high bit of all test vector commitments,
as well as reducing the two that remain outside the field. The test
vectors still pass, and can now also be used in the Rust implementation.
@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Nov 15, 2018

☀️ Test successful - pr-merge
Approved by: bitcartel
Pushing bc81c12 to master...

@zkbot zkbot merged commit 9aab383 into zcash:master Nov 15, 2018

1 check passed

homu Test successful
Details

Zcashd Team automation moved this from Merge Queue to Released (Merged in Master) Nov 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment