fix the broken comapct block hashing behavior (#738)

made nonce for short_ids explicit to help with this

api/src/types.rs

@@ -20,6 +20,7 @@ use core::core::SwitchCommitHash;
 use chain;
 use p2p;
 use util;
+use util::LOGGER;
 use util::secp::pedersen;
 use util::secp::constants::MAX_PROOF_SIZE;
 use serde;

core/src/core/block.rs

@@ -235,13 +235,13 @@ pub struct CompactBlock {
 /// Implementation of Writeable for a compact block, defines how to write the block to a
 /// binary writer. Differentiates between writing the block for the purpose of
 /// full serialization and the one of just extracting a hash.
-/// Note: compact block hash uses both the header *and* the nonce.
 impl Writeable for CompactBlock {
 	fn write<W: Writer>(&self, writer: &mut W) -> Result<(), ser::Error> {
 		try!(self.header.write(writer));
-		try!(writer.write_u64(self.nonce));
 
 		if writer.serialization_mode() != ser::SerializationMode::Hash {
+			try!(writer.write_u64(self.nonce));
+
 			ser_multiwrite!(
 				writer,
 				[write_u64, self.out_full.len() as u64],
@@ -454,9 +454,6 @@ impl Block {
 		let header = self.header.clone();
 		let nonce = thread_rng().next_u64();
 
-		// concatenate the nonce with our block_header to build the hash
-		let hash = (self, nonce).hash();
-
 		let mut out_full = self.outputs
 			.iter()
 			.filter(|x| x.features.contains(OutputFeatures::COINBASE_OUTPUT))
@@ -470,7 +467,7 @@ impl Block {
 			if k.features.contains(KernelFeatures::COINBASE_KERNEL) {
 				kern_full.push(k.clone());
 			} else {
-				kern_ids.push(k.short_id(&hash));
+				kern_ids.push(k.short_id(&header.hash(), nonce));
 			}
 		}
 
@@ -1166,16 +1163,18 @@ mod test {
 		let cb1 = b.as_compact_block();
 		let cb2 = b.as_compact_block();
 
-		// random nonce included in hash each time we generate a compact_block
-		// so the hash will always be unique (we use this to generate unique short_ids)
-		assert!(cb1.hash() != cb2.hash());
+		// random nonce will not affect the hash of the compact block itself
+		// hash is based on header only
+		assert!(cb1.nonce != cb2.nonce);
+		assert_eq!(b.hash(), cb1.hash());
+		assert_eq!(cb1.hash(), cb2.hash());
 
 		assert!(cb1.kern_ids[0] != cb2.kern_ids[0]);
 
 		// check we can identify the specified kernel from the short_id
-		// in either of the compact_blocks
-		assert_eq!(cb1.kern_ids[0], tx.kernels[0].short_id(&cb1.hash()));
-		assert_eq!(cb2.kern_ids[0], tx.kernels[0].short_id(&cb2.hash()));
+		// correctly in both of the compact_blocks
+		assert_eq!(cb1.kern_ids[0], tx.kernels[0].short_id(&cb1.hash(), cb1.nonce));
+		assert_eq!(cb2.kern_ids[0], tx.kernels[0].short_id(&cb2.hash(), cb2.nonce));
 	}
 
 	#[test]
@@ -1195,7 +1194,7 @@ mod test {
 				.iter()
 				.find(|x| !x.features.contains(KernelFeatures::COINBASE_KERNEL))
 				.unwrap()
-				.short_id(&cb.hash())
+				.short_id(&cb.hash(), cb.nonce)
 		);
 	}
 

core/src/core/id.rs

@@ -32,25 +32,28 @@ pub const SHORT_ID_SIZE: usize = 6;
 pub trait ShortIdentifiable {
 	/// The short_id of a kernel uses a hash built from the block_header *and* a
 	/// connection specific nonce to minimize the effect of collisions.
-	fn short_id(&self, hash: &Hash) -> ShortId;
+	fn short_id(&self, hash: &Hash, nonce: u64) -> ShortId;
 }
 
 impl<H: Hashed> ShortIdentifiable for H {
 	/// Generate a short_id via the following -
 	///
-	///   * extract k0/k1 from block_hash (first two u64 values)
+	///   * extract k0/k1 from block_hash hashed with the nonce (first two u64 values)
 	///   * initialize a siphasher24 with k0/k1
 	///   * self.hash() passing in the siphasher24 instance
 	///   * drop the 2 most significant bytes (to return a 6 byte short_id)
 	///
-	fn short_id(&self, hash: &Hash) -> ShortId {
+	fn short_id(&self, hash: &Hash, nonce: u64) -> ShortId {
+		// take the block hash and the nonce and hash them together
+		let hash_with_nonce = (hash, nonce).hash();
+
 		// we "use" core::hash::Hash in the outer namespace
 		// so doing this here in the fn to minimize collateral damage/confusion
 		use std::hash::Hasher;
 
 		// extract k0/k1 from the block_hash
-		let k0 = LittleEndian::read_u64(&hash.0[0..8]);
-		let k1 = LittleEndian::read_u64(&hash.0[8..16]);
+		let k0 = LittleEndian::read_u64(&hash_with_nonce.0[0..8]);
+		let k1 = LittleEndian::read_u64(&hash_with_nonce.0[8..16]);
 
 		// initialize a siphasher24 with k0/k1
 		let mut sip_hasher = SipHasher24::new_with_keys(k0, k1);
@@ -142,14 +145,14 @@ mod test {
 		}
 
 		let foo = Foo(0);
+
 		let expected_hash = Hash::from_hex(
 			"81e47a19e6b29b0a65b9591762ce5143ed30d0261e5d24a3201752506b20f15c",
 		).unwrap();
 		assert_eq!(foo.hash(), expected_hash);
 
 		let other_hash = Hash::zero();
-		println!("{:?}", foo.short_id(&other_hash));
-		assert_eq!(foo.short_id(&other_hash), ShortId::from_hex("e973960ba690").unwrap());
+		assert_eq!(foo.short_id(&other_hash, foo.0), ShortId::from_hex("4cc808b62476").unwrap());
 
 		let foo = Foo(5);
 		let expected_hash = Hash::from_hex(
@@ -158,8 +161,7 @@ mod test {
 		assert_eq!(foo.hash(), expected_hash);
 
 		let other_hash = Hash::zero();
-		println!("{:?}", foo.short_id(&other_hash));
-		assert_eq!(foo.short_id(&other_hash), ShortId::from_hex("f0c06e838e59").unwrap());
+		assert_eq!(foo.short_id(&other_hash, foo.0), ShortId::from_hex("02955a094534").unwrap());
 
 		let foo = Foo(5);
 		let expected_hash = Hash::from_hex(
@@ -170,7 +172,6 @@ mod test {
 		let other_hash = Hash::from_hex(
 			"81e47a19e6b29b0a65b9591762ce5143ed30d0261e5d24a3201752506b20f15c",
 		).unwrap();
-		println!("{:?}", foo.short_id(&other_hash));
-		assert_eq!(foo.short_id(&other_hash), ShortId::from_hex("95bf0ca12d5b").unwrap());
+		assert_eq!(foo.short_id(&other_hash, foo.0), ShortId::from_hex("3e9cde72a687").unwrap());
 	}
 }

core/src/core/transaction.rs

@@ -717,7 +717,7 @@ impl Writeable for Output {
 	fn write<W: Writer>(&self, writer: &mut W) -> Result<(), ser::Error> {
 		writer.write_u8(self.features.bits())?;
 		writer.write_fixed_bytes(&self.commit)?;
-		// Hash of an output doesn't cover the switch commit, it should 
+		// Hash of an output doesn't cover the switch commit, it should
 		// be wound into the range proof separately
 		if writer.serialization_mode() != ser::SerializationMode::Hash {
 			writer.write_fixed_bytes(&self.switch_commit_hash)?;
@@ -921,13 +921,13 @@ impl Readable for OutputStoreable {
 			features: features,
 		})
 	}
-} 
+}
 
 /// A structure which contains fields that are to be commited to within
-/// an Output's range (bullet) proof. 
+/// an Output's range (bullet) proof.
 #[derive(Serialize, Deserialize, Debug, Clone, PartialEq)]
 pub struct ProofMessageElements {
-	/// The amount, stored to allow for wallet reconstruction as 
+	/// The amount, stored to allow for wallet reconstruction as
 	/// rewinding isn't supported in bulletproofs just yet
 	pub value: u64,
 }
@@ -1120,8 +1120,10 @@ fn input_short_id() {
 		"3a42e66e46dd7633b57d1f921780a1ac715e6b93c19ee52ab714178eb3a9f673",
 	).unwrap();
 
-	let short_id = input.short_id(&block_hash);
-	assert_eq!(short_id, ShortId::from_hex("3e1262905b7a").unwrap());
+	let nonce = 0;
+
+	let short_id = input.short_id(&block_hash, nonce);
+	assert_eq!(short_id, ShortId::from_hex("28fea5a693af").unwrap());
 
 	// now generate the short_id for a *very* similar output (single feature flag different)
 	// and check it generates a different short_id
@@ -1135,7 +1137,7 @@ fn input_short_id() {
 		"3a42e66e46dd7633b57d1f921780a1ac715e6b93c19ee52ab714178eb3a9f673",
 	).unwrap();
 
-	let short_id = input.short_id(&block_hash);
-	assert_eq!(short_id, ShortId::from_hex("90653c1c870a").unwrap());
+	let short_id = input.short_id(&block_hash, nonce);
+	assert_eq!(short_id, ShortId::from_hex("c8af83b54e46").unwrap());
 }
 }

pool/src/pool.rs

@@ -84,7 +84,7 @@ where
 		for tx in self.transactions.values() {
 			for kernel in &tx.kernels {
 				// rehash each kernel to calculate the block specific short_id
-				let short_id = kernel.short_id(&cb.hash());
+				let short_id = kernel.short_id(&cb.hash(), cb.nonce);
 
 				// if any kernel matches then keep the tx for later
 				if cb.kern_ids.contains(&short_id) {