removed redundant StorageTransactionOutputProvider, clarified bip30

sync/src/utils/memory_pool_transaction_provider.rs

@@ -4,13 +4,12 @@ use db::{PreviousTransactionOutputProvider, TransactionOutputObserver};
 use miner::{DoubleSpendCheckResult, HashedOutPoint, NonFinalDoubleSpendSet};
 use verification::TransactionError;
 use super::super::types::{MemoryPoolRef, StorageRef};
-use super::StorageTransactionOutputProvider;
 
 /// Transaction output observer, which looks into both storage && into memory pool.
 /// It also allows to replace non-final transactions in the memory pool.
 pub struct MemoryPoolTransactionOutputProvider {
 	/// Storage provider
-	storage_provider: StorageTransactionOutputProvider,
+	storage_provider: StorageRef,
 	/// Transaction inputs from memory pool transactions
 	mempool_inputs: HashMap<HashedOutPoint, Option<TransactionOutput>>,
 	/// Previous outputs, for which we should return 'Not spent' value.
@@ -29,7 +28,7 @@ impl MemoryPoolTransactionOutputProvider {
 			DoubleSpendCheckResult::DoubleSpend(_, hash, index) => Err(TransactionError::UsingSpentOutput(hash, index)),
 			// there are no transactions, which are spending same inputs in memory pool
 			DoubleSpendCheckResult::NoDoubleSpend => Ok(MemoryPoolTransactionOutputProvider {
-				storage_provider: StorageTransactionOutputProvider::with_storage(storage),
+				storage_provider: storage,
 				mempool_inputs: transaction.inputs.iter()
 					.map(|input| (
 						input.previous_output.clone().into(),
@@ -39,7 +38,7 @@ impl MemoryPoolTransactionOutputProvider {
 			}),
 			// there are non-final transactions, which are spending same inputs in memory pool
 			DoubleSpendCheckResult::NonFinalDoubleSpend(nonfinal_spends) => Ok(MemoryPoolTransactionOutputProvider {
-				storage_provider: StorageTransactionOutputProvider::with_storage(storage),
+				storage_provider: storage,
 				mempool_inputs: transaction.inputs.iter()
 					.map(|input| (
 						input.previous_output.clone().into(),

sync/src/utils/mod.rs

@@ -11,7 +11,6 @@ mod message_block_headers_provider;
 mod orphan_blocks_pool;
 mod orphan_transactions_pool;
 mod partial_merkle_tree;
-mod storage_transaction_provider;
 mod synchronization_state;
 
 pub use self::average_speed_meter::AverageSpeedMeter;
@@ -27,7 +26,6 @@ pub use self::message_block_headers_provider::MessageBlockHeadersProvider;
 pub use self::orphan_blocks_pool::OrphanBlocksPool;
 pub use self::orphan_transactions_pool::{OrphanTransactionsPool, OrphanTransaction};
 pub use self::partial_merkle_tree::{PartialMerkleTree, build_partial_merkle_tree};
-pub use self::storage_transaction_provider::StorageTransactionOutputProvider;
 pub use self::synchronization_state::SynchronizationState;
 
 /// Block height type

sync/src/utils/storage_transaction_provider.rs

@@ -1,31 +0,0 @@
-use chain::{TransactionOutput, OutPoint};
-use db::{PreviousTransactionOutputProvider, TransactionOutputObserver};
-use super::super::types::StorageRef;
-
-/// Transaction output observer, which looks into storage
-pub struct StorageTransactionOutputProvider {
-	/// Storage reference
-	storage: StorageRef,
-}
-
-impl StorageTransactionOutputProvider {
-	pub fn with_storage(storage: StorageRef) -> Self {
-		StorageTransactionOutputProvider {
-			storage: storage,
-		}
-	}
-}
-
-impl TransactionOutputObserver for StorageTransactionOutputProvider {
-	fn is_spent(&self, prevout: &OutPoint) -> Option<bool> {
-		self.storage
-			.transaction_meta(&prevout.hash)
-			.and_then(|tm| tm.is_spent(prevout.index as usize))
-	}
-}
-
-impl PreviousTransactionOutputProvider for StorageTransactionOutputProvider {
-	fn previous_transaction_output(&self, prevout: &OutPoint, transaction_index: usize) -> Option<TransactionOutput> {
-		self.storage.as_previous_transaction_output_provider().previous_transaction_output(prevout, transaction_index)
-	}
-}

verification/src/accept_transaction.rs

@@ -58,7 +58,6 @@ impl<'a> TransactionAcceptor<'a> {
 }
 
 pub struct MemoryPoolTransactionAcceptor<'a> {
-	pub bip30: TransactionBip30<'a>,
 	pub missing_inputs: TransactionMissingInputs<'a>,
 	pub maturity: TransactionMaturity<'a>,
 	pub overspent: TransactionOverspent<'a>,
@@ -85,7 +84,6 @@ impl<'a> MemoryPoolTransactionAcceptor<'a> {
 		let params = network.consensus_params();
 		let transaction_index = 0;
 		MemoryPoolTransactionAcceptor {
-			bip30: TransactionBip30::new_for_mempool(transaction, meta_store),
 			missing_inputs: TransactionMissingInputs::new(transaction, prevout_store, transaction_index),
 			maturity: TransactionMaturity::new(transaction, meta_store, height),
 			overspent: TransactionOverspent::new(transaction, prevout_store),
@@ -96,8 +94,8 @@ impl<'a> MemoryPoolTransactionAcceptor<'a> {
 	}
 
 	pub fn check(&self) -> Result<(), TransactionError> {
-		// TODO: b82 fails, when this is enabled, fix this
-		//try!(self.bip30.check());
+		// Bip30 is not checked because we don't need to allow tx pool acceptance of an unspent duplicate.
+		// Tx pool validation is not strinctly a matter of consensus.
 		try!(self.missing_inputs.check());
 		try!(self.maturity.check());
 		try!(self.overspent.check());
@@ -112,6 +110,15 @@ pub trait TransactionRule {
 	fn check(&self) -> Result<(), TransactionError>;
 }
 
+/// Bip30 validation
+///
+/// A transaction hash that exists in the chain is not acceptable even if
+/// the original is spent in the new block. This is not necessary nor is it
+/// described by BIP30, but it is in the code referenced by BIP30. As such
+/// the tx pool need only test against the chain, skipping the pool.
+///
+/// source:
+/// https://github.com/libbitcoin/libbitcoin/blob/61759b2fd66041bcdbc124b2f04ed5ddc20c7312/src/chain/transaction.cpp#L780-L785
 pub struct TransactionBip30<'a> {
 	transaction: CanonTransaction<'a>,
 	store: &'a TransactionMetaProvider,
@@ -134,27 +141,10 @@ impl<'a> TransactionBip30<'a> {
 			exception: exception,
 		}
 	}
-
-	fn new_for_mempool(transaction: CanonTransaction<'a>, store: &'a TransactionMetaProvider) -> Self {
-		TransactionBip30 {
-			transaction: transaction,
-			store: store,
-			exception: false,
-		}
-	}
 }
 
 impl<'a> TransactionRule for TransactionBip30<'a> {
 	fn check(&self) -> Result<(), TransactionError> {
-		// we allow optionals here, cause previous output may be a part of current block
-		// yet, we do not need to check current block, cause duplicated transactions
-		// in the same block are also forbidden
-		//
-		// update*
-		// TODO:
-		// There is a potential consensus failure here, cause transaction before this one
-		// may have fully spent the output, and we, by checking only storage, have no knowladge
-		// of it
 		match self.store.transaction_meta(&self.transaction.hash) {
 			Some(ref meta) if !meta.is_fully_spent() && !self.exception => {
 				Err(TransactionError::UnspentTransactionWithTheSameHash)