From ebe4890f83e7679ca2be4f01cfe95fd37c097de2 Mon Sep 17 00:00:00 2001 From: Niklas Adolfsson Date: Wed, 23 Oct 2019 09:44:13 +0200 Subject: [PATCH] [stable]: backport #10691 and #10683 (#11143) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Fix compiler warning (that will become an error) (#10683) * Remove annoying compiler warnings * Fix compiler warning (that will become an error) Fixes https://github.com/paritytech/parity-ethereum/issues/10648 I'm not sure this fix is Good™ but rustc seems happy enough. There's a deeper issue which may or may not be related to this: the Engine is not shutdown properly and the `StepService` thread keeps running indefinitely after Ctrl-C (so `update_sealing()` is called repeatedly for 300sec). I don't think this is related to Clique as I've seen this happen on mainnet as well, but I wonder if the effects of it are worse for a PoA network where the node can create new blocks all on its own? * Use saturating_sub * WIP * Fix warning, second attempt The idea here is to avoid using `Arc::get_mut()` (which does not work: fails every time here) and instead trust `drop()` to do the right thing. This is a conservative change. I think this can be reformed further, e.g. by `impl Drop for StepService` and halt the thread there, or even skip `join()`ing the thread entirely and trust the `AtomicBool` to signal shutdown. I also have doubts abut the `Option`: seems a bit much to have an `Option` there and it makes things cumbersome. * Refactor Clique stepping (#10691) * Use Drop to shutdown stepper thread Make period == 0 an error and remove the Option from step_service * Remove StepService Remove StepService and spawn the stepping thread in `Clique::new()`. Don't store the thread handle and instead trust the `AtomicBool` to signal shutdown time. Don't check for `period > 0`: we assume a valid chainspec file. * Don't shutdown the stepper thread at all, just let it run until exit Also: fix a few warnings and tests * Put kvdb_memorydb back * Warn&exit when engine is dropped Don't sleep too long! * Don't delay stepping thread * Better formatting --- ethcore/src/client/client.rs | 10 --- ethcore/src/engines/clique/mod.rs | 55 ++++++++-------- ethcore/src/engines/clique/step_service.rs | 77 ---------------------- ethcore/src/engines/mod.rs | 3 - 4 files changed, 29 insertions(+), 116 deletions(-) delete mode 100644 ethcore/src/engines/clique/step_service.rs diff --git a/ethcore/src/client/client.rs b/ethcore/src/client/client.rs index 12372b83ee1..a876159348b 100644 --- a/ethcore/src/client/client.rs +++ b/ethcore/src/client/client.rs @@ -2558,16 +2558,6 @@ impl ProvingBlockChainClient for Client { impl SnapshotClient for Client {} -impl Drop for Client { - fn drop(&mut self) { - if let Some(c) = Arc::get_mut(&mut self.engine) { - c.stop() - } else { - warn!(target: "shutdown", "unable to get mut ref for engine for shutdown."); - } - } -} - /// Returns `LocalizedReceipt` given `LocalizedTransaction` /// and a vector of receipts from given block up to transaction index. fn transaction_receipt( diff --git a/ethcore/src/engines/clique/mod.rs b/ethcore/src/engines/clique/mod.rs index 8be54da2663..133e66b968b 100644 --- a/ethcore/src/engines/clique/mod.rs +++ b/ethcore/src/engines/clique/mod.rs @@ -64,7 +64,7 @@ use std::collections::VecDeque; use std::sync::{Arc, Weak}; use std::thread; use std::time; -use std::time::{Duration, SystemTime, UNIX_EPOCH}; +use std::time::{Instant, Duration, SystemTime, UNIX_EPOCH}; use block::ExecutedBlock; use bytes::Bytes; @@ -88,11 +88,9 @@ use types::header::{ExtendedHeader, Header}; use self::block_state::CliqueBlockState; use self::params::CliqueParams; -use self::step_service::StepService; mod params; mod block_state; -mod step_service; mod util; // TODO(niklasad1): extract tester types into a separate mod to be shared in the code base @@ -167,7 +165,6 @@ pub struct Clique { block_state_by_hash: RwLock>, proposals: RwLock>, signer: RwLock>>, - step_service: Option>, } #[cfg(test)] @@ -180,30 +177,45 @@ pub struct Clique { pub block_state_by_hash: RwLock>, pub proposals: RwLock>, pub signer: RwLock>>, - pub step_service: Option>, } impl Clique { /// Initialize Clique engine from empty state. - pub fn new(our_params: CliqueParams, machine: EthereumMachine) -> Result, Error> { - let mut engine = Clique { - epoch_length: our_params.epoch, - period: our_params.period, + pub fn new(params: CliqueParams, machine: EthereumMachine) -> Result, Error> { + /// Step Clique at most every 2 seconds + const SEALING_FREQ: Duration = Duration::from_secs(2); + + let engine = Clique { + epoch_length: params.epoch, + period: params.period, client: Default::default(), block_state_by_hash: RwLock::new(LruCache::new(STATE_CACHE_NUM)), proposals: Default::default(), signer: Default::default(), machine, - step_service: None, }; + let engine = Arc::new(engine); + let weak_eng = Arc::downgrade(&engine); - let res = Arc::new(engine); - - if our_params.period > 0 { - engine.step_service = Some(StepService::start(Arc::downgrade(&res) as Weak>)); - } + thread::Builder::new().name("StepService".into()) + .spawn(move || { + loop { + let next_step_at = Instant::now() + SEALING_FREQ; + trace!(target: "miner", "StepService: triggering sealing"); + if let Some(eng) = weak_eng.upgrade() { + eng.step() + } else { + warn!(target: "shutdown", "StepService: engine is dropped; exiting."); + break; + } - Ok(res) + let now = Instant::now(); + if now < next_step_at { + thread::sleep(next_step_at - now); + } + } + })?; + Ok(engine) } #[cfg(test)] @@ -221,7 +233,6 @@ impl Clique { proposals: Default::default(), signer: Default::default(), machine: Spec::new_test_machine(), - step_service: None, } } @@ -695,7 +706,7 @@ impl Engine for Clique { trace!(target: "engine", "populate_from_parent in sealing"); // It's unclear how to prevent creating new blocks unless we are authorized, the best way (and geth does this too) - // it's just to ignore setting an correct difficulty here, we will check authorization in next step in generate_seal anyway. + // it's just to ignore setting a correct difficulty here, we will check authorization in next step in generate_seal anyway. if let Some(signer) = self.signer.read().as_ref() { let state = match self.state(&parent) { Err(e) => { @@ -744,14 +755,6 @@ impl Engine for Clique { } } - fn stop(&mut self) { - if let Some(mut s) = self.step_service.as_mut() { - Arc::get_mut(&mut s).map(|x| x.stop()); - } else { - warn!(target: "engine", "Stopping `CliqueStepService` failed requires mutable access"); - } - } - /// Clique timestamp is set to parent + period , or current time which ever is higher. fn open_block_header_timestamp(&self, parent_timestamp: u64) -> u64 { let now = time::SystemTime::now().duration_since(time::UNIX_EPOCH).unwrap_or_default(); diff --git a/ethcore/src/engines/clique/step_service.rs b/ethcore/src/engines/clique/step_service.rs deleted file mode 100644 index 7a4b5269d2b..00000000000 --- a/ethcore/src/engines/clique/step_service.rs +++ /dev/null @@ -1,77 +0,0 @@ -// Copyright 2015-2019 Parity Technologies (UK) Ltd. -// This file is part of Parity Ethereum. - -// Parity Ethereum 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 -// (at your option) any later version. - -// Parity Ethereum is distributed in the hope that it will be useful, -// but WITHOUT ANY WARRANTY; without even the implied warranty of -// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -// GNU General Public License for more details. - -// You should have received a copy of the GNU General Public License -// along with Parity Ethereum. If not, see . - - -use std::sync::Weak; -use std::sync::atomic::{AtomicBool, Ordering}; -use std::time::Duration; -use std::thread; -use std::sync::Arc; - -use engines::Engine; -use machine::Machine; - -/// Service that is managing the engine -pub struct StepService { - shutdown: Arc, - thread: Option>, -} - -impl StepService { - /// Start the `StepService` - pub fn start(engine: Weak>) -> Arc { - let shutdown = Arc::new(AtomicBool::new(false)); - let s = shutdown.clone(); - - let thread = thread::Builder::new() - .name("CliqueStepService".into()) - .spawn(move || { - // startup delay. - thread::sleep(Duration::from_secs(5)); - - loop { - // see if we are in shutdown. - if shutdown.load(Ordering::Acquire) { - trace!(target: "miner", "CliqueStepService: received shutdown signal!"); - break; - } - - trace!(target: "miner", "CliqueStepService: triggering sealing"); - - // Try sealing - engine.upgrade().map(|x| x.step()); - - // Yield - thread::sleep(Duration::from_millis(2000)); - } - trace!(target: "miner", "CliqueStepService: shutdown."); - }).expect("CliqueStepService thread failed"); - - Arc::new(StepService { - shutdown: s, - thread: Some(thread), - }) - } - - /// Stop the `StepService` - pub fn stop(&mut self) { - trace!(target: "miner", "CliqueStepService: shutting down."); - self.shutdown.store(true, Ordering::Release); - if let Some(t) = self.thread.take() { - t.join().expect("CliqueStepService thread panicked!"); - } - } -} diff --git a/ethcore/src/engines/mod.rs b/ethcore/src/engines/mod.rs index 5124f079db2..8b67c5d48a9 100644 --- a/ethcore/src/engines/mod.rs +++ b/ethcore/src/engines/mod.rs @@ -425,9 +425,6 @@ pub trait Engine: Sync + Send { /// Trigger next step of the consensus engine. fn step(&self) {} - /// Stops any services that the may hold the Engine and makes it safe to drop. - fn stop(&mut self) {} - /// Create a factory for building snapshot chunks and restoring from them. /// Returning `None` indicates that this engine doesn't support snapshot creation. fn snapshot_components(&self) -> Option> {