fixup error handling in wallet restore (was failing when unable to recover amount) (#526) (#527)

src/bin/grin.rs

@@ -270,12 +270,14 @@ fn main() {
 		}
 
 		// client commands and options
-		("client", Some(client_args)) => match client_args.subcommand() {
+		("client", Some(client_args)) => {
+			match client_args.subcommand() {
 				("status", _) => {
 					println!("status info...");
 				}
 				_ => panic!("Unknown client command, use 'grin help client' for details"),
-		},
+			}
+		}
 
 		// client commands and options
 		("wallet", Some(wallet_args)) => {
@@ -382,14 +384,14 @@ fn wallet_command(wallet_args: &ArgMatches, global_config: GlobalConfig) {
 		wallet_config.check_node_api_http_addr = sa.to_string().clone();
 	}
 
-	let mut key_derivations:u32=1000;
+	let mut key_derivations: u32 = 1000;
 	if let Some(kd) = wallet_args.value_of("key_derivations") {
-		key_derivations=kd.parse().unwrap();
+		key_derivations = kd.parse().unwrap();
 	}
 
-	let mut show_spent=false;
+	let mut show_spent = false;
 	if wallet_args.is_present("show_spent") {
-		show_spent=true;
+		show_spent = true;
 	}
 
 	// Derive the keychain based on seed from seed file and specified passphrase.
@@ -403,12 +405,12 @@ fn wallet_command(wallet_args: &ArgMatches, global_config: GlobalConfig) {
 
 	let wallet_seed =
 		wallet::WalletSeed::from_file(&wallet_config).expect("Failed to read wallet seed file.");
-	let passphrase = wallet_args
+	let passphrase = wallet_args.value_of("pass").expect(
-		.value_of("pass")
+		"Failed to read passphrase.",
-		.expect("Failed to read passphrase.");
+	);
-	let keychain = wallet_seed
+	let keychain = wallet_seed.derive_keychain(&passphrase).expect(
-		.derive_keychain(&passphrase)
+		"Failed to derive keychain from seed file and passphrase.",
-		.expect("Failed to derive keychain from seed file and passphrase.");
+	);
 
 	match wallet_args.subcommand() {
 		("listen", Some(listen_args)) => {
@@ -416,38 +418,38 @@ fn wallet_command(wallet_args: &ArgMatches, global_config: GlobalConfig) {
 				wallet_config.api_listen_port = port.to_string();
 			}
 			wallet::server::start_rest_apis(wallet_config, keychain);
-		},
+		}
 		("receive", Some(receive_args)) => {
-			let input = receive_args
+			let input = receive_args.value_of("input").expect("Input file required");
-				.value_of("input")
+			let mut file = File::open(input).expect("Unable to open transaction file.");
-				.expect("Input file required");
-			let mut file = File::open(input)
-				.expect("Unable to open transaction file.");
 			let mut contents = String::new();
-			file.read_to_string(&mut contents)
+			file.read_to_string(&mut contents).expect(
-				.expect("Unable to read transaction file.");
+				"Unable to read transaction file.",
+			);
 			wallet::receive_json_tx_str(&wallet_config, &keychain, contents.as_str()).unwrap();
-		},
+		}
 		("send", Some(send_args)) => {
-			let amount = send_args
+			let amount = send_args.value_of("amount").expect(
-				.value_of("amount")
+				"Amount to send required",
-				.expect("Amount to send required");
+			);
-			let amount = core::core::amount_from_hr_string(amount)
+			let amount = core::core::amount_from_hr_string(amount).expect(
-				.expect("Could not parse amount as a number with optional decimal point.");
+				"Could not parse amount as a number with optional decimal point.",
-			let minimum_confirmations: u64 = send_args
+			);
+			let minimum_confirmations: u64 =
+				send_args
 					.value_of("minimum_confirmations")
 					.unwrap()
 					.parse()
 					.expect("Could not parse minimum_confirmations as a whole number.");
-			let selection_strategy = send_args
+			let selection_strategy = send_args.value_of("selection_strategy").expect(
-				.value_of("selection_strategy")
+				"Selection strategy required",
-				.expect("Selection strategy required");
+			);
 			let mut dest = "stdout";
 			if let Some(d) = send_args.value_of("dest") {
 				dest = d;
 			}
 			let max_outputs = 500;
-			let result=wallet::issue_send_tx(
+			let result = wallet::issue_send_tx(
 				&wallet_config,
 				&keychain,
 				amount,
@@ -464,30 +466,29 @@ fn wallet_command(wallet_args: &ArgMatches, global_config: GlobalConfig) {
 						amount_to_hr_string(amount),
 						dest,
 						selection_strategy,
-					)},
+					)
+				}
 				Err(wallet::Error::NotEnoughFunds(available)) => {
 					error!(
 						LOGGER,
 						"Tx not sent: insufficient funds (max: {})",
 						amount_to_hr_string(available),
 					);
-				},
+				}
 				Err(e) => {
-					error!(
+					error!(LOGGER, "Tx not sent: {:?}", e);
-						LOGGER,
+				}
-						"Tx not sent: {:?}",
-						e
-					);
-				},
 			};
 		}
 		("burn", Some(send_args)) => {
-			let amount = send_args
+			let amount = send_args.value_of("amount").expect(
-				.value_of("amount")
+				"Amount to burn required",
-				.expect("Amount to burn required");
+			);
-			let amount = core::core::amount_from_hr_string(amount)
+			let amount = core::core::amount_from_hr_string(amount).expect(
-				.expect("Could not parse amount as number with optional decimal point.");
+				"Could not parse amount as number with optional decimal point.",
-			let minimum_confirmations: u64 = send_args
+			);
+			let minimum_confirmations: u64 =
+				send_args
 					.value_of("minimum_confirmations")
 					.unwrap()
 					.parse()
@@ -508,7 +509,7 @@ fn wallet_command(wallet_args: &ArgMatches, global_config: GlobalConfig) {
 			wallet::show_outputs(&wallet_config, &keychain, show_spent);
 		}
 		("restore", Some(_)) => {
-			let _=wallet::restore(&wallet_config, &keychain, key_derivations);
+			let _ = wallet::restore(&wallet_config, &keychain, key_derivations);
 		}
 		_ => panic!("Unknown wallet command, use 'grin help wallet' for details"),
 	}

