From bf242552f3afb692baa6b7c08433f143dd8eefc0 Mon Sep 17 00:00:00 2001 From: Andreas Fackler Date: Fri, 24 May 2019 10:30:31 +0200 Subject: [PATCH] Add SealingState; don't prepare block when not ready. (#10529) --- ethcore/src/client/client.rs | 4 +- ethcore/src/engines/authority_round/mod.rs | 74 ++++++++++++++++++---- ethcore/src/engines/basic_authority.rs | 18 ++++-- ethcore/src/engines/clique/mod.rs | 10 +-- ethcore/src/engines/instant_seal.rs | 4 +- ethcore/src/engines/mod.rs | 17 +++-- ethcore/src/miner/miner.rs | 38 ++++++----- 7 files changed, 117 insertions(+), 48 deletions(-) diff --git a/ethcore/src/client/client.rs b/ethcore/src/client/client.rs index fa3db14d2..02f1f2f82 100644 --- a/ethcore/src/client/client.rs +++ b/ethcore/src/client/client.rs @@ -60,7 +60,7 @@ use client::{ IoClient, BadBlocks, }; use client::bad_blocks; -use engines::{MAX_UNCLE_AGE, EthEngine, EpochTransition, ForkChoice, EngineError}; +use engines::{MAX_UNCLE_AGE, EthEngine, EpochTransition, ForkChoice, EngineError, SealingState}; use engines::epoch::PendingTransition; use error::{ ImportError, ExecutionError, CallError, BlockError, @@ -2431,7 +2431,7 @@ impl ImportSealedBlock for Client { &[], route.enacted(), route.retracted(), - self.engine.seals_internally().is_some(), + self.engine.sealing_state() != SealingState::External, ); self.notify(|notify| { notify.new_blocks( diff --git a/ethcore/src/engines/authority_round/mod.rs b/ethcore/src/engines/authority_round/mod.rs index 55c09c460..8ac23851b 100644 --- a/ethcore/src/engines/authority_round/mod.rs +++ b/ethcore/src/engines/authority_round/mod.rs @@ -26,7 +26,7 @@ use std::time::{UNIX_EPOCH, SystemTime, Duration}; use block::*; use client::EngineClient; -use engines::{Engine, Seal, EngineError, ConstructedVerifier}; +use engines::{Engine, Seal, SealingState, EngineError, ConstructedVerifier}; use engines::block_reward; use engines::block_reward::{BlockRewardContract, RewardKind}; use error::{Error, BlockError}; @@ -219,9 +219,15 @@ impl EpochManager { } } - // zoom to epoch for given header. returns true if succeeded, false otherwise. - fn zoom_to(&mut self, client: &EngineClient, machine: &EthereumMachine, validators: &ValidatorSet, header: &Header) -> bool { - let last_was_parent = self.finality_checker.subchain_head() == Some(*header.parent_hash()); + // Zooms to the epoch after the header with the given hash. Returns true if succeeded, false otherwise. + fn zoom_to_after( + &mut self, + client: &EngineClient, + machine: &EthereumMachine, + validators: &ValidatorSet, + hash: H256 + ) -> bool { + let last_was_parent = self.finality_checker.subchain_head() == Some(hash); // early exit for current target == chain head, but only if the epochs are // the same. @@ -230,13 +236,13 @@ impl EpochManager { } self.force = false; - debug!(target: "engine", "Zooming to epoch for block {}", header.hash()); + debug!(target: "engine", "Zooming to epoch after block {}", hash); // epoch_transition_for can be an expensive call, but in the absence of // forks it will only need to be called for the block directly after // epoch transition, in which case it will be O(1) and require a single // DB lookup. - let last_transition = match client.epoch_transition_for(*header.parent_hash()) { + let last_transition = match client.epoch_transition_for(hash) { Some(t) => t, None => { // this really should never happen unless the block passed @@ -723,7 +729,7 @@ impl AuthorityRound { } }; - if !epoch_manager.zoom_to(&*client, &self.machine, &*self.validators, header) { + if !epoch_manager.zoom_to_after(&*client, &self.machine, &*self.validators, *header.parent_hash()) { debug!(target: "engine", "Unable to zoom to epoch."); return Err(EngineError::RequiresClient.into()) } @@ -831,7 +837,7 @@ impl AuthorityRound { }; let mut epoch_manager = self.epoch_manager.lock(); - if !epoch_manager.zoom_to(&*client, &self.machine, &*self.validators, chain_head) { + if !epoch_manager.zoom_to_after(&*client, &self.machine, &*self.validators, *chain_head.parent_hash()) { return Vec::new(); } @@ -1002,9 +1008,51 @@ impl Engine for AuthorityRound { header.set_difficulty(score); } - fn seals_internally(&self) -> Option { - // TODO: accept a `&Call` here so we can query the validator set. - Some(self.signer.read().is_some()) + fn sealing_state(&self) -> SealingState { + let our_addr = match *self.signer.read() { + Some(ref signer) => signer.address(), + None => { + warn!(target: "engine", "Not preparing block; cannot sign."); + return SealingState::NotReady; + } + }; + + let client = match self.client.read().as_ref().and_then(|weak| weak.upgrade()) { + Some(client) => client, + None => { + warn!(target: "engine", "Not preparing block: missing client ref."); + return SealingState::NotReady; + } + }; + + let parent = match client.as_full_client() { + Some(full_client) => full_client.best_block_header(), + None => { + debug!(target: "engine", "Not preparing block: not a full client."); + return SealingState::NotReady; + }, + }; + + let validators = if self.immediate_transitions { + CowLike::Borrowed(&*self.validators) + } else { + let mut epoch_manager = self.epoch_manager.lock(); + if !epoch_manager.zoom_to_after(&*client, &self.machine, &*self.validators, parent.hash()) { + debug!(target: "engine", "Not preparing block: Unable to zoom to epoch."); + return SealingState::NotReady; + } + CowLike::Owned(epoch_manager.validators().clone()) + }; + + let step = self.step.inner.load(); + + if !is_step_proposer(&*validators, &parent.hash(), step, &our_addr) { + trace!(target: "engine", "Not preparing block: not a proposer for step {}. (Our address: {})", + step, our_addr); + return SealingState::NotReady; + } + + SealingState::Ready } fn handle_message(&self, rlp: &[u8]) -> Result<(), EngineError> { @@ -1013,7 +1061,7 @@ impl Engine for AuthorityRound { } let rlp = Rlp::new(rlp); - let empty_step: EmptyStep = rlp.as_val().map_err(fmt_err)?;; + let empty_step: EmptyStep = rlp.as_val().map_err(fmt_err)?; if empty_step.verify(&*self.validators).unwrap_or(false) { if self.step.inner.check_future(empty_step.step).is_ok() { @@ -1388,7 +1436,7 @@ impl Engine for AuthorityRound { }; let mut epoch_manager = self.epoch_manager.lock(); - if !epoch_manager.zoom_to(&*client, &self.machine, &*self.validators, chain_head) { + if !epoch_manager.zoom_to_after(&*client, &self.machine, &*self.validators, *chain_head.parent_hash()) { return None; } diff --git a/ethcore/src/engines/basic_authority.rs b/ethcore/src/engines/basic_authority.rs index 69b8d07c5..0dd7d8e85 100644 --- a/ethcore/src/engines/basic_authority.rs +++ b/ethcore/src/engines/basic_authority.rs @@ -21,7 +21,7 @@ use ethereum_types::{H256, H520}; use parking_lot::RwLock; use ethkey::{self, Signature}; use block::*; -use engines::{Engine, Seal, ConstructedVerifier, EngineError}; +use engines::{Engine, Seal, SealingState, ConstructedVerifier, EngineError}; use engines::signer::EngineSigner; use error::{BlockError, Error}; use ethjson; @@ -98,8 +98,12 @@ impl Engine for BasicAuthority { // One field - the signature fn seal_fields(&self, _header: &Header) -> usize { 1 } - fn seals_internally(&self) -> Option { - Some(self.signer.read().is_some()) + fn sealing_state(&self) -> SealingState { + if self.signer.read().is_some() { + SealingState::Ready + } else { + SealingState::NotReady + } } /// Attempt to seal the block internally. @@ -220,7 +224,7 @@ mod tests { use accounts::AccountProvider; use types::header::Header; use spec::Spec; - use engines::Seal; + use engines::{Seal, SealingState}; use tempdir::TempDir; /// Create a new test chain spec with `BasicAuthority` consensus engine. @@ -272,13 +276,13 @@ mod tests { } #[test] - fn seals_internally() { + fn sealing_state() { let tap = AccountProvider::transient_provider(); let authority = tap.insert_account(keccak("").into(), &"".into()).unwrap(); let engine = new_test_authority().engine; - assert!(!engine.seals_internally().unwrap()); + assert_eq!(SealingState::NotReady, engine.sealing_state()); engine.set_signer(Box::new((Arc::new(tap), authority, "".into()))); - assert!(engine.seals_internally().unwrap()); + assert_eq!(SealingState::Ready, engine.sealing_state()); } } diff --git a/ethcore/src/engines/clique/mod.rs b/ethcore/src/engines/clique/mod.rs index fff6cb3ab..7d771068a 100644 --- a/ethcore/src/engines/clique/mod.rs +++ b/ethcore/src/engines/clique/mod.rs @@ -38,8 +38,8 @@ /// /// 1. Set a signer using `Engine::set_signer()`. If a miner account was set up through /// a config file or CLI flag `MinerService::set_author()` will eventually set the signer -/// 2. We check that the engine seals internally through `Clique::seals_internally()` -/// Note: This is always true for Clique +/// 2. We check that the engine is ready for sealing through `Clique::sealing_state()` +/// Note: This is always `SealingState::Ready` for Clique /// 3. Calling `Clique::new()` will spawn a `StepService` thread. This thread will call `Engine::step()` /// periodically. Internally, the Clique `step()` function calls `Client::update_sealing()`, which is /// what makes and seals a block. @@ -69,7 +69,7 @@ use std::time::{Duration, SystemTime, UNIX_EPOCH}; use block::ExecutedBlock; use client::{BlockId, EngineClient}; use engines::clique::util::{extract_signers, recover_creator}; -use engines::{Engine, EngineError, Seal}; +use engines::{Engine, EngineError, Seal, SealingState}; use error::{BlockError, Error}; use ethereum_types::{Address, H64, H160, H256, U256}; use ethkey::Signature; @@ -459,8 +459,8 @@ impl Engine for Clique { } /// Clique doesn't require external work to seal, so we always return true here. - fn seals_internally(&self) -> Option { - Some(true) + fn sealing_state(&self) -> SealingState { + SealingState::Ready } /// Returns if we are ready to seal, the real sealing (signing extra_data) is actually done in `on_seal_block()`. diff --git a/ethcore/src/engines/instant_seal.rs b/ethcore/src/engines/instant_seal.rs index a578c990b..71ef193c6 100644 --- a/ethcore/src/engines/instant_seal.rs +++ b/ethcore/src/engines/instant_seal.rs @@ -14,7 +14,7 @@ // You should have received a copy of the GNU General Public License // along with Parity Ethereum. If not, see . -use engines::{Engine, Seal}; +use engines::{Engine, Seal, SealingState}; use machine::Machine; use types::header::{Header, ExtendedHeader}; use block::ExecutedBlock; @@ -57,7 +57,7 @@ impl Engine for InstantSeal { fn machine(&self) -> &M { &self.machine } - fn seals_internally(&self) -> Option { Some(true) } + fn sealing_state(&self) -> SealingState { SealingState::Ready } fn generate_seal(&self, block: &ExecutedBlock, _parent: &Header) -> Seal { if block.transactions.is_empty() { diff --git a/ethcore/src/engines/mod.rs b/ethcore/src/engines/mod.rs index 5124f079d..e80a858b3 100644 --- a/ethcore/src/engines/mod.rs +++ b/ethcore/src/engines/mod.rs @@ -160,6 +160,17 @@ pub enum Seal { None, } +/// The type of sealing the engine is currently able to perform. +#[derive(Debug, PartialEq, Eq)] +pub enum SealingState { + /// The engine is ready to seal a block. + Ready, + /// The engine can't seal at the moment, and no block should be prepared and queued. + NotReady, + /// The engine does not seal internally. + External, +} + /// A system-calling closure. Enacts calls on a block's state from the system address. pub type SystemCall<'a> = FnMut(Address, Vec) -> Result, String> + 'a; @@ -302,10 +313,8 @@ pub trait Engine: Sync + Send { /// Allow mutating the header during seal generation. Currently only used by Clique. fn on_seal_block(&self, _block: &mut ExecutedBlock) -> Result<(), Error> { Ok(()) } - /// None means that it requires external input (e.g. PoW) to seal a block. - /// Some(true) means the engine is currently prime for seal generation (i.e. node is the current validator). - /// Some(false) means that the node might seal internally but is not qualified now. - fn seals_internally(&self) -> Option { None } + /// Returns the engine's current sealing state. + fn sealing_state(&self) -> SealingState { SealingState::External } /// Attempt to seal the block internally. /// diff --git a/ethcore/src/miner/miner.rs b/ethcore/src/miner/miner.rs index ec269c03e..070cf6fe9 100644 --- a/ethcore/src/miner/miner.rs +++ b/ethcore/src/miner/miner.rs @@ -53,7 +53,7 @@ use client::{ BlockChain, ChainInfo, BlockProducer, SealedBlockImporter, Nonce, TransactionInfo, TransactionId }; use client::{BlockId, ClientIoMessage}; -use engines::{EthEngine, Seal, EngineSigner}; +use engines::{EthEngine, Seal, SealingState, EngineSigner}; use error::Error; use executed::ExecutionError; use executive::contract_address; @@ -288,7 +288,7 @@ impl Miner { sealing: Mutex::new(SealingWork { queue: UsingQueue::new(options.work_queue_size), enabled: options.force_sealing - || spec.engine.seals_internally().is_some(), + || spec.engine.sealing_state() != SealingState::External, next_allowed_reseal: Instant::now(), next_mandatory_reseal: Instant::now() + options.reseal_max_period, last_request: None, @@ -625,7 +625,7 @@ impl Miner { // keep sealing enabled if any of the conditions is met let sealing_enabled = self.forced_sealing() || self.transaction_queue.has_local_pending_transactions() - || self.engine.seals_internally() == Some(true) + || self.engine.sealing_state() == SealingState::Ready || had_requests; let should_disable_sealing = !sealing_enabled; @@ -634,7 +634,7 @@ impl Miner { should_disable_sealing, self.forced_sealing(), self.transaction_queue.has_local_pending_transactions(), - self.engine.seals_internally(), + self.engine.sealing_state(), had_requests, ); @@ -806,6 +806,11 @@ impl Miner { } }; + if self.engine.sealing_state() != SealingState::External { + trace!(target: "miner", "prepare_pending_block: engine not sealing externally; not preparing"); + return BlockPreparationStatus::NotPrepared; + } + let preparation_status = if prepare_new { // -------------------------------------------------------------------------- // | NOTE Code below requires sealing locks. | @@ -842,7 +847,9 @@ impl Miner { // Make sure to do it after transaction is imported and lock is dropped. // We need to create pending block and enable sealing. - if self.engine.seals_internally().unwrap_or(false) || self.prepare_pending_block(chain) == BlockPreparationStatus::NotPrepared { + let sealing_state = self.engine.sealing_state(); + if sealing_state == SealingState::Ready + || self.prepare_pending_block(chain) == BlockPreparationStatus::NotPrepared { // If new block has not been prepared (means we already had one) // or Engine might be able to seal internally, // we need to update sealing. @@ -872,7 +879,7 @@ impl miner::MinerService for Miner { self.params.write().author = author.address(); if let Author::Sealer(signer) = author { - if self.engine.seals_internally().is_some() { + if self.engine.sealing_state() != SealingState::External { // Enable sealing self.sealing.lock().enabled = true; // -------------------------------------------------------------------------- @@ -1150,6 +1157,11 @@ impl miner::MinerService for Miner { return; } + let sealing_state = self.engine.sealing_state(); + if sealing_state == SealingState::NotReady { + return; + } + // -------------------------------------------------------------------------- // | NOTE Code below requires sealing locks. | // | Make sure to release the locks before calling that method. | @@ -1170,19 +1182,15 @@ impl miner::MinerService for Miner { } } - match self.engine.seals_internally() { - Some(true) => { + match sealing_state { + SealingState::Ready => { trace!(target: "miner", "update_sealing: engine indicates internal sealing"); if self.seal_and_import_block_internally(chain, block) { trace!(target: "miner", "update_sealing: imported internally sealed block"); } }, - Some(false) => { - trace!(target: "miner", "update_sealing: engine is not keen to seal internally right now"); - // anyway, save the block for later use - self.sealing.lock().queue.set_pending(block); - }, - None => { + SealingState::NotReady => unreachable!("We returned right after sealing_state was computed. qed."), + SealingState::External => { trace!(target: "miner", "update_sealing: engine does not seal internally, preparing work"); self.prepare_work(block, original_work_hash) }, @@ -1196,7 +1204,7 @@ impl miner::MinerService for Miner { fn work_package(&self, chain: &C) -> Option<(H256, BlockNumber, u64, U256)> where C: BlockChain + CallContract + BlockProducer + SealedBlockImporter + Nonce + Sync, { - if self.engine.seals_internally().is_some() { + if self.engine.sealing_state() != SealingState::External { return None; }