From d0c480565d831aebd9f00e112c02a7b16d693189 Mon Sep 17 00:00:00 2001 From: debris Date: Tue, 13 Dec 2016 10:50:56 +0100 Subject: [PATCH] applying suggestions from review and adding comments --- db/src/transaction_meta_provider.rs | 8 +++++++- db/src/transaction_provider.rs | 2 +- miner/src/block_assembler.rs | 12 ++---------- verification/src/accept_block.rs | 3 --- verification/src/accept_transaction.rs | 6 ++---- verification/src/lib.rs | 9 +++++++++ verification/src/sigops.rs | 22 +++++++++------------- verification/src/timestamp.rs | 3 +++ verification/src/verify_block.rs | 5 +++-- verification/src/verify_transaction.rs | 5 +++-- 10 files changed, 39 insertions(+), 36 deletions(-) diff --git a/db/src/transaction_meta_provider.rs b/db/src/transaction_meta_provider.rs index a13beaac..2bf2d934 100644 --- a/db/src/transaction_meta_provider.rs +++ b/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; } +/// 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; } diff --git a/db/src/transaction_provider.rs b/db/src/transaction_provider.rs index 1ac512c0..0ee4b2bd 100644 --- a/db/src/transaction_provider.rs +++ b/db/src/transaction_provider.rs @@ -15,7 +15,7 @@ pub trait TransactionProvider { fn transaction(&self, hash: &H256) -> Option; } -/// 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 { diff --git a/miner/src/block_assembler.rs b/miner/src/block_assembler.rs index 489b77ce..abf4a268 100644 --- a/miner/src/block_assembler.rs +++ b/miner/src/block_assembler.rs @@ -185,17 +185,9 @@ impl<'a, T> Iterator for FittingTransactionsIterator<'a, T> where T: Iterator 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); diff --git a/verification/src/accept_block.rs b/verification/src/accept_block.rs index a196960f..5e158a33 100644 --- a/verification/src/accept_block.rs +++ b/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::>>() - .ok_or_else(|| Error::MaximumSigops)? - .into_iter() .sum::(); if sigops > self.max_sigops { diff --git a/verification/src/accept_transaction.rs b/verification/src/accept_transaction.rs index 4250bfbb..08390954 100644 --- a/verification/src/accept_transaction.rs +++ b/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(()) diff --git a/verification/src/lib.rs b/verification/src/lib.rs index 4dbd3099..a5db9899 100644 --- a/verification/src/lib.rs +++ b/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 diff --git a/verification/src/sigops.rs b/verification/src/sigops.rs index f7d8b1f9..c7f28652 100644 --- a/verification/src/sigops.rs +++ b/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 { - 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 { 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 } diff --git a/verification/src/timestamp.rs b/verification/src/timestamp.rs index 69ddb087..72aa7b33 100644 --- a/verification/src/timestamp.rs +++ b/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 { diff --git a/verification/src/verify_block.rs b/verification/src/verify_block.rs index 674d919a..cb8937c0 100644 --- a/verification/src/verify_block.rs +++ b/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::(); if sigops > self.max_sigops { diff --git a/verification/src/verify_transaction.rs b/verification/src/verify_transaction.rs index b6f2a9c0..49647e40 100644 --- a/verification/src/verify_transaction.rs +++ b/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 {