rework WalletData and OutputData serialization so we serialize block hashes cleanly in wallet.dat (#625)

keychain/src/extkey.rs

@@ -74,7 +74,7 @@ impl error::Error for Error {
 	}
 }
 
-#[derive(Clone, PartialEq, Eq, Hash)]
+#[derive(Clone, PartialEq, Eq, Ord, Hash, PartialOrd)]
 pub struct Identifier([u8; IDENTIFIER_SIZE]);
 
 impl ser::Serialize for Identifier {

wallet/src/checker.rs

@@ -64,7 +64,7 @@ fn refresh_missing_block_hashes(config: &WalletConfig, keychain: &Keychain) -> R
 			.values()
 			.filter(|x| {
 				x.root_key_id == keychain.root_key_id() &&
-				x.block_hash == Hash::zero() &&
+				x.block.hash() == Hash::zero() &&
 				x.status == OutputStatus::Unspent
 			})
 		{
@@ -139,7 +139,7 @@ fn refresh_missing_block_hashes(config: &WalletConfig, keychain: &Keychain) -> R
 			if let Entry::Occupied(mut output) = wallet_data.outputs.entry(id.to_hex()) {
 				if let Some(b) = api_blocks.get(&commit) {
 					let output = output.get_mut();
-					output.block_hash = Hash::from_hex(&b.hash).unwrap();
+					output.block = BlockIdentifier::from_str(&b.hash).unwrap();
 					output.height = b.height;
 				}
 			}

wallet/src/receiver.rs

@@ -25,7 +25,6 @@ use serde_json;
 use api;
 use core::consensus::reward;
 use core::core::{build, Block, Output, Transaction, TxKernel, amount_to_hr_string};
-use core::core::hash::Hash;
 use core::{global, ser};
 use keychain::{Identifier, Keychain};
 use types::*;
@@ -97,7 +96,7 @@ fn handle_sender_initiation(
 			height: 0,
 			lock_height: 0,
 			is_coinbase: false,
-			block_hash: Hash::zero(),
+			block: BlockIdentifier::zero(),
 		});
 
 		key_id
@@ -282,7 +281,7 @@ pub fn receive_coinbase(
 			height: height,
 			lock_height: lock_height,
 			is_coinbase: true,
-			block_hash: Hash::zero(),
+			block: BlockIdentifier::zero(),
 		});
 
 		(key_id, derivation)
@@ -363,7 +362,7 @@ fn build_final_transaction(
 			height: 0,
 			lock_height: 0,
 			is_coinbase: false,
-			block_hash: Hash::zero(),
+			block: BlockIdentifier::zero(),
 		});
 
 		(key_id, derivation)

wallet/src/restore.rs

@@ -18,9 +18,8 @@ use util::secp::pedersen;
 use api;
 use core::global;
 use core::core::{Output, SwitchCommitHash};
-use core::core::hash::Hash;
 use core::core::transaction::{COINBASE_OUTPUT, DEFAULT_OUTPUT};
-use types::{WalletConfig, WalletData, OutputData, OutputStatus, Error};
+use types::{BlockIdentifier, WalletConfig, WalletData, OutputData, OutputStatus, Error};
 use byteorder::{BigEndian, ByteOrder};
 
 
@@ -311,7 +310,7 @@ pub fn restore(
 							height: output.4,
 							lock_height: output.5,
 							is_coinbase: output.6,
-							block_hash: Hash::zero(),
+							block: BlockIdentifier::zero(),
 						});
 					};
 				}

wallet/src/sender.rs

@@ -16,7 +16,6 @@ use api;
 use client;
 use checker;
 use core::core::{build, Transaction, amount_to_hr_string};
-use core::core::hash::Hash;
 use core::ser;
 use keychain::{BlindingFactor, Identifier, Keychain};
 use receiver::TxWrapper;
@@ -267,9 +266,9 @@ fn inputs_and_change(
 	for coin in coins {
 		let key_id = keychain.derive_key_id(coin.n_child)?;
 		if coin.is_coinbase {
-			parts.push(build::coinbase_input(coin.value, coin.block_hash, key_id));
+			parts.push(build::coinbase_input(coin.value, coin.block.hash(), key_id));
 		} else {
-			parts.push(build::input(coin.value, coin.block_hash, key_id));
+			parts.push(build::input(coin.value, coin.block.hash(), key_id));
 		}
 	}
 
@@ -288,7 +287,7 @@ fn inputs_and_change(
 			height: 0,
 			lock_height: 0,
 			is_coinbase: false,
-			block_hash: Hash::zero(),
+			block: BlockIdentifier::zero(),
 		});
 
 		change_key

wallet/src/types.rs

@@ -24,6 +24,7 @@ use std::collections::HashMap;
 use std::cmp::min;
 
 use hyper;
+use serde;
 use serde_json;
 use tokio_core::reactor;
 use tokio_retry::Retry;
