Add ZIP-0244 TxId Digest support

[conradoplg]

Motivation

ZIP-0244 specifies a new Transaction ID Digest which the hash of a "tree" of hashes from specific fields of the transaction, rather than the hash of the entire transaction itself.

Solution

This uses librustzcash to compute the digest. In order to call it, the transaction is re-serialized and deserialized with librustzcash.

While the implementation is done, it still has this pending issues:

• The test still fails because librustzcash support for ZIP-0244 is not finished yet. Should be finished soon. (The tests of this PR won't pass until that)
• Only a single test is done, which does not use Orchard data. This is because zebra does not parse Orchard data yet. However, a list of test vectors is already included. (The tests of this PR will pass even if that's not ready, but they'll need a simple change after Orchard support is added, in order to also test with Orchard test vectors)

The code in this pull request has:

• Documentation Comments
• Unit Tests and Property Tests

Review

@teor2345 already reviewed the draft so I'm assuming they'll do the final review

Related Issues

Closes #2050

Follow Up Work

ZIP-0244 also specifies a new sighash and a new authorizing data digest, which need to be implemented too.

[teor2345]

This looks really good!

I made a few suggestions, but overall the structure looks like what we'd want here. (And I'm glad it works.)

Let's also add some documentation comments:

• Add a short summary to the top of each module
• Explain the purpose of every new object
• Explain what each method does, and each method argument

Doc comments don't need to be long, 1-2 sentences is enough. Check out some of the other modules for examples.

And let's add some more tests:

• deserialization and serialization round-trip for zcash_primitives::transaction::Transaction::read using fake v5 transactions
  • this test helps us make sure our serialization works the same as librustzcash
  • for an example, see

    zebra/zebra-chain/src/transaction/tests/vectors.rs

    Lines 159 to 165 in 29893f2

    	/// Do a round-trip test on fake v5 transactions created from v4 transactions
    	/// in the block test vectors.
    	///
    	/// Covers Sapling only, Transparent only, and Sapling/Transparent v5
    	/// transactions.
    	#[test]
    	fn fake_v5_round_trip() {

• deserialization and serialization round-trip for zcash_primitives::transaction::Transaction::read using an empty v5 transaction
  • we might want to turn the empty transaction struct into a lazy_static! constant, then use it in both tests
  • for an example, see

    zebra/zebra-chain/src/transaction/tests/vectors.rs

    Lines 97 to 102 in 29893f2

    	/// An empty transaction v5, with no Orchard, Sapling, or Transparent data
    	///
    	/// empty transaction are invalid, but Zebra only checks this rule in
    	/// zebra_consensus::transaction::Verifier
    	#[test]
    	fn empty_v5_round_trip() {

zcash_primitives might reject empty transactions - if that's the case, let's skip that test.

[conradoplg]

@teor2345 I believe I've made all the requested changes. The new tests share a lot of code with the older ones, though. Do you think it warrants some refactoring?

I'm also considering moving the reserialization code (get branch ID from tx, convert to librustzcash enum, call librustzcash read) to a separate function, since it's repeated a lot in the tests now.

[teor2345]

This looks good so far, let's take it out of draft once you've done the refactoring.

[conradoplg]

Finished refactoring and took it out of draft.

[teor2345]

Looks good, I just had one suggestion to future-proof the code.

Looking forward to the librustzcash update for transaction IDs, so we can test this code properly.

[conradoplg]

Solved conflicts and updated test to not skip Orchard transactions now that we parse them.

The only thing left is the test to pass. It's possibly a librustzcash issue which we decided to not investigate yet since their implementation is still under review.

[conradoplg]

This is finally ready 🎉

I had to implement Arbitrary in some types to make the tests pass since librustzcash validates the fields when computing the txid and we can no longer generate invalid public keys, etc.

However I'm not sure if my approach is correct, since proptest advises against using prop_filter but I'm not sure how bad it is in this particular case. Feedback is welcome.

These changes are in a seprate commit (b7bda55) to make reviewing them easier

The requested changes have been made

[teor2345]

I made some specific suggestions about proptests and test vectors. But they are all optional.

I'd like @dconnolly or @jvff to do the final review here. Whoever is available after this PR is revised.

[jvff]

Looks good! I did add some suggestions and nit-picks 😅

[conradoplg]

Another round of fixes, thanks for the great suggestions!

[jvff]

Looks good! Just one optional suggestion.

[dconnolly]

Love it! I resolved a lot of conversations too