From b92b064764525b40e316d1ef88d723c38dd3a242 Mon Sep 17 00:00:00 2001 From: danda Date: Wed, 27 Sep 2023 21:37:28 -0700 Subject: [PATCH] fix logic error in get_latest_balance_height() Addresses https://github.com/Neptune-Crypto/neptune-core/pull/56#issuecomment-1737063330 1. rewrite get_latest_balance_height() to find latest spent utxo anywhere in list of monitored_utxos. 2. change type of Confirmations field from u64 to BlockHeight in server and Dashboard. --- src/bin/dashboard_src/overview_screen.rs | 5 +- src/models/blockchain/transaction/amount.rs | 10 ++++ src/models/state/wallet/wallet_state.rs | 51 ++++++++++++++++++--- src/rpc_server.rs | 15 +++--- 4 files changed, 65 insertions(+), 16 deletions(-) diff --git a/src/bin/dashboard_src/overview_screen.rs b/src/bin/dashboard_src/overview_screen.rs index c4f4c622..0e9e5f17 100644 --- a/src/bin/dashboard_src/overview_screen.rs +++ b/src/bin/dashboard_src/overview_screen.rs @@ -11,6 +11,7 @@ use chrono::DateTime; use itertools::Itertools; use neptune_core::config_models::network::Network; use neptune_core::models::blockchain::block::block_header::BlockHeader; +use neptune_core::models::blockchain::block::block_height::BlockHeight; use neptune_core::models::blockchain::shared::Hash; use neptune_core::models::blockchain::transaction::amount::Amount; use neptune_core::rpc_server::RPCClient; @@ -31,7 +32,7 @@ use super::screen::Screen; #[derive(Debug, Clone)] pub struct OverviewData { synced_balance: Option, - confirmations: Option, + confirmations: Option, synchronization_percentage: Option, network: Network, @@ -91,7 +92,7 @@ impl OverviewData { pub fn test() -> Self { OverviewData { synced_balance: Some(Amount::zero()), - confirmations: Some(17), + confirmations: Some(17.into()), synchronization_percentage: Some(99.5), listen_address: None, diff --git a/src/models/blockchain/transaction/amount.rs b/src/models/blockchain/transaction/amount.rs index 15e3297e..80c001ff 100644 --- a/src/models/blockchain/transaction/amount.rs +++ b/src/models/blockchain/transaction/amount.rs @@ -43,6 +43,16 @@ pub enum Sign { Negative, } +impl Display for Sign { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + let s = match *self { + Self::NonNegative => "", + Self::Negative => "-", + }; + write!(f, "{}", s) + } +} + pub const AMOUNT_SIZE_FOR_U32: usize = 4; #[derive(Clone, Copy, Debug, Serialize, Deserialize, Eq, BFieldCodec)] diff --git a/src/models/state/wallet/wallet_state.rs b/src/models/state/wallet/wallet_state.rs index b922ff54..809ee8e9 100644 --- a/src/models/state/wallet/wallet_state.rs +++ b/src/models/state/wallet/wallet_state.rs @@ -860,22 +860,59 @@ impl WalletState { /// Retrieve block height of last change to wallet balance. /// + /// note: this fn could be implemented as: + /// 1. get_balance_history() + /// 2. sort by height + /// 3. return height of last entry. + /// + /// this implementation is a bit more efficient as it avoids + /// the sort and some minor work looking up amount and confirmation + /// height of each utxo. + /// + /// Presently this is o(n) with the number of monitored utxos. + /// if storage could keep track of latest spend utxo, then this could + /// be o(1). + /// /// todo: ignore abandoned/unsynced utxo. /// also an issue for get_balance_history(). /// see: https://github.com/Neptune-Crypto/neptune-core/issues/28 pub async fn get_latest_balance_height(&self) -> Option { - let db_lock = self.wallet_db.lock().await; - let len = db_lock.monitored_utxos.len(); + let monitored_utxos = self.wallet_db.lock().await.monitored_utxos.clone(); + let len = monitored_utxos.len(); + if len > 0 { - let utxo = db_lock.monitored_utxos.get(len - 1); - if let Some((.., confirmation_height)) = utxo.confirmed_in_block { + let mut latest: BlockHeight = 0.into(); + + // note: would be nicer to use an iterator to find max spending_height. + // perhaps someday: https://github.com/Neptune-Crypto/twenty-first/issues/139 + for i in 0..len { + // latest change to balance could be a spend utxo, and it could be anywhere + // in the monitored_utxos list, so we must check each entry in list. + let utxo = monitored_utxos.get(i); if let Some((.., spending_height)) = utxo.spent_in_block { - return Some(spending_height); + if spending_height > latest { + latest = spending_height; + } + } + + // if latest change is an incoming utxo, we can save some work by checking only + // the last entry because monitored_utxos is already ordered by + // confirmation_height ascending + if i == len - 1 { + if let Some((.., confirmation_height)) = utxo.confirmed_in_block { + if confirmation_height > latest { + latest = confirmation_height; + } + } } - return Some(confirmation_height); } + assert!( + latest > 0.into() || (len == 1 && monitored_utxos.get(0).spent_in_block.is_none()) + ); + Some(latest) + } else { + None } - None } } diff --git a/src/rpc_server.rs b/src/rpc_server.rs index 98c3e0d9..1e4b33de 100644 --- a/src/rpc_server.rs +++ b/src/rpc_server.rs @@ -41,7 +41,7 @@ pub struct DashBoardOverviewDataFromClient { // # of confirmations since last wallet balance change. // `None` indicates that wallet balance has never changed. - pub confirmations: Option, + pub confirmations: Option, } #[tarpc::service] @@ -58,10 +58,10 @@ pub trait RPC { /// Returns the number of blocks (confirmations) since wallet balance last changed. /// - /// returns Option + /// returns Option /// /// return value will be None if wallet has not received any incoming funds. - async fn get_confirmations() -> Option; + async fn get_confirmations() -> Option; /// Returns info about the peers we are connected to async fn get_peer_info() -> Vec; @@ -143,7 +143,7 @@ impl RPC for NeptuneRPCServer { type GetNetworkFut = Ready; type GetListenAddressForPeersFut = Ready>; type BlockHeightFut = Ready; - type GetConfirmationsFut = Ready>; + type GetConfirmationsFut = Ready>; type GetPeerInfoFut = Ready>; type HeadFut = Ready; type HeadsFut = Ready>; @@ -197,11 +197,12 @@ impl RPC for NeptuneRPCServer { executor::block_on(self.state.chain.light_state.get_latest_block_header()); assert!(tip_block_header.height >= latest_balance_height); - assert!(tip_block_header.height - latest_balance_height <= u64::MAX.into()); // subtract latest balance height from chain tip. - // the cast to u64 is safe given we passed the above asserts. - let confirmations: u64 = (tip_block_header.height - latest_balance_height) as u64; + // note: BlockHeight is u64 internally and BlockHeight::sub() returns i128. + // The subtraction and cast is safe given we passed the above assert. + let confirmations: BlockHeight = + ((tip_block_header.height - latest_balance_height) as u64).into(); future::ready(Some(confirmations)) } None => future::ready(None),