Add ZIP-244 signature hash support

[conradoplg]

Motivation

ZIP-244 and ZIP-225 specify a new signature hash that reuses many digests computed during the transaction ID digest computation. We need to implement it for network upgrade 5.

Solution

Use librustzcash to compute it, to save implementation time.

I had to make some changes to librustzcash in order to be able to call it. I'm discussing these with the ECC team to incorporate in the library. Currently this PR makes zebra point to a branch in our fork of librustzcash with those changes.

The test is still not passing, I haven't investigated the reason yet. But it may be better to wait for ECC to run on their test vectors first.

The solution is not particularly efficient because it doesn't reuse the digest computation from the transaction ID. But we have discussed previously that we should make it work first.

The code in this pull request has:

• Documentation Comments
• Unit Tests and Property Tests

Review

It doesn't seem urgent but I think it's best to review it in this sprint.

@teor2345 is already familiar with the librustzcash integration, but @dconnolly may be interested in taking a look

One particular point I'd like feedback is the location of the test. It's on vectors.rs with the other ZIP-244 tests, but all previous sighashes tests are in sighash.rs. Should I move it there?

Related Issues

Closes #2051

This was implemented on top of #2050, so that should be merged first.

Follow Up Work

Follow up with librustzcash to make sure the test passes.

#2183: also use librustzcash for V4 and earlier

[teor2345]

Hey @conradoplg it looks like this PR is based on #2129, so I've changed the base branch.

GitHub will automatically rebase the PR to main when #2129 merges. But you might need to rebase force-push the branch if we do a squash.

[teor2345]

One particular point I'd like feedback is the location of the test. It's on vectors.rs with the other ZIP-244 tests, but all previous sighashes tests are in sighash.rs. Should I move it there?

We try to put tests in their own files to speed up compilation. So please move all the tests from sighash.rs to vectors.rs.

[teor2345]

Looks good, I made some suggestions about return types and testing

[conradoplg]

One particular point I'd like feedback is the location of the test. It's on vectors.rs with the other ZIP-244 tests, but all previous sighashes tests are in sighash.rs. Should I move it there?

We try to put tests in their own files to speed up compilation. So please move all the tests from sighash.rs to vectors.rs.

Sure! But the tests access private methods (e.g. hash_header) so from what I understand I'd need to make them pub(super) which does not seem great. What do you think?

[teor2345]

Sure! But the tests access private methods (e.g. hash_header) so from what I understand I'd need to make them pub(super) which does not seem great. What do you think?

pub(super) or pub(crate) is fine for tests. (We try not to expose implementation details outside the crate, but we can be flexible inside the crate.)

[conradoplg]

Sure! But the tests access private methods (e.g. hash_header) so from what I understand I'd need to make them pub(super) which does not seem great. What do you think?

pub(super) or pub(crate) is fine for tests. (We try not to expose implementation details outside the crate, but we can be flexible inside the crate.)

Makes sense! I've moved the tests.

[conradoplg]

Rebased with #2129, updated test to not skip Orchard txs. V5 test still not passing.

[conradoplg]

Test fixed and passing.

Added a round-trip test suggested in #2129

[dconnolly]

Some naming questions and other stuff 🤩

[dconnolly]

lgtm! 🌠

[conradoplg]

@teor2345 or @dconnolly can you reapprove? I had to solve merge conflicts after the txid PR was merged

[conradoplg]

It seems @teor2345's approval is required, but I swear the merging button was available after @dconnolly approved it the first time 🤔 (It seems I can dismiss Teor's request for changes but it doesn't feel like a good practice, so I'll wait for now)

[teor2345]

This is fine, please don't block on me.