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

Add test for sighash.json test vectors #2217

Closed
wants to merge 1 commit into from
Closed

Conversation

conradoplg
Copy link
Collaborator

Motivation

Testing against zcashd sighash.json is useful to make sure everything works as expected.

Solution

sighash.json was added to the repo and a test that uses it.

I'm not sure if include_str! is appropriate here (the file is 3MB)

The test is currently failing, see #2215 for details. We'd need to make some adjustments, or maybe conclude that the test is not worth adding.

The code in this pull request has:

  • Documentation Comments
  • Unit Tests and Property Tests

Review

This is not urgent, and we must decide if it's worth doing it in the first place.

Related Issues

Closes #2215

Follow Up Work

pub script: Vec<u8>,
pub input_index: u64,
pub hash_type: u64,
pub branch_id: u64,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want these types to be like JSON, or match Zcash?

It might be easier to just do the conversions all at once as we deserialize.



pub static ref SIGHASH: Vec<SighashTest> = {
let json_str = include_str!("vectors/sighash.json");
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not particularly concerned about that binary size, but I would like to avoid making Zebra's git repository 3MB larger.

How big is the JSON file when it's compressed? Can we use a compressor that's already in Zebra's transitive dependencies? (Check cargo-tree for gzip, zip, flate, lzma, zstd or something similar.)

You'll need include_bytes! for the compressed data.

@conradoplg
Copy link
Collaborator Author

Closing this per sprint discussion - we need to decide if we really want to do this test, see #2215

I've also removed sighash.json from the repository since it's pretty big. We can re-add it later if needed.

@conradoplg conradoplg closed this Jun 9, 2021
@teor2345 teor2345 deleted the sighash-test-vectors branch March 21, 2022 21:35
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.

Test/fix signature hashes using sighash.json from zcashd
2 participants