Fix bug in sighash for coinbase transactions

[conradoplg]

Motivation

The sighash of coinbase transactions was being computed incorrectly because it ignored the "outpoint" of coinbase inputs while they shouldn't be.

Closes #1939.

Specifications

https://zips.z.cash/zip-0143, which does not have any exceptions for the coinbase transaction. (It could be more explicit about it...)
https://developer.bitcoin.org/reference/transactions.html#coinbase-input-the-input-of-the-first-transaction-in-a-block describes the "outpoint" of a coinbase transaction (a null hash, and a 0xFFFFFFFF index).

Designs

N/A

Solution

Recreate the "fake" outpoint of coinbase inputs in order to hash it.

I added two tests:

• One test was based on @dconnolly's branch cited in Fix sapling binding signature errors #1939 which iterates the test blocks and verifies the Sapling binding signature for each transaction where applicable. This reproduces the bug.
• One test was added based on ZIP-243 test vectors from https://github.com/zcash-hackworks/zcash-test-vectors/ . This does not reproduce the bug since it generates random hashes, which are almost impossible to be all-zero (which would emulate a coinbase transaction and trigger the bug). I added it while investigating and I thought it wouldn't hurt to be added. (It will also be useful for Also use librustzcash for signature hashes (sighash) in V4 transactions and earlier #2183)

Note: our intention, as discussed in Discord, was to replace the entire sighash with librustzcash implementation. However I ended up identifying the bug while preparing the tests the replacement. We'll probably end up going forward with this strategy (#2183), but I wanted to fix the bug before doing that (and deleting the zebra sighash code) so that if for whatever reason we decide one day to go back to our own implementation, it won't have this bug.

Review

See #1939 for what this is blocking.

I think anyone can review this.

Reviewer Checklist

• Code implements Specs and Designs
• Tests for Expected Behaviour
• Tests for Errors

Follow Up Work

We'll probably follow this up with #2183

[dconnolly]

https://zips.z.cash/zip-0143, which does not have any exceptions for the coinbase transaction. (It could be more explicit about it...)

All the signature hash zips miss specifying this coinbase detail, and all the test vectors in zips and in zcash-hackworks fail to exercise it, and the new ZIP-244 non-malleable tx id mentions coinbase but not relevant to this bug. I'm not sure exactly where this should be clarified but it is underspecified and under-tested to achieve consensus from the specs.

[teor2345]

@conradoplg can you open a ticket in https://github.com/zcash/zips to specify the coinbase output sighash?

There's no need to open a PR, just describing the spec gap is enough.

Edit:
Let's also list ZIP-244 in the bug, because it also doesn't specify what happens to coinbase inputs:
https://zips.z.cash/zip-0244#t-2a-prevouts-digest

[teor2345]

Let's merge this after we've opened the ZIP bug ticket.

Then we can open another PR to switch to librustzcash sighash and delete this code.

(Please feel free to dismiss my review once the ticket is open.)

[conradoplg]

@conradoplg can you open a ticket in https://github.com/zcash/zips to specify the coinbase output sighash?

Opened in zcash/zips#535

Created the ticket as requested, which was the only thing left to do.