refactor(state): Simplify difficulty and median-time-past state and mempool requests (#6031)

* Clarify function docs, rename variables, and fix log typos

* Add a ReadState best chain clone method, but don't use it yet

* Use the new calculate_median_time_past() function in existing code

* Skip a state request if the lock time is a height

* Remove dummy arguments and extra blocks from median-time-past calculation

* Update tests to remove requests that are no longer sent

* Simplify getting the best chain

Co-authored-by: Arya <aryasolhi@gmail.com>

* Clarify some function docs

Co-authored-by: Arya <aryasolhi@gmail.com>

* assigns `next_median_time_past` value from if statement

---------

Co-authored-by: Arya <aryasolhi@gmail.com>

zebra-chain/src/transaction.rs

@@ -363,6 +363,17 @@ impl Transaction {
         }
     }
 
+    /// Returns `true` if this transaction's `lock_time` is a [`LockTime::Time`].
+    /// Returns `false` if it is a [`LockTime::Height`] (locked or unlocked), is unlocked,
+    /// or if the transparent input sequence numbers have disabled lock times.
+    pub fn lock_time_is_time(&self) -> bool {
+        if let Some(lock_time) = self.lock_time() {
+            return lock_time.is_time();
+        }
+
+        false
+    }
+
     /// Get this transaction's expiry height, if any.
     pub fn expiry_height(&self) -> Option<block::Height> {
         match self {

zebra-chain/src/transaction/lock_time.rs

@@ -93,6 +93,11 @@ impl LockTime {
                 .expect("in-range number of seconds and valid nanosecond"),
         )
     }
+
+    /// Returns `true` if this lock time is a [`LockTime::Time`], or `false` if it is a [`LockTime::Height`].
+    pub fn is_time(&self) -> bool {
+        matches!(self, LockTime::Time(_))
+    }
 }
 
 impl ZcashSerialize for LockTime {

zebra-consensus/src/transaction.rs

@@ -347,29 +347,18 @@ where
             if let Some(block_time) = req.block_time() {
                 check::lock_time_has_passed(&tx, req.height(), block_time)?;
             } else {
-                // This state query is much faster than loading UTXOs from the database,
-                // so it doesn't need to be executed in parallel
-                let state = state.clone();
-                let next_median_time_past = Self::mempool_best_chain_next_median_time_past(state).await?;
+                // Skip the state query if we don't need the time for this check.
+                let next_median_time_past = if tx.lock_time_is_time() {
+                    // This state query is much faster than loading UTXOs from the database,
+                    // so it doesn't need to be executed in parallel
+                    let state = state.clone();
+                    Some(Self::mempool_best_chain_next_median_time_past(state).await?.to_chrono())
+                } else {
+                    None
+                };
 
-                // # Consensus
-                //
-                // > the nTime field MUST represent a time strictly greater than the median of the
-                // > timestamps of the past PoWMedianBlockSpan blocks.
-                // <https://zips.z.cash/protocol/protocol.pdf#blockheader>
-                //
-                // > The transaction can be added to any block whose block time is greater than the locktime.
-                // <https://developer.bitcoin.org/devguide/transactions.html#locktime-and-sequence-number>
-                //
-                // If the transaction's lock time is less than the median-time-past,
-                // it will always be less than the next block's time,
-                // because the next block's time is strictly greater than the median-time-past.
-                //
-                // This is the rule implemented by `zcashd`'s mempool:
-                // <https://github.com/zcash/zcash/blob/9e1efad2d13dca5ee094a38e6aa25b0f2464da94/src/main.cpp#L776-L784>
-                //
                 // This consensus check makes sure Zebra produces valid block templates.
-                check::lock_time_has_passed(&tx, req.height(), next_median_time_past.to_chrono())?;
+                check::lock_time_has_passed(&tx, req.height(), next_median_time_past)?;
             }
 
             // "The consensus rules applied to valueBalance, vShieldedOutput, and bindingSig

zebra-consensus/src/transaction/check.rs

@@ -19,19 +19,47 @@ use crate::error::TransactionError;
 
 /// Checks if the transaction's lock time allows this transaction to be included in a block.
 ///
-/// Consensus rule:
+/// Arguments:
+/// - `block_height`: the height of the mined block, or the height of the next block for mempool
+///                   transactions
+/// - `block_time`: the time in the mined block header, or the median-time-past of the next block
+///                 for the mempool. Optional if the lock time is a height.
+///
+/// # Panics
+///
+/// If the lock time is a time, and `block_time` is `None`.
+///
+/// # Consensus
 ///
 /// > The transaction must be finalized: either its locktime must be in the past (or less
 /// > than or equal to the current block height), or all of its sequence numbers must be
 /// > 0xffffffff.
 ///
 /// [`Transaction::lock_time`] validates the transparent input sequence numbers, returning [`None`]
-/// if they indicate that the transaction is finalized by them. Otherwise, this function validates
-/// if the lock time is in the past.
+/// if they indicate that the transaction is finalized by them.
+/// Otherwise, this function checks that the lock time is in the past.
+///
+/// ## Mempool Consensus for Block Templates
+///
+/// > the nTime field MUST represent a time strictly greater than the median of the
+/// > timestamps of the past PoWMedianBlockSpan blocks.
+/// <https://zips.z.cash/protocol/protocol.pdf#blockheader>
+///
+/// > The transaction can be added to any block whose block time is greater than the locktime.
+/// <https://developer.bitcoin.org/devguide/transactions.html#locktime-and-sequence-number>
+///
+/// If the transaction's lock time is less than the median-time-past,
+/// it will always be less than the next block's time,
+/// because the next block's time is strictly greater than the median-time-past.
+/// (That is, `lock-time < median-time-past < block-header-time`.)
+///
+/// Using `median-time-past + 1s` (the next block's mintime) would also satisfy this consensus rule,
+/// but we prefer the rule implemented by `zcashd`'s mempool:
+/// <https://github.com/zcash/zcash/blob/9e1efad2d13dca5ee094a38e6aa25b0f2464da94/src/main.cpp#L776-L784>
 pub fn lock_time_has_passed(
     tx: &Transaction,
     block_height: Height,
-    block_time: DateTime<Utc>,
+    block_time: impl Into<Option<DateTime<Utc>>>,
 ) -> Result<(), TransactionError> {
     match tx.lock_time() {
         Some(LockTime::Height(unlock_height)) => {
@@ -48,6 +76,10 @@ pub fn lock_time_has_passed(
         Some(LockTime::Time(unlock_time)) => {
             // > The transaction can be added to any block whose block time is greater than the locktime.
             // https://developer.bitcoin.org/devguide/transactions.html#locktime-and-sequence-number
+            let block_time = block_time
+                .into()
+                .expect("time must be provided if LockTime is a time");
+
             if block_time > unlock_time {
                 Ok(())
             } else {

zebra-consensus/src/transaction/tests.rs

@@ -1,4 +1,6 @@
 //! Tests for Zcash transaction consensus checks.
+//
+// TODO: split fixed test vectors into a `vectors` module?
 
 use std::{collections::HashMap, sync::Arc};
 
@@ -195,13 +197,8 @@ async fn mempool_request_with_missing_input_is_rejected() {
     };
 
     tokio::spawn(async move {
-        state
-            .expect_request(zebra_state::Request::BestChainNextMedianTimePast)
-            .await
-            .expect("verifier should call mock state service with correct request")
-            .respond(zebra_state::Response::BestChainNextMedianTimePast(
-                DateTime32::MAX,
-            ));
+        // The first non-coinbase transaction with transparent inputs in our test vectors
+        // does not use a lock time, so we don't see Request::BestChainNextMedianTimePast here
 
         state
             .expect_request(zebra_state::Request::UnspentBestChainUtxo(input_outpoint))
@@ -261,14 +258,6 @@ async fn mempool_request_with_present_input_is_accepted() {
     };
 
     tokio::spawn(async move {
-        state
-            .expect_request(zebra_state::Request::BestChainNextMedianTimePast)
-            .await
-            .expect("verifier should call mock state service with correct request")
-            .respond(zebra_state::Response::BestChainNextMedianTimePast(
-                DateTime32::MAX,
-            ));
-
         state
             .expect_request(zebra_state::Request::UnspentBestChainUtxo(input_outpoint))
             .await
@@ -405,16 +394,6 @@ async fn mempool_request_with_unlocked_lock_time_is_accepted() {
     };
 
     tokio::spawn(async move {
-        state
-            .expect_request(zebra_state::Request::BestChainNextMedianTimePast)
-            .await
-            .expect("verifier should call mock state service with correct request")
-            .respond(zebra_state::Response::BestChainNextMedianTimePast(
-                DateTime32::from(
-                    u32::try_from(LockTime::MIN_TIMESTAMP).expect("min time is valid"),
-                ),
-            ));
-
         state
             .expect_request(zebra_state::Request::UnspentBestChainUtxo(input_outpoint))
             .await
@@ -480,16 +459,6 @@ async fn mempool_request_with_lock_time_max_sequence_number_is_accepted() {
     };
 
     tokio::spawn(async move {
-        state
-            .expect_request(zebra_state::Request::BestChainNextMedianTimePast)
-            .await
-            .expect("verifier should call mock state service with correct request")
-            .respond(zebra_state::Response::BestChainNextMedianTimePast(
-                DateTime32::from(
-                    u32::try_from(LockTime::MIN_TIMESTAMP).expect("min time is valid"),
-                ),
-            ));
-
         state
             .expect_request(zebra_state::Request::UnspentBestChainUtxo(input_outpoint))
             .await

zebra-state/src/service.rs

@@ -47,7 +47,7 @@ use crate::{
         block_iter::any_ancestor_blocks,
         chain_tip::{ChainTipBlock, ChainTipChange, ChainTipSender, LatestChainTip},
         finalized_state::{FinalizedState, ZebraDb},
-        non_finalized_state::NonFinalizedState,
+        non_finalized_state::{Chain, NonFinalizedState},
         pending_utxos::PendingUtxos,
         queued_blocks::QueuedBlocks,
         watch_receiver::WatchReceiver,
@@ -803,6 +803,12 @@ impl ReadStateService {
     fn latest_non_finalized_state(&self) -> NonFinalizedState {
         self.non_finalized_state_receiver.cloned_watch_data()
     }
+
+    /// Gets a clone of the latest, best non-finalized chain from the `non_finalized_state_receiver`
+    #[allow(dead_code)]
+    fn latest_best_chain(&self) -> Option<Arc<Chain>> {
+        self.latest_non_finalized_state().best_chain().cloned()
+    }
 }
 
 impl Service<Request> for StateService {
@@ -1152,7 +1158,7 @@ impl Service<ReadRequest> for ReadStateService {
                         Ok(ReadResponse::Depth(depth))
                     })
                 })
-                .map(|join_result| join_result.expect("panic in ReadRequest::Tip"))
+                .map(|join_result| join_result.expect("panic in ReadRequest::Depth"))
                 .boxed()
             }
 
@@ -1268,7 +1274,9 @@ impl Service<ReadRequest> for ReadStateService {
                         Ok(ReadResponse::TransactionIdsForBlock(transaction_ids))
                     })
                 })
-                .map(|join_result| join_result.expect("panic in ReadRequest::Block"))
+                .map(|join_result| {
+                    join_result.expect("panic in ReadRequest::TransactionIdsForBlock")
+                })
                 .boxed()
             }
 
@@ -1348,7 +1356,7 @@ impl Service<ReadRequest> for ReadStateService {
                         ))
                     })
                 })
-                .map(|join_result| join_result.expect("panic in ReadRequest::Tip"))
+                .map(|join_result| join_result.expect("panic in ReadRequest::BlockLocator"))
                 .boxed()
             }
 
@@ -1379,7 +1387,7 @@ impl Service<ReadRequest> for ReadStateService {
                         Ok(ReadResponse::BlockHashes(block_hashes))
                     })
                 })
