Merge pull request #293 from ethcore/previous_tx_output_provider

PreviousTransactionOutputProvider refactor

db/src/indexed_block.rs

@@ -3,7 +3,7 @@ use chain::{Block, OutPoint, TransactionOutput, merkle_root};
 use serialization::{Serializable, CompactInteger};
 use indexed_header::IndexedBlockHeader;
 use indexed_transaction::IndexedTransaction;
-use PreviousTransactionOutputProvider;
+use {TransactionOutputObserver, PreviousTransactionOutputProvider};
 
 #[derive(Debug, Clone)]
 pub struct IndexedBlock {
@@ -16,9 +16,11 @@ impl PreviousTransactionOutputProvider for IndexedBlock {
 		let txs: &[_] = &self.transactions;
 		txs.previous_transaction_output(prevout)
 	}
+}
 
-	fn is_spent(&self, _prevout: &OutPoint) -> bool {
-		false
+impl TransactionOutputObserver for IndexedBlock {
+	fn is_spent(&self, prevout: &OutPoint) -> Option<bool> {
+		self.previous_transaction_output(prevout).map(|_output| false)
 	}
 }
 

db/src/indexed_transaction.rs

@@ -40,8 +40,4 @@ impl<'a> PreviousTransactionOutputProvider for &'a [IndexedTransaction] {
 			.and_then(|tx| tx.raw.outputs.get(prevout.index as usize))
 			.cloned()
 	}
-
-	fn is_spent(&self, _prevout: &OutPoint) -> bool {
-		unimplemented!();
-	}
 }

db/src/lib.rs

@@ -71,7 +71,7 @@ pub use storage::{Storage, Store, AsSubstore};
 pub use error::{Error, ConsistencyError};
 pub use kvdb::Database;
 pub use transaction_provider::{TransactionProvider, PreviousTransactionOutputProvider};
-pub use transaction_meta_provider::TransactionMetaProvider;
+pub use transaction_meta_provider::{TransactionMetaProvider, TransactionOutputObserver};
 pub use block_stapler::{BlockStapler, BlockInsertedChain};
 pub use block_provider::{BlockProvider, BlockHeaderProvider};
 pub use indexed_block::IndexedBlock;

db/src/storage.rs

@@ -16,7 +16,7 @@ use transaction_meta::TransactionMeta;
 use error::{Error, ConsistencyError, MetaError};
 use update_context::UpdateContext;
 use block_provider::{BlockProvider, BlockHeaderProvider};
-use transaction_provider::TransactionProvider;
+use transaction_provider::{TransactionProvider, PreviousTransactionOutputProvider};
 use transaction_meta_provider::TransactionMetaProvider;
 use block_stapler::{BlockStapler, BlockInsertedChain, Reorganization};
 
@@ -61,10 +61,12 @@ pub trait AsSubstore: BlockProvider + BlockStapler + TransactionProvider + Trans
 
 	fn as_transaction_provider(&self) -> &TransactionProvider;
 
+	fn as_previous_transaction_output_provider(&self) -> &PreviousTransactionOutputProvider;
+
 	fn as_transaction_meta_provider(&self) -> &TransactionMetaProvider;
 }
 
