iterate once over txs in a block (#233)

Split out verify_sig and build_kernel to make kernel explicit.

core/src/core/block.rs

@@ -25,6 +25,7 @@ use core::{Input, Output, Proof, SwitchCommitHash, Transaction, TxKernel, COINBA
 use consensus::{exceeds_weight, reward, MINIMUM_DIFFICULTY, REWARD};
 use core::hash::{Hash, Hashed, ZERO_HASH};
 use core::target::Difficulty;
+use core::transaction;
 use ser::{self, read_and_verify_sorted, Readable, Reader, Writeable, WriteableSorted, Writer};
 use util::LOGGER;
 use global;
@@ -47,8 +48,18 @@ pub enum Error {
 		/// The lock_height causing this validation error
 		lock_height: u64,
 	},
+	/// Underlying tx related error
+	Transaction(transaction::Error),
 	/// Underlying Secp256k1 error (signature validation or invalid public key typically)
 	Secp(secp::Error),
+	/// Underlying keychain related error
+	Keychain(keychain::Error),
+}
+
+impl From<transaction::Error> for Error {
+	fn from(e: transaction::Error) -> Error {
+		Error::Transaction(e)
+	}
 }
 
 impl From<secp::Error> for Error {
@@ -57,6 +68,12 @@ impl From<secp::Error> for Error {
 	}
 }
 
+impl From<keychain::Error> for Error {
+	fn from(e: keychain::Error) -> Error {
+		Error::Keychain(e)
+	}
+}
+
 /// Block header, fairly standard compared to other blockchains.
 #[derive(Clone, Debug, PartialEq)]
 pub struct BlockHeader {
@@ -266,7 +283,7 @@ impl Block {
 		txs: Vec<&Transaction>,
 		keychain: &keychain::Keychain,
 		key_id: &keychain::Identifier,
-	) -> Result<Block, keychain::Error> {
+	) -> Result<Block, Error> {
 		let fees = txs.iter().map(|tx| tx.fee).sum();
 		let (reward_out, reward_proof) = Block::reward_output(keychain, key_id, fees)?;
 		let block = Block::with_reward(prev, txs, reward_out, reward_proof)?;
@@ -281,30 +298,37 @@ impl Block {
 		txs: Vec<&Transaction>,
 		reward_out: Output,
 		reward_kern: TxKernel,
-	) -> Result<Block, secp::Error> {
-		// note: the following reads easily but may not be the most efficient due to
-  // repeated iterations, revisit if a problem
+	) -> Result<Block, Error> {
 		let secp = Secp256k1::with_caps(secp::ContextFlag::Commit);
 
-		// validate each transaction and gather their kernels
-		let mut kernels = try_map_vec!(txs, |tx| tx.verify_sig(&secp));
-		kernels.push(reward_kern);
+		let mut kernels = vec![];
+		let mut inputs = vec![];
+		let mut outputs = vec![];
 
-		// 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 mut inputs = tx.inputs.clone();
-			acc.append(&mut inputs);
-			acc
-		});
-		let mut outputs = txs.iter().fold(vec![], |mut acc, ref tx| {
-			let mut outputs = tx.outputs.clone();
-			acc.append(&mut outputs);
-			acc
-		});
+		// iterate over the all the txs
+		// build the kernel for each
+		// and collect all the kernels, inputs and outputs
+		// to build the block (which we can sort of think of as one big tx?)
+		for tx in txs {
+			// validate each transaction and gather their kernels
+			let excess = tx.validate(&secp)?;
+			let kernel = tx.build_kernel(excess);
+			kernels.push(kernel);
+
+			for input in tx.inputs.clone() {
+				inputs.push(input);
+			}
+
+			for output in tx.outputs.clone() {
+				outputs.push(output);
+			}
+		}
+
+		// also include the reward kernel and output
+		kernels.push(reward_kern);
 		outputs.push(reward_out);
 
+		// now sort everything to the block is built deterministically
 		inputs.sort();
 		outputs.sort();
 		kernels.sort();

core/src/core/transaction.rs

@@ -26,7 +26,6 @@ use core::hash::Hashed;
 use core::pmmr::Summable;
 use keychain::{Identifier, Keychain};
 use ser::{self, read_and_verify_sorted, Readable, Reader, Writeable, WriteableSorted, Writer};
-use util::LOGGER;
 
 /// The size to use for the stored blake2 hash of a switch_commitment
 pub const SWITCH_COMMIT_HASH_SIZE: usize = 20;
@@ -65,7 +64,7 @@ macro_rules! hashable_ord {
 }
 
 /// Errors thrown by Block validation
-#[derive(Debug, PartialEq)]
+#[derive(Clone, Debug, PartialEq)]
 pub enum Error {
 	/// Transaction fee can't be odd, due to half fee burning
 	OddFee,
@@ -305,49 +304,46 @@ impl Transaction {
 	/// sum to zero as they should in r.G + v.H then only k.G the excess
 	/// of the sum of r.G should be left. And r.G is the definition of a
 	/// public key generated using r as a private key.
-	pub fn verify_sig(&self, secp: &Secp256k1) -> Result<TxKernel, secp::Error> {
+	pub fn verify_sig(&self, secp: &Secp256k1) -> Result<Commitment, secp::Error> {
 		let rsum = self.sum_commitments(secp)?;
 
 		let msg = Message::from_slice(&kernel_sig_msg(self.fee, self.lock_height))?;
 		let sig = Signature::from_der(secp, &self.excess_sig)?;
 
 		// pretend the sum is a public key (which it is, being of the form r.G) and
-  // verify the transaction sig with it
-  //
-  // we originally converted the commitment to a key_id here (commitment to zero)
-  // and then passed the key_id to secp.verify()
-  // the secp api no longer allows us to do this so we have wrapped the complexity
-  // of generating a public key from a commitment behind verify_from_commit
+		// verify the transaction sig with it
+		//
+		// we originally converted the commitment to a key_id here (commitment to zero)
+		// and then passed the key_id to secp.verify()
+		// the secp api no longer allows us to do this so we have wrapped the complexity
+		// of generating a public key from a commitment behind verify_from_commit
 		secp.verify_from_commit(&msg, &sig, &rsum)?;
 
-		let kernel = TxKernel {
+		Ok(rsum)
+	}
+
+	pub fn build_kernel(&self, excess: Commitment) -> TxKernel {
+		TxKernel {
 			features: DEFAULT_KERNEL,
-			excess: rsum,
+			excess: excess,
 			excess_sig: self.excess_sig.clone(),
 			fee: self.fee,
 			lock_height: self.lock_height,
-		};
-		debug!(
-			LOGGER,
-			"tx verify_sig: fee - {}, lock_height - {}",
-			kernel.fee,
-			kernel.lock_height
-		);
-
-		Ok(kernel)
+		}
 	}
 
 	/// Validates all relevant parts of a fully built transaction. Checks the
 	/// excess value against the signature as well as range proofs for each
 	/// output.
-	pub fn validate(&self, secp: &Secp256k1) -> Result<TxKernel, Error> {
+	pub fn validate(&self, secp: &Secp256k1) -> Result<Commitment, Error> {
 		if self.fee & 1 != 0 {
 			return Err(Error::OddFee);
 		}
 		for out in &self.outputs {
 			out.verify_proof(secp)?;
 		}
-		self.verify_sig(secp).map_err(&From::from)
+		let excess = self.verify_sig(secp)?;
+		Ok(excess)
 	}
 }