-                .map(|join_result| join_result.expect("panic in ReadRequest::Tip"))
+                .map(|join_result| join_result.expect("panic in ReadRequest::FindBlockHashes"))
                 .boxed()
             }
 
@@ -1415,7 +1423,7 @@ impl Service<ReadRequest> for ReadStateService {
                         Ok(ReadResponse::BlockHeaders(block_headers))
                     })
                 })
-                .map(|join_result| join_result.expect("panic in ReadRequest::Tip"))
+                .map(|join_result| join_result.expect("panic in ReadRequest::FindBlockHeaders"))
                 .boxed()
             }
 
@@ -1728,12 +1736,12 @@ impl Service<ReadRequest> for ReadStateService {
                         });
 
                         // The work is done in the future.
-                        timer.finish(module_path!(), line!(), "ReadRequest::ChainInfo");
+                        timer.finish(module_path!(), line!(), "ReadRequest::SolutionRate");
 
                         Ok(ReadResponse::SolutionRate(solution_rate))
                     })
                 })
-                .map(|join_result| join_result.expect("panic in ReadRequest::ChainInfo"))
+                .map(|join_result| join_result.expect("panic in ReadRequest::SolutionRate"))
                 .boxed()
             }
 

zebra-state/src/service/check/difficulty.rs

