Make InstantSeal Instant again (#11186)

* Make InstantSeal Instant again

* update_sealing if there are transactions in pool after impoerting a block, some line formatting

* Apply suggestions from code review

Co-Authored-By: Tomasz Drwięga <tomusdrw@users.noreply.github.com>

* InstantSeal specific behaviour

* introduce engine.should_reseal_on_update, remove InstantSealService

* remove unused code

* add force param to update_sealing

* better docc

* even better docs

* revert code changes, doc corrections, sort dep

* code optimization

* fix test

* fix bench

ethcore/light/src/client/mod.rs

@@ -18,7 +18,7 @@
 
 use std::sync::{Weak, Arc};
 
-use ethcore::client::{ClientReport, EnvInfo, ClientIoMessage};
+use ethcore::client::{ClientReport, EnvInfo, ClientIoMessage, traits::ForceUpdateSealing};
 use ethcore::engines::{epoch, EthEngine, EpochChange, EpochTransition, Proof};
 use ethcore::machine::EthereumMachine;
 use ethcore::error::{Error, EthcoreResult};
@@ -620,7 +620,7 @@ impl<T: ChainDataFetcher> ::ethcore::client::ChainInfo for Client<T> {
 }
 
 impl<T: ChainDataFetcher> ::ethcore::client::EngineClient for Client<T> {
-	fn update_sealing(&self) { }
+	fn update_sealing(&self, _force: ForceUpdateSealing) {}
 	fn submit_seal(&self, _block_hash: H256, _seal: Vec<Vec<u8>>) { }
 	fn broadcast_consensus_message(&self, _message: Vec<u8>) { }
 

ethcore/src/client/client.rs

@@ -62,7 +62,7 @@ use client::{
 	BlockId, TransactionId, UncleId, TraceId, ClientConfig, BlockChainClient,
 	TraceFilter, CallAnalytics, Mode,
 	ChainNotify, NewBlocks, ChainRoute, PruningInfo, ProvingBlockChainClient, EngineInfo, ChainMessageType,
-	IoClient, BadBlocks,
+	IoClient, BadBlocks, traits::ForceUpdateSealing
 };
 use client::bad_blocks;
 use engines::{MAX_UNCLE_AGE, EthEngine, EpochTransition, ForkChoice, EngineError};
@@ -2424,7 +2424,9 @@ impl ImportSealedBlock for Client {
 		let raw = block.rlp_bytes();
 		let header = block.header.clone();
 		let hash = header.hash();
-		self.notify(|n| n.block_pre_import(&raw, &hash, header.difficulty()));
+		self.notify(|n| {
+			n.block_pre_import(&raw, &hash, header.difficulty())
+		});
 
 		let route = {
 			// Do a super duper basic verification to detect potential bugs
@@ -2512,19 +2514,22 @@ impl ::miner::TransactionVerifierClient for Client {}
 impl ::miner::BlockChainClient for Client {}
 
 impl super::traits::EngineClient for Client {
-	fn update_sealing(&self) {
-		self.importer.miner.update_sealing(self)
+	fn update_sealing(&self, force: ForceUpdateSealing) {
+		self.importer.miner.update_sealing(self, force)
 	}
 
 	fn submit_seal(&self, block_hash: H256, seal: Vec<Bytes>) {
-		let import = self.importer.miner.submit_seal(block_hash, seal).and_then(|block| self.import_sealed_block(block));
+		let import = self.importer.miner.submit_seal(block_hash, seal)
+			.and_then(|block| self.import_sealed_block(block));
 		if let Err(err) = import {
 			warn!(target: "poa", "Wrong internal seal submission! {:?}", err);
 		}
 	}
 
 	fn broadcast_consensus_message(&self, message: Bytes) {
-		self.notify(|notify| notify.broadcast(ChainMessageType::Consensus(message.clone())));
+		self.notify(|notify| {
+			notify.broadcast(ChainMessageType::Consensus(message.clone()))
+		});
 	}
 
 	fn epoch_transition_for(&self, parent_hash: H256) -> Option<::engines::EpochTransition> {
@@ -2598,13 +2603,21 @@ impl ImportExportBlocks for Client {
 			if i % 10000 == 0 {
 				info!("#{}", i);
 			}
-			let b = self.block(BlockId::Number(i)).ok_or("Error exporting incomplete chain")?.into_inner();
+			let b = self.block(BlockId::Number(i))
+				.ok_or("Error exporting incomplete chain")?
+				.into_inner();
 			match format {
 				DataFormat::Binary => {
-					out.write(&b).map_err(|e| format!("Couldn't write to stream. Cause: {}", e))?;
+					out.write(&b)
+						.map_err(|e| {
+							format!("Couldn't write to stream. Cause: {}", e)
+						})?;
 				}
 				DataFormat::Hex => {
-					out.write_fmt(format_args!("{}\n", b.pretty())).map_err(|e| format!("Couldn't write to stream. Cause: {}", e))?;
+					out.write_fmt(format_args!("{}\n", b.pretty()))
+						.map_err(|e| {
+							format!("Couldn't write to stream. Cause: {}", e)
+						})?;
 				}
 			}
 		}
@@ -2624,7 +2637,10 @@ impl ImportExportBlocks for Client {
 		let format = match format {
 			Some(format) => format,
 			None => {
-				first_read = source.read(&mut first_bytes).map_err(|_| "Error reading from the file/stream.")?;
+				first_read = source.read(&mut first_bytes)
+					.map_err(|_| {
+						"Error reading from the file/stream."
+					})?;
 				match first_bytes[0] {
 					0xf9 => DataFormat::Binary,
 					_ => DataFormat::Hex,
@@ -2635,7 +2651,9 @@ impl ImportExportBlocks for Client {
 		let do_import = |bytes: Vec<u8>| {
 			let block = Unverified::from_rlp(bytes).map_err(|_| "Invalid block rlp")?;
 			let number = block.header.number();
-			while self.queue_info().is_full() { std::thread::sleep(Duration::from_secs(1)); }
+			while self.queue_info().is_full() {
+				std::thread::sleep(Duration::from_secs(1));
+			}
 			match self.import_block(block) {
 				Err(Error(EthcoreErrorKind::Import(ImportErrorKind::AlreadyInChain), _)) => {
 					trace!("Skipping block #{}: already in chain.", number);
@@ -2656,33 +2674,45 @@ impl ImportExportBlocks for Client {
 					} else {
 						let mut bytes = vec![0; READAHEAD_BYTES];
 						let n = source.read(&mut bytes)
-							.map_err(|err| format!("Error reading from the file/stream: {:?}", err))?;
+							.map_err(|err| {
+								format!("Error reading from the file/stream: {:?}", err)
+							})?;
 						(bytes, n)
 					};
 					if n == 0 { break; }
 					first_read = 0;
 					let s = PayloadInfo::from(&bytes)
-						.map_err(|e| format!("Invalid RLP in the file/stream: {:?}", e))?.total();
+						.map_err(|e| {
+							format!("Invalid RLP in the file/stream: {:?}", e)
+						})?.total();
 					bytes.resize(s, 0);
 					source.read_exact(&mut bytes[n..])
-						.map_err(|err| format!("Error reading from the file/stream: {:?}", err))?;
+						.map_err(|err| {
+							format!("Error reading from the file/stream: {:?}", err)
+						})?;
 					do_import(bytes)?;
 				}
 			}
 			DataFormat::Hex => {
 				for line in BufReader::new(source).lines() {
 					let s = line
-						.map_err(|err| format!("Error reading from the file/stream: {:?}", err))?;
+						.map_err(|err| {
+							format!("Error reading from the file/stream: {:?}", err)
+						})?;
 					let s = if first_read > 0 {
 						from_utf8(&first_bytes)
-							.map_err(|err| format!("Invalid UTF-8: {:?}", err))?
+							.map_err(|err| {
+								format!("Invalid UTF-8: {:?}", err)
+							})?
 							.to_owned() + &(s[..])
 					} else {
 						s
 					};
 					first_read = 0;
 					let bytes = s.from_hex()
-						.map_err(|err| format!("Invalid hex in file/stream: {:?}", err))?;
+						.map_err(|err| {
+							format!("Invalid hex in file/stream: {:?}", err)
+						})?;
 					do_import(bytes)?;
 				}
 			}

ethcore/src/client/test_client.rs

@@ -56,7 +56,7 @@ use client::{
 	TransactionId, UncleId, TraceId, TraceFilter, LastHashes, CallAnalytics,
 	ProvingBlockChainClient, ScheduleInfo, ImportSealedBlock, BroadcastProposalBlock, ImportBlock, StateOrBlock,
 	Call, StateClient, EngineInfo, AccountData, BlockChain, BlockProducer, SealedBlockImporter, IoClient,
-	BadBlocks,
+	BadBlocks, traits::ForceUpdateSealing
 };
 use engines::EthEngine;
 use error::{Error, EthcoreResult};
@@ -931,8 +931,8 @@ impl ProvingBlockChainClient for TestBlockChainClient {
 }
 
 impl super::traits::EngineClient for TestBlockChainClient {
-	fn update_sealing(&self) {
-		self.miner.update_sealing(self)
+	fn update_sealing(&self, force: ForceUpdateSealing)  {
+		self.miner.update_sealing(self, force)
 	}
 
 	fn submit_seal(&self, block_hash: H256, seal: Vec<Bytes>) {

ethcore/src/client/traits.rs

@@ -423,10 +423,19 @@ pub trait BroadcastProposalBlock {
 /// Provides methods to import sealed block and broadcast a block proposal
 pub trait SealedBlockImporter: ImportSealedBlock + BroadcastProposalBlock {}
 
+/// Do we want to force update sealing?
+#[derive(Debug, Copy, Clone, PartialEq)]
+pub enum ForceUpdateSealing {
+	/// Ideally you want to use `No` at all times as `Yes` skips `reseal_required` checks.
+	Yes,
+	/// Don't skip `reseal_required` checks
+	No
+}
+
 /// Client facilities used by internally sealing Engines.
 pub trait EngineClient: Sync + Send + ChainInfo {
 	/// Make a new block and seal it.
-	fn update_sealing(&self);
+	fn update_sealing(&self, force: ForceUpdateSealing);
 
 	/// Submit a seal for a block in the mining queue.
 	fn submit_seal(&self, block_hash: H256, seal: Vec<Bytes>);

ethcore/src/engines/authority_round/mod.rs

@@ -25,7 +25,7 @@ use std::sync::{Weak, Arc};
 use std::time::{UNIX_EPOCH, Duration};
 
 use block::*;
-use client::EngineClient;
+use client::{EngineClient, traits::ForceUpdateSealing};
 use engines::{Engine, Seal, EngineError, ConstructedVerifier};
 use engines::block_reward;
 use engines::block_reward::{BlockRewardContract, RewardKind};
@@ -908,7 +908,7 @@ impl IoHandler<()> for TransitionHandler {
 				self.step.can_propose.store(true, AtomicOrdering::SeqCst);
 				if let Some(ref weak) = *self.client.read() {
 					if let Some(c) = weak.upgrade() {
-						c.update_sealing();
+						c.update_sealing(ForceUpdateSealing::No);
 					}
 				}
 			}
@@ -936,7 +936,7 @@ impl Engine<EthereumMachine> for AuthorityRound {
 		self.step.can_propose.store(true, AtomicOrdering::SeqCst);
 		if let Some(ref weak) = *self.client.read() {
 			if let Some(c) = weak.upgrade() {
-				c.update_sealing();
+				c.update_sealing(ForceUpdateSealing::No);
 			}
 		}
 	}

ethcore/src/engines/clique/mod.rs

@@ -68,7 +68,7 @@ use std::time::{Instant, Duration, SystemTime, UNIX_EPOCH};
 
 use block::ExecutedBlock;
 use bytes::Bytes;
-use client::{BlockId, EngineClient};
+use client::{BlockId, EngineClient, traits::ForceUpdateSealing};
 use engines::clique::util::{extract_signers, recover_creator};
 use engines::{Engine, EngineError, Seal};
 use error::{BlockError, Error};
@@ -749,7 +749,7 @@ impl Engine<EthereumMachine> for Clique {
 		if self.signer.read().is_some() {
 			if let Some(ref weak) = *self.client.read() {
 				if let Some(c) = weak.upgrade() {
-					c.update_sealing();
+					c.update_sealing(ForceUpdateSealing::No);
 				}
 			}
 		}

ethcore/src/engines/instant_seal.rs

@@ -63,6 +63,13 @@ impl<M: Machine> Engine<M> for InstantSeal<M> {
 
 	fn seals_internally(&self) -> Option<bool> { Some(true) }
 
+	fn should_reseal_on_update(&self) -> bool {
+		// We would like for the miner to `update_sealing` if there are local_pending_transactions
+		// in the pool to prevent transactions sent in parallel from stalling in the transaction
+		// pool. (see #9660)
+		true
+	}
+
 	fn generate_seal(&self, block: &ExecutedBlock, _parent: &Header) -> Seal {
 		if !block.transactions.is_empty() {
 			let block_number = block.header.number();

ethcore/src/engines/mod.rs

@@ -307,6 +307,14 @@ pub trait Engine<M: Machine>: Sync + Send {
 	/// Some(false) means that the node might seal internally but is not qualified now.
 	fn seals_internally(&self) -> Option<bool> { None }
 
+	/// Called in `miner.chain_new_blocks` if the engine wishes to `update_sealing`
+	/// after a block was recently sealed.
+	///
+	/// returns false by default
+	fn should_reseal_on_update(&self) -> bool {
+		false
+	}
+
 	/// Attempt to seal the block internally.
 	///
 	/// If `Some` is returned, then you get a valid seal.

ethcore/src/engines/validator_set/multi.rs

@@ -151,7 +151,7 @@ mod tests {
 	use std::collections::BTreeMap;
 	use hash::keccak;
 	use accounts::AccountProvider;
-	use client::{BlockChainClient, ChainInfo, BlockInfo, ImportBlock};
+	use client::{BlockChainClient, ChainInfo, BlockInfo, ImportBlock, traits::ForceUpdateSealing};
 	use engines::EpochChange;
 	use engines::validator_set::ValidatorSet;
 	use ethkey::Secret;
@@ -181,24 +181,24 @@ mod tests {
 		let signer = Box::new((tap.clone(), v1, "".into()));
 		client.miner().set_author(miner::Author::Sealer(signer));
 		client.transact_contract(Default::default(), Default::default()).unwrap();
-		::client::EngineClient::update_sealing(&*client);
+		EngineClient::update_sealing(&*client, ForceUpdateSealing::No);
 		assert_eq!(client.chain_info().best_block_number, 0);
 		// Right signer for the first block.
 		let signer = Box::new((tap.clone(), v0, "".into()));
 		client.miner().set_author(miner::Author::Sealer(signer));
-		::client::EngineClient::update_sealing(&*client);
+		EngineClient::update_sealing(&*client, ForceUpdateSealing::No);
 		assert_eq!(client.chain_info().best_block_number, 1);
 		// This time v0 is wrong.
 		client.transact_contract(Default::default(), Default::default()).unwrap();
-		::client::EngineClient::update_sealing(&*client);
+		EngineClient::update_sealing(&*client, ForceUpdateSealing::No);
 		assert_eq!(client.chain_info().best_block_number, 1);
 		let signer = Box::new((tap.clone(), v1, "".into()));
 		client.miner().set_author(miner::Author::Sealer(signer));
-		::client::EngineClient::update_sealing(&*client);
+		EngineClient::update_sealing(&*client, ForceUpdateSealing::No);
 		assert_eq!(client.chain_info().best_block_number, 2);
 		// v1 is still good.
 		client.transact_contract(Default::default(), Default::default()).unwrap();
-		::client::EngineClient::update_sealing(&*client);
+		EngineClient::update_sealing(&*client, ForceUpdateSealing::No);
 		assert_eq!(client.chain_info().best_block_number, 3);
 
 		// Check syncing.

ethcore/src/engines/validator_set/safe_contract.rs

@@ -447,7 +447,7 @@ mod tests {
 	use spec::Spec;
 	use accounts::AccountProvider;
 	use types::transaction::{Transaction, Action};
-	use client::{ChainInfo, BlockInfo, ImportBlock};
+	use client::{ChainInfo, BlockInfo, ImportBlock, traits::ForceUpdateSealing};
 	use ethkey::Secret;
 	use miner::{self, MinerService};
 	use test_helpers::{generate_dummy_client_with_spec, generate_dummy_client_with_spec_and_data};
@@ -488,7 +488,7 @@ mod tests {
 			data: "bfc708a000000000000000000000000082a978b3f5962a5b0957d9ee9eef472ee55b42f1".from_hex().unwrap(),
 		}.sign(&s0, Some(chain_id));
 		client.miner().import_own_transaction(client.as_ref(), tx.into()).unwrap();
-		::client::EngineClient::update_sealing(&*client);
+		EngineClient::update_sealing(&*client, ForceUpdateSealing::No);
 		assert_eq!(client.chain_info().best_block_number, 1);
 		// Add "1" validator back in.
 		let tx = Transaction {
@@ -500,14 +500,14 @@ mod tests {
 			data: "4d238c8e00000000000000000000000082a978b3f5962a5b0957d9ee9eef472ee55b42f1".from_hex().unwrap(),
 		}.sign(&s0, Some(chain_id));
 		client.miner().import_own_transaction(client.as_ref(), tx.into()).unwrap();
-		::client::EngineClient::update_sealing(&*client);
+		EngineClient::update_sealing(&*client, ForceUpdateSealing::No);
 		// The transaction is not yet included so still unable to seal.
 		assert_eq!(client.chain_info().best_block_number, 1);
 
 		// Switch to the validator that is still there.
 		let signer = Box::new((tap.clone(), v0, "".into()));
 		client.miner().set_author(miner::Author::Sealer(signer));
-		::client::EngineClient::update_sealing(&*client);
+		EngineClient::update_sealing(&*client, ForceUpdateSealing::No);
 		assert_eq!(client.chain_info().best_block_number, 2);
 		// Switch back to the added validator, since the state is updated.
 		let signer = Box::new((tap.clone(), v1, "".into()));
@@ -521,7 +521,7 @@ mod tests {
 			data: Vec::new(),
 		}.sign(&s0, Some(chain_id));
 		client.miner().import_own_transaction(client.as_ref(), tx.into()).unwrap();
-		::client::EngineClient::update_sealing(&*client);
+		EngineClient::update_sealing(&*client, ForceUpdateSealing::No);
 		// Able to seal again.
 		assert_eq!(client.chain_info().best_block_number, 3);