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

@@ -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.

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<alloc::sync::Arc<zebra_chain::block::Block>><alloc::sync::Arc<zebra_chain::block::Block>>, len=101)

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.

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

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(()));