fix(rpc): Calculate getblocktemplate RPC testnet min and max times correctly (#5925)

* Clean up some getblocktemplate difficulty code and tests

* Fix minimum difficulty adjustment

* Use clamp rather than max/min

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

* Remove unused imports

* Document the Zebra-specific standard rule that allows testnet miners time to mine a block

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Co-authored-by: Arya <aryasolhi@gmail.com>

zebra-rpc/src/methods/tests/snapshot/get_block_template_rpcs.rs

@@ -106,6 +106,7 @@ pub async fn test_responses<State, ReadState>(
     let fake_cur_time = DateTime32::from(1654008617);
     // nu5 block time + 123
     let fake_max_time = DateTime32::from(1654008728);
+    let fake_difficulty = CompactDifficulty::from(ExpandedDifficulty::from(U256::one()));
 
     let (mock_chain_tip, mock_chain_tip_sender) = MockChainTip::new();
     mock_chain_tip_sender.send_best_tip_height(fake_tip_height);
@@ -181,7 +182,7 @@ pub async fn test_responses<State, ReadState>(
             .expect_request_that(|req| matches!(req, ReadRequest::ChainInfo))
             .await
             .respond(ReadResponse::ChainInfo(GetBlockTemplateChainInfo {
-                expected_difficulty: CompactDifficulty::from(ExpandedDifficulty::from(U256::one())),
+                expected_difficulty: fake_difficulty,
                 tip_height: fake_tip_height,
                 tip_hash: fake_tip_hash,
                 cur_time: fake_cur_time,
@@ -235,7 +236,7 @@ pub async fn test_responses<State, ReadState>(
             .expect_request_that(|req| matches!(req, ReadRequest::ChainInfo))
             .await
             .respond(ReadResponse::ChainInfo(GetBlockTemplateChainInfo {
-                expected_difficulty: CompactDifficulty::from(ExpandedDifficulty::from(U256::one())),
+                expected_difficulty: fake_difficulty,
                 tip_height: fake_tip_height,
                 tip_hash: fake_tip_hash,
                 cur_time: fake_cur_time,

zebra-rpc/src/methods/tests/vectors.rs

@@ -953,6 +953,7 @@ async fn rpc_getblocktemplate_mining_address(use_p2pkh: bool) {
     let fake_cur_time = DateTime32::from(1654008617);
     // nu5 block time + 123
     let fake_max_time = DateTime32::from(1654008728);
+    let fake_difficulty = CompactDifficulty::from(ExpandedDifficulty::from(U256::one()));
 
     let (mock_chain_tip, mock_chain_tip_sender) = MockChainTip::new();
     mock_chain_tip_sender.send_best_tip_height(fake_tip_height);
@@ -976,7 +977,7 @@ async fn rpc_getblocktemplate_mining_address(use_p2pkh: bool) {
             .expect_request_that(|req| matches!(req, ReadRequest::ChainInfo))
             .await
             .respond(ReadResponse::ChainInfo(GetBlockTemplateChainInfo {
-                expected_difficulty: CompactDifficulty::from(ExpandedDifficulty::from(U256::one())),
+                expected_difficulty: fake_difficulty,
                 tip_height: fake_tip_height,
                 tip_hash: fake_tip_hash,
                 cur_time: fake_cur_time,

zebra-state/src/response.rs

@@ -166,16 +166,14 @@ pub struct GetBlockTemplateChainInfo {
     /// Depends on the `tip_hash`.
     pub history_tree: Arc<zebra_chain::history_tree::HistoryTree>,
 
-    // Data derived from the state tip and recent blocks.
-    //
-    /// The expected difficulty of the candidate block.
-    /// Depends on the `tip_hash`.
-    pub expected_difficulty: CompactDifficulty,
-
     // Data derived from the state tip and recent blocks, and the current local clock.
     //
+    /// The expected difficulty of the candidate block.
+    /// Depends on the `tip_hash`, and the local clock on testnet.
+    pub expected_difficulty: CompactDifficulty,
+
     /// The current system time, adjusted to fit within `min_time` and `max_time`.
-    /// Depends on the local clock and the `tip_hash`.
+    /// Always depends on the local clock and the `tip_hash`.
     pub cur_time: DateTime32,
 
     /// The mininimum time the miner can use in this block.

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

@@ -26,6 +26,11 @@ use crate::{
     BoxError, GetBlockTemplateChainInfo,
 };
 
+/// The amount of extra time we allow for a miner to mine a standard difficulty block on testnet.
+///
+/// This is a Zebra-specific standard rule.
+pub const EXTRA_TIME_TO_MINE_A_BLOCK: u32 = POST_BLOSSOM_POW_TARGET_SPACING * 2;
+
 /// Returns the [`GetBlockTemplateChainInfo`] for the current best chain.
 ///
 /// # Panics
@@ -117,7 +122,9 @@ pub fn solution_rate(
 }
 
 /// Do a consistency check by checking the finalized tip before and after all other database queries.
-/// Returns and error if the tip obtained before and after is not the same.
+///
+/// Returns the state tip, recent blocks in reverse order from the tip, and the tip history tree.
+/// Returns an error if the tip obtained before and after is not the same.
 ///
 /// # Panics
 ///
@@ -173,6 +180,8 @@ fn relevant_chain_and_history_tree(
 
 /// Returns the [`GetBlockTemplateChainInfo`] for the current best chain.
 ///
+/// The `relevant_chain` has recent blocks in reverse order from the tip.
+///
 /// See [`get_block_template_chain_info()`] for details.
 fn difficulty_time_and_history_tree(
     relevant_chain: [Arc<Block>; POW_ADJUSTMENT_BLOCK_SPAN],
@@ -227,12 +236,13 @@ fn difficulty_time_and_history_tree(
         network,
         relevant_data.iter().cloned(),
     );
+    let expected_difficulty = difficulty_adjustment.expected_difficulty_threshold();
 
     let mut result = GetBlockTemplateChainInfo {
         tip_hash,
         tip_height,
         history_tree,
-        expected_difficulty: difficulty_adjustment.expected_difficulty_threshold(),
+        expected_difficulty,
         cur_time,
         min_time,
         max_time,
@@ -244,6 +254,8 @@ fn difficulty_time_and_history_tree(
 }
 
 /// Adjust the difficulty and time for the testnet minimum difficulty rule.
+///
+/// The `relevant_data` has recent block difficulties and times in reverse order from the tip.
 fn adjust_difficulty_and_time_for_testnet(
     result: &mut GetBlockTemplateChainInfo,
     network: Network,
@@ -275,7 +287,8 @@ fn adjust_difficulty_and_time_for_testnet(
     // There is still a small chance that miners will produce an invalid block, if they are
     // just below the max time, and don't check it.
 
-    let previous_block_time = relevant_data.last().expect("has at least one block").1;
+    // The tip is the first relevant data block, because they are in reverse order.
+    let previous_block_time = relevant_data.first().expect("has at least one block").1;
     let previous_block_time: DateTime32 = previous_block_time
         .try_into()
         .expect("valid blocks have in-range times");
@@ -296,28 +309,40 @@ fn adjust_difficulty_and_time_for_testnet(
         .expect("a valid block time plus a small constant is in-range");
 
     // If a miner is likely to find a block with the cur_time and standard difficulty
+    // within a target block interval or two, keep the original difficulty.
+    // Otherwise, try to use the minimum difficulty.
+    //
+    // This is a Zebra-specific standard rule.
     //
     // We don't need to undo the clamping here:
     // - if cur_time is clamped to min_time, then we're more likely to have a minimum
     //    difficulty block, which makes mining easier;
-    // - if cur_time gets clamped to max_time, this is already a minimum difficulty block.
+    // - if cur_time gets clamped to max_time, this is almost always a minimum difficulty block.
     let local_std_difficulty_limit = std_difficulty_max_time
-        .checked_sub(Duration32::from_seconds(
+        .checked_sub(Duration32::from_seconds(EXTRA_TIME_TO_MINE_A_BLOCK))
-            POST_BLOSSOM_POW_TARGET_SPACING * 2,
-        ))
         .expect("a valid block time minus a small constant is in-range");
 
     if result.cur_time <= local_std_difficulty_limit {
-        // Standard difficulty: the max time needs to exclude min difficulty blocks
+        // Standard difficulty: the cur and max time need to exclude min difficulty blocks
-        result.max_time = std_difficulty_max_time;
-    } else {
-        // Minimum difficulty: the min and cur time need to exclude min difficulty blocks
-        result.min_time = min_difficulty_min_time;
-        if result.cur_time < min_difficulty_min_time {
-            result.cur_time = min_difficulty_min_time;
-        }
 
-        // And then the difficulty needs to be updated for cur_time
+        // The maximum time can only be decreased, and only as far as min_time.
+        // The old minimum is still required by other consensus rules.
+        result.max_time = std_difficulty_max_time.clamp(result.min_time, result.max_time);
+
+        // The current time only needs to be decreased if the max_time decreased past it.
+        // Decreasing the current time can't change the difficulty.
+        result.cur_time = result.cur_time.clamp(result.min_time, result.max_time);
+    } else {
+        // Minimum difficulty: the min and cur time need to exclude std difficulty blocks
+
+        // The minimum time can only be increased, and only as far as max_time.
+        // The old maximum is still required by other consensus rules.
+        result.min_time = min_difficulty_min_time.clamp(result.min_time, result.max_time);
+
+        // The current time only needs to be increased if the min_time increased past it.
+        result.cur_time = result.cur_time.clamp(result.min_time, result.max_time);
+
+        // And then the difficulty needs to be updated for cur_time.
         result.expected_difficulty = AdjustedDifficulty::new_from_header_time(
             result.cur_time.into(),
             previous_block_height,