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

Stop using prop_filter_map to produce valid sapling shielded data #2579

Merged
merged 2 commits into from
Aug 6, 2021

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Aug 6, 2021

Motivation

To test Zebra using generated data, we need proptests to:

  • be efficient,
  • produce good error logs
  • produce proptest seeds (especially in CI)

This is related to tickets #2381 and #1895, and a lot of other current validation tickets.
But it doesn't close any of those tickets.

Solution

  • generate valid sapling data on every generation run, by always making sure there is at least one spend or output

Previously, on 1/16 runs Zebra would generate invalid data, then skip it using prop_filter_map. This was a particular issue during test case minimisation, because minimisation sets vector sizes to zero. (Zero spends and zero outputs is invalid.)

Review

Anyone can review this PR. It's not particularly urgent.

Reviewer Checklist

  • Code makes sense
  • Existing tests still pass

This change is Reviewable

This improves proptest results in CI and locally.

Proptests should be faster, because they are not discarding 1/16 results.

Failures should be minimised more often, improving failure logs,
and generating proptest seeds locally and in CI.
@teor2345 teor2345 added NU-1 Sapling Network Upgrade: Sapling specific tasks A-rust Area: Updates to Rust code C-enhancement Category: This is an improvement P-Medium labels Aug 6, 2021
@teor2345 teor2345 self-assigned this Aug 6, 2021
Copy link
Collaborator

@conradoplg conradoplg left a comment

Choose a reason for hiding this comment

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

Great solution! 👍

@conradoplg conradoplg enabled auto-merge (squash) August 6, 2021 17:06
@conradoplg conradoplg merged commit faae1c0 into main Aug 6, 2021
@conradoplg conradoplg deleted the prop-minimise-sapling branch August 6, 2021 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rust Area: Updates to Rust code C-enhancement Category: This is an improvement NU-1 Sapling Network Upgrade: Sapling specific tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants