From 43b20334151b2872b0969bf3fd56e7c5c2af62bd Mon Sep 17 00:00:00 2001 From: Impala123 <91881574+Impala123@users.noreply.github.com> Date: Mon, 11 Oct 2021 22:45:45 +0200 Subject: [PATCH 1/3] fix: u64->i64->u64 conversion; chain split height as u64 (#3442) Description removing an unnecessary conversion and using u64 for chain height --- How Has This Been Tested? --- rust build & test --- .../src/grpc/base_node_grpc_server.rs | 33 ++++++++++++++----- base_layer/core/src/base_node/proto/rpc.proto | 2 +- .../src/base_node/sync/header_sync/error.rs | 2 +- .../sync/header_sync/synchronizer.rs | 6 ++-- .../core/src/base_node/sync/rpc/service.rs | 2 +- 5 files changed, 30 insertions(+), 15 deletions(-) diff --git a/applications/tari_base_node/src/grpc/base_node_grpc_server.rs b/applications/tari_base_node/src/grpc/base_node_grpc_server.rs index 649a8c33b2..3a24c91249 100644 --- a/applications/tari_base_node/src/grpc/base_node_grpc_server.rs +++ b/applications/tari_base_node/src/grpc/base_node_grpc_server.rs @@ -324,19 +324,34 @@ impl tari_rpc::base_node_server::BaseNode for BaseNodeGrpcServer { let num_headers = cmp::min(num_headers, LIST_HEADERS_MAX_NUM_HEADERS); let (mut tx, rx) = mpsc::channel(LIST_HEADERS_PAGE_SIZE); - let headers: Vec = if request.from_height != 0 { + let from_height = cmp::min(request.from_height, tip); + + let headers: Vec = if from_height != 0 { match sorting { - Sorting::Desc => ((cmp::max(0, request.from_height as i64 - num_headers as i64 + 1) as u64)..= - request.from_height) - .rev() - .collect(), - Sorting::Asc => (request.from_height..(request.from_height + num_headers)).collect(), + Sorting::Desc => { + let from = match from_height.overflowing_sub(num_headers) { + (_, true) => 0, + (res, false) => res + 1, + }; + (from..=from_height).rev().collect() + }, + Sorting::Asc => { + let to = match from_height.overflowing_add(num_headers) { + (_, true) => u64::MAX, + (res, false) => res, + }; + (from_height..to).collect() + }, } } else { match sorting { - Sorting::Desc => ((cmp::max(0, tip as i64 - num_headers as i64 + 1) as u64)..=tip) - .rev() - .collect(), + Sorting::Desc => { + let from = match tip.overflowing_sub(num_headers) { + (_, true) => 0, + (res, false) => res + 1, + }; + (from..=tip).rev().collect() + }, Sorting::Asc => (0..num_headers).collect(), } }; diff --git a/base_layer/core/src/base_node/proto/rpc.proto b/base_layer/core/src/base_node/proto/rpc.proto index 968693ff8e..f1c4ee9d6f 100644 --- a/base_layer/core/src/base_node/proto/rpc.proto +++ b/base_layer/core/src/base_node/proto/rpc.proto @@ -39,7 +39,7 @@ message FindChainSplitResponse { // An ordered list of headers starting from next header after the matching hash, up until `FindChainSplitRequest::count` repeated tari.core.BlockHeader headers = 1; // The index of the hash that matched from `FindChainSplitRequest::block_hashes`. This value could also be used to know how far back a split occurs. - uint32 fork_hash_index = 2; + uint64 fork_hash_index = 2; /// The current header height of this node uint64 tip_height = 3; } diff --git a/base_layer/core/src/base_node/sync/header_sync/error.rs b/base_layer/core/src/base_node/sync/header_sync/error.rs index aee8cf22e7..fc39debac4 100644 --- a/base_layer/core/src/base_node/sync/header_sync/error.rs +++ b/base_layer/core/src/base_node/sync/header_sync/error.rs @@ -42,7 +42,7 @@ pub enum BlockHeaderSyncError { #[error("Sync failed for all peers")] SyncFailedAllPeers, #[error("Peer sent a found hash index that was out of range (Expected less than {0}, Found: {1})")] - FoundHashIndexOutOfRange(u32, u32), + FoundHashIndexOutOfRange(u64, u64), #[error("Failed to ban peer: {0}")] FailedToBan(ConnectivityError), #[error("Connectivity Error: {0}")] diff --git a/base_layer/core/src/base_node/sync/header_sync/synchronizer.rs b/base_layer/core/src/base_node/sync/header_sync/synchronizer.rs index 2ca121d647..b75124cabf 100644 --- a/base_layer/core/src/base_node/sync/header_sync/synchronizer.rs +++ b/base_layer/core/src/base_node/sync/header_sync/synchronizer.rs @@ -374,7 +374,7 @@ impl<'a, B: BlockchainBackend + 'static> HeaderSynchronizer<'a, B> { peer ); - if fork_hash_index >= block_hashes.len() as u32 { + if fork_hash_index >= block_hashes.len() as u64 { let _ = self .ban_peer_long(peer.clone(), BanReason::SplitHashGreaterThanHashes { fork_hash_index, @@ -382,7 +382,7 @@ impl<'a, B: BlockchainBackend + 'static> HeaderSynchronizer<'a, B> { }) .await; return Err(BlockHeaderSyncError::FoundHashIndexOutOfRange( - block_hashes.len() as u32, + block_hashes.len() as u64, fork_hash_index, )); } @@ -658,7 +658,7 @@ enum BanReason { ({num_block_hashes})" )] SplitHashGreaterThanHashes { - fork_hash_index: u32, + fork_hash_index: u64, num_block_hashes: usize, }, #[error("Peer sent invalid header: {0}")] diff --git a/base_layer/core/src/base_node/sync/rpc/service.rs b/base_layer/core/src/base_node/sync/rpc/service.rs index d10ce6a0ea..9e20df058a 100644 --- a/base_layer/core/src/base_node/sync/rpc/service.rs +++ b/base_layer/core/src/base_node/sync/rpc/service.rs @@ -356,7 +356,7 @@ impl BaseNodeSyncService for BaseNodeSyncRpcServ .map_err(RpcStatus::log_internal_error(LOG_TARGET))?; Ok(Response::new(FindChainSplitResponse { - fork_hash_index: idx as u32, + fork_hash_index: idx as u64, headers: headers.into_iter().map(Into::into).collect(), tip_height: metadata.height_of_longest_chain(), })) From e23ceecfa441f6739412dafc02e1d5fcc95ff9ab Mon Sep 17 00:00:00 2001 From: Hansie Odendaal <39146854+hansieodendaal@users.noreply.github.com> Date: Tue, 12 Oct 2021 13:56:05 +0200 Subject: [PATCH 2/3] feat: add sql query to obtain balance (#3446) Description --- - Replaced the `get_balance` function containing multiple sql queries with a single raw sql query. - Removed redundant code (`fn fetch_pending_outgoing_outputs`) as a result of this change. - Added more test points for time-locked balance. - **Note:** To properly test transaction validation in `async fn test_txo_validation()`, access to the backend to obtain pending incoming transactions is needed via ` fn fetch_pending_incoming_outputs`, although that function is not used in production code anymore. If it is also removed other methods will have to be added to the backend to obtain the data for testing. Retaining the current function was chosen in lieu of adding other code. Motivation and Context --- Get balance used a lot of RAM and was really slow due to multiple database interactions. Comparison of old vs. new query time performance for a wallet with 251,000 UTXOs in the database shown below: - **Full scale** ![image](https://user-images.githubusercontent.com/39146854/136885517-1cb6f274-b85a-4281-a6d8-0edf42842baa.png) - **Y-axis zoomed in** ![image](https://user-images.githubusercontent.com/39146854/136885891-2339c7b5-0638-402c-a380-86cceb51b1b2.png) How Has This Been Tested? --- - Unit tests - System level tests --- .../storage/database.rs | 69 +--------- .../storage/sqlite_db.rs | 125 +++++++++++++++--- .../tests/output_manager_service/service.rs | 15 +++ .../tests/output_manager_service/storage.rs | 9 ++ 4 files changed, 135 insertions(+), 83 deletions(-) diff --git a/base_layer/wallet/src/output_manager_service/storage/database.rs b/base_layer/wallet/src/output_manager_service/storage/database.rs index efc5e78d8f..6d7c6ae910 100644 --- a/base_layer/wallet/src/output_manager_service/storage/database.rs +++ b/base_layer/wallet/src/output_manager_service/storage/database.rs @@ -33,7 +33,7 @@ use std::{ sync::Arc, }; use tari_common_types::types::{BlindingFactor, Commitment, HashOutput, PrivateKey}; -use tari_core::transactions::{tari_amount::MicroTari, transaction::TransactionOutput}; +use tari_core::transactions::transaction::TransactionOutput; const LOG_TARGET: &str = "wallet::output_manager_service::database"; @@ -51,7 +51,6 @@ pub trait OutputManagerBackend: Send + Sync + Clone { /// Modify the state the of the backend with a write operation fn write(&self, op: WriteOperation) -> Result, OutputManagerStorageError>; fn fetch_pending_incoming_outputs(&self) -> Result, OutputManagerStorageError>; - fn fetch_pending_outgoing_outputs(&self) -> Result, OutputManagerStorageError>; fn set_received_output_mined_height( &self, @@ -119,6 +118,8 @@ pub trait OutputManagerBackend: Send + Sync + Clone { fn set_coinbase_abandoned(&self, tx_id: TxId, abandoned: bool) -> Result<(), OutputManagerStorageError>; /// Reinstate a cancelled inbound output fn reinstate_cancelled_inbound_output(&self, tx_id: TxId) -> Result<(), OutputManagerStorageError>; + /// Return the available, time locked, pending incoming and pending outgoing balance + fn get_balance(&self, tip: Option) -> Result; } /// Holds the state of the KeyManager being used by the Output Manager Service @@ -276,69 +277,9 @@ where T: OutputManagerBackend + 'static pub async fn get_balance(&self, current_chain_tip: Option) -> Result { let db_clone = self.db.clone(); - let db_clone2 = self.db.clone(); - let db_clone3 = self.db.clone(); - let db_clone4 = self.db.clone(); - - let unspent_outputs = tokio::task::spawn_blocking(move || match db_clone.fetch(&DbKey::UnspentOutputs) { - Ok(None) => log_error( - DbKey::UnspentOutputs, - OutputManagerStorageError::UnexpectedResult("Could not retrieve unspent outputs".to_string()), - ), - Ok(Some(DbValue::UnspentOutputs(uo))) => Ok(uo), - Ok(Some(other)) => unexpected_result(DbKey::UnspentOutputs, other), - Err(e) => log_error(DbKey::UnspentOutputs, e), - }) - .await - .map_err(|err| OutputManagerStorageError::BlockingTaskSpawnError(err.to_string()))??; - - let pending_incoming_outputs = tokio::task::spawn_blocking(move || db_clone2.fetch_pending_incoming_outputs()) - .await - .map_err(|err| OutputManagerStorageError::BlockingTaskSpawnError(err.to_string()))??; - - let pending_outgoing_outputs = tokio::task::spawn_blocking(move || db_clone3.fetch_pending_outgoing_outputs()) - .await - .map_err(|err| OutputManagerStorageError::BlockingTaskSpawnError(err.to_string()))??; - - let time_locked_balance = if let Some(tip) = current_chain_tip { - let time_locked_outputs = tokio::task::spawn_blocking(move || { - db_clone4.fetch(&DbKey::TimeLockedUnspentOutputs(tip))?.ok_or_else(|| { - OutputManagerStorageError::UnexpectedResult("Time-locked Outputs cannot be retrieved".to_string()) - }) - }) + tokio::task::spawn_blocking(move || db_clone.get_balance(current_chain_tip)) .await - .map_err(|err| OutputManagerStorageError::BlockingTaskSpawnError(err.to_string()))??; - if let DbValue::UnspentOutputs(time_locked_uo) = time_locked_outputs { - Some( - time_locked_uo - .iter() - .fold(MicroTari::from(0), |acc, x| acc + x.unblinded_output.value), - ) - } else { - None - } - } else { - None - }; - - let available_balance = unspent_outputs - .iter() - .fold(MicroTari::from(0), |acc, x| acc + x.unblinded_output.value); - - let pending_incoming = pending_incoming_outputs - .iter() - .fold(MicroTari::from(0), |acc, x| acc + x.unblinded_output.value); - - let pending_outgoing = pending_outgoing_outputs - .iter() - .fold(MicroTari::from(0), |acc, x| acc + x.unblinded_output.value); - - Ok(Balance { - available_balance, - time_locked_balance, - pending_incoming_balance: pending_incoming, - pending_outgoing_balance: pending_outgoing, - }) + .map_err(|err| OutputManagerStorageError::BlockingTaskSpawnError(err.to_string()))? } /// This method is called when a transaction is built to be sent. It will encumber unspent outputs against a pending diff --git a/base_layer/wallet/src/output_manager_service/storage/sqlite_db.rs b/base_layer/wallet/src/output_manager_service/storage/sqlite_db.rs index e71187118e..487ac37097 100644 --- a/base_layer/wallet/src/output_manager_service/storage/sqlite_db.rs +++ b/base_layer/wallet/src/output_manager_service/storage/sqlite_db.rs @@ -23,6 +23,7 @@ use crate::{ output_manager_service::{ error::OutputManagerStorageError, + service::Balance, storage::{ database::{DbKey, DbKeyValuePair, DbValue, KeyManagerState, OutputManagerBackend, WriteOperation}, models::{DbUnblindedOutput, KnownOneSidedPaymentScript, OutputStatus}, @@ -38,7 +39,7 @@ use crate::{ }; use aes_gcm::{aead::Error as AeadError, Aes256Gcm, Error}; use chrono::{NaiveDateTime, Utc}; -use diesel::{prelude::*, result::Error as DieselError, SqliteConnection}; +use diesel::{prelude::*, result::Error as DieselError, sql_query, SqliteConnection}; use log::*; use std::{ convert::{TryFrom, TryInto}, @@ -330,24 +331,6 @@ impl OutputManagerBackend for OutputManagerSqliteDatabase { .collect::, _>>() } - fn fetch_pending_outgoing_outputs(&self) -> Result, OutputManagerStorageError> { - let conn = self.database_connection.acquire_lock(); - - let mut outputs = OutputSql::index_status(OutputStatus::EncumberedToBeSpent, &conn)?; - outputs.extend(OutputSql::index_status( - OutputStatus::ShortTermEncumberedToBeSpent, - &conn, - )?); - outputs.extend(OutputSql::index_status(OutputStatus::SpentMinedUnconfirmed, &conn)?); - for o in outputs.iter_mut() { - self.decrypt_if_necessary(o)?; - } - outputs - .iter() - .map(|o| DbUnblindedOutput::try_from(o.clone())) - .collect::, _>>() - } - fn write(&self, op: WriteOperation) -> Result, OutputManagerStorageError> { let conn = self.database_connection.acquire_lock(); @@ -650,6 +633,12 @@ impl OutputManagerBackend for OutputManagerSqliteDatabase { } } + fn get_balance(&self, tip: Option) -> Result { + let conn = self.database_connection.acquire_lock(); + + OutputSql::get_balance(tip, &(*conn)) + } + fn cancel_pending_transaction(&self, tx_id: TxId) -> Result<(), OutputManagerStorageError> { let conn = self.database_connection.acquire_lock(); @@ -1047,6 +1036,104 @@ impl OutputSql { .first::(conn)?) } + /// Return the available, time locked, pending incoming and pending outgoing balance + pub fn get_balance(tip: Option, conn: &SqliteConnection) -> Result { + #[derive(QueryableByName, Clone)] + struct BalanceQueryResult { + #[sql_type = "diesel::sql_types::BigInt"] + amount: i64, + #[sql_type = "diesel::sql_types::Text"] + category: String, + } + let balance_query_result = if let Some(val) = tip { + let balance_query = sql_query( + "SELECT coalesce(sum(value), 0) as amount, 'available_balance' as category \ + FROM outputs WHERE status = ? \ + UNION ALL \ + SELECT coalesce(sum(value), 0) as amount, 'time_locked_balance' as category \ + FROM outputs WHERE status = ? AND maturity > ? \ + UNION ALL \ + SELECT coalesce(sum(value), 0) as amount, 'pending_incoming_balance' as category \ + FROM outputs WHERE status = ? OR status = ? OR status = ? \ + UNION ALL \ + SELECT coalesce(sum(value), 0) as amount, 'pending_outgoing_balance' as category \ + FROM outputs WHERE status = ? OR status = ? OR status = ?", + ) + // available_balance + .bind::(OutputStatus::Unspent as i32) + // time_locked_balance + .bind::(OutputStatus::Unspent as i32) + .bind::(val as i64) + // pending_incoming_balance + .bind::(OutputStatus::EncumberedToBeReceived as i32) + .bind::(OutputStatus::ShortTermEncumberedToBeReceived as i32) + .bind::(OutputStatus::UnspentMinedUnconfirmed as i32) + // pending_outgoing_balance + .bind::(OutputStatus::EncumberedToBeSpent as i32) + .bind::(OutputStatus::ShortTermEncumberedToBeSpent as i32) + .bind::(OutputStatus::SpentMinedUnconfirmed as i32); + balance_query.load::(conn)? + } else { + let balance_query = sql_query( + "SELECT coalesce(sum(value), 0) as amount, 'available_balance' as category \ + FROM outputs WHERE status = ? \ + UNION ALL \ + SELECT coalesce(sum(value), 0) as amount, 'pending_incoming_balance' as category \ + FROM outputs WHERE status = ? OR status = ? OR status = ? \ + UNION ALL \ + SELECT coalesce(sum(value), 0) as amount, 'pending_outgoing_balance' as category \ + FROM outputs WHERE status = ? OR status = ? OR status = ?", + ) + // available_balance + .bind::(OutputStatus::Unspent as i32) + // pending_incoming_balance + .bind::(OutputStatus::EncumberedToBeReceived as i32) + .bind::(OutputStatus::ShortTermEncumberedToBeReceived as i32) + .bind::(OutputStatus::UnspentMinedUnconfirmed as i32) + // pending_outgoing_balance + .bind::(OutputStatus::EncumberedToBeSpent as i32) + .bind::(OutputStatus::ShortTermEncumberedToBeSpent as i32) + .bind::(OutputStatus::SpentMinedUnconfirmed as i32); + balance_query.load::(conn)? + }; + let mut available_balance = None; + let mut time_locked_balance = Some(None); + let mut pending_incoming_balance = None; + let mut pending_outgoing_balance = None; + for balance in balance_query_result.clone() { + match balance.category.as_str() { + "available_balance" => available_balance = Some(MicroTari::from(balance.amount as u64)), + "time_locked_balance" => time_locked_balance = Some(Some(MicroTari::from(balance.amount as u64))), + "pending_incoming_balance" => pending_incoming_balance = Some(MicroTari::from(balance.amount as u64)), + "pending_outgoing_balance" => pending_outgoing_balance = Some(MicroTari::from(balance.amount as u64)), + _ => { + return Err(OutputManagerStorageError::UnexpectedResult( + "Unexpected category in balance query".to_string(), + )) + }, + } + } + + Ok(Balance { + available_balance: available_balance.ok_or_else(|| { + OutputManagerStorageError::UnexpectedResult("Available balance could not be calculated".to_string()) + })?, + time_locked_balance: time_locked_balance.ok_or_else(|| { + OutputManagerStorageError::UnexpectedResult("Time locked balance could not be calculated".to_string()) + })?, + pending_incoming_balance: pending_incoming_balance.ok_or_else(|| { + OutputManagerStorageError::UnexpectedResult( + "Pending incoming balance could not be calculated".to_string(), + ) + })?, + pending_outgoing_balance: pending_outgoing_balance.ok_or_else(|| { + OutputManagerStorageError::UnexpectedResult( + "Pending outgoing balance could not be calculated".to_string(), + ) + })?, + }) + } + pub fn find_by_commitment( commitment: &[u8], conn: &SqliteConnection, diff --git a/base_layer/wallet/tests/output_manager_service/service.rs b/base_layer/wallet/tests/output_manager_service/service.rs index f52d70664f..46f781d45c 100644 --- a/base_layer/wallet/tests/output_manager_service/service.rs +++ b/base_layer/wallet/tests/output_manager_service/service.rs @@ -759,6 +759,7 @@ async fn test_get_balance() { let balance = oms.get_balance().await.unwrap(); assert_eq!(output_val, balance.available_balance); + assert_eq!(output_val, balance.time_locked_balance.unwrap()); assert_eq!(recv_value + change_val, balance.pending_incoming_balance); assert_eq!(output_val, balance.pending_outgoing_balance); } @@ -776,6 +777,10 @@ async fn sending_transaction_with_short_term_clear() { let (_ti, uo) = make_input(&mut OsRng.clone(), available_balance, &factories.commitment); oms.add_output(uo).await.unwrap(); + let balance = oms.get_balance().await.unwrap(); + assert_eq!(balance.available_balance, available_balance); + assert_eq!(balance.time_locked_balance.unwrap(), available_balance); + // Check that funds are encumbered and then unencumbered if the pending tx is not confirmed before restart let _stp = oms .prepare_transaction_to_send( @@ -790,6 +795,8 @@ async fn sending_transaction_with_short_term_clear() { .unwrap(); let balance = oms.get_balance().await.unwrap(); + assert_eq!(balance.available_balance, MicroTari::from(0)); + assert_eq!(balance.time_locked_balance.unwrap(), MicroTari::from(0)); assert_eq!(balance.pending_outgoing_balance, available_balance); drop(oms); @@ -797,6 +804,7 @@ async fn sending_transaction_with_short_term_clear() { let balance = oms.get_balance().await.unwrap(); assert_eq!(balance.available_balance, available_balance); + assert_eq!(balance.time_locked_balance.unwrap(), available_balance); // Check that is the pending tx is confirmed that the encumberance persists after restart let stp = oms @@ -817,6 +825,8 @@ async fn sending_transaction_with_short_term_clear() { let (mut oms, _, _shutdown, _, _, _, _, _) = setup_output_manager_service(backend, true).await; let balance = oms.get_balance().await.unwrap(); + assert_eq!(balance.available_balance, MicroTari::from(0)); + assert_eq!(balance.time_locked_balance.unwrap(), MicroTari::from(0)); assert_eq!(balance.pending_outgoing_balance, available_balance); } @@ -1080,6 +1090,7 @@ async fn test_txo_validation() { balance.available_balance, MicroTari::from(output2_value) + MicroTari::from(output3_value) ); + assert_eq!(balance.available_balance, balance.time_locked_balance.unwrap()); assert_eq!(balance.pending_outgoing_balance, MicroTari::from(output1_value)); assert_eq!( balance.pending_incoming_balance, @@ -1179,6 +1190,7 @@ async fn test_txo_validation() { balance.available_balance, MicroTari::from(output2_value) + MicroTari::from(output3_value) ); + assert_eq!(balance.available_balance, balance.time_locked_balance.unwrap()); assert_eq!(oms.get_unspent_outputs().await.unwrap().len(), 2); @@ -1226,6 +1238,7 @@ async fn test_txo_validation() { ); assert_eq!(balance.pending_outgoing_balance, MicroTari::from(0)); assert_eq!(balance.pending_incoming_balance, MicroTari::from(0)); + assert_eq!(balance.available_balance, balance.time_locked_balance.unwrap()); // Trigger another validation and only Output3 should be checked oms.validate_txos().await.unwrap(); @@ -1331,6 +1344,7 @@ async fn test_txo_validation() { balance.pending_incoming_balance, MicroTari::from(output1_value) - MicroTari::from(900_300) ); + assert_eq!(balance.available_balance, balance.time_locked_balance.unwrap()); // Now we will update the mined_height in the responses so that the outputs on the reorged chain are confirmed // Output 1: Spent in Block 5 - Confirmed @@ -1390,6 +1404,7 @@ async fn test_txo_validation() { ); assert_eq!(balance.pending_outgoing_balance, MicroTari::from(0)); assert_eq!(balance.pending_incoming_balance, MicroTari::from(0)); + assert_eq!(balance.available_balance, balance.time_locked_balance.unwrap()); } #[tokio::test] diff --git a/base_layer/wallet/tests/output_manager_service/storage.rs b/base_layer/wallet/tests/output_manager_service/storage.rs index d22b17d46a..2d1a8e1f12 100644 --- a/base_layer/wallet/tests/output_manager_service/storage.rs +++ b/base_layer/wallet/tests/output_manager_service/storage.rs @@ -69,6 +69,15 @@ pub fn test_db_backend(backend: T) { assert_eq!(time_locked_outputs.len(), 0); let time_locked_balance = unspent_outputs[4].unblinded_output.value; + for i in 0..4usize { + let balance = runtime.block_on(db.get_balance(Some(i as u64))).unwrap(); + let mut sum = MicroTari::from(0); + for output in unspent_outputs.iter().take(5).skip(i + 1) { + sum += output.unblinded_output.value; + } + assert_eq!(balance.time_locked_balance.unwrap(), sum); + } + unspent_outputs.sort(); let outputs = runtime.block_on(db.fetch_sorted_unspent_outputs()).unwrap(); From 6cd9228d61a4b827c4062267074d40b1908773a6 Mon Sep 17 00:00:00 2001 From: Hansie Odendaal <39146854+hansieodendaal@users.noreply.github.com> Date: Tue, 12 Oct 2021 15:21:36 +0200 Subject: [PATCH 3/3] fix: fix confusing names in get_balance functions (#3447) Description --- Fix confusing names in `get_balance` functions Motivation and Context --- Comments as per PR #3446 How Has This Been Tested? --- cargo clippy cargo formal all --- .../src/output_manager_service/service.rs | 19 +++++++++++++------ .../storage/database.rs | 12 +++++++++--- .../storage/sqlite_db.rs | 16 +++++++++++----- 3 files changed, 33 insertions(+), 14 deletions(-) diff --git a/base_layer/wallet/src/output_manager_service/service.rs b/base_layer/wallet/src/output_manager_service/service.rs index 07d1bb4c8b..238e3dd476 100644 --- a/base_layer/wallet/src/output_manager_service/service.rs +++ b/base_layer/wallet/src/output_manager_service/service.rs @@ -201,11 +201,11 @@ where .await .map(|_| OutputManagerResponse::OutputMetadataSignatureUpdated), OutputManagerRequest::GetBalance => { - let current_chain_tip = match self.base_node_service.get_chain_metadata().await { + let current_tip_for_time_lock_calculation = match self.base_node_service.get_chain_metadata().await { Ok(metadata) => metadata.map(|m| m.height_of_longest_chain()), Err(_) => None, }; - self.get_balance(current_chain_tip) + self.get_balance(current_tip_for_time_lock_calculation) .await .map(OutputManagerResponse::Balance) }, @@ -406,8 +406,15 @@ where Ok(()) } - async fn get_balance(&self, current_chain_tip: Option) -> Result { - let balance = self.resources.db.get_balance(current_chain_tip).await?; + async fn get_balance( + &self, + current_tip_for_time_lock_calculation: Option, + ) -> Result { + let balance = self + .resources + .db + .get_balance(current_tip_for_time_lock_calculation) + .await?; trace!(target: LOG_TARGET, "Balance: {:?}", balance); Ok(balance) } @@ -938,8 +945,8 @@ where let enough_spendable = utxos_total_value > amount + fee_with_change; if !perfect_utxo_selection && !enough_spendable { - let current_chain_tip = chain_metadata.map(|cm| cm.height_of_longest_chain()); - let balance = self.get_balance(current_chain_tip).await?; + let current_tip_for_time_lock_calculation = chain_metadata.map(|cm| cm.height_of_longest_chain()); + let balance = self.get_balance(current_tip_for_time_lock_calculation).await?; let pending_incoming = balance.pending_incoming_balance; if utxos_total_value + pending_incoming >= amount + fee_with_change { return Err(OutputManagerError::FundsPending); diff --git a/base_layer/wallet/src/output_manager_service/storage/database.rs b/base_layer/wallet/src/output_manager_service/storage/database.rs index 6d7c6ae910..f70197c95e 100644 --- a/base_layer/wallet/src/output_manager_service/storage/database.rs +++ b/base_layer/wallet/src/output_manager_service/storage/database.rs @@ -119,7 +119,10 @@ pub trait OutputManagerBackend: Send + Sync + Clone { /// Reinstate a cancelled inbound output fn reinstate_cancelled_inbound_output(&self, tx_id: TxId) -> Result<(), OutputManagerStorageError>; /// Return the available, time locked, pending incoming and pending outgoing balance - fn get_balance(&self, tip: Option) -> Result; + fn get_balance( + &self, + current_tip_for_time_lock_calculation: Option, + ) -> Result; } /// Holds the state of the KeyManager being used by the Output Manager Service @@ -275,9 +278,12 @@ where T: OutputManagerBackend + 'static Ok(()) } - pub async fn get_balance(&self, current_chain_tip: Option) -> Result { + pub async fn get_balance( + &self, + current_tip_for_time_lock_calculation: Option, + ) -> Result { let db_clone = self.db.clone(); - tokio::task::spawn_blocking(move || db_clone.get_balance(current_chain_tip)) + tokio::task::spawn_blocking(move || db_clone.get_balance(current_tip_for_time_lock_calculation)) .await .map_err(|err| OutputManagerStorageError::BlockingTaskSpawnError(err.to_string()))? } diff --git a/base_layer/wallet/src/output_manager_service/storage/sqlite_db.rs b/base_layer/wallet/src/output_manager_service/storage/sqlite_db.rs index 487ac37097..8a448ff1fb 100644 --- a/base_layer/wallet/src/output_manager_service/storage/sqlite_db.rs +++ b/base_layer/wallet/src/output_manager_service/storage/sqlite_db.rs @@ -633,10 +633,13 @@ impl OutputManagerBackend for OutputManagerSqliteDatabase { } } - fn get_balance(&self, tip: Option) -> Result { + fn get_balance( + &self, + current_tip_for_time_lock_calculation: Option, + ) -> Result { let conn = self.database_connection.acquire_lock(); - OutputSql::get_balance(tip, &(*conn)) + OutputSql::get_balance(current_tip_for_time_lock_calculation, &(*conn)) } fn cancel_pending_transaction(&self, tx_id: TxId) -> Result<(), OutputManagerStorageError> { @@ -1037,7 +1040,10 @@ impl OutputSql { } /// Return the available, time locked, pending incoming and pending outgoing balance - pub fn get_balance(tip: Option, conn: &SqliteConnection) -> Result { + pub fn get_balance( + current_tip_for_time_lock_calculation: Option, + conn: &SqliteConnection, + ) -> Result { #[derive(QueryableByName, Clone)] struct BalanceQueryResult { #[sql_type = "diesel::sql_types::BigInt"] @@ -1045,7 +1051,7 @@ impl OutputSql { #[sql_type = "diesel::sql_types::Text"] category: String, } - let balance_query_result = if let Some(val) = tip { + let balance_query_result = if let Some(current_tip) = current_tip_for_time_lock_calculation { let balance_query = sql_query( "SELECT coalesce(sum(value), 0) as amount, 'available_balance' as category \ FROM outputs WHERE status = ? \ @@ -1063,7 +1069,7 @@ impl OutputSql { .bind::(OutputStatus::Unspent as i32) // time_locked_balance .bind::(OutputStatus::Unspent as i32) - .bind::(val as i64) + .bind::(current_tip as i64) // pending_incoming_balance .bind::(OutputStatus::EncumberedToBeReceived as i32) .bind::(OutputStatus::ShortTermEncumberedToBeReceived as i32)