From 936168b40dd7edb76d0c60b61141f57a7b72b09c Mon Sep 17 00:00:00 2001 From: teor Date: Fri, 2 Jul 2021 09:21:22 +1000 Subject: [PATCH] Fix failing legacy chain tests (#2427) * Skip invalid legacy chain check test cases Add proptest seeds for the failing test. And improve some unclear documentation. * Fix the legacy chain test blocks order Also fix unclear documentation that might have led to this bug. --- zebra-chain/src/block/arbitrary.rs | 4 +- .../proptest-regressions/service/tests.txt | 7 ++++ zebra-state/src/service.rs | 2 +- zebra-state/src/service/arbitrary.rs | 1 + zebra-state/src/service/tests.rs | 38 ++++++++++++++++--- 5 files changed, 43 insertions(+), 9 deletions(-) create mode 100644 zebra-state/proptest-regressions/service/tests.txt diff --git a/zebra-chain/src/block/arbitrary.rs b/zebra-chain/src/block/arbitrary.rs index 186ff06f..5091fdfc 100644 --- a/zebra-chain/src/block/arbitrary.rs +++ b/zebra-chain/src/block/arbitrary.rs @@ -47,7 +47,7 @@ pub struct LedgerState { /// Every V5 and later transaction has a valid `network_upgrade` field. /// - /// If `false`, some transactions have invalid network upgrades. + /// If `false`, zero or more transactions may have invalid network upgrades. transaction_has_valid_network_upgrade: bool, /// Generate coinbase transactions. @@ -79,7 +79,7 @@ pub struct LedgerStateOverride { /// Every V5 and later transaction has a valid `network_upgrade` field. /// - /// If `false`, some transactions have invalid network upgrades. + /// If `false`, zero or more transactions may have invalid network upgrades. pub transaction_has_valid_network_upgrade: bool, /// Every block has exactly one coinbase transaction. diff --git a/zebra-state/proptest-regressions/service/tests.txt b/zebra-state/proptest-regressions/service/tests.txt new file mode 100644 index 00000000..2aa3a91e --- /dev/null +++ b/zebra-state/proptest-regressions/service/tests.txt @@ -0,0 +1,7 @@ +# Seeds for failure cases proptest has generated in the past. It is +# automatically read and these particular cases re-run before any +# novel cases are generated. +# +# It is recommended to check this file in to source control so that +# everyone who runs the test benefits from these saved cases. +cc 37aea4b0880d7d9029ea4fad0136bd8553f81eea0435122737ec513f4f6fb73c # shrinks to (network, nu_activation_height, chain) = (Mainnet, Height(1046400), alloc::vec::Vec>>, len=101) diff --git a/zebra-state/src/service.rs b/zebra-state/src/service.rs index 024f3add..6a38c2a4 100644 --- a/zebra-state/src/service.rs +++ b/zebra-state/src/service.rs @@ -315,7 +315,7 @@ impl StateService { } /// Return an iterator over the relevant chain of the block identified by - /// `hash`. + /// `hash`, in order from the largest height to the genesis block. /// /// The block identified by `hash` is included in the chain of blocks yielded /// by the iterator. `hash` can come from any chain. diff --git a/zebra-state/src/service/arbitrary.rs b/zebra-state/src/service/arbitrary.rs index 6ff5325d..3b85151c 100644 --- a/zebra-state/src/service/arbitrary.rs +++ b/zebra-state/src/service/arbitrary.rs @@ -97,6 +97,7 @@ impl Strategy for PreparedChain { /// Arguments: /// - `transaction_version_override`: See `LedgerState::height_strategy` for details. /// - `transaction_has_valid_network_upgrade`: See `LedgerState::height_strategy` for details. +/// Note: `false` allows zero or more invalid network upgrades. /// - `blocks_after_nu_activation`: The number of blocks the strategy will generate /// after the provided `network_upgrade`. /// - `network_upgrade` - The network upgrade that we are using to simulate from where the diff --git a/zebra-state/src/service/tests.rs b/zebra-state/src/service/tests.rs index 90cda9ed..7ba0201f 100644 --- a/zebra-state/src/service/tests.rs +++ b/zebra-state/src/service/tests.rs @@ -202,7 +202,7 @@ proptest! { fn some_block_less_than_network_upgrade( (network, nu_activation_height, chain) in arbitrary::partial_nu5_chain_strategy(4, true, BLOCKS_AFTER_NU5/2, NetworkUpgrade::Canopy) ) { - let response = crate::service::legacy_chain_check(nu_activation_height, chain.into_iter(), network) + let response = crate::service::legacy_chain_check(nu_activation_height, chain.into_iter().rev(), network) .map_err(|error| error.to_string()); prop_assert_eq!(response, Ok(())); @@ -213,7 +213,7 @@ proptest! { fn no_transaction_with_network_upgrade( (network, nu_activation_height, chain) in arbitrary::partial_nu5_chain_strategy(4, true, BLOCKS_AFTER_NU5, NetworkUpgrade::Canopy) ) { - let response = crate::service::legacy_chain_check(nu_activation_height, chain.into_iter(), network) + let response = crate::service::legacy_chain_check(nu_activation_height, chain.into_iter().rev(), network) .map_err(|error| error.to_string()); prop_assert_eq!( @@ -227,12 +227,38 @@ proptest! { fn at_least_one_transaction_with_inconsistent_network_upgrade( (network, nu_activation_height, chain) in arbitrary::partial_nu5_chain_strategy(5, false, BLOCKS_AFTER_NU5, NetworkUpgrade::Canopy) ) { - let response = crate::service::legacy_chain_check(nu_activation_height, chain.into_iter(), network) - .map_err(|error| error.to_string()); + // this test requires that an invalid block is encountered + // before a valid block (and before the check gives up), + // but setting `transaction_has_valid_network_upgrade` to false + // sometimes generates blocks with all valid (or missing) network upgrades + + // we must check at least one block, and the first checked block must be invalid + let first_checked_block = chain + .iter() + .rev() + .take_while(|block| block.coinbase_height().unwrap() >= nu_activation_height) + .take(100) + .next(); + prop_assume!(first_checked_block.is_some()); + prop_assume!( + first_checked_block + .unwrap() + .check_transaction_network_upgrade_consistency(network) + .is_err() + ); + + let response = crate::service::legacy_chain_check( + nu_activation_height, + chain.clone().into_iter().rev(), + network + ).map_err(|error| error.to_string()); prop_assert_eq!( response, - Err("inconsistent network upgrade found in transaction".into()) + Err("inconsistent network upgrade found in transaction".into()), + "first: {:?}, last: {:?}", + chain.first().map(|block| block.coinbase_height()), + chain.last().map(|block| block.coinbase_height()), ); } @@ -241,7 +267,7 @@ proptest! { fn at_least_one_transaction_with_valid_network_upgrade( (network, nu_activation_height, chain) in arbitrary::partial_nu5_chain_strategy(5, true, BLOCKS_AFTER_NU5/2, NetworkUpgrade::Canopy) ) { - let response = crate::service::legacy_chain_check(nu_activation_height, chain.into_iter(), network) + let response = crate::service::legacy_chain_check(nu_activation_height, chain.into_iter().rev(), network) .map_err(|error| error.to_string()); prop_assert_eq!(response, Ok(()));