Add SealingState; don't prepare block when not ready. (#10529)

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(

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<EthereumMachine> for AuthorityRound {
 		header.set_difficulty(score);
 	}
 
-	fn seals_internally(&self) -> Option<bool> {
-		// 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<EthereumMachine> 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<EthereumMachine> 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;
 			}
 

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<EthereumMachine> for BasicAuthority {
 	// One field - the signature
 	fn seal_fields(&self, _header: &Header) -> usize { 1 }
 
-	fn seals_internally(&self) -> Option<bool> {
-		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());
 	}
 }

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<EthereumMachine> for Clique {
 	}
 
 	/// Clique doesn't require external work to seal, so we always return true here.
-	fn seals_internally(&self) -> Option<bool> {
-		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()`.

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 <http://www.gnu.org/licenses/>.
 
-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<M: Machine> Engine<M> for InstantSeal<M> {
 
 	fn machine(&self) -> &M { &self.machine }
 
-	fn seals_internally(&self) -> Option<bool> { Some(true) }
+	fn sealing_state(&self) -> SealingState { SealingState::Ready }
 
 	fn generate_seal(&self, block: &ExecutedBlock, _parent: &Header) -> Seal {
 		if block.transactions.is_empty() {

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<u8>) -> Result<Vec<u8>, String> + 'a;
 
@@ -302,10 +313,8 @@ pub trait Engine<M: Machine>: 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<bool> { None }
+	/// Returns the engine's current sealing state.
+	fn sealing_state(&self) -> SealingState { SealingState::External }
 
 	/// Attempt to seal the block internally.
 	///

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<C>(&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;
 		}