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

Accept high version numbers in non-overwinter transactions #2207

Closed
3 tasks
conradoplg opened this issue May 26, 2021 · 2 comments
Closed
3 tasks

Accept high version numbers in non-overwinter transactions #2207

conradoplg opened this issue May 26, 2021 · 2 comments
Labels
A-consensus Area: Consensus rule updates A-rust Area: Updates to Rust code C-bug Category: This is a bug I-consensus Zebra breaks a Zcash consensus rule NU Sprout Network Upgrade: Sprout specific tasks (before Overwinter)

Comments

@conradoplg
Copy link
Collaborator

conradoplg commented May 26, 2021

Version

zebra-chain v1.0.0-alpha.8 commit 227757a

Platform

Linux 5.8.0-53-generic #60~20.04.1-Ubuntu SMP Thu May 6 09:52:46 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux

Description

Zebra does not accept high version numbers in pre-overwinter transactions.

I tried this:

Deserialize transaction from https://github.com/zcash/zcash/blob/master/src/test/data/sighash.json (e.g. the first one)

I expected to see this happen: the transaction to be successfully deserialized

Instead, this happened: parse error: bad tx header

See section 7.1 of the spec:

Version constraints apply to the effectiveVersion, which is equal to min(2, version) when fOverwintered = 0 and
to version otherwise.

Solution

  • Add a u32 version field to Transaction::V2
  • Serialize and deserialize it
  • Add round-trip serialization tests with the test vectors that are currently failing

Commands

N/A

Logs

N/A

@conradoplg conradoplg added C-bug Category: This is a bug S-needs-triage Status: A bug report needs triage labels May 26, 2021
@teor2345 teor2345 added A-consensus Area: Consensus rule updates A-rust Area: Updates to Rust code I-consensus Zebra breaks a Zcash consensus rule NU Sprout Network Upgrade: Sprout specific tasks (before Overwinter) P-Low labels May 26, 2021
@teor2345
Copy link
Contributor

In practice this issue never actually happened on the Mainnet or Testnet chain, so it's a low-priority fix. (Unless it's blocking other necessary work.)

@conradoplg
Copy link
Collaborator Author

We don't need this since we checkpoint on Canopy and no existing transactions has high version numbers.

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 A-rust Area: Updates to Rust code C-bug Category: This is a bug I-consensus Zebra breaks a Zcash consensus rule NU Sprout Network Upgrade: Sprout specific tasks (before Overwinter)
Projects
None yet
Development

No branches or pull requests

3 participants