fix: bug in block timing grpc method (#3926)

Description
---

- Fixes a bug where the grpc method for block timing was requesting the headers in the wrong order, resulting in wrong calculation 
- Removes the deprecated duplicate GetCalcTiming method 
- Un-ignores the chain balance test which is working again

applications/tari_app_grpc/proto/base_node.proto

@@ -33,10 +33,6 @@ service BaseNode {
     rpc GetHeaderByHash(GetHeaderByHashRequest) returns (BlockHeaderResponse);
     // Returns blocks in the current best chain. Currently only supports querying by height
     rpc GetBlocks(GetBlocksRequest) returns (stream HistoricalBlock);
-    // Returns the calc timing for the chain heights
-    rpc GetCalcTiming(HeightRequest) returns (CalcTimingResponse) {
-        option deprecated = true;
-    };
     // Returns the block timing for the chain heights
     rpc GetBlockTiming(HeightRequest) returns (BlockTimingResponse);
     // Returns the network Constants
@@ -234,14 +230,6 @@ message HeightRequest {
     uint64 end_height = 3;
 }
 
-// The return type of the rpc GetCalcTiming
-message CalcTimingResponse {
-    option deprecated = true;
-    uint64 max = 1;
-    uint64 min = 2;
-    double avg = 3;
-}
-
 // The return type of the rpc GetBlockTiming
 message BlockTimingResponse {
     uint64 max = 1;

applications/tari_base_node/src/commands/command/mod.rs

@@ -118,7 +118,6 @@ pub enum Command {
     PeriodStats(period_stats::Args),
     HeaderStats(header_stats::Args),
     BlockTiming(block_timing::Args),
-    CalcTiming(block_timing::Args),
     ListReorgs(list_reorgs::Args),
     DiscoverPeer(discover_peer::Args),
     GetBlock(get_block::Args),
@@ -234,7 +233,7 @@ impl HandleCommand<Command> for CommandContext {
             Command::CheckDb(args) => self.handle_command(args).await,
             Command::PeriodStats(args) => self.handle_command(args).await,
             Command::HeaderStats(args) => self.handle_command(args).await,
-            Command::BlockTiming(args) | Command::CalcTiming(args) => self.handle_command(args).await,
+            Command::BlockTiming(args) => self.handle_command(args).await,
             Command::ListReorgs(args) => self.handle_command(args).await,
             Command::DiscoverPeer(args) => self.handle_command(args).await,
             Command::GetBlock(args) => self.handle_command(args).await,

applications/tari_base_node/src/grpc/base_node_grpc_server.rs

@@ -80,6 +80,8 @@ const LIST_HEADERS_PAGE_SIZE: usize = 10;
 // The `num_headers` value if none is provided.
 const LIST_HEADERS_DEFAULT_NUM_HEADERS: u64 = 10;
 
+const BLOCK_TIMING_MAX_BLOCKS: u64 = 10_000;
+
 pub struct BaseNodeGrpcServer {
     node_service: LocalNodeCommsInterface,
     mempool_service: LocalMempoolService,
@@ -1153,22 +1155,6 @@ impl tari_rpc::base_node_server::BaseNode for BaseNodeGrpcServer {
         Ok(Response::new(rx))
     }
 
-    // deprecated
-    async fn get_calc_timing(
-        &self,
-        request: Request<tari_rpc::HeightRequest>,
-    ) -> Result<Response<tari_rpc::CalcTimingResponse>, Status> {
-        debug!(
-            target: LOG_TARGET,
-            "Incoming GRPC request for deprecated GetCalcTiming. Forwarding to GetBlockTiming.",
-        );
-
-        let tari_rpc::BlockTimingResponse { max, min, avg } = self.get_block_timing(request).await?.into_inner();
-        let response = tari_rpc::CalcTimingResponse { max, min, avg };
-
-        Ok(Response::new(response))
-    }
-
     async fn get_block_timing(
         &self,
         request: Request<tari_rpc::HeightRequest>,
@@ -1185,8 +1171,19 @@ impl tari_rpc::base_node_server::BaseNode for BaseNodeGrpcServer {
         let mut handler = self.node_service.clone();
         let (start, end) = get_heights(&request, handler.clone()).await?;
 
+        let num_requested = end.saturating_sub(start);
+        if num_requested > BLOCK_TIMING_MAX_BLOCKS {
+            warn!(
+                target: LOG_TARGET,
+                "GetBlockTiming request for too many blocks. Requested: {}. Max: {}.",
+                num_requested,
+                BLOCK_TIMING_MAX_BLOCKS
+            );
+            return Err(Status::invalid_argument("Max request size exceeded."));
+        }
+
         let headers = match handler.get_headers(start..=end).await {
-            Ok(headers) => headers.into_iter().map(|h| h.into_header()).collect::<Vec<_>>(),
+            Ok(headers) => headers.into_iter().map(|h| h.into_header()).rev().collect::<Vec<_>>(),
             Err(err) => {
                 warn!(target: LOG_TARGET, "Error getting headers for GRPC client: {}", err);
                 Vec::new()

base_layer/core/src/blocks/block_header.rs

@@ -38,6 +38,7 @@
 //! This hash is called the UTXO merkle root, and is used as the output_mr
 
 use std::{
+    cmp::Ordering,
     fmt,
     fmt::{Display, Error, Formatter},
 };
@@ -173,11 +174,25 @@ impl BlockHeader {
         BlockBuilder::new(self.version).with_header(self)
     }
 
-    /// Given a slice of headers (in reverse order), calculate the maximum, minimum and average periods between them
+    /// Given a slice of headers, calculate the maximum, minimum and average periods between them.
+    /// Expects the slice of headers to be ordered from youngest to oldest, but will reverse them if not.
+    /// This function always allocates a vec of the slice length. This is in case it needs to reverse the list.
     pub fn timing_stats(headers: &[BlockHeader]) -> (u64, u64, f64) {
         if headers.len() < 2 {
             (0, 0, 0.0)
         } else {
+            let mut headers = headers.to_vec();
+
+            // ensure the slice is in reverse order
+            let ordering = headers[0].timestamp.cmp(&headers[headers.len() - 1].timestamp);
+            if ordering == Ordering::Less {
+                headers.reverse();
+            }
+
+            // unwraps: length already checked
+            let last_ts = headers.first().unwrap().timestamp;
+            let first_ts = headers.last().unwrap().timestamp;
+
             let (max, min) = headers.windows(2).fold((0u64, std::u64::MAX), |(max, min), next| {
                 let dt = match next[0].timestamp.checked_sub(next[1].timestamp) {
                     Some(delta) => delta.as_u64(),
@@ -186,7 +201,10 @@ impl BlockHeader {
                 (max.max(dt), min.min(dt))
             });
 
-            let dt = headers.first().unwrap().timestamp - headers.last().unwrap().timestamp;
+            let dt = match last_ts.checked_sub(first_ts) {
+                Some(t) => t,
+                None => 0.into(),
+            };
             let n = headers.len() - 1;
             let avg = dt.as_u64() as f64 / n as f64;
 
@@ -384,6 +402,8 @@ pub(crate) mod hash_serializer {
 
 #[cfg(test)]
 mod test {
+    use std::cmp::Ordering;
+
     use tari_crypto::tari_utilities::Hashable;
 
     use crate::blocks::BlockHeader;
@@ -461,10 +481,46 @@ mod test {
                 ..BlockHeader::default()
             })
             .collect::<Vec<BlockHeader>>();
-        let (max, min, avg) = dbg!(BlockHeader::timing_stats(&headers));
+        let (max, min, avg) = BlockHeader::timing_stats(&headers);
+        assert_eq!(max, 60);
+        assert_eq!(min, 60);
+        let error_margin = f64::EPSILON; // Use machine epsilon for comparison of floats
+        assert!((avg - 60f64).abs() < error_margin);
+    }
+
+    #[test]
+    fn timing_wrong_order() {
+        let headers = vec![90, 150]
+            .into_iter()
+            .map(|t| BlockHeader {
+                timestamp: t.into(),
+                ..BlockHeader::default()
+            })
+            .collect::<Vec<BlockHeader>>();
+        let (max, min, avg) = BlockHeader::timing_stats(&headers);
         assert_eq!(max, 60);
         assert_eq!(min, 60);
         let error_margin = f64::EPSILON; // Use machine epsilon for comparison of floats
         assert!((avg - 60f64).abs() < error_margin);
     }
+
+    #[test]
+    fn compare_timestamps() {
+        let headers = vec![90, 90, 150]
+            .into_iter()
+            .map(|t| BlockHeader {
+                timestamp: t.into(),
+                ..BlockHeader::default()
+            })
+            .collect::<Vec<BlockHeader>>();
+
+        let ordering = headers[0].timestamp.cmp(&headers[1].timestamp);
+        assert_eq!(ordering, Ordering::Equal);
+
+        let ordering = headers[1].timestamp.cmp(&headers[2].timestamp);
+        assert_eq!(ordering, Ordering::Less);
+
+        let ordering = headers[2].timestamp.cmp(&headers[0].timestamp);
+        assert_eq!(ordering, Ordering::Greater);
+    }
 }

base_layer/core/src/transactions/test_helpers.rs

@@ -542,7 +542,7 @@ pub fn create_unblinded_txos(
 
     (inputs, outputs)
 }
-/// Create an unconfirmed transaction for testing with a valid fee, unique access_sig, random inputs and outputs, the
+/// Create an unconfirmed transaction for testing with a valid fee, unique excess_sig, random inputs and outputs, the
 /// transaction is only partially constructed
 pub fn create_transaction_with(
     lock_height: u64,

base_layer/core/src/validation/test.rs

@@ -125,11 +125,9 @@ mod header_validators {
 }
 
 #[test]
-// TODO: Fix this test with the new DB structure
-#[ignore = "to be fixed with new db structure"]
 fn chain_balance_validation() {
     let factories = CryptoFactories::default();
-    let consensus_manager = ConsensusManagerBuilder::new(Network::Weatherwax).build();
+    let consensus_manager = ConsensusManagerBuilder::new(Network::Dibbler).build();
     let genesis = consensus_manager.get_genesis_block();
     let faucet_value = 5000 * uT;
     let (faucet_utxo, faucet_key, _) = create_utxo(