wallet/src/restore.rs

@@ -16,22 +16,16 @@ use keychain::{Keychain, Identifier};
 use util::{LOGGER, from_hex};
 use util::secp::pedersen;
 use api;
-use core::core::{Output,SwitchCommitHash};
+use core::core::{Output, SwitchCommitHash};
 use core::core::transaction::{COINBASE_OUTPUT, DEFAULT_OUTPUT, SWITCH_COMMIT_HASH_SIZE};
 use types::{WalletConfig, WalletData, OutputData, OutputStatus, Error};
 use byteorder::{BigEndian, ByteOrder};
 
-pub fn get_chain_height(config: &WalletConfig)->
+pub fn get_chain_height(config: &WalletConfig) -> Result<u64, Error> {
-	Result<u64, Error>{
+	let url = format!("{}/v1/chain", config.check_node_api_http_addr);
-	let url = format!(
-		"{}/v1/chain",
-		config.check_node_api_http_addr
-	);
 
 	match api::client::get::<api::Tip>(url.as_str()) {
-		Ok(tip) => {
+		Ok(tip) => Ok(tip.height),
-			Ok(tip.height)
-		},
 		Err(e) => {
 			// if we got anything other than 200 back from server, bye
 			error!(LOGGER, "Restore failed... unable to contact node: {}", e);
@@ -40,10 +34,9 @@ pub fn get_chain_height(config: &WalletConfig)->
 	}
 }
 
-fn output_with_range_proof(config:&WalletConfig, commit_id: &str) ->
+fn output_with_range_proof(config: &WalletConfig, commit_id: &str) -> Result<api::Output, Error> {
-	Result<api::Output, Error>{
+	let url =
-
+		format!(
-	let url = format!(
 		"{}/v1/chain/utxos/byids?id={}&include_rp&include_switch",
 		config.check_node_api_http_addr,
 		commit_id,
@@ -51,43 +44,59 @@ fn output_with_range_proof(config:&WalletConfig, commit_id: &str) ->
 
 	match api::client::get::<Vec<api::Output>>(url.as_str()) {
 		Ok(outputs) => {
-			Ok(outputs[0].clone())
+			if let Some(output) = outputs.first() {
-		},
+				Ok(output.clone())
+			} else {
+				Err(Error::Node(api::Error::NotFound))
+			}
+		}
 		Err(e) => {
-			// if we got anything other than 200 back from server, don't attempt to refresh the wallet
+			// if we got anything other than 200 back from server, don't attempt to refresh
+			// the wallet
 			// data after
 			Err(Error::Node(e))
 		}
 	}
 }
 
-fn retrieve_amount_and_coinbase_status(config:&WalletConfig, keychain: &Keychain,
+fn retrieve_amount_and_coinbase_status(
-	key_id: Identifier, commit_id: &str) -> (u64, bool) {
+	config: &WalletConfig,
-	let output = output_with_range_proof(config, commit_id).unwrap();
+	keychain: &Keychain,
+	key_id: Identifier,
+	commit_id: &str,
+) -> Result<(u64, bool), Error> {
+	let output = output_with_range_proof(config, commit_id)?;
 	let core_output = Output {
-		features : match output.output_type {
+		features: match output.output_type {
 			api::OutputType::Coinbase => COINBASE_OUTPUT,
 			api::OutputType::Transaction => DEFAULT_OUTPUT,
 		},
-		proof: output.proof.unwrap(),
+		proof: output.proof.expect("output with proof"),
-		switch_commit_hash: output.switch_commit_hash.unwrap(),
+		switch_commit_hash: output.switch_commit_hash.expect("output with switch_commit_hash"),
 		commit: output.commit,
 	};
-	let amount=core_output.recover_value(keychain, &key_id).unwrap();
+	if let Some(amount) = core_output.recover_value(keychain, &key_id) {
 		let is_coinbase = match output.output_type {
 			api::OutputType::Coinbase => true,
 			api::OutputType::Transaction => false,
 		};
-	(amount, is_coinbase)
+		Ok((amount, is_coinbase))
+	} else {
+		Err(Error::GenericError(format!("cannot recover value")))
+	}
 }
 
-pub fn utxos_batch_block(config: &WalletConfig, start_height: u64, end_height:u64)->
+pub fn utxos_batch_block(
-	Result<Vec<api::BlockOutputs>, Error>{
+	config: &WalletConfig,
+	start_height: u64,
+	end_height: u64,
+) -> Result<Vec<api::BlockOutputs>, Error> {
 	// build the necessary query param -
 	// ?height=x
-	let query_param= format!("start_height={}&end_height={}", start_height, end_height);
+	let query_param = format!("start_height={}&end_height={}", start_height, end_height);
 
-	let url = format!(
+	let url =
+		format!(
 		"{}/v1/chain/utxos/atheight?{}",
 		config.check_node_api_http_addr,
 		query_param,
@@ -103,85 +112,158 @@ pub fn utxos_batch_block(config: &WalletConfig, start_height: u64, end_height:u6
 	}
 }
 
-fn find_utxos_with_key(config:&WalletConfig, keychain: &Keychain, 
+fn find_utxos_with_key(
-	switch_commit_cache : &Vec<[u8;SWITCH_COMMIT_HASH_SIZE]>,
+	config: &WalletConfig,
-	block_outputs:api::BlockOutputs, key_iterations: &mut usize, padding: &mut usize) 
+	keychain: &Keychain,
-	-> Vec<(pedersen::Commitment, Identifier, u32, u64, u64, bool) > {
+	switch_commit_cache: &Vec<[u8; SWITCH_COMMIT_HASH_SIZE]>,
-	//let key_id = keychain.clone().root_key_id();
+	block_outputs: api::BlockOutputs,
-	let mut wallet_outputs: Vec<(pedersen::Commitment, Identifier, u32, u64, u64, bool)> = Vec::new();
+	key_iterations: &mut usize,
+	padding: &mut usize,
+) -> Vec<(pedersen::Commitment, Identifier, u32, u64, u64, bool)> {
+	let mut wallet_outputs: Vec<(pedersen::Commitment, Identifier, u32, u64, u64, bool)> =
+		Vec::new();
+
+	info!(
+		LOGGER,
+		"Scanning block {}, {} outputs, over {} key derivations",
+		block_outputs.header.height,
+		block_outputs.outputs.len(),
+		*key_iterations,
+	);
 
-	info!(LOGGER, "Scanning block {} over {} key derivation possibilities.", block_outputs.header.height, *key_iterations);
 	for output in block_outputs.outputs {
 		for i in 0..*key_iterations {
-			if switch_commit_cache[i as usize]==output.switch_commit_hash {
+			if switch_commit_cache[i as usize] == output.switch_commit_hash {
-				info!(LOGGER, "Output found: {:?}, key_index: {:?}", output.switch_commit_hash,i);
+				info!(
-				//add it to result set here
+					LOGGER,
+					"Output found: {:?}, key_index: {:?}",
+					output.switch_commit_hash,
+					i,
+				);
+
+				// add it to result set here
 				let commit_id = from_hex(output.commit.clone()).unwrap();
 				let key_id = keychain.derive_key_id(i as u32).unwrap();
-				let (amount, is_coinbase) = retrieve_amount_and_coinbase_status(config,
+				let res = retrieve_amount_and_coinbase_status(
-					keychain, key_id.clone(), &output.commit);
+					config,
+					keychain,
+					key_id.clone(),
+					&output.commit,
+				);
+
+				if let Ok((amount, is_coinbase)) = res {
 					info!(LOGGER, "Amount: {}", amount);
-				let commit = keychain.commit_with_key_index(BigEndian::read_u64(&commit_id), i as u32).unwrap();
+
-				wallet_outputs.push((commit, key_id.clone(), i as u32, amount, output.height, is_coinbase));
+					let commit = keychain
-				//probably don't have to look for indexes greater than this now
+						.commit_with_key_index(BigEndian::read_u64(&commit_id), i as u32)
-				*key_iterations=i+*padding;
+						.expect("commit with key index");
+
+					wallet_outputs.push((
+						commit,
+						key_id.clone(),
+						i as u32,
+						amount,
+						output.height,
+						is_coinbase,
+					));
+
+					// probably don't have to look for indexes greater than this now
+					*key_iterations = i + *padding;
 					if *key_iterations > switch_commit_cache.len() {
 						*key_iterations = switch_commit_cache.len();
 					}
 					info!(LOGGER, "Setting max key index to: {}", *key_iterations);
 					break;
+				} else {
+					info!(
+						LOGGER,
+						"Unable to retrieve the amount (needs investigating)",
+					);
+
 				}
 			}
 		}
+	}
+	debug!(
+		LOGGER,
+		"Found {} wallet_outputs for block {}",
+		wallet_outputs.len(),
+		block_outputs.header.height,
+	);
+
 	wallet_outputs
 }
 
-pub fn restore(config: &WalletConfig, keychain: &Keychain, key_derivations:u32) ->
+pub fn restore(
-	Result<(), Error>{
+	config: &WalletConfig,
+	keychain: &Keychain,
+	key_derivations: u32,
+) -> Result<(), Error> {
 	// Don't proceed if wallet.dat has anything in it
 	let is_empty = WalletData::read_wallet(&config.data_file_dir, |wallet_data| {
 		wallet_data.outputs.len() == 0
 	})?;
 	if !is_empty {
-		error!(LOGGER, "Not restoring. Please back up and remove existing wallet.dat first.");
+		error!(
-		return Ok(())
+			LOGGER,
+			"Not restoring. Please back up and remove existing wallet.dat first."
+		);
+		return Ok(());
 	}
 
-// Get height of chain from node (we'll check again when done)
+	// Get height of chain from node (we'll check again when done)
 	let chain_height = get_chain_height(config)?;
-	info!(LOGGER, "Starting restore: Chain height is {}.", chain_height);
+	info!(
+		LOGGER,
+		"Starting restore: Chain height is {}.",
+		chain_height
+	);
 
-	let mut switch_commit_cache : Vec<[u8;SWITCH_COMMIT_HASH_SIZE]> = vec![];
+	let mut switch_commit_cache: Vec<[u8; SWITCH_COMMIT_HASH_SIZE]> = vec![];
-	info!(LOGGER, "Building key derivation cache to index {}.", key_derivations);
+	info!(
+		LOGGER,
+		"Building key derivation cache ({}) ...",
+		key_derivations,
+	);
 	for i in 0..key_derivations {
 		let switch_commit = keychain.switch_commit_from_index(i as u32).unwrap();
 		let switch_commit_hash = SwitchCommitHash::from_switch_commit(switch_commit);
 		switch_commit_cache.push(switch_commit_hash.hash);
 	}
+	debug!(LOGGER, "... done");
 
-	let batch_size=100;
+	let batch_size = 100;
-	//this will start here, then lower as outputs are found, moving backwards on the chain
+	// this will start here, then lower as outputs are found, moving backwards on
-	let mut key_iterations=key_derivations as usize;
+	// the chain
-	//set to a percentage of the key_derivation value
+	let mut key_iterations = key_derivations as usize;
-	let mut padding = (key_iterations as f64 *0.25) as usize;
+	// set to a percentage of the key_derivation value
+	let mut padding = (key_iterations as f64 * 0.25) as usize;
 	let mut h = chain_height;
 	while {
-		let end_batch=h;
+		let end_batch = h;
 		if h >= batch_size {
-			h-=batch_size;
+			h -= batch_size;
 		} else {
-			h=0;
+			h = 0;
 		}
-		let mut blocks = utxos_batch_block(config, h+1, end_batch)?;
+		let mut blocks = utxos_batch_block(config, h + 1, end_batch)?;
 		blocks.reverse();
+
+		let _ = WalletData::with_wallet(&config.data_file_dir, |wallet_data| {
 			for block in blocks {
-			let result_vec=find_utxos_with_key(config, keychain, &switch_commit_cache,
+				let result_vec = find_utxos_with_key(
-				block, &mut key_iterations, &mut padding);
+					config,
+					keychain,
+					&switch_commit_cache,
+					block,
+					&mut key_iterations,
+					&mut padding,
+				);
 				if result_vec.len() > 0 {
 					for output in result_vec.clone() {
-					let _ = WalletData::with_wallet(&config.data_file_dir, |wallet_data| {
 						let root_key_id = keychain.root_key_id();
-						//Just plonk it in for now, and refresh actual values via wallet info command later 
+						// Just plonk it in for now, and refresh actual values via wallet info
+						// command later
 						wallet_data.add_output(OutputData {
 							root_key_id: root_key_id.clone(),
 							key_id: output.1.clone(),
@@ -192,11 +274,12 @@ pub fn restore(config: &WalletConfig, keychain: &Keychain, key_derivations:u32)
 							lock_height: 0,
 							is_coinbase: output.5,
 						});
+					};
+				}
+			}
 		});
-				}
-			}
-		}
 		h > 0
-	}{} 
+	}
+	{}
 	Ok(())
 }

wallet/src/types.rs

@@ -78,6 +78,7 @@ pub enum Error {
 	Hyper(hyper::Error),
 	/// Error originating from hyper uri parsing.
 	Uri(hyper::error::UriError),
+	GenericError(String,)
 }
 
 impl error::Error for Error {