Stop using prop_filter_map to produce valid sapling shielded data (#2579

) 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. Co-authored-by: Conrado Gouvea <conrado@zfnd.org>
ZcashFoundation · Aug 6, 2021 · faae1c0 · faae1c0
1 parent 4eb0344
commit faae1c0
Showing 1 changed file with 63 additions and 49 deletions.
diff --git a/zebra-chain/src/transaction/arbitrary.rs b/zebra-chain/src/transaction/arbitrary.rs
@@ -268,32 +268,39 @@ impl Arbitrary for sapling::TransferData<PerSpendAnchor> {
     type Parameters = ();
 
     fn arbitrary_with(_args: Self::Parameters) -> Self::Strategy {
-        // TODO: add an extra spend or output using Either, and stop using filter_map
-        (
-            vec(
-                any::<sapling::Spend<PerSpendAnchor>>(),
-                0..MAX_ARBITRARY_ITEMS,
-            ),
-            vec(any::<sapling::Output>(), 0..MAX_ARBITRARY_ITEMS),
-        )
-            .prop_filter_map(
-                "arbitrary v4 transfers with no spends and no outputs",
-                |(spends, outputs)| {
-                    if !spends.is_empty() {
-                        Some(sapling::TransferData::SpendsAndMaybeOutputs {
-                            shared_anchor: FieldNotPresent,
-                            spends: spends.try_into().unwrap(),
-                            maybe_outputs: outputs,
-                        })
-                    } else if !outputs.is_empty() {
-                        Some(sapling::TransferData::JustOutputs {
-                            outputs: outputs.try_into().unwrap(),
-                        })
+        vec(any::<sapling::Output>(), 0..MAX_ARBITRARY_ITEMS)
+            .prop_flat_map(|outputs| {
+                (
+                    if outputs.is_empty() {
+                        // must have at least one spend or output
+                        vec(
+                            any::<sapling::Spend<PerSpendAnchor>>(),
+                            1..MAX_ARBITRARY_ITEMS,
+                        )
                     } else {
-                        None
+                        vec(
+                            any::<sapling::Spend<PerSpendAnchor>>(),
+                            0..MAX_ARBITRARY_ITEMS,
+                        )
+                    },
+                    Just(outputs),
+                )
+            })
+            .prop_map(|(spends, outputs)| {
+                if !spends.is_empty() {
+                    sapling::TransferData::SpendsAndMaybeOutputs {
+                        shared_anchor: FieldNotPresent,
+                        spends: spends.try_into().unwrap(),
+                        maybe_outputs: outputs,
                     }
-                },
-            )
+                } else if !outputs.is_empty() {
+                    sapling::TransferData::JustOutputs {
+                        outputs: outputs.try_into().unwrap(),
+                    }
+                } else {
+                    unreachable!("there must be at least one generated spend or output")
+                }
+            })
             .boxed()
     }
 
@@ -304,33 +311,40 @@ impl Arbitrary for sapling::TransferData<SharedAnchor> {
     type Parameters = ();
 
     fn arbitrary_with(_args: Self::Parameters) -> Self::Strategy {
-        // TODO: add an extra spend or output using Either, and stop using filter_map
-        (
-            any::<sapling::tree::Root>(),
-            vec(
-                any::<sapling::Spend<SharedAnchor>>(),
-                0..MAX_ARBITRARY_ITEMS,
-            ),
-            vec(any::<sapling::Output>(), 0..MAX_ARBITRARY_ITEMS),
-        )
-            .prop_filter_map(
-                "arbitrary v5 transfers with no spends and no outputs",
-                |(shared_anchor, spends, outputs)| {
-                    if !spends.is_empty() {
-                        Some(sapling::TransferData::SpendsAndMaybeOutputs {
-                            shared_anchor,
-                            spends: spends.try_into().unwrap(),
-                            maybe_outputs: outputs,
-                        })
-                    } else if !outputs.is_empty() {
-                        Some(sapling::TransferData::JustOutputs {
-                            outputs: outputs.try_into().unwrap(),
-                        })
+        vec(any::<sapling::Output>(), 0..MAX_ARBITRARY_ITEMS)
+            .prop_flat_map(|outputs| {
+                (
+                    any::<sapling::tree::Root>(),
+                    if outputs.is_empty() {
+                        // must have at least one spend or output
+                        vec(
+                            any::<sapling::Spend<SharedAnchor>>(),
+                            1..MAX_ARBITRARY_ITEMS,
+                        )
                     } else {
-                        None
+                        vec(
+                            any::<sapling::Spend<SharedAnchor>>(),
+                            0..MAX_ARBITRARY_ITEMS,
+                        )
+                    },
+                    Just(outputs),
+                )
+            })
+            .prop_map(|(shared_anchor, spends, outputs)| {
+                if !spends.is_empty() {
+                    sapling::TransferData::SpendsAndMaybeOutputs {
+                        shared_anchor,
+                        spends: spends.try_into().unwrap(),
+                        maybe_outputs: outputs,
                     }
-                },
-            )
+                } else if !outputs.is_empty() {
+                    sapling::TransferData::JustOutputs {
+                        outputs: outputs.try_into().unwrap(),
+                    }
+                } else {
+                    unreachable!("there must be at least one generated spend or output")
+                }
+            })
             .boxed()
     }