applying suggestions from review and adding comments

db/src/transaction_meta_provider.rs

@@ -2,11 +2,17 @@ use primitives::hash::H256;
 use chain::OutPoint;
 use transaction_meta::TransactionMeta;
 
+/// Transaction output observers track if output has been spent
 pub trait TransactionOutputObserver: Send + Sync {
+	/// Returns None if we have no information about previous output
+	/// Returns Some(false) if we know that output hasn't been spent
+	/// Returns Some(true) if we know that output has been spent
 	fn is_spent(&self, prevout: &OutPoint) -> Option<bool>;
 }
 
+/// Transaction meta provider stores transaction meta information
 pub trait TransactionMetaProvider: Send + Sync {
-	/// get transaction metadata
+	/// Returns None if transactin with given hash does not exist
+	/// Otherwise returns transaction meta object
 	fn transaction_meta(&self, hash: &H256) -> Option<TransactionMeta>;
 }

db/src/transaction_provider.rs

@@ -15,7 +15,7 @@ pub trait TransactionProvider {
 	fn transaction(&self, hash: &H256) -> Option<chain::Transaction>;
 }
 
-/// During transaction the only part of old transaction that we need is `TransactionOutput`.
+/// During transaction verifiction the only part of old transaction that we need is `TransactionOutput`.
 /// Structures like `IndexedBlock` or `MemoryPool` already have it in memory, so it would be
 /// a shame to clone the whole transaction just to get single output.
 pub trait PreviousTransactionOutputProvider: Send + Sync {

miner/src/block_assembler.rs

@@ -185,17 +185,9 @@ impl<'a, T> Iterator for FittingTransactionsIterator<'a, T> where T: Iterator<It
 				}
 			};
 
-			let bip16_active = true;
 			let transaction_size = entry.size as u32;
-			// we may have ignored previous transaction, cause it wasn't fitting the block
-			// if we did that, than transaction_sigops returns None, and we also need
-			// to ommit current transaction
-			let sigops_count = match transaction_sigops(&entry.transaction, self, bip16_active) {
-				Some(count) => count as u32,
-				None => {
-					continue;
-				},
-			};
+			let bip16_active = true;
+			let sigops_count = transaction_sigops(&entry.transaction, self, bip16_active) as u32;
 
 			let size_step = self.block_size.decide(transaction_size);
 			let sigops_step = self.sigops.decide(sigops_count);

verification/src/accept_block.rs

