From 8b749367fd5fea897cee98bd892fff1ce90f8260 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Drwi=C4=99ga?= Date: Mon, 22 Jan 2018 23:44:59 +0100 Subject: [PATCH] AuRa fix for 1.7.x series (#7666) * Fix Temporarily Invalid blocks handling (#7613) * Handle temporarily invalid blocks in sync. * Fix tests. * Bump rustc-serialize * Bump version. * Update .gitlab-ci.yml fix lint * Remove slash from gitlab ci script to fix builds * Start build. --- .gitlab-ci.yml | 8 +- Cargo.lock | 50 ++++---- Cargo.toml | 2 +- ethcore/src/engines/authority_round/mod.rs | 114 +++++++++--------- ethcore/src/engines/validator_set/contract.rs | 2 +- parity/run.rs | 1 + sync/src/block_sync.rs | 4 + util/Cargo.toml | 2 +- 8 files changed, 94 insertions(+), 89 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 2ac768fa3..d64e6cf73 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -11,7 +11,7 @@ variables: CARGOFLAGS: "" CI_SERVER_NAME: "GitLab CI" cache: - key: "$CI_BUILD_STAGE/$CI_BUILD_REF_NAME" + key: "$CI_BUILD_STAGE-$CI_BUILD_REF_NAME" untracked: true linux-stable: stage: build @@ -95,7 +95,7 @@ linux-snap: paths: - scripts/parity_*_amd64.snap name: "stable-x86_64-unknown-snap-gnu_parity" - allow_failure: true + allow_failure: true linux-stable-debian: stage: build image: parity/rust-debian:gitlab-ci @@ -246,7 +246,7 @@ linux-i686: - strip target/$PLATFORM/release/parity - strip target/$PLATFORM/release/parity-evm - strip target/$PLATFORM/release/ethstore - - strip target/$PLATFORM/release/ethkey + - strip target/$PLATFORM/release/ethkey - strip target/$PLATFORM/release/parity - md5sum target/$PLATFORM/release/parity > parity.md5 - export SHA3=$(target/$PLATFORM/release/parity tools hash target/$PLATFORM/release/parity) @@ -488,7 +488,7 @@ darwin: name: "x86_64-apple-darwin_parity" windows: cache: - key: "%CI_BUILD_STAGE%/%CI_BUILD_REF_NAME%" + key: "%CI_BUILD_STAGE%-%CI_BUILD_REF_NAME%" untracked: true stage: build only: diff --git a/Cargo.lock b/Cargo.lock index da3cf6d24..89f4532b8 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -273,7 +273,7 @@ dependencies = [ name = "common-types" version = "0.1.0" dependencies = [ - "ethcore-util 1.7.12", + "ethcore-util 1.7.13", "ethjson 0.1.0", "rlp 0.2.0", "rustc-hex 1.0.0 (registry+https://github.com/rust-lang/crates.io-index)", @@ -472,7 +472,7 @@ dependencies = [ "ethcore-ipc-nano 1.7.0", "ethcore-logger 1.7.0", "ethcore-stratum 1.7.0", - "ethcore-util 1.7.12", + "ethcore-util 1.7.13", "ethjson 0.1.0", "ethkey 0.2.0", "ethstore 0.1.0", @@ -541,7 +541,7 @@ name = "ethcore-ipc" version = "1.7.0" dependencies = [ "ethcore-devtools 1.7.0", - "ethcore-util 1.7.12", + "ethcore-util 1.7.13", "nanomsg 0.5.1 (git+https://github.com/paritytech/nanomsg.rs.git?branch=parity-1.7)", "semver 0.6.0 (registry+https://github.com/rust-lang/crates.io-index)", ] @@ -590,7 +590,7 @@ dependencies = [ "ethcore-ipc 1.7.0", "ethcore-ipc-codegen 1.7.0", "ethcore-ipc-nano 1.7.0", - "ethcore-util 1.7.12", + "ethcore-util 1.7.13", "log 0.3.7 (registry+https://github.com/rust-lang/crates.io-index)", "nanomsg 0.5.1 (git+https://github.com/paritytech/nanomsg.rs.git?branch=parity-1.7)", "semver 0.6.0 (registry+https://github.com/rust-lang/crates.io-index)", @@ -607,7 +607,7 @@ dependencies = [ "ethcore-ipc 1.7.0", "ethcore-ipc-codegen 1.7.0", "ethcore-network 1.7.0", - "ethcore-util 1.7.12", + "ethcore-util 1.7.13", "evm 0.1.0", "futures 0.1.11 (registry+https://github.com/rust-lang/crates.io-index)", "itertools 0.5.9 (registry+https://github.com/rust-lang/crates.io-index)", @@ -646,7 +646,7 @@ dependencies = [ "ethcore-devtools 1.7.0", "ethcore-io 1.7.0", "ethcore-logger 1.7.0", - "ethcore-util 1.7.12", + "ethcore-util 1.7.13", "ethcrypto 0.1.0", "ethkey 0.2.0", "igd 0.6.0 (registry+https://github.com/rust-lang/crates.io-index)", @@ -677,7 +677,7 @@ dependencies = [ "ethcore-ipc-codegen 1.7.0", "ethcore-ipc-nano 1.7.0", "ethcore-logger 1.7.0", - "ethcore-util 1.7.12", + "ethcore-util 1.7.13", "ethcrypto 0.1.0", "ethkey 0.2.0", "futures 0.1.11 (registry+https://github.com/rust-lang/crates.io-index)", @@ -707,7 +707,7 @@ dependencies = [ "ethcore-ipc-codegen 1.7.0", "ethcore-ipc-nano 1.7.0", "ethcore-logger 1.7.0", - "ethcore-util 1.7.12", + "ethcore-util 1.7.13", "futures 0.1.11 (registry+https://github.com/rust-lang/crates.io-index)", "jsonrpc-core 7.0.0 (git+https://github.com/paritytech/jsonrpc.git?branch=parity-1.7)", "jsonrpc-macros 7.0.0 (git+https://github.com/paritytech/jsonrpc.git?branch=parity-1.7)", @@ -720,7 +720,7 @@ dependencies = [ [[package]] name = "ethcore-util" -version = "1.7.12" +version = "1.7.13" dependencies = [ "ansi_term 0.9.0 (registry+https://github.com/rust-lang/crates.io-index)", "clippy 0.0.103 (registry+https://github.com/rust-lang/crates.io-index)", @@ -770,7 +770,7 @@ name = "ethjson" version = "0.1.0" dependencies = [ "clippy 0.0.103 (registry+https://github.com/rust-lang/crates.io-index)", - "ethcore-util 1.7.12", + "ethcore-util 1.7.13", "rustc-hex 1.0.0 (registry+https://github.com/rust-lang/crates.io-index)", "serde 1.0.9 (registry+https://github.com/rust-lang/crates.io-index)", "serde_derive 1.0.9 (registry+https://github.com/rust-lang/crates.io-index)", @@ -851,7 +851,7 @@ dependencies = [ "ethcore-ipc-nano 1.7.0", "ethcore-light 1.7.0", "ethcore-network 1.7.0", - "ethcore-util 1.7.12", + "ethcore-util 1.7.13", "ethkey 0.2.0", "heapsize 0.4.0 (registry+https://github.com/rust-lang/crates.io-index)", "log 0.3.7 (registry+https://github.com/rust-lang/crates.io-index)", @@ -870,7 +870,7 @@ dependencies = [ "bit-set 0.4.0 (registry+https://github.com/rust-lang/crates.io-index)", "byteorder 1.0.0 (registry+https://github.com/rust-lang/crates.io-index)", "common-types 0.1.0", - "ethcore-util 1.7.12", + "ethcore-util 1.7.13", "ethjson 0.1.0", "evmjit 1.7.0", "lazy_static 0.2.8 (registry+https://github.com/rust-lang/crates.io-index)", @@ -887,7 +887,7 @@ version = "0.1.0" dependencies = [ "docopt 0.8.1 (registry+https://github.com/rust-lang/crates.io-index)", "ethcore 1.7.0", - "ethcore-util 1.7.12", + "ethcore-util 1.7.13", "ethjson 0.1.0", "evm 0.1.0", "rustc-hex 1.0.0 (registry+https://github.com/rust-lang/crates.io-index)", @@ -1132,7 +1132,7 @@ version = "1.7.0" dependencies = [ "ethcore-ipc 1.7.0", "ethcore-ipc-codegen 1.7.0", - "ethcore-util 1.7.12", + "ethcore-util 1.7.13", "semver 0.6.0 (registry+https://github.com/rust-lang/crates.io-index)", ] @@ -1526,7 +1526,7 @@ version = "0.1.0" dependencies = [ "byteorder 1.0.0 (registry+https://github.com/rust-lang/crates.io-index)", "ethabi 2.0.1 (registry+https://github.com/rust-lang/crates.io-index)", - "ethcore-util 1.7.12", + "ethcore-util 1.7.13", "futures 0.1.11 (registry+https://github.com/rust-lang/crates.io-index)", "native-contract-generator 0.1.0", ] @@ -1731,7 +1731,7 @@ dependencies = [ [[package]] name = "parity" -version = "1.7.12" +version = "1.7.13" dependencies = [ "ansi_term 0.9.0 (registry+https://github.com/rust-lang/crates.io-index)", "app_dirs 1.1.1 (registry+https://github.com/rust-lang/crates.io-index)", @@ -1751,7 +1751,7 @@ dependencies = [ "ethcore-logger 1.7.0", "ethcore-secretstore 1.0.0", "ethcore-stratum 1.7.0", - "ethcore-util 1.7.12", + "ethcore-util 1.7.13", "ethkey 0.2.0", "ethsync 1.7.0", "fdlimit 0.1.1 (registry+https://github.com/rust-lang/crates.io-index)", @@ -1797,7 +1797,7 @@ dependencies = [ "clippy 0.0.103 (registry+https://github.com/rust-lang/crates.io-index)", "env_logger 0.4.2 (registry+https://github.com/rust-lang/crates.io-index)", "ethcore-devtools 1.7.0", - "ethcore-util 1.7.12", + "ethcore-util 1.7.13", "fetch 0.1.0", "futures 0.1.11 (registry+https://github.com/rust-lang/crates.io-index)", "jsonrpc-core 7.0.0 (git+https://github.com/paritytech/jsonrpc.git?branch=parity-1.7)", @@ -1841,7 +1841,7 @@ name = "parity-hash-fetch" version = "1.7.0" dependencies = [ "ethabi 2.0.1 (registry+https://github.com/rust-lang/crates.io-index)", - "ethcore-util 1.7.12", + "ethcore-util 1.7.13", "fetch 0.1.0", "futures 0.1.11 (registry+https://github.com/rust-lang/crates.io-index)", "log 0.3.7 (registry+https://github.com/rust-lang/crates.io-index)", @@ -1859,7 +1859,7 @@ version = "1.7.0" dependencies = [ "cid 0.2.2 (registry+https://github.com/rust-lang/crates.io-index)", "ethcore 1.7.0", - "ethcore-util 1.7.12", + "ethcore-util 1.7.13", "jsonrpc-http-server 7.0.0 (git+https://github.com/paritytech/jsonrpc.git?branch=parity-1.7)", "mime 0.2.0 (registry+https://github.com/rust-lang/crates.io-index)", "multihash 0.6.0 (registry+https://github.com/rust-lang/crates.io-index)", @@ -1872,7 +1872,7 @@ version = "0.1.0" dependencies = [ "ethcore 1.7.0", "ethcore-io 1.7.0", - "ethcore-util 1.7.12", + "ethcore-util 1.7.13", "ethkey 0.2.0", "log 0.3.7 (registry+https://github.com/rust-lang/crates.io-index)", "rlp 0.2.0", @@ -1902,7 +1902,7 @@ dependencies = [ "ethcore-ipc 1.7.0", "ethcore-light 1.7.0", "ethcore-logger 1.7.0", - "ethcore-util 1.7.12", + "ethcore-util 1.7.13", "ethcrypto 0.1.0", "ethjson 0.1.0", "ethkey 0.2.0", @@ -1944,7 +1944,7 @@ dependencies = [ name = "parity-rpc-client" version = "1.4.0" dependencies = [ - "ethcore-util 1.7.12", + "ethcore-util 1.7.13", "futures 0.1.11 (registry+https://github.com/rust-lang/crates.io-index)", "jsonrpc-core 7.0.0 (git+https://github.com/paritytech/jsonrpc.git?branch=parity-1.7)", "jsonrpc-ws-server 7.0.0 (git+https://github.com/paritytech/jsonrpc.git?branch=parity-1.7)", @@ -2007,7 +2007,7 @@ dependencies = [ "ethcore 1.7.0", "ethcore-ipc 1.7.0", "ethcore-ipc-codegen 1.7.0", - "ethcore-util 1.7.12", + "ethcore-util 1.7.13", "ethsync 1.7.0", "futures 0.1.11 (registry+https://github.com/rust-lang/crates.io-index)", "ipc-common-types 1.7.0", @@ -2363,7 +2363,7 @@ name = "rpc-cli" version = "1.4.0" dependencies = [ "bigint 3.0.0 (registry+https://github.com/rust-lang/crates.io-index)", - "ethcore-util 1.7.12", + "ethcore-util 1.7.13", "futures 0.1.11 (registry+https://github.com/rust-lang/crates.io-index)", "parity-rpc 1.7.0", "parity-rpc-client 1.4.0", diff --git a/Cargo.toml b/Cargo.toml index f7a3e4112..a34670d68 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,7 +1,7 @@ [package] description = "Parity Ethereum client" name = "parity" -version = "1.7.12" +version = "1.7.13" license = "GPL-3.0" authors = ["Parity Technologies "] build = "build.rs" diff --git a/ethcore/src/engines/authority_round/mod.rs b/ethcore/src/engines/authority_round/mod.rs index 1405b3931..275dab7cb 100644 --- a/ethcore/src/engines/authority_round/mod.rs +++ b/ethcore/src/engines/authority_round/mod.rs @@ -301,9 +301,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> { @@ -328,7 +330,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); @@ -338,16 +340,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() } @@ -366,34 +358,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(()) } } @@ -694,15 +687,28 @@ impl Engine for AuthorityRound { fn verify_block_basic(&self, header: &Header, _block: Option<&[u8]>) -> Result<(), Error> { if header.seal().len() != self.seal_fields() { trace!(target: "engine", "verify_block_basic: wrong number of seal fields"); - Err(From::from(BlockError::InvalidSealArity( + return Err(From::from(BlockError::InvalidSealArity( Mismatch { expected: self.seal_fields(), found: header.seal().len() } - ))) - } else if header.number() >= self.validate_score_transition && *header.difficulty() >= U256::from(U128::max_value()) { - Err(From::from(BlockError::DifficultyOutOfBounds( + ))); + } + + if header.number() >= self.validate_score_transition && *header.difficulty() >= U256::from(U128::max_value()) { + 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(()), } } @@ -720,13 +726,15 @@ impl Engine for AuthorityRound { } 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()))?; } // Report skipped primaries. @@ -738,7 +746,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; } @@ -759,9 +767,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()) { @@ -779,19 +786,12 @@ impl Engine for AuthorityRound { } active_set = epoch_manager.validators().clone(); - (&active_set as &_, epoch_manager.epoch_transition_number) - }; - - 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> { @@ -1133,8 +1133,7 @@ mod tests { assert!(engine.verify_block_family(&header, &parent_header, None).is_ok()); assert!(engine.verify_block_external(&header, None).is_ok()); header.set_seal(vec![encode(&5usize).into_vec(), encode(&(&*signature as &[u8])).into_vec()]); - assert!(engine.verify_block_family(&header, &parent_header, None).is_ok()); - assert!(engine.verify_block_external(&header, None).is_err()); + assert!(engine.verify_block_basic(&header, None).is_err()); } #[test] @@ -1270,3 +1269,4 @@ mod tests { AuthorityRound::new(Default::default(), params, Default::default()).unwrap(); } } + diff --git a/ethcore/src/engines/validator_set/contract.rs b/ethcore/src/engines/validator_set/contract.rs index 8af9a5a7c..b3464443a 100644 --- a/ethcore/src/engines/validator_set/contract.rs +++ b/ethcore/src/engines/validator_set/contract.rs @@ -180,7 +180,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, None).is_err()); + assert!(client.engine().verify_block_basic(&header, None).is_err()); // Seal a block. client.engine().step(); assert_eq!(client.chain_info().best_block_number, 1); diff --git a/parity/run.rs b/parity/run.rs index ade328940..f8c941f7c 100644 --- a/parity/run.rs +++ b/parity/run.rs @@ -1,6 +1,7 @@ // Copyright 2015-2017 Parity Technologies (UK) Ltd. // This file is part of Parity. + // Parity is free software: you can redistribute it and/or modify // it under the terms of the GNU General Public License as published by // the Free Software Foundation, either version 3 of the License, or diff --git a/sync/src/block_sync.rs b/sync/src/block_sync.rs index b5e15fcf8..adcd243f1 100644 --- a/sync/src/block_sync.rs +++ b/sync/src/block_sync.rs @@ -519,6 +519,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/Cargo.toml b/util/Cargo.toml index 883bc542a..1d08df858 100644 --- a/util/Cargo.toml +++ b/util/Cargo.toml @@ -3,7 +3,7 @@ description = "Ethcore utility library" homepage = "http://parity.io" license = "GPL-3.0" name = "ethcore-util" -version = "1.7.12" +version = "1.7.13" authors = ["Parity Technologies "] build = "build.rs"