@@ -205,7 +206,7 @@ impl WalletConfig {
 /// unconfirmed, spent, unspent, or locked (when it's been used to generate
 /// a transaction but we don't have confirmation that the transaction was
 /// broadcasted or mined).
-#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq)]
+#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, Ord, PartialOrd)]
 pub enum OutputStatus {
 	Unconfirmed,
 	Unspent,
@@ -224,10 +225,64 @@ impl fmt::Display for OutputStatus {
 	}
 }
 
+#[derive(Debug, Clone, PartialEq, PartialOrd, Eq, Ord)]
+pub struct BlockIdentifier(Hash);
+
+impl BlockIdentifier {
+	pub fn hash(&self) -> Hash {
+		self.0
+	}
+
+	pub fn from_str(hex: &str) -> Result<BlockIdentifier, Error> {
+		let hash = Hash::from_hex(hex)?;
+		Ok(BlockIdentifier(hash))
+	}
+
+	pub fn zero() -> BlockIdentifier {
+		BlockIdentifier(Hash::zero())
+	}
+}
+
+impl serde::ser::Serialize for BlockIdentifier {
+	fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
+	where
+		S: serde::ser::Serializer,
+	{
+		serializer.serialize_str(&self.0.to_hex())
+	}
+}
+
+impl<'de> serde::de::Deserialize<'de> for BlockIdentifier {
+	fn deserialize<D>(deserializer: D) -> Result<BlockIdentifier, D::Error>
+	where
+		D: serde::de::Deserializer<'de>,
+	{
+		deserializer.deserialize_str(BlockIdentifierVisitor)
+	}
+}
+
+struct BlockIdentifierVisitor;
+
+impl<'de> serde::de::Visitor<'de> for BlockIdentifierVisitor {
+	type Value = BlockIdentifier;
+
+	fn expecting(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
+		formatter.write_str("a block hash")
+	}
+
+	fn visit_str<E>(self, s: &str) -> Result<Self::Value, E>
+	where
+		E: serde::de::Error,
+	{
+		let block_hash = Hash::from_hex(s).unwrap();
+		Ok(BlockIdentifier(block_hash))
+	}
+}
+
 /// Information about an output that's being tracked by the wallet. Must be
 /// enough to reconstruct the commitment associated with the ouput when the
 /// root private key is known.
-#[derive(Serialize, Deserialize, Debug, Clone)]
+#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, PartialOrd, Eq, Ord)]
 pub struct OutputData {
 	/// Root key_id that the key for this output is derived from
 	pub root_key_id: keychain::Identifier,
@@ -246,7 +301,7 @@ pub struct OutputData {
 	/// Is this a coinbase output? Is it subject to coinbase locktime?
 	pub is_coinbase: bool,
 	/// Hash of the block this output originated from.
-	pub block_hash: Hash,
+	pub block: BlockIdentifier,
 }
 
 impl OutputData {
@@ -381,12 +436,6 @@ impl WalletSeed {
 
 /// Wallet information tracking all our outputs. Based on HD derivation and
 /// avoids storing any key data, only storing output amounts and child index.
-/// This data structure is directly based on the JSON representation stored
-/// on disk, so selection algorithms are fairly primitive and non optimized.
-///
-/// TODO optimization so everything isn't O(n) or even O(n^2)
-/// TODO account for fees
-/// TODO write locks so files don't get overwritten
 #[derive(Serialize, Deserialize, Debug, Clone)]
 pub struct WalletData {
 	pub outputs: HashMap<String, OutputData>,
@@ -480,8 +529,8 @@ impl WalletData {
 		}
 	}
 
-	/// Read the wallet data from disk.
+	/// Read output_data vec from disk.
-	fn read(data_file_path: &str) -> Result<WalletData, Error> {
+	fn read_outputs(data_file_path: &str) -> Result<Vec<OutputData>, Error> {
 		let data_file = File::open(data_file_path).map_err(|e| {
 			Error::WalletData(format!("Could not open {}: {}", data_file_path, e))
 		})?;
@@ -490,12 +539,26 @@ impl WalletData {
 		})
 	}
 
+	/// Populate wallet_data with output_data from disk.
+	fn read(data_file_path: &str) -> Result<WalletData, Error> {
+		let outputs = WalletData::read_outputs(data_file_path)?;
+		let mut wallet_data = WalletData {
+			outputs: HashMap::new(),
+		};
+		for out in outputs {
+			wallet_data.add_output(out);
+		}
+		Ok(wallet_data)
+	}
+
 	/// Write the wallet data to disk.
 	fn write(&self, data_file_path: &str) -> Result<(), Error> {
 		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(|e| {
+		let mut outputs = self.outputs.values().collect::<Vec<_>>();
+		outputs.sort();
+		let res_json = serde_json::to_vec_pretty(&outputs).map_err(|e| {
 			Error::WalletData(format!("Error serializing wallet data: {}", e))
 		})?;
 		data_file.write_all(res_json.as_slice()).map_err(|e| {