add(consensus): Check consensus branch ids in tx verifier

[upbqdn]

Motivation

Close #9064.

Context

The tx verifier is not checking if the nConsensusBranchId field in V5 txs conforms to the following consensus rule:

[NU5 onward] If effectiveVersion ≥ 5, the nConsensusBranchId field MUST match the consensus branch ID used for SIGHASH transaction hashes, as specified in ZIP-244.

We have this check in the block verifier. However, since the mempool uses only the tx verifier, it can currently accept txs with an incorrect nConsenusBranchId.

Specifications & References

• The consensus rules in § 7.1.2 Transaction Consensus Rules.

Solution

• Add the check mentioned above to the tx verifier.
• Clean-ups:
  • Move test-only tx methods out of the default impl

Tests

• Add unit tests.
• Adjust existing unit tests.

PR Author's Checklist

• The PR name will make sense to users.
• The PR provides a CHANGELOG summary.
• The solution is tested.
• The documentation is up to date.
• The PR has a priority label.

PR Reviewer's Checklist

• The PR Author's checklist is complete.
• The PR resolves the issue.

[conradoplg]

Just for reference I think the 4.10 rules are about the branch ID used in SIGHASH. What we seem to have missed is from 7.1.2:

the nConsensusBranchId field MUST match the consensus branch ID used for SIGHASH transaction hashes

which we check for transactions in blocks, but not for transactions in the mempool.

[conradoplg]

This looks good, but since it's a consensus change, I'll leave approval for the next reviewer

[conradoplg]

I confirmed that running a sendrawtransaction with an outdated-Ywallet-generated tx returns:

error code: 0
error message:
transaction did not pass consensus validation

[daira]

utACK for the non-test code. I did not thoroughly review the tests.

[oxarbitrage]

looks good, thank you @upbqdn

[upbqdn]

Just for reference I think the 4.10 rules are about the branch ID used in SIGHASH. What we seem to have missed is from 7.1.2:

the nConsensusBranchId field MUST match the consensus branch ID used for SIGHASH transaction hashes

which we check for transactions in blocks, but not for transactions in the mempool.

I agree. To reflect this, I updated the implementation in b83ee0b and 4f37471 and the PR description as well.

[upbqdn]

Consensus rules often start with conditions such as "[NU5 onward]". We take those conditions as implicit in the code, but we could implement them by checking what NU is active for the thing being checked. Should we start making those conditions explicit?

[upbqdn]

I made the NU5 onward condition explicit in c698507.

[arya2]

This looks really good, thank you for the fix and the related cleanups.

I left a few suggestions for cleanups but there are no blockers.