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

Test consensus-critical Amount deserialization #2487

Merged
merged 7 commits into from
Jul 15, 2021

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Jul 13, 2021

Motivation

Zebra isn't testing consensus-critical serialization for Amount.

Previously we were testing serde and bincode serialization, which uses a completely different code path.

This change is important to do now, because:

  • it's a consensus rule
  • we are just about to create more tests for ValueBalance using the Amount tests as examples

Specifications

[Sapling onward] valueBalanceSapling MUST be in the range {−MAX_MONEY .. MAX_MONEY}.
[NU5 onward] valueBalanceOrchard MUST be in the range {−MAX_MONEY .. MAX_MONEY} for version 5 transactions.

And similar rules for transparent and Sprout.

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

Solution

  • Replace bincode serialization tests with ZcashDeserialize tests
    • Remove the bincode test dependency
  • Show the constraint in the Amount debug format

Review

@oxarbitrage can review this bug fix, it blocks the tests for PR #2486.

Reviewer Checklist

  • Tests for Expected Behaviour
  • Tests for Errors

teor2345 added 2 commits July 13, 2021 10:11
Previously we were testing `serde` and `bincode` serialization,
which uses a completely different code path.
@teor2345 teor2345 added C-bug Category: This is a bug A-dependencies Area: Dependency file updates A-consensus Area: Consensus rule updates A-rust Area: Updates to Rust code P-Medium labels Jul 13, 2021
@teor2345 teor2345 added this to the 2021 Sprint 13 milestone Jul 13, 2021
@teor2345 teor2345 requested a review from oxarbitrage July 13, 2021 00:37
@teor2345 teor2345 self-assigned this Jul 13, 2021
@teor2345
Copy link
Contributor Author

Linux failed due to #2450, so I restarted CI

Copy link
Contributor

@oxarbitrage oxarbitrage left a comment

Choose a reason for hiding this comment

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

This looks good, thanks.

@oxarbitrage oxarbitrage enabled auto-merge (squash) July 15, 2021 18:36
@oxarbitrage oxarbitrage merged commit cd78241 into main Jul 15, 2021
@oxarbitrage oxarbitrage deleted the fix-amount-tests-debug branch July 15, 2021 21:34
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-dependencies Area: Dependency file updates A-rust Area: Updates to Rust code C-bug Category: This is a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants