From f9c06205766547011fd3181b88aacef2400e0c80 Mon Sep 17 00:00:00 2001 From: teor Date: Sat, 31 Jul 2021 03:49:25 +1000 Subject: [PATCH] Remove unreliable generated chain prevouts tests (#2548) And adjust the chain lengths for better coverage. --- zebra-chain/src/block/arbitrary.rs | 26 +++++++++++-------- zebra-chain/src/block/tests/prop.rs | 15 +---------- zebra-state/src/service/arbitrary.rs | 5 +++- .../service/non_finalized_state/tests/prop.rs | 18 +++---------- 4 files changed, 23 insertions(+), 41 deletions(-) diff --git a/zebra-chain/src/block/arbitrary.rs b/zebra-chain/src/block/arbitrary.rs index 25b15e93..dcf06b82 100644 --- a/zebra-chain/src/block/arbitrary.rs +++ b/zebra-chain/src/block/arbitrary.rs @@ -22,23 +22,25 @@ use crate::{ use super::*; -/// The chain height used to test for prevout inputs. +/// The chain length for zebra-chain proptests. /// -/// This impacts the probability of `has_prevouts` failures in -/// `arbitrary_height_partial_chain_strategy`. +/// Most generated chains will contain transparent spends at or before this height. /// -/// The failure probability calculation is: +/// This height was chosen a tradeoff between chains with no spends, +/// and chains which spend outputs created by previous spends. +/// +/// The raw probability of having no spends during a test run is: /// ```text /// shielded_input = shielded_pool_count / pool_count /// expected_transactions = expected_inputs = MAX_ARBITRARY_ITEMS/2 -/// proptest_cases = 256 -/// number_of_proptests = 5 as of July 2021 (PREVOUTS_CHAIN_HEIGHT and PartialChain tests) -/// shielded_input^(expected_transactions * expected_inputs * PREVOUTS_CHAIN_HEIGHT) * proptest_cases * number_of_proptests +/// shielded_input^(expected_transactions * expected_inputs * (PREVOUTS_CHAIN_HEIGHT - 1)) /// ``` /// -/// `PREVOUTS_CHAIN_HEIGHT` should be increased, and `proptest_cases` should be reduced, -/// so that the failure probability is less than 1 in 1 million. -pub const PREVOUTS_CHAIN_HEIGHT: usize = 20; +/// This probability is approximately 3%. However, proptest generation and +/// minimisation strategies can create additional chains with no transparent spends. +/// +/// To increase the proportion of test runs with proptest spends, increase `PREVOUTS_CHAIN_HEIGHT`. +pub const PREVOUTS_CHAIN_HEIGHT: usize = 4; #[derive(Debug, Clone, Copy)] #[non_exhaustive] @@ -400,8 +402,10 @@ impl Block { block.transactions = new_transactions; // TODO: if needed, fixup: - // - transaction output counts (currently 0..=16, consensus rules require 1..) + // - transparent values and shielded value balances + // - transaction outputs (currently 0..=16 outputs, consensus rules require 1..) // - history and authorizing data commitments + // - the transaction merkle root // now that we've made all the changes, calculate our block hash, // so the next block can use it diff --git a/zebra-chain/src/block/tests/prop.rs b/zebra-chain/src/block/tests/prop.rs index c20895a2..8399fa0e 100644 --- a/zebra-chain/src/block/tests/prop.rs +++ b/zebra-chain/src/block/tests/prop.rs @@ -7,7 +7,6 @@ use zebra_test::prelude::*; use crate::{ parameters::{Network, GENESIS_PREVIOUS_BLOCK_HASH}, serialization::{SerializationError, ZcashDeserializeInto, ZcashSerialize}, - transaction::arbitrary::MAX_ARBITRARY_ITEMS, LedgerState, }; @@ -172,7 +171,7 @@ fn genesis_partial_chain_strategy() -> Result<()> { let strategy = LedgerState::genesis_strategy(None, None, false).prop_flat_map(|init| { Block::partial_chain_strategy( init, - MAX_ARBITRARY_ITEMS, + PREVOUTS_CHAIN_HEIGHT, allow_all_transparent_coinbase_spends, ) }); @@ -212,7 +211,6 @@ fn genesis_partial_chain_strategy() -> Result<()> { /// Make sure our block height strategy generates a chain with: /// - correct coinbase heights /// - correct previous block hashes -/// - at least one transparent PrevOut input in the entire chain #[test] fn arbitrary_height_partial_chain_strategy() -> Result<()> { zebra_test::init(); @@ -230,7 +228,6 @@ fn arbitrary_height_partial_chain_strategy() -> Result<()> { proptest!(|(chain in strategy)| { let mut height = None; let mut previous_block_hash = None; - let mut has_prevouts = false; for block in chain { if height.is_none() { @@ -242,18 +239,8 @@ fn arbitrary_height_partial_chain_strategy() -> Result<()> { prop_assert_eq!(Some(block.header.previous_block_hash), previous_block_hash); } - has_prevouts |= block - .transactions - .iter() - .flat_map(|t| t.inputs()) - .find_map(|i| i.outpoint()) - .is_some(); - previous_block_hash = Some(block.hash()); } - - // this assertion checks that we're covering transparent spends - prop_assert!(has_prevouts, "unexpected missing prevouts in entire chain"); }); Ok(()) diff --git a/zebra-state/src/service/arbitrary.rs b/zebra-state/src/service/arbitrary.rs index ab801c3c..4168df13 100644 --- a/zebra-state/src/service/arbitrary.rs +++ b/zebra-state/src/service/arbitrary.rs @@ -20,7 +20,10 @@ use super::*; /// The chain length for state proptests. /// -/// Shorter lengths increase the probability of proptest failures. +/// Most generated chains will contain transparent spends at or before this height. +/// +/// This height was chosen as a tradeoff between chains with no transparent spends, +/// and chains which spend outputs created by previous spends. /// /// See [`block::arbitrary::PREVOUTS_CHAIN_HEIGHT`] for details. pub const MAX_PARTIAL_CHAIN_BLOCKS: usize = diff --git a/zebra-state/src/service/non_finalized_state/tests/prop.rs b/zebra-state/src/service/non_finalized_state/tests/prop.rs index 1989fb03..4f3d4a8a 100644 --- a/zebra-state/src/service/non_finalized_state/tests/prop.rs +++ b/zebra-state/src/service/non_finalized_state/tests/prop.rs @@ -25,7 +25,6 @@ const DEFAULT_PARTIAL_CHAIN_PROPTEST_CASES: u32 = 1; /// /// Also check for: /// - no transparent spends in the genesis block, because genesis transparent outputs are ignored -/// - at least one transparent PrevOut input in the entire chain #[test] fn forked_equals_pushed() -> Result<()> { zebra_test::init(); @@ -40,7 +39,6 @@ fn forked_equals_pushed() -> Result<()> { let mut full_chain = Chain::new(Default::default(), Default::default()); let mut partial_chain = Chain::new(Default::default(), Default::default()); - let mut has_prevouts = false; for block in chain.iter().take(fork_at_count) { partial_chain = partial_chain.push(block.clone())?; @@ -59,18 +57,12 @@ fn forked_equals_pushed() -> Result<()> { .filter_map(|i| i.outpoint()) .count(), 0, - "unexpected transparent prevout inputs at height {:?}: genesis transparent outputs are ignored", + "unexpected transparent prevout input at height {:?}: \ + genesis transparent outputs must be ignored, \ + so there can not be any spends in the genesis block", block.height, ); } - - has_prevouts |= block - .block - .transactions - .iter() - .flat_map(|t| t.inputs()) - .find_map(|i| i.outpoint()) - .is_some(); } let forked = full_chain @@ -85,10 +77,6 @@ fn forked_equals_pushed() -> Result<()> { // the first check is redundant, but it's useful for debugging prop_assert_eq!(forked.blocks.len(), partial_chain.blocks.len()); prop_assert!(forked.eq_internal_state(&partial_chain)); - - // this assertion checks that we're still generating some transparent spends, - // after proptests remove unshielded and immature transparent coinbase spends - prop_assert!(has_prevouts, "no blocks in chain had prevouts"); }); Ok(())