From 0c4a346c0093a1be9cdb6a513bf47a4e74126df1 Mon Sep 17 00:00:00 2001 From: debris Date: Fri, 18 Nov 2016 00:16:50 +0100 Subject: [PATCH 1/6] fixed enumerate skip order --- verification/src/chain_verifier.rs | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/verification/src/chain_verifier.rs b/verification/src/chain_verifier.rs index 06be939d..c7009c6a 100644 --- a/verification/src/chain_verifier.rs +++ b/verification/src/chain_verifier.rs @@ -58,7 +58,7 @@ impl ChainVerifier { let coinbase_spends = block.transactions()[0].total_spends(); let mut total_unspent = 0u64; - for (tx_index, tx) in block.transactions().iter().skip(1).enumerate() { + for (tx_index, tx) in block.transactions().iter().enumerate().skip(1) { let mut total_claimed: u64 = 0; @@ -68,11 +68,10 @@ impl ChainVerifier { if let Some(previous_meta) = self.store.transaction_meta(&input.previous_output.hash) { // check if it exists only // it will fail a little later if there is no transaction at all - if previous_meta.is_coinbase() - && (at_height < COINBASE_MATURITY || - at_height - COINBASE_MATURITY < previous_meta.height()) + if previous_meta.is_coinbase() && + (at_height < COINBASE_MATURITY || at_height - COINBASE_MATURITY < previous_meta.height()) { - return Err(Error::Transaction(tx_index+1, TransactionError::Maturity)); + return Err(Error::Transaction(tx_index, TransactionError::Maturity)); } } @@ -82,13 +81,13 @@ impl ChainVerifier { // todo: optimize block decomposition vec -> hashmap .or(block.transactions().iter().find(|tx| !tx.is_coinbase() && tx.hash() == input.previous_output.hash).cloned()) .ok_or( - Error::Transaction(tx_index+1, TransactionError::UnknownReference(input.previous_output.hash.clone())) + Error::Transaction(tx_index, TransactionError::UnknownReference(input.previous_output.hash.clone())) ) ); let output = try!(reference_tx.outputs.get(input.previous_output.index as usize) .ok_or( - Error::Transaction(tx_index+1, TransactionError::Input(input.previous_output.index as usize)) + Error::Transaction(tx_index, TransactionError::Input(input.previous_output.index as usize)) ) ); @@ -98,7 +97,7 @@ impl ChainVerifier { let total_spends = tx.total_spends(); if total_claimed < total_spends { - return Err(Error::Transaction(tx_index+1, TransactionError::Overspend)); + return Err(Error::Transaction(tx_index, TransactionError::Overspend)); } // total_claimed is greater than total_spends, checked above and returned otherwise, cannot overflow; qed From 9b535155bc52adfcdecedb8709cd39dcd5279be2 Mon Sep 17 00:00:00 2001 From: debris Date: Fri, 18 Nov 2016 00:42:31 +0100 Subject: [PATCH 2/6] fixing style --- chain/src/transaction.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/chain/src/transaction.rs b/chain/src/transaction.rs index 77e1cb06..91eb3e88 100644 --- a/chain/src/transaction.rs +++ b/chain/src/transaction.rs @@ -69,6 +69,10 @@ impl OutPoint { pub fn index(&self) -> u32 { self.index } + + pub fn is_null(&self) -> bool { + self.hash.is_zero() && self.index == u32::max_value() + } } #[derive(Debug, PartialEq, Default, Clone)] @@ -247,8 +251,7 @@ impl Transaction { } pub fn is_coinbase(&self) -> bool { - if self.inputs.len() != 1 { return false; } - self.inputs[0].previous_output.hash.is_zero() && self.inputs[0].previous_output.index == 0xffffffff + self.inputs.len() == 1 && self.inputs[0].previous_output.is_null() } pub fn total_spends(&self) -> u64 { From 09537c3a84b6ca8332f34935f1c16fd107f92e96 Mon Sep 17 00:00:00 2001 From: debris Date: Fri, 18 Nov 2016 01:07:19 +0100 Subject: [PATCH 3/6] replace db printlns with panics --- db/src/storage.rs | 59 +++++++++++++---------------------------- primitives/src/bytes.rs | 4 +++ 2 files changed, 23 insertions(+), 40 deletions(-) diff --git a/db/src/storage.rs b/db/src/storage.rs index 4219aef0..4e77c028 100644 --- a/db/src/storage.rs +++ b/db/src/storage.rs @@ -7,7 +7,7 @@ use byteorder::{LittleEndian, ByteOrder}; use primitives::hash::H256; use primitives::bytes::Bytes; use super::{BlockRef, BestBlock, BlockLocation}; -use serialization; +use serialization::{self, deserialize}; use chain::{self, RepresentH256}; use parking_lot::RwLock; use transaction_meta::TransactionMeta; @@ -110,21 +110,11 @@ impl Storage { self.read_meta(key).map(|val| LittleEndian::read_u32(&val)) } - /// is invoked on database non-fatal query errors - fn db_error(&self, msg: String) { - println!("Low-level database error: {}", &msg); - } - /// get the value of the key in the database - /// if the key is not present, reports non-fatal error and returns nothing fn get(&self, col: u32, key: &[u8]) -> Option { - let res = self.database.get(Some(col), key); - match res { - Err(msg) => { - self.db_error(msg); - None - }, - Ok(val) => val.map(|v| v.into()), + match self.database.get(Some(col), key) { + Ok(val) => val, + Err(msg) => panic!("{}", msg), } } @@ -140,7 +130,7 @@ impl Storage { /// loads block transaction list by the provided block hash fn block_transaction_hashes_by_hash(&self, h: &H256) -> Vec { self.get(COL_BLOCK_TRANSACTIONS, &**h) - .unwrap_or(Vec::new().into()) + .unwrap_or_else(Bytes::new) .chunks(H256::size()) .map(H256::from) .collect() @@ -150,25 +140,19 @@ impl Storage { self.block_transaction_hashes_by_hash(h) .into_iter() .filter_map(|tx_hash| { - self.transaction_bytes(&tx_hash).and_then(|tx_bytes| { - match serialization::deserialize::<_, chain::Transaction>(tx_bytes.as_ref()) { - Ok(tx) => Some(tx), - Err(e) => { - self.db_error(format!("Error deserializing transaction, possible db corruption ({:?})", e)); - None - } - } - }) + self.transaction_bytes(&tx_hash) + .map(|tx_bytes| { + deserialize::<_, chain::Transaction>(tx_bytes.as_ref()) + .expect("Error deserializing transaction, possible db corruption") + }) }) .collect() } fn block_header_by_hash(&self, h: &H256) -> Option { - self.get(COL_BLOCK_HEADERS, &**h).and_then(|val| - serialization::deserialize(val.as_ref()).map_err( - |e| self.db_error(format!("Error deserializing block header, possible db corruption ({:?})", e)) - ).ok() - ) + self.get(COL_BLOCK_HEADERS, &**h).map(|val| { + deserialize(val.as_ref()).expect("Error deserializing block header, possible db corruption") + }) } @@ -330,7 +314,7 @@ impl Storage { Err(e) => { // todo: log error here context.restore(); - println!("Error while reorganizing to {}: {:?}", hash, e); + error!("Error while reorganizing to {}: {:?}", hash, e); Err(e) } } @@ -410,16 +394,11 @@ impl BlockProvider for Storage { fn block(&self, block_ref: BlockRef) -> Option { self.resolve_hash(block_ref).and_then(|block_hash| self.get(COL_BLOCK_HEADERS, &*block_hash) - .and_then(|header_bytes| { - let transactions = self.block_transactions_by_hash(&block_hash);; - let maybe_header = match serialization::deserialize::<_, chain::BlockHeader>(header_bytes.as_ref()) { - Ok(header) => Some(header), - Err(e) => { - self.db_error(format!("Error deserializing header, possible db corruption ({:?})", e)); - None - } - }; - maybe_header.map(|header| chain::Block::new(header, transactions)) + .map(|header_bytes| { + let transactions = self.block_transactions_by_hash(&block_hash); + let header = deserialize::<_, chain::BlockHeader>(header_bytes.as_ref()) + .expect("Error deserializing header, possible db corruption"); + chain::Block::new(header, transactions) }) ) } diff --git a/primitives/src/bytes.rs b/primitives/src/bytes.rs index 55831f6b..17e7d133 100644 --- a/primitives/src/bytes.rs +++ b/primitives/src/bytes.rs @@ -5,6 +5,10 @@ use hex::{ToHex, FromHex, FromHexError}; pub struct Bytes(Vec); impl Bytes { + pub fn new() -> Self { + Bytes::default() + } + pub fn new_with_len(len: usize) -> Self { Bytes(vec![0; len]) } From a9fe6e5cdb000fe5f637b3dfb59f771f0b1f37be Mon Sep 17 00:00:00 2001 From: debris Date: Fri, 18 Nov 2016 01:40:40 +0100 Subject: [PATCH 4/6] simplifing code --- db/src/block_stapler.rs | 4 +-- db/src/error.rs | 4 +++ db/src/kvdb.rs | 2 +- db/src/storage.rs | 58 +++++++++++++++++----------------------- db/src/test_storage.rs | 11 ++++---- db/src/update_context.rs | 3 +-- 6 files changed, 38 insertions(+), 44 deletions(-) diff --git a/db/src/block_stapler.rs b/db/src/block_stapler.rs index 81a33604..de176d2e 100644 --- a/db/src/block_stapler.rs +++ b/db/src/block_stapler.rs @@ -3,7 +3,7 @@ use super::BlockLocation; use chain; use error::Error; -#[derive(Debug)] +#[derive(Debug, PartialEq)] pub struct Reorganization { pub height: u32, canonized: Vec, @@ -24,7 +24,7 @@ impl Reorganization { } } -#[derive(Debug)] +#[derive(Debug, PartialEq)] pub enum BlockInsertedChain { Disconnected, Main, diff --git a/db/src/error.rs b/db/src/error.rs index 923fefb4..2eb834fd 100644 --- a/db/src/error.rs +++ b/db/src/error.rs @@ -23,6 +23,10 @@ impl Error { Error::Consistency(ConsistencyError::UnknownNumber(n)) } + pub fn unknown_spending(h: &H256) -> Self { + Error::Consistency(ConsistencyError::UnknownSpending(h.clone())) + } + pub fn double_spend(h: &H256) -> Self { Error::Consistency(ConsistencyError::DoubleSpend(h.clone())) } diff --git a/db/src/kvdb.rs b/db/src/kvdb.rs index db159a6a..b3372ea7 100644 --- a/db/src/kvdb.rs +++ b/db/src/kvdb.rs @@ -297,7 +297,7 @@ impl Database { cfs = cfnames.iter().enumerate().map(|(i, n)| db.create_cf(n, &cf_options[i]).unwrap()).collect(); Ok(db) }, - err @ Err(_) => err, + err => err, } } } diff --git a/db/src/storage.rs b/db/src/storage.rs index 4e77c028..b8a7f80c 100644 --- a/db/src/storage.rs +++ b/db/src/storage.rs @@ -1,13 +1,13 @@ //! Bitcoin storage -use std::{self, fs}; +use std::fs; use std::path::Path; use kvdb::{Database, DatabaseConfig}; use byteorder::{LittleEndian, ByteOrder}; use primitives::hash::H256; use primitives::bytes::Bytes; use super::{BlockRef, BestBlock, BlockLocation}; -use serialization::{self, deserialize}; +use serialization::{serialize, deserialize}; use chain::{self, RepresentH256}; use parking_lot::RwLock; use transaction_meta::TransactionMeta; @@ -160,15 +160,14 @@ impl Storage { fn update_transactions_meta(&self, context: &mut UpdateContext, number: u32, accepted_txs: &[chain::Transaction]) -> Result<(), Error> { - for (accepted_idx, accepted_tx) in accepted_txs.iter().enumerate() { - if accepted_idx == 0 { - context.meta.insert( - accepted_tx.hash(), - TransactionMeta::new(number, accepted_tx.outputs.len()).coinbase() - ); - continue; - } + if let Some(accepted_tx) = accepted_txs.iter().next() { + context.meta.insert( + accepted_tx.hash(), + TransactionMeta::new(number, accepted_tx.outputs.len()).coinbase() + ); + } + for accepted_tx in accepted_txs.iter().skip(1) { context.meta.insert( accepted_tx.hash(), TransactionMeta::new(number, accepted_tx.outputs.len()) @@ -186,11 +185,8 @@ impl Storage { }, None => false, } { - let mut meta = - try!( - self.transaction_meta(&input.previous_output.hash) - .ok_or(Error::Consistency(ConsistencyError::UnknownSpending(input.previous_output.hash.clone()))) - ); + let mut meta = self.transaction_meta(&input.previous_output.hash) + .ok_or(Error::unknown_spending(&input.previous_output.hash))?; if meta.is_spent(input.previous_output.index as usize) { return Err(Error::double_spend(&input.previous_output.hash)); @@ -298,8 +294,8 @@ impl Storage { try!(self.update_transactions_meta(context, at_height, &transactions)); // only canonical blocks are allowed to wield a number - context.db_transaction.put(Some(COL_BLOCK_HASHES), &u32_key(at_height), std::ops::Deref::deref(hash)); - context.db_transaction.write_u32(Some(COL_BLOCK_NUMBERS), std::ops::Deref::deref(hash), at_height); + context.db_transaction.put(Some(COL_BLOCK_HASHES), &u32_key(at_height), &**hash); + context.db_transaction.write_u32(Some(COL_BLOCK_NUMBERS), &**hash, at_height); Ok(()) } @@ -436,7 +432,7 @@ impl BlockStapler for Storage { context.db_transaction.put( Some(COL_TRANSACTIONS), &*tx_hash, - &serialization::serialize(tx), + &serialize(tx), ); } context.db_transaction.put(Some(COL_BLOCK_TRANSACTIONS), &*block_hash, &tx_refs); @@ -444,7 +440,7 @@ impl BlockStapler for Storage { context.db_transaction.put( Some(COL_BLOCK_HEADERS), &*block_hash, - &serialization::serialize(block.header()) + &serialize(block.header()) ); // the block is continuing the main chain @@ -453,8 +449,8 @@ impl BlockStapler for Storage { context.db_transaction.write_u32(Some(COL_META), KEY_BEST_BLOCK_NUMBER, new_best_number); // updating main chain height reference - context.db_transaction.put(Some(COL_BLOCK_HASHES), &u32_key(new_best_number), std::ops::Deref::deref(&block_hash)); - context.db_transaction.write_u32(Some(COL_BLOCK_NUMBERS), std::ops::Deref::deref(&block_hash), new_best_number); + context.db_transaction.put(Some(COL_BLOCK_HASHES), &u32_key(new_best_number), &*block_hash); + context.db_transaction.write_u32(Some(COL_BLOCK_NUMBERS), &*block_hash, new_best_number); BlockInsertedChain::Main } @@ -472,8 +468,8 @@ impl BlockStapler for Storage { // and we canonize it also by provisioning transactions try!(self.update_transactions_meta(&mut context, new_best_number, block.transactions())); context.db_transaction.write_u32(Some(COL_META), KEY_BEST_BLOCK_NUMBER, new_best_number); - context.db_transaction.put(Some(COL_BLOCK_HASHES), &u32_key(new_best_number), std::ops::Deref::deref(&new_best_hash)); - context.db_transaction.write_u32(Some(COL_BLOCK_NUMBERS), std::ops::Deref::deref(&new_best_hash), new_best_number); + context.db_transaction.put(Some(COL_BLOCK_HASHES), &u32_key(new_best_number), &*new_best_hash); + context.db_transaction.write_u32(Some(COL_BLOCK_NUMBERS), &*new_best_hash, new_best_number); reorg.push_canonized(&new_best_hash); @@ -530,7 +526,7 @@ impl BlockStapler for Storage { }; // we always update best hash even if it is not changed - context.db_transaction.put(Some(COL_META), KEY_BEST_BLOCK_HASH, std::ops::Deref::deref(&new_best_hash)); + context.db_transaction.put(Some(COL_META), KEY_BEST_BLOCK_HASH, &*new_best_hash); // write accumulated transactions meta try!(context.apply(&self.database)); @@ -572,8 +568,7 @@ impl TransactionProvider for Storage { fn transaction(&self, hash: &H256) -> Option { self.transaction_bytes(hash).map(|tx_bytes| { - serialization::deserialize(tx_bytes.as_ref()) - .unwrap_or_else(|e| panic!("Failed to deserialize transaction: db corrupted? ({:?})", e)) + deserialize(tx_bytes.as_ref()).expect("Failed to deserialize transaction: db corrupted?") }) } } @@ -582,7 +577,7 @@ impl TransactionMetaProvider for Storage { fn transaction_meta(&self, hash: &H256) -> Option { self.get(COL_TRANSACTIONS_META, &**hash).map(|val| - TransactionMeta::from_bytes(&val).unwrap_or_else(|e| panic!("Invalid transaction metadata: db corrupted? ({:?})", e)) + TransactionMeta::from_bytes(&val).expect("Invalid transaction metadata: db corrupted?") ) } } @@ -1151,8 +1146,7 @@ mod tests { let inserted_chain = store.insert_block(&test_data::genesis()).unwrap(); - if let BlockInsertedChain::Main = inserted_chain { } - else { panic!("Genesis should become main chain"); } + assert_eq!(inserted_chain, BlockInsertedChain::Main, "Genesis should become main chain"); } #[test] @@ -1165,8 +1159,7 @@ mod tests { let inserted_chain = store.insert_block(&test_data::block_h1()).unwrap(); - if let BlockInsertedChain::Main = inserted_chain { } - else { panic!("h1 should become main chain"); } + assert_eq!(inserted_chain, BlockInsertedChain::Main, "h1 should become main chain"); } #[test] @@ -1192,8 +1185,7 @@ mod tests { let inserted_chain = store.insert_block(&block2_side).unwrap(); - if let BlockInsertedChain::Side = inserted_chain { } - else { panic!("h1 should become main chain"); } + assert_eq!(inserted_chain, BlockInsertedChain::Side, "h1 should become main chain"); } #[test] diff --git a/db/src/test_storage.rs b/db/src/test_storage.rs index cc871d78..4974a761 100644 --- a/db/src/test_storage.rs +++ b/db/src/test_storage.rs @@ -141,12 +141,11 @@ impl BlockStapler for TestStorage { // supports only main chain in test storage fn accepted_location(&self, header: &chain::BlockHeader) -> Option { - if self.best_block().is_none() { return Some(BlockLocation::Main(0)); } - - let best = self.best_block().unwrap(); - if best.hash == header.previous_header_hash { return Some(BlockLocation::Main(best.number + 1)); } - - None + match self.best_block() { + None => Some(BlockLocation::Main(0)), + Some(ref best) if best.hash == header.previous_header_hash => Some(BlockLocation::Main(best.number + 1)), + _ => None + } } } diff --git a/db/src/update_context.rs b/db/src/update_context.rs index e03e5625..7a2537f9 100644 --- a/db/src/update_context.rs +++ b/db/src/update_context.rs @@ -4,7 +4,6 @@ use std::collections::HashMap; use storage::COL_TRANSACTIONS_META; use primitives::hash::H256; use error::Error; -use std; pub struct UpdateContext { pub meta: HashMap, @@ -38,7 +37,7 @@ impl UpdateContext { } pub fn restore(&mut self) { - if let Some(meta_snapshot) = std::mem::replace(&mut self.meta_snapshot, None) { + if let Some(meta_snapshot) = self.meta_snapshot.take() { self.meta = meta_snapshot; self.db_transaction.rollback(); } From 12fbcf6735811c0d9faf4c6efb573041ff93f6ea Mon Sep 17 00:00:00 2001 From: debris Date: Fri, 18 Nov 2016 01:51:43 +0100 Subject: [PATCH 5/6] fatal db-error --- db/src/storage.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/db/src/storage.rs b/db/src/storage.rs index b8a7f80c..765fad89 100644 --- a/db/src/storage.rs +++ b/db/src/storage.rs @@ -112,10 +112,7 @@ impl Storage { /// get the value of the key in the database fn get(&self, col: u32, key: &[u8]) -> Option { - match self.database.get(Some(col), key) { - Ok(val) => val, - Err(msg) => panic!("{}", msg), - } + self.database.get(Some(col), key).expect("fatal db error") } /// resolves hash for the block reference (which can be referenced by number or From 6c1a08b5e80a1a4db62bdb3536b44aa59400ce3d Mon Sep 17 00:00:00 2001 From: Svyatoslav Nikolsky Date: Fri, 18 Nov 2016 17:10:57 +0300 Subject: [PATCH 6/6] fixed style --- sync/src/local_node.rs | 2 +- sync/src/synchronization_client.rs | 7 ++----- sync/src/synchronization_verifier.rs | 8 +------- 3 files changed, 4 insertions(+), 13 deletions(-) diff --git a/sync/src/local_node.rs b/sync/src/local_node.rs index 3d3f4f7d..5337e091 100644 --- a/sync/src/local_node.rs +++ b/sync/src/local_node.rs @@ -282,7 +282,7 @@ mod tests { let server = Arc::new(DummyServer::new()); let config = Config { threads_num: 1 }; let client_core = SynchronizationClientCore::new(config, &handle, executor.clone(), chain.clone()); - let mut verifier = DummyVerifier::new(); + let mut verifier = DummyVerifier::default(); verifier.set_sink(client_core.clone()); let client = SynchronizationClient::new(client_core, verifier); let local_node = LocalNode::new(server.clone(), client, executor.clone()); diff --git a/sync/src/synchronization_client.rs b/sync/src/synchronization_client.rs index 285410e4..5a37dd15 100644 --- a/sync/src/synchronization_client.rs +++ b/sync/src/synchronization_client.rs @@ -1100,10 +1100,7 @@ pub mod tests { let config = Config { threads_num: 1 }; let client_core = SynchronizationClientCore::new(config, &handle, executor.clone(), chain.clone()); - let mut verifier = match verifier { - Some(verifier) => verifier, - None => DummyVerifier::new(), - }; + let mut verifier = verifier.unwrap_or_default(); verifier.set_sink(client_core.clone()); let client = SynchronizationClient::new(client_core, verifier); (event_loop, handle, executor, chain, client) @@ -1768,7 +1765,7 @@ pub mod tests { let b23 = test_data::block_builder().header().parent(b22.hash()).build().build(); // TODO: simulate verification during b21 verification - let mut dummy_verifier = DummyVerifier::new(); + let mut dummy_verifier = DummyVerifier::default(); dummy_verifier.error_when_verifying(b21.hash(), "simulated"); let (_, _, _, _, sync) = create_sync(None, Some(dummy_verifier)); diff --git a/sync/src/synchronization_verifier.rs b/sync/src/synchronization_verifier.rs index e8d45565..d65b6670 100644 --- a/sync/src/synchronization_verifier.rs +++ b/sync/src/synchronization_verifier.rs @@ -157,19 +157,13 @@ pub mod tests { use primitives::hash::H256; use super::{Verifier, VerificationSink}; + #[derive(Default)] pub struct DummyVerifier { sink: Option>>>, errors: HashMap } impl DummyVerifier { - pub fn new() -> Self { - DummyVerifier { - sink: None, - errors: HashMap::new(), - } - } - pub fn set_sink(&mut self, sink: Arc>>) { self.sink = Some(sink); }