Fix duplicate wallet coinbase (#158)

* store wallet output data in hashmap
* cleanup up commented out and debug code

keychain/src/extkey.rs

@@ -52,10 +52,14 @@ impl error::Error for Error {
 	}
 }
 
-#[derive(Serialize, Deserialize, Clone, PartialEq, Debug)]
+#[derive(Serialize, Deserialize, Clone, PartialEq, Eq, Debug, Hash)]
 pub struct Fingerprint(String);
 
 impl Fingerprint {
+	fn zero() -> Fingerprint {
+		Identifier::from_bytes(&[0; 4]).fingerprint()
+	}
+
 	fn from_bytes(bytes: &[u8]) -> Fingerprint {
 		let mut fingerprint = [0; 4];
 		for i in 0..min(4, bytes.len()) {
@@ -63,10 +67,6 @@ impl Fingerprint {
 		}
 		Fingerprint(util::to_hex(fingerprint.to_vec()))
 	}
-
-	fn zero() -> Fingerprint {
-		Fingerprint::from_bytes(&[0; 4])
-	}
 }
 
 impl fmt::Display for Fingerprint {
@@ -75,8 +75,8 @@ impl fmt::Display for Fingerprint {
 	}
 }
 
-#[derive(Serialize, Deserialize, Clone)]
-pub struct Identifier([u8; 20]);
+#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, Hash)]
+pub struct Identifier(String);
 
 impl Identifier {
 	fn from_bytes(bytes: &[u8]) -> Identifier {
@@ -84,27 +84,16 @@ impl Identifier {
 		for i in 0..min(20, bytes.len()) {
 			identifier[i] = bytes[i];
 		}
-		Identifier(identifier)
+		Identifier(util::to_hex(identifier.to_vec()))
+	}
+
+	pub fn to_hex(&self) -> String {
+		self.0.clone()
 	}
 
 	pub fn fingerprint(&self) -> Fingerprint {
-		Fingerprint::from_bytes(&self.0)
-	}
-}
-
-impl PartialEq for Identifier {
-	fn eq(&self, other: &Self) -> bool {
-		self.0.as_ref() == other.0.as_ref()
-	}
-}
-
-impl ::std::fmt::Debug for Identifier {
-	fn fmt(&self, f: &mut ::std::fmt::Formatter) -> ::std::fmt::Result {
-		try!(write!(f, "{}(", stringify!(Identifier)));
-		for i in self.0.iter().cloned() {
-			try!(write!(f, "{:02x}", i));
-		}
-		write!(f, ")")
+		let hex = &self.0[0..8];
+		Fingerprint(String::from(hex))
 	}
 }
 
@@ -199,8 +188,9 @@ impl ExtendedKey {
 
 		let mut secret_key = SecretKey::from_slice(&secp, &derived.as_bytes()[0..32])
 			.expect("Error deriving key");
-		secret_key.add_assign(secp, &self.key)
-			.expect("Error deriving key");
+		secret_key.add_assign(secp, &self.key).expect(
+			"Error deriving key",
+		);
 		// TODO check if key != 0 ?
 
 		let mut chain_code: [u8; 32] = [0; 32];
@@ -233,18 +223,26 @@ mod test {
 		let s = Secp256k1::new();
 		let seed = from_hex("000102030405060708090a0b0c0d0e0f");
 		let extk = ExtendedKey::from_seed(&s, &seed.as_slice()).unwrap();
-		let sec =
-			from_hex("c3f5ae520f474b390a637de4669c84d0ed9bbc21742577fac930834d3c3083dd");
+		let sec = from_hex(
+			"c3f5ae520f474b390a637de4669c84d0ed9bbc21742577fac930834d3c3083dd",
+		);
 		let secret_key = SecretKey::from_slice(&s, sec.as_slice()).unwrap();
-		let chaincode =
-			from_hex("e7298e68452b0c6d54837670896e1aee76b118075150d90d4ee416ece106ae72");
+		let chaincode = from_hex(
+			"e7298e68452b0c6d54837670896e1aee76b118075150d90d4ee416ece106ae72",
+		);
 		let identifier = from_hex("942b6c0bd43bdcb24f3edfe7fadbc77054ecc4f2");
 		let fingerprint = from_hex("942b6c0b");
 		let depth = 0;
 		let n_child = 0;
 		assert_eq!(extk.key, secret_key);
-		assert_eq!(extk.identifier(), Identifier::from_bytes(identifier.as_slice()));
-		assert_eq!(extk.fingerprint, Fingerprint::from_bytes(fingerprint.as_slice()));
+		assert_eq!(
+			extk.identifier(),
+			Identifier::from_bytes(identifier.as_slice())
+		);
+		assert_eq!(
+			extk.fingerprint,
+			Fingerprint::from_bytes(fingerprint.as_slice())
+		);
 		assert_eq!(
 			extk.identifier().fingerprint(),
 			Fingerprint::from_bytes(fingerprint.as_slice())
@@ -261,19 +259,27 @@ mod test {
 		let seed = from_hex("000102030405060708090a0b0c0d0e0f");
 		let extk = ExtendedKey::from_seed(&s, &seed.as_slice()).unwrap();
 		let derived = extk.derive(&s, 0).unwrap();
-		let sec =
-			from_hex("d75f70beb2bd3b56f9b064087934bdedee98e4b5aae6280c58b4eff38847888f");
+		let sec = from_hex(
+			"d75f70beb2bd3b56f9b064087934bdedee98e4b5aae6280c58b4eff38847888f",
+		);
 		let secret_key = SecretKey::from_slice(&s, sec.as_slice()).unwrap();
-		let chaincode =
-			from_hex("243cb881e1549e714db31d23af45540b13ad07941f64a786bbf3313b4de1df52");
+		let chaincode = from_hex(
+			"243cb881e1549e714db31d23af45540b13ad07941f64a786bbf3313b4de1df52",
+		);
 		let fingerprint = from_hex("942b6c0b");
 		let identifier = from_hex("8b011f14345f3f0071e85f6eec116de1e575ea10");
 		let identifier_fingerprint = from_hex("8b011f14");
 		let depth = 1;
 		let n_child = 0;
 		assert_eq!(derived.key, secret_key);
-		assert_eq!(derived.identifier(), Identifier::from_bytes(identifier.as_slice()));
-		assert_eq!(derived.fingerprint, Fingerprint::from_bytes(fingerprint.as_slice()));
+		assert_eq!(
+			derived.identifier(),
+			Identifier::from_bytes(identifier.as_slice())
+		);
+		assert_eq!(
+			derived.fingerprint,
+			Fingerprint::from_bytes(fingerprint.as_slice())
+		);
 		assert_eq!(
 			derived.identifier().fingerprint(),
 			Fingerprint::from_bytes(identifier_fingerprint.as_slice())

wallet/src/checker.rs

@@ -48,11 +48,12 @@ pub fn refresh_outputs(
 
 	WalletData::with_wallet(&config.data_file_dir, |wallet_data| {
 		// check each output that's not spent
-		for mut out in wallet_data.outputs.iter_mut().filter(|out| {
-			out.status != OutputStatus::Spent
-		})
+		for mut out in wallet_data.outputs
+			.values_mut()
+			.filter(|out| {
+				out.status != OutputStatus::Spent
+			})
 		{
-
 			// TODO check the pool for unconfirmed
 			match get_output_from_node(config, keychain, out.value, out.n_child) {
 				Ok(api_out) => refresh_output(&mut out, api_out, &tip),

wallet/src/info.rs

@@ -27,18 +27,18 @@ pub fn show_info(
 	let _ = WalletData::with_wallet(&config.data_file_dir, |wallet_data| {
 
 		println!("Outputs - ");
-		println!("fingerprint, n_child, height, lock_height, status, value");
+		println!("identifier, height, lock_height, status, value");
 		println!("----------------------------------");
-		for out in &mut wallet_data.outputs
-			.iter()
-			.filter(|out| out.fingerprint == fingerprint)
-		{
-			let pubkey = keychain.derive_pubkey(out.n_child).unwrap();
 
+		let mut outputs = wallet_data.outputs
+			.values()
+			.filter(|out| out.fingerprint == fingerprint)
+			.collect::<Vec<_>>();
+		outputs.sort_by_key(|out| out.n_child);
+		for out in outputs {
 			println!(
-				"{}, {}, {}, {}, {:?}, {}",
-				pubkey.fingerprint(),
-				out.n_child,
+				"{}..., {}, {}, {:?}, {}",
+				out.identifier.fingerprint(),
 				out.height,
 				out.lock_height,
 				out.status,

wallet/src/receiver.rs

@@ -107,7 +107,7 @@ impl ApiEndpoint for WalletReceiver {
 			"coinbase" => {
 				match input {
 					WalletReceiveRequest::Coinbase(cb_fees) => {
-						debug!("Operation {} with amount {}", op, cb_fees.fees);
+						debug!("Operation {} with fees {:?}", op, cb_fees);
 						let (out, kern, derivation) =
 							receive_coinbase(
 								&self.config,
@@ -168,7 +168,6 @@ fn receive_coinbase(
 	fees: u64,
 	mut derivation: u32,
 ) -> Result<(Output, TxKernel, u32), Error> {
-
 	let fingerprint = keychain.clone().fingerprint();
 
 	// operate within a lock on wallet data
@@ -179,8 +178,9 @@ fn receive_coinbase(
 		let pubkey = keychain.derive_pubkey(derivation)?;
 
 		// track the new output and return the stuff needed for reward
-		wallet_data.append_output(OutputData {
+		wallet_data.add_output(OutputData {
 			fingerprint: fingerprint.clone(),
+			identifier: pubkey.clone(),
 			n_child: derivation,
 			value: reward(fees),
 			status: OutputStatus::Unconfirmed,
@@ -213,7 +213,8 @@ fn receive_transaction(
 
 		// TODO - replace with real fee calculation
 		// TODO - note we are not enforcing this in consensus anywhere yet
-		let fee_amount = 1;
+		// Note: consensus rules require this to be an even value so it can be split
+		let fee_amount = 10;
 		let out_amount = amount - fee_amount;
 
 		let (tx_final, _) = build::transaction(vec![
@@ -227,8 +228,9 @@ fn receive_transaction(
 		tx_final.validate(&keychain.secp())?;
 
 		// track the new output and return the finalized transaction to broadcast
-		wallet_data.append_output(OutputData {
+		wallet_data.add_output(OutputData {
 			fingerprint: fingerprint.clone(),
+			identifier: pubkey.clone(),
 			n_child: derivation,
 			value: out_amount,
 			status: OutputStatus::Unconfirmed,

wallet/src/sender.rs

@@ -78,12 +78,13 @@ fn build_send_tx(
 		// derive an additional pubkey for change and build the change output
 		let change_derivation = wallet_data.next_child(fingerprint.clone());
 		let change_key = keychain.derive_pubkey(change_derivation)?;
-		parts.push(build::output(change as u64, change_key));
+		parts.push(build::output(change as u64, change_key.clone()));
 
 		// we got that far, time to start tracking the new output, finalize tx
 		// and lock the outputs used
-		wallet_data.append_output(OutputData {
+		wallet_data.add_output(OutputData {
 			fingerprint: fingerprint.clone(),
+			identifier: change_key.clone(),
 			n_child: change_derivation,
 			value: change as u64,
 			status: OutputStatus::Unconfirmed,
@@ -103,7 +104,6 @@ fn build_send_tx(
 #[cfg(test)]
 mod test {
 	use core::core::build::{input, output, transaction};
-	use types::{OutputData, OutputStatus};
 	use keychain::Keychain;
 
 	#[test]

wallet/src/types.rs

@@ -18,6 +18,7 @@ use std::fs::{self, File, OpenOptions};
 use std::io::Write;
 use std::path::Path;
 use std::path::MAIN_SEPARATOR;
+use std::collections::HashMap;
 
 use serde_json;
 use secp;
@@ -46,27 +47,39 @@ pub enum Error {
 }
 
 impl From<keychain::Error> for Error {
-	fn from(e: keychain::Error) -> Error { Error::Keychain(e) }
+	fn from(e: keychain::Error) -> Error {
+		Error::Keychain(e)
+	}
 }
 
 impl From<secp::Error> for Error {
-	fn from(e: secp::Error) -> Error { Error::Secp(e) }
+	fn from(e: secp::Error) -> Error {
+		Error::Secp(e)
+	}
 }
 
 impl From<transaction::Error> for Error {
-	fn from(e: transaction::Error) -> Error { Error::Transaction(e) }
+	fn from(e: transaction::Error) -> Error {
+		Error::Transaction(e)
+	}
 }
 
 impl From<serde_json::Error> for Error {
-	fn from(e: serde_json::Error) -> Error { Error::Format(e.to_string()) }
+	fn from(e: serde_json::Error) -> Error {
+		Error::Format(e.to_string())
+	}
 }
 
 impl From<num::ParseIntError> for Error {
-	fn from(_: num::ParseIntError) -> Error { Error::Format("Invalid hex".to_string()) }
+	fn from(_: num::ParseIntError) -> Error {
+		Error::Format("Invalid hex".to_string())
+	}
 }
 
 impl From<api::Error> for Error {
-	fn from(e: api::Error) -> Error { Error::Node(e) }
+	fn from(e: api::Error) -> Error {
+		Error::Node(e)
+	}
 }
 
 #[derive(Debug, Clone, Serialize, Deserialize)]
@@ -125,6 +138,7 @@ impl fmt::Display for OutputStatus {
 pub struct OutputData {
 	/// Private key fingerprint (in case the wallet tracks multiple)
 	pub fingerprint: keychain::Fingerprint,
+	pub identifier: keychain::Identifier,
 	/// How many derivations down from the root key
 	pub n_child: u32,
 	/// Value of the output, necessary to rebuild the commitment
@@ -133,12 +147,13 @@ pub struct OutputData {
 	pub status: OutputStatus,
 	/// Height of the output
 	pub height: u64,
+	/// Height we are locked until
 	pub lock_height: u64,
 }
 
 impl OutputData {
 	/// Lock a given output to avoid conflicting use
-	pub fn lock(&mut self) {
+	fn lock(&mut self) {
 		self.status = OutputStatus::Locked;
 	}
 }
@@ -153,7 +168,7 @@ impl OutputData {
 /// TODO write locks so files don't get overwritten
 #[derive(Serialize, Deserialize, Debug, Clone)]
 pub struct WalletData {
-	pub outputs: Vec<OutputData>,
+	pub outputs: HashMap<String, OutputData>,
 }
 
 impl WalletData {
@@ -229,7 +244,7 @@ impl WalletData {
 			WalletData::read(data_file_path)
 		} else {
 			// just create a new instance, it will get written afterward
-			Ok(WalletData { outputs: vec![] })
+			Ok(WalletData { outputs: HashMap::new() })
 		}
 	}
 
@@ -248,7 +263,7 @@ impl WalletData {
 		let mut data_file = File::create(data_file_path).map_err(|e| {
 			Error::WalletData(format!("Could not create {}: {}", data_file_path, e))
 		})?;
-		let res_json = serde_json::to_vec_pretty(self).map_err(|_| {
+		let res_json = serde_json::to_vec_pretty(self).map_err(|e| {
 			Error::WalletData(format!("Error serializing wallet data."))
 		})?;
 		data_file.write_all(res_json.as_slice()).map_err(|e| {
@@ -256,19 +271,20 @@ impl WalletData {
 		})
 	}
 
-	/// Append a new output information to the wallet data.
-	pub fn append_output(&mut self, out: OutputData) {
-		self.outputs.push(out);
+	/// Append a new output data to the wallet data.
+	/// TODO - we should check for overwriting here - only really valid for
+	/// unconfirmed coinbase
+	pub fn add_output(&mut self, out: OutputData) {
+		self.outputs.insert(out.identifier.to_hex(), out.clone());
 	}
 
+	/// Lock an output data.
+	/// TODO - we should track identifier on these outputs (not just n_child)
 	pub fn lock_output(&mut self, out: &OutputData) {
-		if let Some(out_to_lock) =
-			self.outputs.iter_mut().find(|out_to_lock| {
-				out_to_lock.n_child == out.n_child && out_to_lock.fingerprint == out.fingerprint &&
-					out_to_lock.value == out.value
-			})
-		{
-			out_to_lock.lock();
+		if let Some(out_to_lock) = self.outputs.get_mut(&out.identifier.to_hex()) {
+			if out_to_lock.value == out.value {
+				out_to_lock.lock()
+			}
 		}
 	}
 
@@ -277,14 +293,14 @@ impl WalletData {
 	pub fn select(
 		&self,
 		fingerprint: keychain::Fingerprint,
-		amount: u64
+		amount: u64,
 	) -> (Vec<OutputData>, i64) {
 		let mut to_spend = vec![];
 		let mut input_total = 0;
 
 		// TODO very naive impl for now - definitely better coin selection
 		// algos available
-		for out in &self.outputs {
+		for out in self.outputs.values() {
 			if out.status == OutputStatus::Unspent && out.fingerprint == fingerprint {
 				to_spend.push(out.clone());
 				input_total += out.value;
@@ -300,7 +316,7 @@ impl WalletData {
 	/// Next child index when we want to create a new output.
 	pub fn next_child(&self, fingerprint: keychain::Fingerprint) -> u32 {
 		let mut max_n = 0;
-		for out in &self.outputs {
+		for out in self.outputs.values() {
 			if max_n < out.n_child && out.fingerprint == fingerprint {
 				max_n = out.n_child;
 			}