@@ -330,7 +330,7 @@ impl AdjustedDifficulty {
     /// slice of `PoWMedianBlockSpan` (11) blocks in the relevant chain.
     ///
     /// Implements `MedianTime` from the Zcash specification.
-    fn median_time(
+    pub(crate) fn median_time(
         mut median_block_span_times: [DateTime<Utc>; POW_MEDIAN_BLOCK_SPAN],
     ) -> DateTime<Utc> {
         median_block_span_times.sort_unstable();

zebra-state/src/service/read/difficulty.rs

@@ -16,11 +16,16 @@ use crate::{
     service::{
         any_ancestor_blocks,
         check::{
-            difficulty::{BLOCK_MAX_TIME_SINCE_MEDIAN, POW_ADJUSTMENT_BLOCK_SPAN},
+            difficulty::{
+                BLOCK_MAX_TIME_SINCE_MEDIAN, POW_ADJUSTMENT_BLOCK_SPAN, POW_MEDIAN_BLOCK_SPAN,
+            },
             AdjustedDifficulty,
         },
         finalized_state::ZebraDb,
-        read::{self, tree::history_tree, FINALIZED_STATE_QUERY_RETRIES},
+        read::{
+            self, find::calculate_median_time_past, tree::history_tree,
+            FINALIZED_STATE_QUERY_RETRIES,
+        },
         NonFinalizedState,
     },
     BoxError, GetBlockTemplateChainInfo,
@@ -36,36 +41,35 @@ pub const EXTRA_TIME_TO_MINE_A_BLOCK: u32 = POST_BLOSSOM_POW_TARGET_SPACING * 2;
 /// # Panics
 ///
 /// - If we don't have enough blocks in the state.
-/// - If a consistency check fails `RETRIES` times.
 pub fn get_block_template_chain_info(
     non_finalized_state: &NonFinalizedState,
     db: &ZebraDb,
     network: Network,
 ) -> Result<GetBlockTemplateChainInfo, BoxError> {
-    let mut relevant_chain_and_history_tree_result =
-        relevant_chain_and_history_tree(non_finalized_state, db);
+    let mut best_relevant_chain_and_history_tree_result =
+        best_relevant_chain_and_history_tree(non_finalized_state, db);
 
     // Retry the finalized state query if it was interrupted by a finalizing block.
     //
     // TODO: refactor this into a generic retry(finalized_closure, process_and_check_closure) fn
     for _ in 0..FINALIZED_STATE_QUERY_RETRIES {
-        if relevant_chain_and_history_tree_result.is_ok() {
+        if best_relevant_chain_and_history_tree_result.is_ok() {
             break;
         }
 
-        relevant_chain_and_history_tree_result =
-            relevant_chain_and_history_tree(non_finalized_state, db);
+        best_relevant_chain_and_history_tree_result =
+            best_relevant_chain_and_history_tree(non_finalized_state, db);
     }
 
-    let (tip_height, tip_hash, relevant_chain, history_tree) =
-        relevant_chain_and_history_tree_result?;
+    let (best_tip_height, best_tip_hash, best_relevant_chain, best_tip_history_tree) =
+        best_relevant_chain_and_history_tree_result?;
 
     Ok(difficulty_time_and_history_tree(
-        relevant_chain,
-        tip_height,
-        tip_hash,
+        best_relevant_chain,
+        best_tip_height,
+        best_tip_hash,
         network,
-        history_tree,
+        best_tip_history_tree,
     ))
 }
 
@@ -121,15 +125,17 @@ pub fn solution_rate(
     }
 }
 
-/// Do a consistency check by checking the finalized tip before and after all other database queries.
+/// Do a consistency check by checking the finalized tip before and after all other database
+/// queries.
 ///
-/// Returns the state tip, recent blocks in reverse order from the tip, and the tip history tree.
+/// Returns the best chain tip, recent blocks in reverse height order from the tip,
+/// and the tip history tree.
 /// Returns an error if the tip obtained before and after is not the same.
 ///
 /// # Panics
 ///
 /// - If we don't have enough blocks in the state.
-fn relevant_chain_and_history_tree(
+fn best_relevant_chain_and_history_tree(
     non_finalized_state: &NonFinalizedState,
     db: &ZebraDb,
 ) -> Result<
@@ -145,12 +151,13 @@ fn relevant_chain_and_history_tree(
         BoxError::from("Zebra's state is empty, wait until it syncs to the chain tip")
     })?;
 
-    let relevant_chain = any_ancestor_blocks(non_finalized_state, db, state_tip_before_queries.1);
-    let relevant_chain: Vec<_> = relevant_chain
+    let best_relevant_chain =
+        any_ancestor_blocks(non_finalized_state, db, state_tip_before_queries.1);
+    let best_relevant_chain: Vec<_> = best_relevant_chain
         .into_iter()
         .take(POW_ADJUSTMENT_BLOCK_SPAN)
         .collect();
-    let relevant_chain = relevant_chain.try_into().map_err(|_error| {
+    let best_relevant_chain = best_relevant_chain.try_into().map_err(|_error| {
         "Zebra's state only has a few blocks, wait until it syncs to the chain tip"
     })?;
 
@@ -173,14 +180,15 @@ fn relevant_chain_and_history_tree(
     Ok((
         state_tip_before_queries.0,
         state_tip_before_queries.1,
-        relevant_chain,
+        best_relevant_chain,
         history_tree,
     ))
 }
 
-/// Returns the [`GetBlockTemplateChainInfo`] for the current best chain.
+/// Returns the [`GetBlockTemplateChainInfo`] for the supplied `relevant_chain`, tip, `network`,
+/// and `history_tree`.
 ///
-/// The `relevant_chain` has recent blocks in reverse order from the tip.
+/// The `relevant_chain` has recent blocks in reverse height order from the tip.
 ///
 /// See [`get_block_template_chain_info()`] for details.
 fn difficulty_time_and_history_tree(
@@ -197,23 +205,15 @@ fn difficulty_time_and_history_tree(
 
     let cur_time = DateTime32::now();
 
-    // Get the median-time-past, which doesn't depend on the time or the previous block height.
-    // `context` will always have the correct length, because this function takes an array.
-    //
-    // TODO: split out median-time-past into its own struct?
-    let median_time_past = AdjustedDifficulty::new_from_header_time(
-        cur_time.into(),
-        tip_height,
-        network,
-        relevant_data.clone(),
-    )
-    .median_time_past();
-
     // > For each block other than the genesis block , nTime MUST be strictly greater than
     // > the median-time-past of that block.
     // https://zips.z.cash/protocol/protocol.pdf#blockheader
-    let median_time_past =
-        DateTime32::try_from(median_time_past).expect("valid blocks have in-range times");
+    let median_time_past = calculate_median_time_past(
+        relevant_chain[0..POW_MEDIAN_BLOCK_SPAN]
+            .to_vec()
+            .try_into()
+            .expect("slice is correct size"),
+    );
 
     let min_time = median_time_past
         .checked_add(Duration32::from_seconds(1))

zebra-state/src/service/read/find.rs

@@ -20,16 +20,14 @@ use std::{
 use chrono::{DateTime, Utc};
 use zebra_chain::{
     block::{self, Block, Height},
-    parameters::Network,
     serialization::DateTime32,
-    work::difficulty::CompactDifficulty,
 };
 
 use crate::{
     constants,
     service::{
         block_iter::any_ancestor_blocks,
-        check::{difficulty::POW_ADJUSTMENT_BLOCK_SPAN, AdjustedDifficulty},
+        check::{difficulty::POW_MEDIAN_BLOCK_SPAN, AdjustedDifficulty},
         finalized_state::ZebraDb,
         non_finalized_state::{Chain, NonFinalizedState},
         read::{self, block::block_header, FINALIZED_STATE_QUERY_RETRIES},
@@ -573,7 +571,7 @@ pub fn next_median_time_past(
 fn best_relevant_chain(
     non_finalized_state: &NonFinalizedState,
     db: &ZebraDb,
-) -> Result<[Arc<Block>; POW_ADJUSTMENT_BLOCK_SPAN], BoxError> {
+) -> Result<[Arc<Block>; POW_MEDIAN_BLOCK_SPAN], BoxError> {
     let state_tip_before_queries = read::best_tip(non_finalized_state, db).ok_or_else(|| {
         BoxError::from("Zebra's state is empty, wait until it syncs to the chain tip")
     })?;
@@ -582,7 +580,7 @@ fn best_relevant_chain(
         any_ancestor_blocks(non_finalized_state, db, state_tip_before_queries.1);
     let best_relevant_chain: Vec<_> = best_relevant_chain
         .into_iter()
-        .take(POW_ADJUSTMENT_BLOCK_SPAN)
+        .take(POW_MEDIAN_BLOCK_SPAN)
         .collect();
     let best_relevant_chain = best_relevant_chain.try_into().map_err(|_error| {
         "Zebra's state only has a few blocks, wait until it syncs to the chain tip"
@@ -605,32 +603,23 @@ fn best_relevant_chain(
 /// The `relevant_chain` has blocks in reverse height order.
 ///
 /// See [`next_median_time_past()`] for details.
-fn calculate_median_time_past(
-    relevant_chain: [Arc<Block>; POW_ADJUSTMENT_BLOCK_SPAN],
+pub(crate) fn calculate_median_time_past(
+    relevant_chain: [Arc<Block>; POW_MEDIAN_BLOCK_SPAN],
 ) -> DateTime32 {
-    let relevant_data: Vec<(CompactDifficulty, DateTime<Utc>)> = relevant_chain
+    let relevant_data: Vec<DateTime<Utc>> = relevant_chain
         .iter()
-        .map(|block| (block.header.difficulty_threshold, block.header.time))
+        .map(|block| block.header.time)
         .collect();
 
-    // TODO: split out median-time-past into its own struct?
-    let ignored_time = DateTime::default();
-    let ignored_height = Height(0);
-    let ignored_network = Network::Mainnet;
-
-    // Get the median-time-past, which doesn't depend on the time or the previous block height.
-    // `context` will always have the correct length, because this function takes an array.
-    let median_time_past = AdjustedDifficulty::new_from_header_time(
-        ignored_time,
-        ignored_height,
-        ignored_network,
-        relevant_data,
-    )
-    .median_time_past();
-
     // > Define the median-time-past of a block to be the median of the nTime fields of the
     // > preceding PoWMedianBlockSpan blocks (or all preceding blocks if there are fewer than
     // > PoWMedianBlockSpan). The median-time-past of a genesis block is not defined.
     // https://zips.z.cash/protocol/protocol.pdf#blockheader
+    let median_time_past = AdjustedDifficulty::median_time(
+        relevant_data
+            .try_into()
+            .expect("always has the correct length due to function argument type"),
+    );
+
     DateTime32::try_from(median_time_past).expect("valid blocks have in-range times")
 }