Bug: V5 Transaction's network upgrade is not verified before calculating the SigHash

[jvff]

Motivation

In zebra_consensus::transaction::Verifier::verify_v5_transaction, there's a validation of the network upgrade the transaction is part of (Verifier::verify_v5_transaction_upgrade) that prevents V5 transactions from being accepted before NU5 activation. However, the transaction's sighash is calculated before the network upgrade check. This means that an incorrect sighash is calculated unnecessarily if the transaction's network upgrade is before NU5.

@conradoplg noticed that the v5_transaction_is_rejected_before_nu5_activation test was failing after updating the sighash code to use librustzcash (#2183). This was because the test was marked should_panic, and it wasn't panicking anymore.

Initially, the test would panic because there was the sighash algorithm for V5 transactions wasn't implemented yet. After it was implemented, the test was still panicking because the sighash implementation would panic with the incorrect transaction version. Changing all sighash calculations to use librustzcash (#2469) does not panic when calculating the sighash, so using it led to the test failing, because it was now passing successfully when a panic was expected.

However, this should still be prevented in the transaction::Verifier code. A more general option is to change the check to be more generic, so that it can match multiple transaction versions (not just V5) to multiple network upgrades. This check can then be moved into the call method, together with some other more basic checks.

Specifications

These rules are already implemented, but the check needs to be earlier, and all these rules need to be documented:

• [Sapling to Canopy inclusive, pre-NU5] The transaction version number MUST be 4, and the version group ID MUST be 0x892F2085.
• [NU5 onward] The transaction version number MUST be 4 or 5. If the transaction version number is 4 then the version group ID MUST be 0x892F2085. If the transaction version number is 5 then the version group ID MUST be 0x26A7270A.

https://zips.z.cash/protocol/protocol.pdf#txnconsensus

These rules are redundant, or covered by the mandatory Canopy checkpoint:

• The transaction version number MUST be greater than or equal to 1.
• [Overwinter only, pre-Sapling] The transaction version number MUST be 3, and the version group ID MUST be 0x03C48270.

Related Work

#2469 uncovered this issue, initially introduced by how the code was structured in #2437.

[conradoplg]

#2679 reorders the network upgrade check and also adds the check for V4 transactions. But we still need to verify/document the version group ID check.

[conradoplg]

This will only close when #2679 is merged (which will happen soon anyway)