@@ -85,9 +85,6 @@ impl<'a> BlockRule for BlockSigops<'a> {
 		let bip16_active = self.block.header.raw.time >= self.consensus_params.bip16_time;
 		let sigops = self.block.transactions.iter()
 			.map(|tx| transaction_sigops(&tx.raw, &store, bip16_active))
-			.collect::<Option<Vec<usize>>>()
-			.ok_or_else(|| Error::MaximumSigops)?
-			.into_iter()
 			.sum::<usize>();
 
 		if sigops > self.max_sigops {

verification/src/accept_transaction.rs

@@ -294,10 +294,8 @@ impl<'a> TransactionSigops<'a> {
 impl<'a> TransactionRule for TransactionSigops<'a> {
 	fn check(&self) -> Result<(), TransactionError> {
 		let bip16_active = self.time >= self.consensus_params.bip16_time;
-		let error = transaction_sigops(&self.transaction.raw, &self.store, bip16_active)
-			.map(|sigops| sigops > self.max_sigops)
-			.unwrap_or(true);
-		if error {
+		let sigops = transaction_sigops(&self.transaction.raw, &self.store, bip16_active);
+		if sigops > self.max_sigops {
 			Err(TransactionError::MaxSigops)
 		} else {
 			Ok(())

verification/src/lib.rs

@@ -1,5 +1,14 @@
 //! Bitcoin consensus verification
 //!
+//!	Full block verification consists of two phases:
+//!	- pre-verification
+//! - full-verification
+//!
+//! In this library, pre-verification is done by `VerifyXXX` structures
+//! Full-verification is done by `AcceptXXX` structures
+//!
+//! Use cases:
+//!
 //! --> A. on_new_block:
 //!
 //! A.1 VerifyHeader

verification/src/sigops.rs

@@ -2,26 +2,22 @@ use chain::Transaction;
 use db::PreviousTransactionOutputProvider;
 use script::Script;
 
+/// Counts signature operations in given transaction
+/// bip16_active flag indicates if we should also count signature operations
+/// in previous transactions. If one of the previous transaction outputs is
+/// missing, we simply ignore that fact and just carry on counting
 pub fn transaction_sigops(
 	transaction: &Transaction,
 	store: &PreviousTransactionOutputProvider,
 	bip16_active: bool
-) -> Option<usize> {
-	if bip16_active {
-		transaction_sigops_raw(transaction, Some(store))
-	} else {
-		transaction_sigops_raw(transaction, None)
-	}
-}
-
-pub fn transaction_sigops_raw(transaction: &Transaction, store: Option<&PreviousTransactionOutputProvider>) -> Option<usize> {
+) -> usize {
 	let output_sigops: usize = transaction.outputs.iter().map(|output| {
 		let output_script: Script = output.script_pubkey.clone().into();
 		output_script.sigops_count(false)
 	}).sum();
 
 	if transaction.is_coinbase() {
-		return Some(output_sigops);
+		return output_sigops;
 	}
 
 	let mut input_sigops = 0usize;
@@ -30,15 +26,15 @@ pub fn transaction_sigops_raw(transaction: &Transaction, store: Option<&Previous
 	for input in &transaction.inputs {
 		let input_script: Script = input.script_sig.clone().into();
 		input_sigops += input_script.sigops_count(false);
-		if let Some(store) = store {
+		if bip16_active {
 			let previous_output = match store.previous_transaction_output(&input.previous_output) {
 				Some(output) => output,
-				None => return None,
+				None => continue,
 			};
 			let prevout_script: Script = previous_output.script_pubkey.into();
 			bip16_sigops += input_script.pay_to_script_hash_sigops(&prevout_script);
 		}
 	}
 
-	Some(input_sigops + output_sigops + bip16_sigops)
+	input_sigops + output_sigops + bip16_sigops
 }

verification/src/timestamp.rs

@@ -3,6 +3,9 @@ use chain::BlockHeader;
 use db::BlockHeaderProvider;
 use network::Magic;
 
+/// Returns median timestamp, of given header ancestors.
+/// The header should be later expected to have higher timestamp
+/// than this median timestamp
 pub fn median_timestamp(header: &BlockHeader, store: &BlockHeaderProvider, network: Magic) -> u32 {
 	// TODO: timestamp validation on testnet is broken
 	if network == Magic::Testnet {

verification/src/verify_block.rs

@@ -1,6 +1,7 @@
 use std::collections::HashSet;
 use db::IndexedBlock;
-use sigops::transaction_sigops_raw;
+use sigops::transaction_sigops;
+use duplex_store::NoopStore;
 use error::{Error, TransactionError};
 use constants::{MAX_BLOCK_SIZE, MAX_BLOCK_SIGOPS};
 
@@ -178,7 +179,7 @@ impl<'a> BlockRule for BlockSigops<'a> {
 	fn check(&self) -> Result<(), Error> {
 		// We cannot know if bip16 is enabled at this point so we disable it.
 		let sigops = self.block.transactions.iter()
-			.map(|tx| transaction_sigops_raw(&tx.raw, None).expect("bip16 is disabled"))
+			.map(|tx| transaction_sigops(&tx.raw, &NoopStore, false))
 			.sum::<usize>();
 
 		if sigops > self.max_sigops {

verification/src/verify_transaction.rs

@@ -1,7 +1,8 @@
 use std::ops;
 use serialization::Serializable;
 use db::IndexedTransaction;
-use sigops::transaction_sigops_raw;
+use duplex_store::NoopStore;
+use sigops::transaction_sigops;
 use error::TransactionError;
 use constants::{MAX_BLOCK_SIZE, MAX_BLOCK_SIGOPS, MIN_COINBASE_SIZE, MAX_COINBASE_SIZE};
 
@@ -195,7 +196,7 @@ impl<'a> TransactionSigops<'a> {
 
 impl<'a> TransactionRule for TransactionSigops<'a> {
 	fn check(&self) -> Result<(), TransactionError> {
-		let sigops = transaction_sigops_raw(&self.transaction.raw, None).expect("bip16 is disabled");
+		let sigops = transaction_sigops(&self.transaction.raw, &NoopStore, false);
 		if sigops > self.max_sigops {
 			Err(TransactionError::MaxSigops)
 		} else {