fix(rpc): Fix bugs and performance of getnetworksolps & `getnetwork…

…hashps` RPCs (#7647)

* Handle negative and zero getnetworksolsps arguments correctly

* Simplify the solsps loop structure

* Simplify loop by avoiding manual iteration and peeking

* Avoid division by zero

* Use min and max times rather than first and last times

* Refactor block iterators so they are more efficient

* Make finding chains easier

* Simplify block iteration code

* Remove implemented TODO comments

* Simplify internal iterator state

* Implement iteration by any chain item

* Iterate block headers rather than full blocks

* Ignore code that is (sometimes) dead

* Fix a dead code warning

* Add a no blocks in state error constant

* Check result values in the RPC test

* Fix invalid calculation handling

zebra-chain/src/block/height.rs

@@ -80,7 +80,6 @@ impl Height {
     /// # Panics
     ///
     /// - If the current height is at its maximum.
-    // TODO Return an error instead of panicking #7263.
     pub fn next(self) -> Result<Self, HeightError> {
         (self + 1).ok_or(HeightError::Overflow)
     }
@@ -90,7 +89,6 @@ impl Height {
     /// # Panics
     ///
     /// - If the current height is at its minimum.
-    // TODO Return an error instead of panicking #7263.
     pub fn previous(self) -> Result<Self, HeightError> {
         (self - 1).ok_or(HeightError::Underflow)
     }
@@ -99,6 +97,11 @@ impl Height {
     pub fn is_min(self) -> bool {
         self == Self::MIN
     }
+
+    /// Returns the value as a `usize`.
+    pub fn as_usize(self) -> usize {
+        self.0.try_into().expect("fits in usize")
+    }
 }
 
 /// A difference between two [`Height`]s, possibly negative.
@@ -169,6 +172,14 @@ impl TryIntoHeight for String {
     }
 }
 
+impl TryIntoHeight for i32 {
+    type Error = BoxError;
+
+    fn try_into_height(&self) -> Result<Height, Self::Error> {
+        u32::try_from(*self)?.try_into().map_err(Into::into)
+    }
+}
+
 // We don't implement Add<u32> or Sub<u32>, because they cause type inference issues for integer constants.
 
 impl Sub<Height> for Height {

zebra-chain/src/work/difficulty.rs

@@ -129,6 +129,11 @@ pub struct ExpandedDifficulty(U256);
 pub struct Work(u128);
 
 impl Work {
+    /// Returns a value representing no work.
+    pub fn zero() -> Self {
+        Self(0)
+    }
+
     /// Return the inner `u128` value.
     pub fn as_u128(self) -> u128 {
         self.0
@@ -660,6 +665,11 @@ impl std::ops::Add for Work {
 pub struct PartialCumulativeWork(u128);
 
 impl PartialCumulativeWork {
+    /// Returns a value representing no work.
+    pub fn zero() -> Self {
+        Self(0)
+    }
+
     /// Return the inner `u128` value.
     pub fn as_u128(self) -> u128 {
         self.0

zebra-rpc/src/constants.rs

@@ -1,6 +1,6 @@
 //! Constants for RPC methods and server responses.
 
-use jsonrpc_core::ErrorCode;
+use jsonrpc_core::{Error, ErrorCode};
 
 /// The RPC error code used by `zcashd` for incorrect RPC parameters.
 ///
@@ -17,5 +17,24 @@ pub const INVALID_PARAMETERS_ERROR_CODE: ErrorCode = ErrorCode::ServerError(-1);
 /// <https://github.com/zcash/lightwalletd/blob/v0.4.16/common/common.go#L287-L290>
 pub const MISSING_BLOCK_ERROR_CODE: ErrorCode = ErrorCode::ServerError(-8);
 
+/// The RPC error code used by `zcashd` when there are no blocks in the state.
+///
+/// `lightwalletd` expects error code `0` when there are no blocks in the state.
+//
+// TODO: find the source code that expects or generates this error
+pub const NO_BLOCKS_IN_STATE_ERROR_CODE: ErrorCode = ErrorCode::ServerError(0);
+
+/// The RPC error used by `zcashd` when there are no blocks in the state.
+//
+// TODO: find the source code that expects or generates this error text, if there is any
+//       replace literal Error { ... } with this error
+pub fn no_blocks_in_state_error() -> Error {
+    Error {
+        code: NO_BLOCKS_IN_STATE_ERROR_CODE,
+        message: "No blocks in state".to_string(),
+        data: None,
+    }
+}
+
 /// When logging parameter data, only log this much data.
 pub const MAX_PARAMS_LOG_LENGTH: usize = 100;

zebra-rpc/src/methods/get_block_template_rpcs.rs

@@ -11,10 +11,10 @@ use zcash_address::{self, unified::Encoding, TryFromAddress};
 
 use zebra_chain::{
     amount::Amount,
-    block::{self, Block, Height},
+    block::{self, Block, Height, TryIntoHeight},
     chain_sync_status::ChainSyncStatus,
     chain_tip::ChainTip,
-    parameters::Network,
+    parameters::{Network, POW_AVERAGING_WINDOW},
     primitives,
     serialization::ZcashDeserializeInto,
     transparent::{
@@ -142,27 +142,32 @@ pub trait GetBlockTemplateRpc {
     #[rpc(name = "getmininginfo")]
     fn get_mining_info(&self) -> BoxFuture<Result<get_mining_info::Response>>;
 
-    /// Returns the estimated network solutions per second based on the last `num_blocks` before `height`.
-    /// If `num_blocks` is not supplied, uses 120 blocks.
-    /// If `height` is not supplied or is 0, uses the tip height.
+    /// Returns the estimated network solutions per second based on the last `num_blocks` before
+    /// `height`.
+    ///
+    /// If `num_blocks` is not supplied, uses 120 blocks. If it is 0 or -1, uses the difficulty
+    /// averaging window.
+    /// If `height` is not supplied or is -1, uses the tip height.
     ///
     /// zcashd reference: [`getnetworksolps`](https://zcash.github.io/rpc/getnetworksolps.html)
     #[rpc(name = "getnetworksolps")]
     fn get_network_sol_ps(
         &self,
-        num_blocks: Option<usize>,
+        num_blocks: Option<i32>,
         height: Option<i32>,
     ) -> BoxFuture<Result<u64>>;
 
-    /// Returns the estimated network solutions per second based on the last `num_blocks` before `height`.
-    /// If `num_blocks` is not supplied, uses 120 blocks.
-    /// If `height` is not supplied or is 0, uses the tip height.
+    /// Returns the estimated network solutions per second based on the last `num_blocks` before
+    /// `height`.
+    ///
+    /// This method name is deprecated, use [`getnetworksolps`](Self::get_network_sol_ps) instead.
+    /// See that method for details.
     ///
     /// zcashd reference: [`getnetworkhashps`](https://zcash.github.io/rpc/getnetworkhashps.html)
     #[rpc(name = "getnetworkhashps")]
     fn get_network_hash_ps(
         &self,
-        num_blocks: Option<usize>,
+        num_blocks: Option<i32>,
         height: Option<i32>,
     ) -> BoxFuture<Result<u64>> {
         self.get_network_sol_ps(num_blocks, height)
@@ -838,13 +843,22 @@ where
 
     fn get_network_sol_ps(
         &self,
-        num_blocks: Option<usize>,
+        num_blocks: Option<i32>,
         height: Option<i32>,
     ) -> BoxFuture<Result<u64>> {
-        let num_blocks = num_blocks
-            .map(|num_blocks| num_blocks.max(1))
-            .unwrap_or(DEFAULT_SOLUTION_RATE_WINDOW_SIZE);
-        let height = height.and_then(|height| (height > 1).then_some(Height(height as u32)));
+        // Default number of blocks is 120 if not supplied.
+        let mut num_blocks = num_blocks.unwrap_or(DEFAULT_SOLUTION_RATE_WINDOW_SIZE);
+        // But if it is 0 or negative, it uses the proof of work averaging window.
+        if num_blocks < 1 {
+            num_blocks = i32::try_from(POW_AVERAGING_WINDOW).expect("fits in i32");
+        }
+        let num_blocks =
+            usize::try_from(num_blocks).expect("just checked for negatives, i32 fits in usize");
+
+        // Default height is the tip height if not supplied. Negative values also mean the tip
+        // height. Since negative values aren't valid heights, we can just use the conversion.
+        let height = height.and_then(|height| height.try_into_height().ok());
+
         let mut state = self.state.clone();
 
         async move {
@@ -861,11 +875,9 @@ where
                 })?;
 
             let solution_rate = match response {
-                ReadResponse::SolutionRate(solution_rate) => solution_rate.ok_or(Error {
-                    code: ErrorCode::ServerError(0),
-                    message: "No blocks in state".to_string(),
-                    data: None,
-                })?,
+                // zcashd returns a 0 rate when the calculation is invalid
+                ReadResponse::SolutionRate(solution_rate) => solution_rate.unwrap_or(0),
+
                 _ => unreachable!("unmatched response to a solution rate request"),
             };
 

zebra-rpc/src/methods/get_block_template_rpcs/constants.rs

@@ -54,7 +54,7 @@ pub const NOT_SYNCED_ERROR_CODE: ErrorCode = ErrorCode::ServerError(-10);
 /// The default window size specifying how many blocks to check when estimating the chain's solution rate.
 ///
 /// Based on default value in zcashd.
-pub const DEFAULT_SOLUTION_RATE_WINDOW_SIZE: usize = 120;
+pub const DEFAULT_SOLUTION_RATE_WINDOW_SIZE: i32 = 120;
 
 /// The funding stream order in `zcashd` RPC responses.
 ///

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

@@ -1148,32 +1148,39 @@ async fn rpc_getnetworksolps() {
     );
 
     let get_network_sol_ps_inputs = [
-        (None, None),
-        (Some(0), None),
-        (Some(0), Some(0)),
-        (Some(0), Some(-1)),
-        (Some(0), Some(10)),
-        (Some(0), Some(i32::MAX)),
-        (Some(1), None),
-        (Some(1), Some(0)),
-        (Some(1), Some(-1)),
-        (Some(1), Some(10)),
-        (Some(1), Some(i32::MAX)),
-        (Some(usize::MAX), None),
-        (Some(usize::MAX), Some(0)),
-        (Some(usize::MAX), Some(-1)),
-        (Some(usize::MAX), Some(10)),
-        (Some(usize::MAX), Some(i32::MAX)),
+        // num_blocks, height, return value
+        (None, None, Ok(2)),
+        (Some(-4), None, Ok(2)),
+        (Some(-3), Some(0), Ok(0)),
+        (Some(-2), Some(-4), Ok(2)),
+        (Some(-1), Some(10), Ok(2)),
+        (Some(-1), Some(i32::MAX), Ok(2)),
+        (Some(0), None, Ok(2)),
+        (Some(0), Some(0), Ok(0)),
+        (Some(0), Some(-3), Ok(2)),
+        (Some(0), Some(10), Ok(2)),
+        (Some(0), Some(i32::MAX), Ok(2)),
+        (Some(1), None, Ok(4096)),
+        (Some(1), Some(0), Ok(0)),
+        (Some(1), Some(-2), Ok(4096)),
+        (Some(1), Some(10), Ok(4096)),
+        (Some(1), Some(i32::MAX), Ok(4096)),
+        (Some(i32::MAX), None, Ok(2)),
+        (Some(i32::MAX), Some(0), Ok(0)),
+        (Some(i32::MAX), Some(-1), Ok(2)),
+        (Some(i32::MAX), Some(10), Ok(2)),
+        (Some(i32::MAX), Some(i32::MAX), Ok(2)),
     ];
 
-    for (num_blocks_input, height_input) in get_network_sol_ps_inputs {
+    for (num_blocks_input, height_input, return_value) in get_network_sol_ps_inputs {
         let get_network_sol_ps_result = get_block_template_rpc
             .get_network_sol_ps(num_blocks_input, height_input)
             .await;
-        assert!(
-            get_network_sol_ps_result
-                .is_ok(),
-            "get_network_sol_ps({num_blocks_input:?}, {height_input:?}) call with should be ok, got: {get_network_sol_ps_result:?}"
+        assert_eq!(
+            get_network_sol_ps_result, return_value,
+            "get_network_sol_ps({num_blocks_input:?}, {height_input:?}) result\n\
+             should be {return_value:?},\n\
+             got: {get_network_sol_ps_result:?}"
         );
     }
 }

zebra-state/src/request.rs

@@ -938,9 +938,10 @@ pub enum ReadRequest {
     ///
     /// Returns [`ReadResponse::SolutionRate`]
     SolutionRate {
-        /// Specifies over difficulty averaging window.
+        /// The number of blocks to calculate the average difficulty for.
         num_blocks: usize,
-        /// Optionally estimate the network speed at the time when a certain block was found
+        /// Optionally estimate the network solution rate at the time when this height was mined.
+        /// Otherwise, estimate at the current tip height.
         height: Option<block::Height>,
     },