fail deserialization if inputs or outputs are not sorted for txs (#173)

* fail deserialization if input or outputs are not sorted for txs, fail deserialization if inputs or outputs or kernels are not sorted for blocks, make sure we sort them before serialization
* extract consensus rule into verify_sort_order on VerifySortOrder trait
* rework the sort and write into WriteableSorted trait
* fix wallet sending bug (fees related) so we can test
* refactored read_and_verify_ordered out into ser, used in transaction and block

chain/tests/mine_simple_chain.rs

@@ -59,7 +59,6 @@ fn setup(dir_name: &str) -> Chain {
 #[test]
 fn mine_empty_chain() {
 	let chain = setup(".grin");
-
 	let keychain = Keychain::from_random_seed().unwrap();
 
 	// mine and add a few blocks

core/src/consensus.rs

@@ -21,6 +21,7 @@
 
 use std::fmt;
 
+use ser;
 use core::target::Difficulty;
 
 /// The block subsidy amount
@@ -76,8 +77,7 @@ pub const MAX_BLOCK_WEIGHT: usize = 80_000;
 
 /// Whether a block exceeds the maximum acceptable weight
 pub fn exceeds_weight(input_len: usize, output_len: usize, kernel_len: usize) -> bool {
-	input_len * BLOCK_INPUT_WEIGHT +
-		output_len * BLOCK_OUTPUT_WEIGHT +
+	input_len * BLOCK_INPUT_WEIGHT + output_len * BLOCK_OUTPUT_WEIGHT +
 		kernel_len * BLOCK_KERNEL_WEIGHT > MAX_BLOCK_WEIGHT
 }
 
@@ -206,6 +206,12 @@ where
 	)
 }
 
+/// Consensus rule that collections of items are sorted lexicographically over the wire.
+pub trait VerifySortOrder<T> {
+	/// Verify a collection of items is sorted as required.
+	fn verify_sort_order(&self) -> Result<(), ser::Error>;
+}
+
 #[cfg(test)]
 use std;
 

core/src/core/block.rs

@@ -23,7 +23,7 @@ use core::{Input, Output, Proof, TxKernel, Transaction, COINBASE_KERNEL, COINBAS
 use consensus::{MINIMUM_DIFFICULTY, REWARD, reward, exceeds_weight};
 use core::hash::{Hash, Hashed, ZERO_HASH};
 use core::target::Difficulty;
-use ser::{self, Readable, Reader, Writeable, Writer};
+use ser::{self, Readable, Reader, Writeable, Writer, WriteableSorted, read_and_verify_sorted};
 use global;
 use keychain;
 
@@ -189,15 +189,14 @@ impl Writeable for Block {
 				[write_u64, self.kernels.len() as u64]
 			);
 
-			for inp in &self.inputs {
-				try!(inp.write(writer));
-			}
-			for out in &self.outputs {
-				try!(out.write(writer));
-			}
-			for proof in &self.kernels {
-				try!(proof.write(writer));
-			}
+			let mut inputs = self.inputs.clone();
+			let mut outputs = self.outputs.clone();
+			let mut kernels = self.kernels.clone();
+
+			// Consensus rule that everything is sorted in lexicographical order on the wire.
+			try!(inputs.write_sorted(writer));
+			try!(outputs.write_sorted(writer));
+			try!(kernels.write_sorted(writer));
 		}
 		Ok(())
 	}
