Wallet Performance enhancements - cache commit + transaction fetching logic (#2375)

* add optional cached commit to output data, to speed up queries

* rustfmt

* clean up transaction retrieval logic

* rustfmt

* small logic error

* rename cache method

* rustfmt

config/src/comments.rs

@@ -411,6 +411,15 @@ fn comments() -> HashMap<String, String> {
 		"data_file_dir".to_string(),
 		"
 #where to find wallet files (seed, data, etc)
+"
+		.to_string(),
+	);
+	retval.insert(
+		"no_commit_cache".to_string(),
+		"
+#If true, don't store calculated commits in the database
+#better privacy, but at a performance cost of having to 
+#re-calculate commits every time they're used
 "
 		.to_string(),
 	);

wallet/src/libwallet/api.rs

@@ -431,7 +431,7 @@ where
 
 		let res = Ok((
 			validated,
-			updater::retrieve_txs(&mut *w, tx_id, tx_slate_id, Some(&parent_key_id))?,
+			updater::retrieve_txs(&mut *w, tx_id, tx_slate_id, Some(&parent_key_id), false)?,
 		));
 
 		w.close()?;
@@ -867,7 +867,7 @@ where
 			None => w.parent_key_id(),
 		};
 		// Don't do this multiple times
-		let tx = updater::retrieve_txs(&mut *w, None, Some(slate.id), Some(&parent_key_id))?;
+		let tx = updater::retrieve_txs(&mut *w, None, Some(slate.id), Some(&parent_key_id), false)?;
 		for t in &tx {
 			if t.tx_type == TxLogEntryType::TxReceived {
 				return Err(ErrorKind::TransactionAlreadyReceived(slate.id.to_string()).into());

wallet/src/libwallet/internal/restore.rs

@@ -142,6 +142,7 @@ where
 	C: NodeClient,
 	K: Keychain,
 {
+	let commit = wallet.calc_commit_for_cache(output.value, &output.key_id)?;
 	let mut batch = wallet.batch()?;
 
 	let parent_key_id = output.key_id.parent_path();
@@ -167,6 +168,7 @@ where
 		key_id: output.key_id,
 		n_child: output.n_child,
 		mmr_index: Some(output.mmr_index),
+		commit: commit,
 		value: output.value,
 		status: OutputStatus::Unspent,
 		height: output.height,
@@ -198,6 +200,7 @@ where
 			output.tx_log_entry.clone(),
 			None,
 			Some(&parent_key_id),
+			false,
 		)?;
 		if entries.len() > 0 {
 			let mut entry = entries[0].clone();

wallet/src/libwallet/internal/selection.rs

@@ -21,6 +21,8 @@ use crate::libwallet::error::{Error, ErrorKind};
 use crate::libwallet::internal::keys;
 use crate::libwallet::types::*;
 
+use std::collections::HashMap;
+
 /// Initialize a transaction on the sender side, returns a corresponding
 /// libwallet transaction slate with the appropriate inputs selected,
 /// and saves the private wallet identifiers of our selected outputs
@@ -85,9 +87,15 @@ where
 		context.add_input(&input.key_id, &input.mmr_index);
 	}
 
-	// Store change output(s)
+	let mut commits: HashMap<Identifier, Option<String>> = HashMap::new();
-	for (_, id, mmr_index) in &change_amounts_derivations {
+
+	// Store change output(s) and cached commits
+	for (change_amount, id, mmr_index) in &change_amounts_derivations {
 		context.add_output(&id, &mmr_index);
+		commits.insert(
+			id.clone(),
+			wallet.calc_commit_for_cache(*change_amount, &id)?,
+		);
 	}
 
 	let lock_inputs = context.get_inputs().clone();
@@ -119,10 +127,12 @@ where
 			for (change_amount, id, _) in &change_amounts_derivations {
 				t.num_outputs += 1;
 				t.amount_credited += change_amount;
+				let commit = commits.get(&id).unwrap().clone();
 				batch.save(OutputData {
 					root_key_id: parent_key_id.clone(),
 					key_id: id.clone(),
 					n_child: id.to_path().last_path_index(),
+					commit: commit,
 					mmr_index: None,
 					value: change_amount.clone(),
 					status: OutputStatus::Unconfirmed,
@@ -189,6 +199,7 @@ where
 	// Create closure that adds the output to recipient's wallet
 	// (up to the caller to decide when to do)
 	let wallet_add_fn = move |wallet: &mut T| {
+		let commit = wallet.calc_commit_for_cache(amount, &key_id_inner)?;
 		let mut batch = wallet.batch()?;
 		let log_id = batch.next_tx_log_id(&parent_key_id)?;
 		let mut t = TxLogEntry::new(parent_key_id.clone(), TxLogEntryType::TxReceived, log_id);
@@ -200,6 +211,7 @@ where
 			key_id: key_id_inner.clone(),
 			mmr_index: None,
 			n_child: key_id_inner.to_path().last_path_index(),
+			commit: commit,
 			value: amount,
 			status: OutputStatus::Unconfirmed,
 			height: height,

wallet/src/libwallet/internal/tx.rs

@@ -161,7 +161,7 @@ where
 	} else if let Some(tx_slate_id) = tx_slate_id {
 		tx_id_string = tx_slate_id.to_string();
 	}
-	let tx_vec = updater::retrieve_txs(wallet, tx_id, tx_slate_id, Some(&parent_key_id))?;
+	let tx_vec = updater::retrieve_txs(wallet, tx_id, tx_slate_id, Some(&parent_key_id), false)?;
 	if tx_vec.len() != 1 {
 		return Err(ErrorKind::TransactionDoesntExist(tx_id_string))?;
 	}
@@ -191,7 +191,7 @@ where
 	C: NodeClient,
 	K: Keychain,
 {
-	let tx_vec = updater::retrieve_txs(wallet, Some(tx_id), None, Some(parent_key_id))?;
+	let tx_vec = updater::retrieve_txs(wallet, Some(tx_id), None, Some(parent_key_id), false)?;
 	if tx_vec.len() != 1 {
 		return Err(ErrorKind::TransactionDoesntExist(tx_id.to_string()))?;
 	}
@@ -207,7 +207,7 @@ where
 	K: Keychain,
 {
 	// finalize command
-	let tx_vec = updater::retrieve_txs(wallet, None, Some(slate.id), None)?;
+	let tx_vec = updater::retrieve_txs(wallet, None, Some(slate.id), None, false)?;
 	let mut tx = None;
 	// don't want to assume this is the right tx, in case of self-sending
 	for t in tx_vec {

wallet/src/libwallet/internal/updater.rs

@@ -74,7 +74,10 @@ where
 	let res = outputs
 		.into_iter()
 		.map(|out| {
-			let commit = keychain.commit(out.value, &out.key_id).unwrap();
+			let commit = match out.commit.clone() {
+				Some(c) => pedersen::Commitment::from_vec(util::from_hex(c).unwrap()),
+				None => keychain.commit(out.value, &out.key_id).unwrap(),
+			};
 			(out, commit)
 		})
 		.collect();
@@ -88,30 +91,39 @@ pub fn retrieve_txs<T: ?Sized, C, K>(
 	tx_id: Option<u32>,
 	tx_slate_id: Option<Uuid>,
 	parent_key_id: Option<&Identifier>,
+	outstanding_only: bool,
 ) -> Result<Vec<TxLogEntry>, Error>
 where
 	T: WalletBackend<C, K>,
 	C: NodeClient,
 	K: Keychain,
 {
-	// just read the wallet here, no need for a write lock
+	let mut txs: Vec<TxLogEntry> = wallet
-	let mut txs = if let Some(id) = tx_id {
-		wallet.tx_log_iter().filter(|t| t.id == id).collect()
-	} else if tx_slate_id.is_some() {
-		wallet
 		.tx_log_iter()
-			.filter(|t| t.tx_slate_id == tx_slate_id)
+		.filter(|tx_entry| {
-			.collect()
+			let f_pk = match parent_key_id {
-	} else {
+				Some(k) => tx_entry.parent_key_id == *k,
-		wallet.tx_log_iter().collect::<Vec<_>>()
+				None => true,
 			};
-	if let Some(k) = parent_key_id {
+			let f_tx_id = match tx_id {
-		txs = txs
+				Some(i) => tx_entry.id == i,
-			.iter()
+				None => true,
-			.filter(|t| t.parent_key_id == *k)
+			};
-			.map(|t| t.clone())
+			let f_txs = match tx_slate_id {
-			.collect();
+				Some(t) => tx_entry.tx_slate_id == Some(t),
+				None => true,
+			};
+			let f_outstanding = match outstanding_only {
+				true => {
+					!tx_entry.confirmed
+						&& (tx_entry.tx_type == TxLogEntryType::TxReceived
+							|| tx_entry.tx_type == TxLogEntryType::TxSent)
 				}
+				false => true,
+			};
+			f_pk && f_tx_id && f_txs && f_outstanding
+		})
+		.collect();
 	txs.sort_by_key(|tx| tx.creation_ts);
 	Ok(txs)
 }
@@ -153,20 +165,18 @@ where
 		.filter(|x| x.root_key_id == *parent_key_id && x.status != OutputStatus::Spent)
 		.collect();
 
+	let tx_entries = retrieve_txs(wallet, None, None, Some(&parent_key_id), true)?;
+
 	// Only select outputs that are actually involved in an outstanding transaction
 	let unspents: Vec<OutputData> = match update_all {
 		false => unspents
 			.into_iter()
 			.filter(|x| match x.tx_log_entry.as_ref() {
 				Some(t) => {
-					let entries = retrieve_txs(wallet, Some(*t), None, Some(&parent_key_id));
+					if let Some(_) = tx_entries.iter().find(|&te| te.id == *t) {
-					match entries {
+						true
-						Err(_) => true,
+					} else {
-						Ok(e) => {
+						false
-							e.len() > 0
-								&& !e[0].confirmed && (e[0].tx_type == TxLogEntryType::TxReceived
-								|| e[0].tx_type == TxLogEntryType::TxSent)
-						}
 					}
 				}
 				None => true,
@@ -176,7 +186,10 @@ where
 	};
 
 	for out in unspents {
-		let commit = keychain.commit(out.value, &out.key_id)?;
+		let commit = match out.commit.clone() {
+			Some(c) => pedersen::Commitment::from_vec(util::from_hex(c).unwrap()),
+			None => keychain.commit(out.value, &out.key_id).unwrap(),
+		};
 		wallet_outputs.insert(commit, (out.key_id.clone(), out.mmr_index));
 	}
 	Ok(wallet_outputs)
@@ -466,13 +479,16 @@ where
 
 	{
 		// Now acquire the wallet lock and write the new output.
+		let amount = reward(block_fees.fees);
+		let commit = wallet.calc_commit_for_cache(amount, &key_id)?;
 		let mut batch = wallet.batch()?;
 		batch.save(OutputData {
 			root_key_id: parent_key_id,
 			key_id: key_id.clone(),
 			n_child: key_id.to_path().last_path_index(),
 			mmr_index: None,
-			value: reward(block_fees.fees),
+			commit: commit,
+			value: amount,
 			status: OutputStatus::Unconfirmed,
 			height: height,
 			lock_height: lock_height,

wallet/src/libwallet/types.rs

@@ -67,6 +67,13 @@ where
 	/// Return the client being used to communicate with the node
 	fn w2n_client(&mut self) -> &mut C;
 
+	/// return the commit for caching if allowed, none otherwise
+	fn calc_commit_for_cache(
+		&mut self,
+		amount: u64,
+		id: &Identifier,
+	) -> Result<Option<String>, Error>;
+
 	/// Set parent key id by stored account name
 	fn set_parent_key_id_by_name(&mut self, label: &str) -> Result<(), Error>;
 
@@ -241,6 +248,8 @@ pub struct OutputData {
 	pub key_id: Identifier,
 	/// How many derivations down from the root key
 	pub n_child: u32,
+	/// The actual commit, optionally stored
+	pub commit: Option<String>,
 	/// PMMR Index, used on restore in case of duplicate wallets using the same
 	/// key_id (2 wallets using same seed, for instance
 	pub mmr_index: Option<u64>,

wallet/src/lmdb_wallet.rs

@@ -199,6 +199,21 @@ where
 		&mut self.w2n_client
 	}
 
+	/// return the version of the commit for caching
+	fn calc_commit_for_cache(
+		&mut self,
+		amount: u64,
+		id: &Identifier,
+	) -> Result<Option<String>, Error> {
+		if self.config.no_commit_cache == Some(true) {
+			Ok(None)
+		} else {
+			Ok(Some(util::to_hex(
+				self.keychain().commit(amount, &id)?.0.to_vec(),
+			)))
+		}
+	}
+
 	/// Set parent path by account name
 	fn set_parent_key_id_by_name(&mut self, label: &str) -> Result<(), Error> {
 		let label = label.to_owned();

wallet/src/types.rs

@@ -52,6 +52,9 @@ pub struct WalletConfig {
 	pub owner_api_include_foreign: Option<bool>,
 	// The directory in which wallet files are stored
 	pub data_file_dir: String,
+	/// If Some(true), don't cache commits alongside output data
+	/// speed improvement, but your commits are in the database
+	pub no_commit_cache: Option<bool>,
 	/// TLS certificate file
 	pub tls_certificate_file: Option<String>,
 	/// TLS certificate private key file
@@ -74,6 +77,7 @@ impl Default for WalletConfig {
 			check_node_api_http_addr: "http://127.0.0.1:3413".to_string(),
 			owner_api_include_foreign: Some(false),
 			data_file_dir: ".".to_string(),
+			no_commit_cache: Some(false),
 			tls_certificate_file: None,
 			tls_certificate_key: None,
 			dark_background_color_scheme: Some(true),