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 39 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.
  • Document that we now implement the consensus rules listed above.
  • Impl NetworkUpgrade::previous_upgrade and refactor NetworkUpgrade::next_upgrade so that it doesn't explicitly enumerate all upgrades.
  • Don't make NETWORK_UPGRADES_IN_ORDER pub.

Tests

  • Add tests that check that tx SIGHASH precomputation and computation fail 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.

@github-actions github-actions bot added the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label Jan 30, 2025
@upbqdn upbqdn removed the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label Jan 30, 2025
Copy link
Contributor

@arya2 arya2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thank you!

zebra-consensus/src/transaction.rs Show resolved Hide resolved
Comment on lines +444 to +449
verifier
.is_valid(NetworkUpgrade::Blossom, input_index + 1)
.expect_err("verification should fail");
verifier
.is_valid(NetworkUpgrade::Blossom, input_index + 1)
.expect_err("verification should fail");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
verifier
.is_valid(NetworkUpgrade::Blossom, input_index + 1)
.expect_err("verification should fail");
verifier
.is_valid(NetworkUpgrade::Blossom, input_index + 1)
.expect_err("verification should fail");
verifier
.is_valid(NetworkUpgrade::Blossom, input_index + 1)
.expect_err("verification should fail");

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 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
3 participants