@@ -209,12 +208,12 @@ impl Readable for Block {
 	fn read(reader: &mut Reader) -> Result<Block, ser::Error> {
 		let header = try!(BlockHeader::read(reader));
 
-		let (input_len, output_len, proof_len) =
+		let (input_len, output_len, kernel_len) =
 			ser_multiread!(reader, read_u64, read_u64, read_u64);
 
-		let inputs = try!((0..input_len).map(|_| Input::read(reader)).collect());
-		let outputs = try!((0..output_len).map(|_| Output::read(reader)).collect());
-		let kernels = try!((0..proof_len).map(|_| TxKernel::read(reader)).collect());
+		let inputs = read_and_verify_sorted(reader, input_len)?;
+		let outputs = read_and_verify_sorted(reader, output_len)?;
+		let kernels = read_and_verify_sorted(reader, kernel_len)?;
 
 		Ok(Block {
 			header: header,
@@ -292,7 +291,7 @@ impl Block {
 		// build vectors with all inputs and all outputs, ordering them by hash
 		// needs to be a fold so we don't end up with a vector of vectors and we
 		// want to fully own the refs (not just a pointer like flat_map).
-		let mut inputs = txs.iter().fold(vec![], |mut acc, ref tx| {
+		let inputs = txs.iter().fold(vec![], |mut acc, ref tx| {
 			let mut inputs = tx.inputs.clone();
 			acc.append(&mut inputs);
 			acc
@@ -304,9 +303,6 @@ impl Block {
 		});
 		outputs.push(reward_out);
 
-		inputs.sort_by_key(|inp| inp.hash());
-		outputs.sort_by_key(|out| out.hash());
-
 		// calculate the overall Merkle tree and fees
 
 		Ok(
@@ -401,9 +397,6 @@ impl Block {
 		let mut all_kernels = self.kernels.clone();
 		all_kernels.append(&mut other.kernels.clone());
 
-		all_inputs.sort_by_key(|inp| inp.hash());
-		all_outputs.sort_by_key(|out| out.hash());
-
 		Block {
 			// compact will fix the merkle tree
 			header: BlockHeader {

core/src/core/hash.rs

@@ -23,6 +23,7 @@ use std::convert::AsRef;
 
 use blake2::blake2b::Blake2b;
 
+use consensus::VerifySortOrder;
 use ser::{self, Reader, Readable, Writer, Writeable, Error, AsFixedBytes};
 
 /// A hash consisting of all zeroes, used as a sentinel. No known preimage.
@@ -191,3 +192,18 @@ impl Hashed for [u8; 0] {
 		Hash(ret)
 	}
 }
+
+impl<T: Hashed> VerifySortOrder<T> for Vec<T> {
+	fn verify_sort_order(&self) -> Result<(), ser::Error> {
+		match self
+			.iter()
+			.map(|item| item.hash())
+			.collect::<Vec<_>>()
+			.windows(2)
+			.any(|pair| pair[0] > pair[1])
+		{
+			true => Err(ser::Error::BadlySorted),
+			false => Ok(()),
+		}
+	}
+}

core/src/core/transaction.rs

@@ -22,7 +22,8 @@ use std::ops;
 use core::Committed;
 use core::pmmr::Summable;
 use keychain::{Identifier, Keychain};
-use ser::{self, Reader, Writer, Readable, Writeable};
+use ser::{self, Reader, Writer, Readable, Writeable, WriteableSorted, read_and_verify_sorted};
+
 
 bitflags! {
 	/// Options for a kernel's structure or use
@@ -153,16 +154,19 @@ impl Writeable for Transaction {
 			[write_u64, self.inputs.len() as u64],
 			[write_u64, self.outputs.len() as u64]
 		);
-		for inp in &self.inputs {
-			try!(inp.write(writer));
-		}
-		for out in &self.outputs {
-			try!(out.write(writer));
-		}
+
+		// Consensus rule that everything is sorted in lexicographical order on the wire.
+		let mut inputs = self.inputs.clone();
+		let mut outputs = self.outputs.clone();
+
+		try!(inputs.write_sorted(writer));
+		try!(outputs.write_sorted(writer));
+
 		Ok(())
 	}
 }
 
+
 /// Implementation of Readable for a transaction, defines how to read a full
 /// transaction from a binary stream.
 impl Readable for Transaction {
@@ -170,8 +174,8 @@ impl Readable for Transaction {
 		let (fee, lock_height, excess_sig, input_len, output_len) =
 			ser_multiread!(reader, read_u64, read_u64, read_vec, read_u64, read_u64);
 
-		let inputs = try!((0..input_len).map(|_| Input::read(reader)).collect());
-		let outputs = try!((0..output_len).map(|_| Output::read(reader)).collect());
+		let inputs = read_and_verify_sorted(reader, input_len)?;
+		let outputs = read_and_verify_sorted(reader, output_len)?;
 
 		Ok(Transaction {
 			fee: fee,
@@ -184,7 +188,6 @@ impl Readable for Transaction {
 	}
 }
 
-
 impl Committed for Transaction {
 	fn inputs_committed(&self) -> &Vec<Input> {
 		&self.inputs

core/src/ser.rs

@@ -23,6 +23,8 @@ use std::{error, fmt, cmp};
 use std::io::{self, Write, Read};
 use byteorder::{ByteOrder, ReadBytesExt, BigEndian};
 use keychain::{Identifier, IDENTIFIER_SIZE};
+use core::hash::Hashed;
+use consensus::VerifySortOrder;
 use secp::pedersen::Commitment;
 use secp::pedersen::RangeProof;
 use secp::constants::{MAX_PROOF_SIZE, PEDERSEN_COMMITMENT_SIZE};
@@ -43,6 +45,8 @@ pub enum Error {
 	CorruptedData,
 	/// When asked to read too much data
 	TooLargeReadErr,
+	/// Something was not sorted when consensus rules requires it to be sorted
+	BadlySorted,
 }
 
 impl From<io::Error> for Error {
@@ -61,6 +65,7 @@ impl fmt::Display for Error {
 			} => write!(f, "expected {:?}, got {:?}", e, r),
 			Error::CorruptedData => f.write_str("corrupted data"),
 			Error::TooLargeReadErr => f.write_str("too large read"),
+			Error::BadlySorted => f.write_str("badly sorted data"),
 		}
 	}
 }
@@ -82,6 +87,7 @@ impl error::Error for Error {
 			} => "unexpected data",
 			Error::CorruptedData => "corrupted data",
 			Error::TooLargeReadErr => "too large read",
+			Error::BadlySorted => "badly sorted data",
 		}
 	}
 }
@@ -180,6 +186,25 @@ pub trait Writeable {
 	fn write<W: Writer>(&self, writer: &mut W) -> Result<(), Error>;
 }
 
+/// Trait to allow a collection of Writeables to be written in lexicographical sort order.
+pub trait WriteableSorted {
+	/// Write the data but sort it first.
+	fn write_sorted<W: Writer>(&mut self, writer: &mut W) -> Result<(), Error>;
+}
+
+/// Reads a collection of serialized items into a Vec
+/// and verifies they are lexicographically ordered.
+///
+/// A consensus rule requires everything is sorted lexicographically to avoid
+/// leaking any information through specific ordering of items.
+pub fn read_and_verify_sorted<T>(reader: &mut Reader, count: u64) -> Result<Vec<T>, Error>
+	where T: Readable + Hashed
+{
+	let result: Vec<T> = try!((0..count).map(|_| T::read(reader)).collect());
+	result.verify_sort_order()?;
+	Ok(result)
+}
+
 /// Trait that every type that can be deserialized from binary must implement.
 /// Reads directly to a Reader, a utility type thinly wrapping an
 /// underlying Read implementation.
@@ -390,6 +415,19 @@ where
 	}
 }
 
+impl<T> WriteableSorted for Vec<T>
+where
+	T: Writeable + Hashed,
+{
+	fn write_sorted<W: Writer>(&mut self, writer: &mut W) -> Result<(), Error> {
+		self.sort_by_key(|elmt| elmt.hash());
+		for elmt in self {
+			elmt.write(writer)?;
+		}
+		Ok(())
+	}
+}
+
 impl<'a, A: Writeable> Writeable for &'a A {
 	fn write<W: Writer>(&self, writer: &mut W) -> Result<(), Error> {
 		Writeable::write(*self, writer)