From 3ad579ea29db6ad5c2c79d97628d255e55c1a105 Mon Sep 17 00:00:00 2001 From: NikVolf Date: Mon, 14 Nov 2016 23:56:43 +0300 Subject: [PATCH 1/5] proper tx sigopcounting --- Cargo.lock | 1 + db/Cargo.toml | 1 + db/src/lib.rs | 1 + db/src/storage.rs | 2 + verification/src/chain_verifier.rs | 64 +++++------------------------- verification/src/lib.rs | 4 -- verification/src/utils.rs | 18 +++++++++ 7 files changed, 33 insertions(+), 58 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index d429e819..cfc396be 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -147,6 +147,7 @@ dependencies = [ "chain 0.1.0", "elastic-array 0.5.0 (registry+https://github.com/rust-lang/crates.io-index)", "ethcore-devtools 1.3.0", + "log 0.3.6 (registry+https://github.com/rust-lang/crates.io-index)", "parking_lot 0.3.6 (registry+https://github.com/rust-lang/crates.io-index)", "primitives 0.1.0", "rocksdb 0.4.5 (git+https://github.com/ethcore/rust-rocksdb)", diff --git a/db/Cargo.toml b/db/Cargo.toml index a1b72dfa..c75acc83 100644 --- a/db/Cargo.toml +++ b/db/Cargo.toml @@ -14,6 +14,7 @@ serialization = { path = "../serialization" } parking_lot = "0.3" test-data = { path = "../test-data" } bit-vec = "0.4" +log = "0.3" [features] dev = [] diff --git a/db/src/lib.rs b/db/src/lib.rs index 0a8b5d5c..73911093 100644 --- a/db/src/lib.rs +++ b/db/src/lib.rs @@ -8,6 +8,7 @@ extern crate byteorder; extern crate chain; extern crate serialization; extern crate bit_vec; +#[macro_use] extern crate log; #[cfg(test)] extern crate ethcore_devtools as devtools; diff --git a/db/src/storage.rs b/db/src/storage.rs index 1c84e4c1..d116e963 100644 --- a/db/src/storage.rs +++ b/db/src/storage.rs @@ -341,6 +341,8 @@ impl Storage { /// all transaction meta is removed /// DOES NOT update best block fn decanonize_block(&self, context: &mut UpdateContext, hash: &H256) -> Result<(), Error> { + trace!(target: "reorg", "Decanonizing block {}", hash); + // ensure that block is of the main chain try!(self.block_number(hash).ok_or(Error::NotMain(hash.clone()))); diff --git a/verification/src/chain_verifier.rs b/verification/src/chain_verifier.rs index e95bcb18..079077d2 100644 --- a/verification/src/chain_verifier.rs +++ b/verification/src/chain_verifier.rs @@ -94,7 +94,7 @@ impl ChainVerifier { Ok(()) } - fn verify_transaction(&self, block: &chain::Block, transaction: &chain::Transaction) -> Result { + fn verify_transaction(&self, block: &chain::Block, transaction: &chain::Transaction) -> Result<(), TransactionError> { use script::{ TransactionInputSigner, TransactionSignatureChecker, @@ -103,7 +103,6 @@ impl ChainVerifier { verify_script, }; - let mut sigops: usize = 0; for (input_index, input) in transaction.inputs().iter().enumerate() { let store_parent_transaction = self.store.transaction(&input.previous_output.hash); let parent_transaction = match store_parent_transaction { @@ -129,15 +128,6 @@ impl ChainVerifier { let input: Script = input.script_sig().to_vec().into(); let output: Script = paired_output.script_pubkey.to_vec().into(); - sigops += - try!(output.sigop_count().map_err(|e| TransactionError::ReferenceSignatureMallformed(format!("{}", e)))) + - try!(input.sigop_count().map_err(|e| TransactionError::SignatureMallformed(format!("{}", e)))); - - if sigops > MAX_BLOCK_SIGOPS { - // already overflown - return Err(TransactionError::SigopsAmount); - } - let flags = VerificationFlags::default().verify_p2sh(true); // for tests only, skips as late as possible @@ -150,7 +140,7 @@ impl ChainVerifier { } } - Ok(sigops) + Ok(()) } } @@ -186,12 +176,18 @@ impl Verify for ChainVerifier { // verify transactions (except coinbase) let mut block_sigops = 0; for (idx, transaction) in block.transactions().iter().skip(1).enumerate() { - let tx_sigops = try!(self.verify_transaction(block, transaction).map_err(|e| Error::Transaction(idx+1, e))); - block_sigops += tx_sigops; + + block_sigops += try!( + utils::transaction_sigops(transaction) + .map_err(|e| Error::Transaction(idx+1, TransactionError::SignatureMallformed(format!("{}", e)))) + ); if block_sigops >= MAX_BLOCK_SIGOPS { return Err(Error::MaximumSigops); } + + try!(self.verify_transaction(block, transaction).map_err(|e| Error::Transaction(idx+1, e))); + } trace!(target: "verification", "Block {} total sigops: {}", &hash, &block_sigops); @@ -477,46 +473,6 @@ mod tests { assert_eq!(expected, verifier.verify(&block)) } - #[test] - fn sigops_overflow_tx() { - let path = RandomTempPath::create_dir(); - let storage = Storage::new(path.as_path()).unwrap(); - - let genesis = test_data::block_builder() - .transaction() - .coinbase() - .build() - .transaction() - .output().value(50).build() - .build() - .merkled_header().build() - .build(); - - storage.insert_block(&genesis).unwrap(); - let reference_tx = genesis.transactions()[1].hash(); - - let mut builder = script::Builder::default(); - for _ in 0..21000 { - builder = builder.push_opcode(script::Opcode::OP_CHECKSIG) - } - - let block = test_data::block_builder() - .transaction().coinbase().build() - .transaction() - .input() - .hash(reference_tx) - .signature_bytes(builder.into_script().to_bytes()) - .build() - .build() - .merkled_header().parent(genesis.hash()).build() - .build(); - - let verifier = ChainVerifier::new(Arc::new(storage)).pow_skip(); - - let expected = Err(Error::Transaction(1, TransactionError::SigopsAmount)); - assert_eq!(expected, verifier.verify(&block)); - } - #[test] fn sigops_overflow_block() { let path = RandomTempPath::create_dir(); diff --git a/verification/src/lib.rs b/verification/src/lib.rs index e6221379..02ab979e 100644 --- a/verification/src/lib.rs +++ b/verification/src/lib.rs @@ -69,10 +69,6 @@ pub enum TransactionError { Overspend, /// Signature script can't be properly parsed SignatureMallformed(String), - /// Signature script in referenced transaction can't be properly parsed - ReferenceSignatureMallformed(String), - /// Too many sigops - SigopsAmount, } #[derive(PartialEq, Debug)] diff --git a/verification/src/utils.rs b/verification/src/utils.rs index 6226fe2a..72b964b0 100644 --- a/verification/src/utils.rs +++ b/verification/src/utils.rs @@ -1,6 +1,8 @@ //! Verification utilities use primitives::hash::H256; use byteorder::{BigEndian, ByteOrder}; +use chain; +use script::{self, Script}; pub fn check_nbits(hash: &H256, n_bits: u32) -> bool { let hash_bytes: &[u8] = &**hash; @@ -52,6 +54,22 @@ pub fn block_reward_satoshi(block_height: u32) -> u64 { res } +pub fn transaction_sigops(transaction: &chain::Transaction) -> Result { + let mut result = 0usize; + + for input in transaction.inputs.iter() { + let input_script: Script = input.script_sig().to_vec().into(); + result += try!(input_script.sigop_count()); + } + + for output in transaction.outputs.iter() { + let output_script: Script = output.script_pubkey.to_vec().into(); + result += try!(output_script.sigop_count()); + } + + Ok(result) +} + #[cfg(test)] mod tests { From efd6c7a14322e3f30d2628a52c3f8308d0b4957a Mon Sep 17 00:00:00 2001 From: NikVolf Date: Tue, 15 Nov 2016 00:13:07 +0300 Subject: [PATCH 2/5] refactoring of opcounting --- verification/src/chain_verifier.rs | 10 +++++++--- verification/src/queue.rs | 2 +- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/verification/src/chain_verifier.rs b/verification/src/chain_verifier.rs index 079077d2..e802e8fb 100644 --- a/verification/src/chain_verifier.rs +++ b/verification/src/chain_verifier.rs @@ -134,7 +134,7 @@ impl ChainVerifier { if self.skip_sig { continue; } if let Err(e) = verify_script(&input, &output, &flags, &checker) { - println!("transaction signature verification failure: {:?}", e); + trace!(target: "verification", "transaction signature verification failure: {}", e); // todo: log error here return Err(TransactionError::Signature(input_index)) } @@ -187,10 +187,14 @@ impl Verify for ChainVerifier { } try!(self.verify_transaction(block, transaction).map_err(|e| Error::Transaction(idx+1, e))); - } - trace!(target: "verification", "Block {} total sigops: {}", &hash, &block_sigops); + trace!( + target: "verification", "Block {} (transactons: {}, sigops: {}) verification finished", + &hash, + block.transactions().len(), + &block_sigops + ); // todo: pre-process projected block number once verification is parallel! match self.store.accepted_location(block.header()) { diff --git a/verification/src/queue.rs b/verification/src/queue.rs index c0cac158..a8999acd 100644 --- a/verification/src/queue.rs +++ b/verification/src/queue.rs @@ -157,7 +157,7 @@ impl Queue { items.push_front(hash, ScheduleItem::Continued(item.block(), num)); }, Err(e) => { - println!("Verification failed: {:?}", e); + trace!(target: "verification", "Verification of block {} failed: {:?}", &hash, e); let mut invalid = self.invalid.write(); let mut processing = self.processing.write(); From b7014f4ea55efab3db09b40c668c7393d15a4b1d Mon Sep 17 00:00:00 2001 From: NikVolf Date: Tue, 15 Nov 2016 12:37:20 +0300 Subject: [PATCH 3/5] invalid output script counts as 1 --- verification/src/utils.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/verification/src/utils.rs b/verification/src/utils.rs index 72b964b0..bee7f09f 100644 --- a/verification/src/utils.rs +++ b/verification/src/utils.rs @@ -64,7 +64,8 @@ pub fn transaction_sigops(transaction: &chain::Transaction) -> Result Date: Tue, 15 Nov 2016 12:57:58 +0300 Subject: [PATCH 4/5] count coinbase outputs also --- verification/src/chain_verifier.rs | 6 +++++- verification/src/utils.rs | 14 ++++++++------ 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/verification/src/chain_verifier.rs b/verification/src/chain_verifier.rs index e802e8fb..cd0bb5b7 100644 --- a/verification/src/chain_verifier.rs +++ b/verification/src/chain_verifier.rs @@ -174,7 +174,11 @@ impl Verify for ChainVerifier { } // verify transactions (except coinbase) - let mut block_sigops = 0; + let mut block_sigops = try!( + utils::transaction_sigops(&block.transactions()[0]) + .map_err(|e| Error::Transaction(1, TransactionError::SignatureMallformed(format!("{}", e)))) + ); + for (idx, transaction) in block.transactions().iter().skip(1).enumerate() { block_sigops += try!( diff --git a/verification/src/utils.rs b/verification/src/utils.rs index bee7f09f..32e0eae0 100644 --- a/verification/src/utils.rs +++ b/verification/src/utils.rs @@ -57,15 +57,17 @@ pub fn block_reward_satoshi(block_height: u32) -> u64 { pub fn transaction_sigops(transaction: &chain::Transaction) -> Result { let mut result = 0usize; - for input in transaction.inputs.iter() { - let input_script: Script = input.script_sig().to_vec().into(); - result += try!(input_script.sigop_count()); - } - for output in transaction.outputs.iter() { let output_script: Script = output.script_pubkey.to_vec().into(); // todo: not always allow malformed output? - result += output_script.sigop_count().unwrap_or(1); + result += output_script.sigop_count().unwrap_or(0); + } + + if transaction.is_coinbase() { return Ok(result); } + + for input in transaction.inputs.iter() { + let input_script: Script = input.script_sig().to_vec().into(); + result += try!(input_script.sigop_count()); } Ok(result) From 7d6ee29e6b08fd61294ed2eaf17fd860f9f6c786 Mon Sep 17 00:00:00 2001 From: NikVolf Date: Tue, 15 Nov 2016 13:23:49 +0300 Subject: [PATCH 5/5] strict greater there --- verification/src/chain_verifier.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/verification/src/chain_verifier.rs b/verification/src/chain_verifier.rs index cd0bb5b7..d6f3cad7 100644 --- a/verification/src/chain_verifier.rs +++ b/verification/src/chain_verifier.rs @@ -186,7 +186,7 @@ impl Verify for ChainVerifier { .map_err(|e| Error::Transaction(idx+1, TransactionError::SignatureMallformed(format!("{}", e)))) ); - if block_sigops >= MAX_BLOCK_SIGOPS { + if block_sigops > MAX_BLOCK_SIGOPS { return Err(Error::MaximumSigops); }