From 628f0878b167b46c487856cbd62339768f0a2a47 Mon Sep 17 00:00:00 2001 From: Marek Kotewicz Date: Tue, 23 Jan 2018 10:58:08 +0100 Subject: [PATCH] Backports to stable (#7661) * Update .gitlab-ci.yml fix cache:key * Fixed delegatecall's from/to (#7568) * Fixed delegatecall's from/to, closes #7166 * added tests for delegatecall traces, #7167 * Fix Temporarily Invalid blocks handling (#7613) * Handle temporarily invalid blocks in sync. * Fix tests. * Improve handling of RocksDB corruption (#7630) * kvdb-rocksdb: update rust-rocksdb version * kvdb-rocksdb: mark corruptions and attempt repair on db open * kvdb-rocksdb: better corruption detection on open * kvdb-rocksdb: add corruption_file_name const * kvdb-rocksdb: rename mark_corruption to check_for_corruption --- Cargo.lock | 5 +- ethcore/src/engines/authority_round/mod.rs | 112 +++++++++--------- ethcore/src/engines/validator_set/contract.rs | 2 +- ethcore/src/state/mod.rs | 14 +-- ethcore/src/trace/types/trace.rs | 24 ++-- sync/src/block_sync.rs | 4 + util/kvdb-rocksdb/src/lib.rs | 56 +++++++-- 7 files changed, 131 insertions(+), 86 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 196e56885..5b282fb63 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2810,7 +2810,7 @@ dependencies = [ [[package]] name = "rocksdb" version = "0.4.5" -source = "git+https://github.com/paritytech/rust-rocksdb#7adec2311d31387a832b0ef051472cdef906b480" +source = "git+https://github.com/paritytech/rust-rocksdb#ecf06adf3148ab10f6f7686b724498382ff4f36e" dependencies = [ "libc 0.2.31 (registry+https://github.com/rust-lang/crates.io-index)", "local-encoding 0.2.0 (registry+https://github.com/rust-lang/crates.io-index)", @@ -2820,10 +2820,11 @@ dependencies = [ [[package]] name = "rocksdb-sys" version = "0.3.0" -source = "git+https://github.com/paritytech/rust-rocksdb#7adec2311d31387a832b0ef051472cdef906b480" +source = "git+https://github.com/paritytech/rust-rocksdb#ecf06adf3148ab10f6f7686b724498382ff4f36e" dependencies = [ "cc 1.0.0 (registry+https://github.com/rust-lang/crates.io-index)", "libc 0.2.31 (registry+https://github.com/rust-lang/crates.io-index)", + "local-encoding 0.2.0 (registry+https://github.com/rust-lang/crates.io-index)", "snappy-sys 0.1.0 (git+https://github.com/paritytech/rust-snappy)", ] diff --git a/ethcore/src/engines/authority_round/mod.rs b/ethcore/src/engines/authority_round/mod.rs index 75df50e43..5f9bc9863 100644 --- a/ethcore/src/engines/authority_round/mod.rs +++ b/ethcore/src/engines/authority_round/mod.rs @@ -44,8 +44,7 @@ use bigint::hash::{H256, H520}; use semantic_version::SemanticVersion; use parking_lot::{Mutex, RwLock}; use unexpected::{Mismatch, OutOfBounds}; -use util::*; -use bytes::Bytes; +use util::Address; mod finality; @@ -291,9 +290,11 @@ struct EpochVerifier { impl super::EpochVerifier for EpochVerifier { fn verify_light(&self, header: &Header) -> Result<(), Error> { + // Validate the timestamp + verify_timestamp(&*self.step, header_step(header)?)?; // always check the seal since it's fast. // nothing heavier to do. - verify_external(header, &self.subchain_validators, &*self.step, |_| {}) + verify_external(header, &self.subchain_validators) } fn check_finality_proof(&self, proof: &[u8]) -> Option> { @@ -317,7 +318,7 @@ impl super::EpochVerifier for EpochVerifier { // // `verify_external` checks that signature is correct and author == signer. if header.seal().len() != 2 { return None } - otry!(verify_external(header, &self.subchain_validators, &*self.step, |_| {}).ok()); + otry!(verify_external(header, &self.subchain_validators).ok()); let newly_finalized = otry!(finality_checker.push_hash(header.hash(), header.author().clone()).ok()); finalized.extend(newly_finalized); @@ -327,16 +328,6 @@ impl super::EpochVerifier for EpochVerifier { } } -// Report misbehavior -#[derive(Debug)] -#[allow(dead_code)] -enum Report { - // Malicious behavior - Malicious(Address, BlockNumber, Bytes), - // benign misbehavior - Benign(Address, BlockNumber), -} - fn header_step(header: &Header) -> Result { UntrustedRlp::new(&header.seal().get(0).expect("was either checked with verify_block_basic or is genesis; has 2 fields; qed (Make sure the spec file has a correct genesis seal)")).as_val() } @@ -355,34 +346,35 @@ fn is_step_proposer(validators: &ValidatorSet, bh: &H256, step: usize, address: step_proposer(validators, bh, step) == *address } -fn verify_external(header: &Header, validators: &ValidatorSet, step: &Step, report: F) - -> Result<(), Error> -{ - let header_step = header_step(header)?; - +fn verify_timestamp(step: &Step, header_step: usize) -> Result<(), BlockError> { match step.check_future(header_step) { Err(None) => { - trace!(target: "engine", "verify_block_external: block from the future"); - report(Report::Benign(*header.author(), header.number())); - return Err(BlockError::InvalidSeal.into()) + trace!(target: "engine", "verify_timestamp: block from the future"); + Err(BlockError::InvalidSeal.into()) }, Err(Some(oob)) => { - trace!(target: "engine", "verify_block_external: block too early"); - return Err(BlockError::TemporarilyInvalid(oob).into()) + // NOTE This error might be returned only in early stage of verification (Stage 1). + // Returning it further won't recover the sync process. + trace!(target: "engine", "verify_timestamp: block too early"); + Err(BlockError::TemporarilyInvalid(oob).into()) }, - Ok(_) => { - let proposer_signature = header_signature(header)?; - let correct_proposer = validators.get(header.parent_hash(), header_step); - let is_invalid_proposer = *header.author() != correct_proposer || - !verify_address(&correct_proposer, &proposer_signature, &header.bare_hash())?; + Ok(_) => Ok(()), + } +} - if is_invalid_proposer { - trace!(target: "engine", "verify_block_external: bad proposer for step: {}", header_step); - Err(EngineError::NotProposer(Mismatch { expected: correct_proposer, found: header.author().clone() }))? - } else { - Ok(()) - } - } +fn verify_external(header: &Header, validators: &ValidatorSet) -> Result<(), Error> { + let header_step = header_step(header)?; + + let proposer_signature = header_signature(header)?; + let correct_proposer = validators.get(header.parent_hash(), header_step); + let is_invalid_proposer = *header.author() != correct_proposer || + !verify_address(&correct_proposer, &proposer_signature, &header.bare_hash())?; + + if is_invalid_proposer { + trace!(target: "engine", "verify_block_external: bad proposer for step: {}", header_step); + Err(EngineError::NotProposer(Mismatch { expected: correct_proposer, found: header.author().clone() }))? + } else { + Ok(()) } } @@ -655,26 +647,38 @@ impl Engine for AuthorityRound { /// Check the number of seal fields. fn verify_block_basic(&self, header: &Header) -> Result<(), Error> { if header.number() >= self.validate_score_transition && *header.difficulty() >= U256::from(U128::max_value()) { - Err(From::from(BlockError::DifficultyOutOfBounds( + return Err(From::from(BlockError::DifficultyOutOfBounds( OutOfBounds { min: None, max: Some(U256::from(U128::max_value())), found: *header.difficulty() } - ))) - } else { - Ok(()) + ))); + } + + // TODO [ToDr] Should this go from epoch manager? + // If yes then probably benign reporting needs to be moved further in the verification. + let set_number = header.number(); + + match verify_timestamp(&*self.step, header_step(header)?) { + Err(BlockError::InvalidSeal) => { + self.validators.report_benign(header.author(), set_number, header.number()); + Err(BlockError::InvalidSeal.into()) + } + Err(e) => Err(e.into()), + Ok(()) => Ok(()), } } /// Do the step and gas limit validation. fn verify_block_family(&self, header: &Header, parent: &Header) -> Result<(), Error> { let step = header_step(header)?; - let parent_step = header_step(parent)?; + // TODO [ToDr] Should this go from epoch manager? + let set_number = header.number(); // Ensure header is from the step after parent. if step == parent_step || (header.number() >= self.validate_step_transition && step <= parent_step) { trace!(target: "engine", "Multiple blocks proposed for step {}.", parent_step); - self.validators.report_malicious(header.author(), header.number(), header.number(), Default::default()); + self.validators.report_malicious(header.author(), set_number, header.number(), Default::default()); Err(EngineError::DoubleVote(header.author().clone()))?; } @@ -687,7 +691,7 @@ impl Engine for AuthorityRound { let skipped_primary = step_proposer(&*self.validators, &parent.hash(), s); // Do not report this signer. if skipped_primary != me { - self.validators.report_benign(&skipped_primary, header.number(), header.number()); + self.validators.report_benign(&skipped_primary, set_number, header.number()); } // Stop reporting once validators start repeating. if !reported.insert(skipped_primary) { break; } @@ -702,9 +706,8 @@ impl Engine for AuthorityRound { // fetch correct validator set for current epoch, taking into account // finality of previous transitions. let active_set; - - let (validators, set_number) = if self.immediate_transitions { - (&*self.validators, header.number()) + let validators = if self.immediate_transitions { + &*self.validators } else { // get correct validator set for epoch. let client = match self.client.read().as_ref().and_then(|weak| weak.upgrade()) { @@ -722,21 +725,12 @@ impl Engine for AuthorityRound { } active_set = epoch_manager.validators().clone(); - (&active_set as &_, epoch_manager.epoch_transition_number) - }; - - // always report with "self.validators" so that the report actually gets - // to the contract. - let report = |report| match report { - Report::Benign(address, block_number) => - self.validators.report_benign(&address, set_number, block_number), - Report::Malicious(address, block_number, proof) => - self.validators.report_malicious(&address, set_number, block_number, proof), + &active_set as &_ }; // verify signature against fixed list, but reports should go to the // contract itself. - verify_external(header, validators, &*self.step, report) + verify_external(header, validators) } fn genesis_epoch_data(&self, header: &Header, call: &Call) -> Result, String> { @@ -1059,8 +1053,7 @@ mod tests { assert!(engine.verify_block_family(&header, &parent_header).is_ok()); assert!(engine.verify_block_external(&header).is_ok()); header.set_seal(vec![encode(&5usize).into_vec(), encode(&(&*signature as &[u8])).into_vec()]); - assert!(engine.verify_block_family(&header, &parent_header).is_ok()); - assert!(engine.verify_block_external(&header).is_err()); + assert!(engine.verify_block_basic(&header).is_err()); } #[test] @@ -1200,3 +1193,4 @@ mod tests { AuthorityRound::new(params, machine).unwrap(); } } + diff --git a/ethcore/src/engines/validator_set/contract.rs b/ethcore/src/engines/validator_set/contract.rs index 05b85eba4..35251b47b 100644 --- a/ethcore/src/engines/validator_set/contract.rs +++ b/ethcore/src/engines/validator_set/contract.rs @@ -192,7 +192,7 @@ mod tests { header.set_number(2); header.set_parent_hash(client.chain_info().best_block_hash); // `reportBenign` when the designated proposer releases block from the future (bad clock). - assert!(client.engine().verify_block_external(&header).is_err()); + assert!(client.engine().verify_block_basic(&header).is_err()); // Seal a block. client.engine().step(); assert_eq!(client.chain_info().best_block_number, 1); diff --git a/ethcore/src/state/mod.rs b/ethcore/src/state/mod.rs index 96fd46ec5..f5a52eea5 100644 --- a/ethcore/src/state/mod.rs +++ b/ethcore/src/state/mod.rs @@ -1409,7 +1409,7 @@ mod tests { } #[test] - fn should_not_trace_delegatecall() { + fn should_trace_delegatecall_properly() { init_log(); let mut state = get_temp_state(); @@ -1429,7 +1429,7 @@ mod tests { }.sign(&secret(), None); state.init_code(&0xa.into(), FromHex::from_hex("6000600060006000600b618000f4").unwrap()).unwrap(); - state.init_code(&0xb.into(), FromHex::from_hex("6000").unwrap()).unwrap(); + state.init_code(&0xb.into(), FromHex::from_hex("60056000526001601ff3").unwrap()).unwrap(); let result = state.apply(&info, &machine, &t, true).unwrap(); let expected_trace = vec![FlatTrace { @@ -1444,23 +1444,23 @@ mod tests { call_type: CallType::Call, }), result: trace::Res::Call(trace::CallResult { - gas_used: U256::from(721), // in post-eip150 + gas_used: U256::from(736), // in post-eip150 output: vec![] }), }, FlatTrace { trace_address: vec![0].into_iter().collect(), subtraces: 0, action: trace::Action::Call(trace::Call { - from: "9cce34f7ab185c7aba1b7c8140d620b4bda941d6".into(), - to: 0xa.into(), + from: 0xa.into(), + to: 0xb.into(), value: 0.into(), gas: 32768.into(), input: vec![], call_type: CallType::DelegateCall, }), result: trace::Res::Call(trace::CallResult { - gas_used: 3.into(), - output: vec![], + gas_used: 18.into(), + output: vec![5], }), }]; diff --git a/ethcore/src/trace/types/trace.rs b/ethcore/src/trace/types/trace.rs index 8c48e6a4a..3a20a6745 100644 --- a/ethcore/src/trace/types/trace.rs +++ b/ethcore/src/trace/types/trace.rs @@ -77,13 +77,23 @@ pub struct Call { impl From for Call { fn from(p: ActionParams) -> Self { - Call { - from: p.sender, - to: p.address, - value: p.value.value(), - gas: p.gas, - input: p.data.unwrap_or_else(Vec::new), - call_type: p.call_type, + match p.call_type { + CallType::DelegateCall => Call { + from: p.address, + to: p.code_address, + value: p.value.value(), + gas: p.gas, + input: p.data.unwrap_or_else(Vec::new), + call_type: p.call_type, + }, + _ => Call { + from: p.sender, + to: p.address, + value: p.value.value(), + gas: p.gas, + input: p.data.unwrap_or_else(Vec::new), + call_type: p.call_type, + }, } } } diff --git a/sync/src/block_sync.rs b/sync/src/block_sync.rs index fbd7eddae..611c71547 100644 --- a/sync/src/block_sync.rs +++ b/sync/src/block_sync.rs @@ -522,6 +522,10 @@ impl BlockDownloader { trace!(target: "sync", "Unknown new block parent, restarting sync"); break; }, + Err(BlockImportError::Block(BlockError::TemporarilyInvalid(_))) => { + debug!(target: "sync", "Block temporarily invalid, restarting sync"); + break; + }, Err(e) => { debug!(target: "sync", "Bad block {:?} : {:?}", h, e); bad = true; diff --git a/util/kvdb-rocksdb/src/lib.rs b/util/kvdb-rocksdb/src/lib.rs index 901ed9f80..4dcee3a1a 100644 --- a/util/kvdb-rocksdb/src/lib.rs +++ b/util/kvdb-rocksdb/src/lib.rs @@ -32,7 +32,7 @@ use std::cmp; use std::collections::HashMap; use std::marker::PhantomData; use std::path::{PathBuf, Path}; -use std::{mem, fs, io}; +use std::{fs, io, mem, result}; use parking_lot::{Mutex, MutexGuard, RwLock}; use rocksdb::{ @@ -256,7 +256,25 @@ pub struct Database { flushing_lock: Mutex, } +#[inline] +fn check_for_corruption>(path: P, res: result::Result) -> result::Result { + if let Err(ref s) = res { + if s.starts_with("Corruption:") { + warn!("DB corrupted: {}. Repair will be triggered on next restart", s); + let _ = fs::File::create(path.as_ref().join(Database::CORRUPTION_FILE_NAME)); + } + } + + res +} + +fn is_corrupted(s: &str) -> bool { + s.starts_with("Corruption:") || s.starts_with("Invalid argument: You have to open all column families") +} + impl Database { + const CORRUPTION_FILE_NAME: &'static str = "CORRUPTED"; + /// Open database with default settings. pub fn open_default(path: &str) -> Result { Database::open(&DatabaseConfig::default(), path) @@ -286,6 +304,14 @@ impl Database { block_opts.set_cache(cache); } + // attempt database repair if it has been previously marked as corrupted + let db_corrupted = Path::new(path).join(Database::CORRUPTION_FILE_NAME); + if db_corrupted.exists() { + warn!("DB has been previously marked as corrupted, attempting repair"); + DB::repair(&opts, path)?; + fs::remove_file(db_corrupted).map_err(|e| e.to_string())?; + } + let columns = config.columns.unwrap_or(0) as usize; let mut cf_options = Vec::with_capacity(columns); @@ -305,12 +331,11 @@ impl Database { let mut cfs: Vec = Vec::new(); let db = match config.columns { - Some(columns) => { + Some(_) => { match DB::open_cf(&opts, path, &cfnames, &cf_options) { Ok(db) => { cfs = cfnames.iter().map(|n| db.cf_handle(n) .expect("rocksdb opens a cf_handle for each cfname; qed")).collect(); - assert!(cfs.len() == columns as usize); Ok(db) } Err(_) => { @@ -320,7 +345,7 @@ impl Database { cfs = cfnames.iter().enumerate().map(|(i, n)| db.create_cf(n, &cf_options[i])).collect::>()?; Ok(db) }, - err @ Err(_) => err, + err => err, } } } @@ -330,14 +355,18 @@ impl Database { let db = match db { Ok(db) => db, - Err(ref s) if s.starts_with("Corruption:") => { - info!("{}", s); - info!("Attempting DB repair for {}", path); + Err(ref s) if is_corrupted(s) => { + warn!("DB corrupted: {}, attempting repair", s); DB::repair(&opts, path)?; match cfnames.is_empty() { true => DB::open(&opts, path)?, - false => DB::open_cf(&opts, path, &cfnames, &cf_options)? + false => { + let db = DB::open_cf(&opts, path, &cfnames, &cf_options)?; + cfs = cfnames.iter().map(|n| db.cf_handle(n) + .expect("rocksdb opens a cf_handle for each cfname; qed")).collect(); + db + }, } }, Err(s) => { return Err(s); } @@ -424,7 +453,11 @@ impl Database { } } } - db.write_opt(batch, &self.write_opts)?; + + check_for_corruption( + &self.path, + db.write_opt(batch, &self.write_opts))?; + for column in self.flushing.write().iter_mut() { column.clear(); column.shrink_to_fit(); @@ -470,7 +503,10 @@ impl Database { }, } } - db.write_opt(batch, &self.write_opts) + + check_for_corruption( + &self.path, + db.write_opt(batch, &self.write_opts)).map_err(Into::into) }, None => Err("Database is closed".to_owned()) }