Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(consensus): Verify consensus branch ID in SIGHASH precomputation #9139

Open
wants to merge 24 commits into
base: main
Choose a base branch
from

Conversation

upbqdn
Copy link
Member

@upbqdn upbqdn commented Jan 18, 2025

Motivation

Close #9089.

Specifications & References

§ 4.10 SIGHASH Transaction Hashing

[NU5 only, pre-NU6] All transactions MUST use the NU5 consensus branch ID 0xF919A198 as defined in ZIP-252.

[NU6 only] All transactions MUST use the NU6 consensus branch ID 0xC8E71055 as defined in ZIP-253.

Solution

  • Implement a check ensuring compliance with the consensus rules listed above. This check makes the tx SIGHASH precomputation panic if the tx contains an invalid value in its nConsensusBranchId field. This precomputation is implemented in zebra-chain, and is used by the tx verifier in zebra-consensus and by the transparent script verifier in zebra-script, but this verifier is only called from the tx verifier. It might be worth optimizing this, but that is currently not possible, as described in Compute the tx SIGHASH only once per tx verification #9165. The panic should never occur in regular use because the tx verifier implements another consensus branch ID check according to another consensus rule implemented here: https://github.com/zcashfoundation/zebra/blob/05a9b6171ccbc06bcc888470e4be8a60b1d574ef/zebra-consensus/src/transaction/check.rs#L513-L552, which takes effect early on and doesn't panic.
  • Add new error types in zebra-chain.
  • Pass NetworkUpgrade instead of ConsensusBranchId to the SIGHASH precomputation and tx conversion.
  • Use TryFrom instead of ad-hoc functions for conversion.
  • Remove a redundant TryFrom<&Transaction> for zcash_primitives::transaction::Transaction

Tests

  • Add tests that check that tx verification fails when a tx attempts to use an invalid consensus branch id.

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.

@upbqdn upbqdn added C-bug Category: This is a bug A-consensus Area: Consensus rule updates NU-6 Network Upgrade: NU6 specific tasks P-Medium ⚡ labels Jan 24, 2025
@upbqdn upbqdn changed the title fix(consensus): Verify consensus branch ID before SIGHASH computation fix(consensus): Verify consensus branch ID in SIGHASH precomputation Jan 24, 2025
@github-actions github-actions bot added the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label Jan 24, 2025
@upbqdn upbqdn marked this pull request as ready for review January 24, 2025 15:18
@upbqdn upbqdn requested review from a team as code owners January 24, 2025 15:18
@upbqdn upbqdn requested review from arya2 and removed request for a team January 24, 2025 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-consensus Area: Consensus rule updates C-bug Category: This is a bug C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG NU-6 Network Upgrade: NU6 specific tasks P-Medium ⚡
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The tx verifier does not enforce using the correct consensus branch ID in the SIGHASH precomputation
2 participants