-impl<T> AsSubstore for T where T: BlockProvider + BlockStapler + TransactionProvider + TransactionMetaProvider {
+impl<T> AsSubstore for T where T: BlockProvider + BlockStapler + TransactionProvider + TransactionMetaProvider + PreviousTransactionOutputProvider {
 	fn as_block_provider(&self) -> &BlockProvider {
 		&*self
 	}
@@ -81,6 +83,10 @@ impl<T> AsSubstore for T where T: BlockProvider + BlockStapler + TransactionProv
 		&*self
 	}
 
+	fn as_previous_transaction_output_provider(&self) -> &PreviousTransactionOutputProvider {
+		&*self
+	}
+
 	fn as_transaction_meta_provider(&self) -> &TransactionMetaProvider {
 		&*self
 	}
@@ -243,7 +249,10 @@ impl Storage {
 				match context.meta.entry(input.previous_output.hash.clone()) {
 					Entry::Occupied(mut entry) => {
 						let meta = entry.get_mut();
-						if meta.is_spent(input.previous_output.index as usize) {
+						let is_spent = meta.is_spent(input.previous_output.index as usize)
+							.ok_or(Error::unknown_spending(&input.previous_output.hash))?;
+
+						if is_spent {
 							return Err(Error::double_spend(&input.previous_output.hash));
 						}
 						meta.denote_used(input.previous_output.index as usize);
@@ -251,8 +260,10 @@ impl Storage {
 					Entry::Vacant(entry) => {
 						let mut meta = self.transaction_meta(&input.previous_output.hash)
 							.ok_or(Error::unknown_spending(&input.previous_output.hash))?;
+						let is_spent = meta.is_spent(input.previous_output.index as usize)
+							.ok_or(Error::unknown_spending(&input.previous_output.hash))?;
 
-						if meta.is_spent(input.previous_output.index as usize) {
+						if is_spent {
 							return Err(Error::double_spend(&input.previous_output.hash));
 						}
 
@@ -672,7 +683,6 @@ impl BlockStapler for Storage {
 }
 
 impl TransactionProvider for Storage {
-
 	fn transaction_bytes(&self, hash: &H256) -> Option<Bytes> {
 		self.get(COL_TRANSACTIONS, &**hash)
 	}
@@ -680,53 +690,43 @@ impl TransactionProvider for Storage {
 	fn transaction(&self, hash: &H256) -> Option<chain::Transaction> {
 		let mut cache = self.transaction_cache.write();
 
-		let (tx, is_cached) = {
-			let cached_transaction = cache.get_mut(hash);
-			match cached_transaction {
-				None => {
-					(
-						self.transaction_bytes(hash).map(|tx_bytes| {
-							let tx: chain::Transaction = deserialize(tx_bytes.as_ref())
-								.expect("Failed to deserialize transaction: db corrupted?");
-							tx
-						}),
-						false
-					)
-				},
-				Some(tx) => (Some(tx.clone()), true)
-			}
-		};
+		if let Some(cached_transaction) = cache.get_mut(hash) {
+			return Some(cached_transaction.clone());
+		}
 
-		match tx {
-			Some(ref tx) => { if !is_cached { cache.insert(hash.clone(), tx.clone()); } }
-			None => {}
-		};
+		let tx: Option<chain::Transaction> = self.transaction_bytes(hash).map(|tx_bytes| {
+			deserialize(tx_bytes.as_ref()).expect("Failed to deserialize transaction: db corrupted?")
+		});
+
+		if let Some(ref tx) = tx {
+			cache.insert(hash.clone(), tx.clone());
+		}
 
 		tx
 	}
 }
 
-impl TransactionMetaProvider for Storage {
+impl PreviousTransactionOutputProvider for Storage {
+	fn previous_transaction_output(&self, prevout: &chain::OutPoint) -> Option<chain::TransactionOutput> {
+		self.transaction(&prevout.hash)
+			.and_then(|tx| tx.outputs.into_iter().nth(prevout.index as usize))
+	}
+}
 
+impl TransactionMetaProvider for Storage {
 	fn transaction_meta(&self, hash: &H256) -> Option<TransactionMeta> {
 		let mut cache = self.meta_cache.write();
 
-		let (meta, is_cached) = {
-			let cached_meta = cache.get_mut(hash);
-			match cached_meta {
-				None => {
-					(self.get(COL_TRANSACTIONS_META, &**hash).map(|val|
-						TransactionMeta::from_bytes(&val).expect("Invalid transaction metadata: db corrupted?")
-					), false)
-				},
-				Some(meta) => (Some(meta.clone()), true)
-			}
-		};
+		if let Some(cached_meta) = cache.get_mut(hash) {
+			return Some(cached_meta.clone());
+		}
 
-		match meta {
-			Some(ref meta) => { if !is_cached { cache.insert(hash.clone(), meta.clone()); } }
-			None => {}
-		};
+		let meta = self.get(COL_TRANSACTIONS_META, &**hash)
+			.map(|val| TransactionMeta::from_bytes(&val).expect("Invalid transaction metadata: db corrupted?"));
+
+		if let Some(ref meta) = meta {
+			cache.insert(hash.clone(), meta.clone());
+		}
 
 		meta
 	}
@@ -862,7 +862,7 @@ mod tests {
 		let genesis_coinbase = genesis.transactions()[0].hash();
 
 		let genesis_meta = store.transaction_meta(&genesis_coinbase).unwrap();
-		assert!(!genesis_meta.is_spent(0));
+		assert!(!genesis_meta.is_spent(0).unwrap());
 
 		let forged_block = test_data::block_builder()
 			.header().parent(genesis.hash()).build()
@@ -876,7 +876,7 @@ mod tests {
 		store.insert_block(&forged_block).unwrap();
 
 		let genesis_meta = store.transaction_meta(&genesis_coinbase).unwrap();
-		assert!(genesis_meta.is_spent(0));
+		assert!(genesis_meta.is_spent(0).unwrap());
 
 		assert_eq!(store.best_block().expect("genesis block inserted").number, 1);
 	}
@@ -906,8 +906,8 @@ mod tests {
 		store.insert_block(&block).unwrap();
 
 		let meta = store.transaction_meta(&block.transactions()[1].hash()).unwrap();
-		assert!(meta.is_spent(0), "Transaction #1 first output in the new block should be recorded as spent");
-		assert!(!meta.is_spent(1), "Transaction #1 second output in the new block should be recorded as unspent");
+		assert!(meta.is_spent(0).unwrap(), "Transaction #1 first output in the new block should be recorded as spent");
+		assert!(!meta.is_spent(1).unwrap(), "Transaction #1 second output in the new block should be recorded as unspent");
 	}
 
 	#[test]
@@ -951,12 +951,12 @@ mod tests {
 		store.insert_block(&block2).unwrap();
 
 		let meta = store.transaction_meta(&tx_big).unwrap();
-		assert!(meta.is_spent(0), "Transaction #1 output #0 in the new block should be recorded as spent");
-		assert!(meta.is_spent(2), "Transaction #1 output #2 in the new block should be recorded as spent");
-		assert!(meta.is_spent(5), "Transaction #1 output #5 in the new block should be recorded as spent");
+		assert!(meta.is_spent(0).unwrap(), "Transaction #1 output #0 in the new block should be recorded as spent");
+		assert!(meta.is_spent(2).unwrap(), "Transaction #1 output #2 in the new block should be recorded as spent");
+		assert!(meta.is_spent(5).unwrap(), "Transaction #1 output #5 in the new block should be recorded as spent");
 
-		assert!(!meta.is_spent(1), "Transaction #1 output #1 in the new block should be recorded as unspent");
-		assert!(!meta.is_spent(3), "Transaction #1 second #3 in the new block should be recorded as unspent");
+		assert!(!meta.is_spent(1).unwrap(), "Transaction #1 output #1 in the new block should be recorded as unspent");
+		assert!(!meta.is_spent(3).unwrap(), "Transaction #1 second #3 in the new block should be recorded as unspent");
 	}
 
 	#[test]
@@ -1159,15 +1159,15 @@ mod tests {
 		// outputs 0, 1, 2 should be spent in the side chain branch
 		// we reorganized to the side chain branch
 		// so, outputs 0, 1, 2 should  be spent
-		assert!(meta.is_spent(0));
-		assert!(meta.is_spent(1));
-		assert!(meta.is_spent(2));
+		assert!(meta.is_spent(0).unwrap());
+		assert!(meta.is_spent(1).unwrap());
+		assert!(meta.is_spent(2).unwrap());
 
 		// outputs 3, 4 should not be spent in the side chain branch
 		// we reorganized to the side chain branch
 		// so, outputs 3, 4 should not be spent
-		assert!(!meta.is_spent(3));
-		assert!(!meta.is_spent(4));
+		assert!(!meta.is_spent(3).unwrap());
+		assert!(!meta.is_spent(4).unwrap());
 
 		// Reorganizing back to main chain with 2 blocks in a row
 
@@ -1195,15 +1195,15 @@ mod tests {
 		// outputs 1, 3, 4 should be spent in the main branch
 		// we reorganized to the main branch again after reorganized to side branch
 		// so, outputs 1, 3, 4 should be spent
-		assert!(meta.is_spent(1));
-		assert!(meta.is_spent(3));
-		assert!(meta.is_spent(4));
+		assert!(meta.is_spent(1).unwrap());
+		assert!(meta.is_spent(3).unwrap());
+		assert!(meta.is_spent(4).unwrap());
 
 		// outputs 0, 2 should not be spent in the main branch
 		// we reorganized to the main branch again after reorganized to side branch
 		// so, outputs 0, 2 should not be spent
-		assert!(!meta.is_spent(0));
-		assert!(!meta.is_spent(2));
+		assert!(!meta.is_spent(0).unwrap());
+		assert!(!meta.is_spent(2).unwrap());
 	}
 
 	// test simulates when main chain and side chain are competing all along, each adding
@@ -1282,7 +1282,7 @@ mod tests {
 
 		let genesis_meta = store.transaction_meta(&genesis_coinbase)
 			.expect("Transaction meta for the genesis coinbase transaction should exist");
-		assert!(genesis_meta.is_spent(0), "Genesis coinbase should be recorded as spent because block#1 transaction spends it");
+		assert!(genesis_meta.is_spent(0).unwrap(), "Genesis coinbase should be recorded as spent because block#1 transaction spends it");
 
 		let mut update_context = UpdateContext::new(&store.database, &block_hash);
 		store.decanonize_block(&mut update_context, &block_hash)
@@ -1292,7 +1292,7 @@ mod tests {
 
 		let genesis_meta = store.transaction_meta(&genesis_coinbase)
 			.expect("Transaction meta for the genesis coinbase transaction should exist");
-		assert!(!genesis_meta.is_spent(0), "Genesis coinbase should be recorded as unspent because we retracted block #1");
+		assert!(!genesis_meta.is_spent(0).unwrap(), "Genesis coinbase should be recorded as unspent because we retracted block #1");
 
 		assert_eq!(store.block_number(&block_hash), None);
 	}

db/src/test_storage.rs

@@ -2,7 +2,7 @@
 
 use super::{
 	BlockRef, Store, Error, BestBlock, BlockLocation, BlockInsertedChain, BlockProvider,
-	BlockStapler, TransactionMetaProvider, TransactionProvider,
+	BlockStapler, TransactionMetaProvider, TransactionProvider, PreviousTransactionOutputProvider,
 	IndexedBlock, BlockHeaderProvider,
 };
 use chain::{self, Block};
@@ -173,7 +173,6 @@ impl BlockStapler for TestStorage {
 }
 
 impl TransactionProvider for TestStorage {
-
 	fn transaction_bytes(&self, hash: &H256) -> Option<Bytes> {
 		self.transaction(hash).map(|tx| serialization::serialize(&tx))
 	}
@@ -186,6 +185,13 @@ impl TransactionProvider for TestStorage {
 	}
 }
 
+impl PreviousTransactionOutputProvider for TestStorage {
+	fn previous_transaction_output(&self, prevout: &chain::OutPoint) -> Option<chain::TransactionOutput> {
+		self.transaction(&prevout.hash)
+			.and_then(|tx| tx.outputs.into_iter().nth(prevout.index as usize))
+	}
+}
+
 impl TransactionMetaProvider for TestStorage {
 	// just spawns new meta so far, use real store for proper tests
 	fn transaction_meta(&self, hash: &H256) -> Option<TransactionMeta> {

db/src/transaction_meta.rs

@@ -69,8 +69,8 @@ impl TransactionMeta {
 		self.block_height
 	}
 
-	pub fn is_spent(&self, idx: usize) -> bool {
-		self.bits.get(idx + 1).expect("Index should be verified by the caller")
+	pub fn is_spent(&self, idx: usize) -> Option<bool> {
+		self.bits.get(idx + 1) //.expect("Index should be verified by the caller")
 	}
 
 	pub fn is_fully_spent(&self) -> bool {

db/src/transaction_meta_provider.rs

@@ -1,5 +1,10 @@
-use transaction_meta::TransactionMeta;
 use primitives::hash::H256;
+use chain::OutPoint;
+use transaction_meta::TransactionMeta;
+
+pub trait TransactionOutputObserver {
+	fn is_spent(&self, prevout: &OutPoint) -> Option<bool>;
+}
 
 pub trait TransactionMetaProvider {
 	/// get transaction metadata

db/src/transaction_provider.rs

@@ -3,7 +3,6 @@ use primitives::bytes::Bytes;
 use chain;
 
 pub trait TransactionProvider {
-
 	/// returns true if store contains given transaction
 	fn contains_transaction(&self, hash: &H256) -> bool {
 		self.transaction(hash).is_some()
@@ -21,7 +20,4 @@ pub trait TransactionProvider {
 /// a shame to clone the whole transaction just to get single output.
 pub trait PreviousTransactionOutputProvider {
 	fn previous_transaction_output(&self, prevout: &chain::OutPoint) -> Option<chain::TransactionOutput>;
-
-	// TODO: this should not be here, cause it requires meta data
-	fn is_spent(&self, prevout: &chain::OutPoint) -> bool;
 }

miner/src/memory_pool.rs

@@ -669,6 +669,11 @@ impl MemoryPool {
 		self.storage.get_transactions_ids()
 	}
 
+	/// Returns true if output was spent
+	pub fn is_spent(&self, prevout: &OutPoint) -> bool {
+		self.storage.is_output_spent(prevout)
+	}
+
 	fn make_entry(&mut self, t: Transaction) -> Entry {
 		let hash = t.hash();
 		let ancestors = self.get_ancestors(&t);
@@ -741,10 +746,6 @@ impl PreviousTransactionOutputProvider for MemoryPool {
 			.and_then(|tx| tx.outputs.get(prevout.index as usize))
 			.cloned()
 	}
-
-	fn is_spent(&self, prevout: &OutPoint) -> bool {
-		self.storage.is_output_spent(prevout)
-	}
 }
 
 impl HeapSizeOf for MemoryPool {
@@ -792,7 +793,6 @@ impl<'a> Iterator for MemoryPoolIterator<'a> {
 
 #[cfg(test)]
 mod tests {
-	use db::PreviousTransactionOutputProvider;
 	use chain::{Transaction, OutPoint};
 	use heapsize::HeapSizeOf;
 	use super::{MemoryPool, OrderingStrategy};

sync/src/synchronization_verifier.rs

@@ -7,7 +7,7 @@ use network::Magic;
 use primitives::hash::H256;
 use synchronization_chain::ChainRef;
 use verification::{ChainVerifier, Verify as VerificationVerify, Chain};
-use db::{SharedStore, IndexedBlock, PreviousTransactionOutputProvider};
+use db::{SharedStore, IndexedBlock, PreviousTransactionOutputProvider, TransactionOutputObserver};
 use time::get_time;
 
 /// Block verification events sink
@@ -153,7 +153,7 @@ impl<T> Verifier for SyncVerifier<T> where T: VerificationSink {
 }
 
 /// Execute single verification task
-fn execute_verification_task<T: VerificationSink, U: PreviousTransactionOutputProvider>(sink: &Arc<T>, tx_output_provider: &U, verifier: &ChainVerifier, task: VerificationTask) {
+fn execute_verification_task<T: VerificationSink, U: PreviousTransactionOutputProvider + TransactionOutputObserver>(sink: &Arc<T>, tx_output_provider: &U, verifier: &ChainVerifier, task: VerificationTask) {
 	let mut tasks_queue: VecDeque<VerificationTask> = VecDeque::new();
 	tasks_queue.push_back(task);
 
@@ -200,9 +200,14 @@ impl PreviousTransactionOutputProvider for ChainMemoryPoolTransactionOutputProvi
 	fn previous_transaction_output(&self, prevout: &OutPoint) -> Option<TransactionOutput> {
 		self.chain.read().memory_pool().previous_transaction_output(prevout)
 	}
+}
 
-	fn is_spent(&self, prevout: &OutPoint) -> bool {
-		self.chain.read().storage().transaction_meta(&prevout.hash).and_then(|tm| Some(tm.is_spent(prevout.index as usize))).unwrap_or(false)
+impl TransactionOutputObserver for ChainMemoryPoolTransactionOutputProvider {
+	fn is_spent(&self, prevout: &OutPoint) -> Option<bool> {
+		self.chain.read()
+			.storage()
+			.transaction_meta(&prevout.hash)
+			.and_then(|tm| tm.is_spent(prevout.index as usize))
 	}
 }
 
@@ -210,9 +215,11 @@ impl PreviousTransactionOutputProvider for EmptyTransactionOutputProvider {
 	fn previous_transaction_output(&self, _prevout: &OutPoint) -> Option<TransactionOutput> {
 		None
 	}
+}
 
-	fn is_spent(&self, _prevout: &OutPoint) -> bool {
-		false
+impl TransactionOutputObserver for EmptyTransactionOutputProvider {
+	fn is_spent(&self, _prevout: &OutPoint) -> Option<bool> {
+		None
 	}
 }
 

verification/src/chain_verifier.rs

@@ -3,7 +3,7 @@
 use std::collections::BTreeSet;
 use scoped_pool::Pool;
 use hash::H256;
-use db::{self, BlockLocation, PreviousTransactionOutputProvider, BlockHeaderProvider};
+use db::{self, BlockLocation, PreviousTransactionOutputProvider, BlockHeaderProvider, TransactionOutputObserver};
 use network::{Magic, ConsensusParams};
 use error::{Error, TransactionError};
 use sigops::{StoreWithUnretainedOutputs, transaction_sigops};
@@ -168,7 +168,7 @@ impl ChainVerifier {
 		time: u32,
 		transaction: &chain::Transaction,
 		sequence: usize
-	) -> Result<(), TransactionError> where T: PreviousTransactionOutputProvider {
+	) -> Result<(), TransactionError> where T: PreviousTransactionOutputProvider + TransactionOutputObserver {
 
 		use script::{
 			TransactionInputSigner,
@@ -194,7 +194,14 @@ impl ChainVerifier {
 				_ => return Err(TransactionError::UnknownReference(input.previous_output.hash.clone()))
 			};
 
-			if prevout_provider.is_spent(&input.previous_output) {
+			// unwrap_or(false) is actually not right!
+			// but can be here because of two reasons
+			// - this function is not responsible for checking if previous transactions
+			// in currently processed block / mempool already spent this output
+			// - if we process transactions from mempool we shouldn't care if transactions before it
+			// spent this output, cause they may not make their way into the block due to their size
+			// or sigops limit
+			if prevout_provider.is_spent(&input.previous_output).unwrap_or(false) {
 				return Err(TransactionError::UsingSpentOutput(input.previous_output.hash.clone(), input.previous_output.index))
 			}
 
@@ -501,12 +508,12 @@ mod tests {
 			.build();
 
 		storage.insert_block(&genesis).expect("Genesis should be inserted with no errors");
-		let genesis_coinbase = genesis.transactions()[1].hash();
+		let first_tx_hash = genesis.transactions()[1].hash();
 
 		let block = test_data::block_builder()
 			.transaction().coinbase().build()
 			.transaction()
-				.input().hash(genesis_coinbase).build()
+				.input().hash(first_tx_hash).build()
 				.output().value(30).build()
 				.output().value(20).build()
 				.build()

verification/src/sigops.rs

@@ -22,10 +22,6 @@ impl<'a, T> PreviousTransactionOutputProvider for StoreWithUnretainedOutputs<'a,
 			.and_then(|tx| tx.outputs.into_iter().nth(prevout.index as usize))
 			.or_else(|| self.outputs.previous_transaction_output(prevout))
 	}
-
-	fn is_spent(&self, _prevout: &OutPoint) -> bool {
-		unimplemented!();
-	}
 }
 
 pub fn transaction_sigops(