--reseal-on-uncle (#5940)

* --reseal-on-uncle

* Optimized uncle check

* Additional uncle check

* Updated comment

ethcore/src/client/client.rs

@@ -1718,6 +1718,33 @@ impl MiningBlockChainClient for Client {
 		open_block
 	}
 
+	fn reopen_block(&self, block: ClosedBlock) -> OpenBlock {
+		let engine = &*self.engine;
+		let mut block = block.reopen(engine);
+		let max_uncles = engine.maximum_uncle_count();
+		if block.uncles().len() < max_uncles {
+			let chain = self.chain.read();
+			let h = chain.best_block_hash();
+			// Add new uncles
+			let uncles = chain
+				.find_uncle_hashes(&h, engine.maximum_uncle_age())
+				.unwrap_or_else(Vec::new);
+
+			for h in uncles {
+				if !block.uncles().iter().any(|header| header.hash() == h) {
+					let uncle = chain.block_header(&h).expect("find_uncle_hashes only returns hashes for existing headers; qed");
+					block.push_uncle(uncle).expect("pushing up to maximum_uncle_count;
+												push_uncle is not ok only if more than maximum_uncle_count is pushed;
+												so all push_uncle are Ok;
+												qed");
+					if block.uncles().len() >= max_uncles { break }
+				}
+			}
+
+		}
+		block
+	}
+
 	fn vm_factory(&self) -> &EvmFactory {
 		&self.factories.vm
 	}

ethcore/src/client/test_client.rs

@@ -44,7 +44,7 @@ use types::mode::Mode;
 use types::pruning_info::PruningInfo;
 
 use verification::queue::QueueInfo;
-use block::{OpenBlock, SealedBlock};
+use block::{OpenBlock, SealedBlock, ClosedBlock};
 use executive::Executed;
 use error::CallError;
 use trace::LocalizedTrace;
@@ -381,6 +381,10 @@ impl MiningBlockChainClient for TestBlockChainClient {
 		open_block
 	}
 
+	fn reopen_block(&self, block: ClosedBlock) -> OpenBlock {
+		block.reopen(&*self.spec.engine)
+	}
+
 	fn vm_factory(&self) -> &EvmFactory {
 		&self.vm_factory
 	}

ethcore/src/client/traits.rs

@@ -19,7 +19,7 @@ use util::{U256, Address, H256, H2048, Bytes, Itertools};
 use util::hashdb::DBValue;
 use blockchain::TreeRoute;
 use verification::queue::QueueInfo as BlockQueueInfo;
-use block::{OpenBlock, SealedBlock};
+use block::{OpenBlock, SealedBlock, ClosedBlock};
 use header::{BlockNumber};
 use transaction::{LocalizedTransaction, PendingTransaction, SignedTransaction};
 use transaction_import::TransactionImportResult;
@@ -288,6 +288,9 @@ pub trait MiningBlockChainClient: BlockChainClient {
 		extra_data: Bytes
 	) -> OpenBlock;
 
+	/// Reopens an OpenBlock and updates uncles.
+	fn reopen_block(&self, block: ClosedBlock) -> OpenBlock;
+
 	/// Returns EvmFactory.
 	fn vm_factory(&self) -> &EvmFactory;
 

ethcore/src/error.rs

@@ -129,6 +129,8 @@ pub enum BlockError {
 	UncleIsBrother(OutOfBounds<BlockNumber>),
 	/// An uncle is already in the chain.
 	UncleInChain(H256),
+	/// An uncle is included twice.
+	DuplicateUncle(H256),
 	/// An uncle has a parent not in the chain.
 	UncleParentNotInChain(H256),
 	/// State root header field is invalid.
@@ -188,6 +190,7 @@ impl fmt::Display for BlockError {
 			UncleTooOld(ref oob) => format!("Uncle block is too old. {}", oob),
 			UncleIsBrother(ref oob) => format!("Uncle from same generation as block. {}", oob),
 			UncleInChain(ref hash) => format!("Uncle {} already in chain", hash),
+			DuplicateUncle(ref hash) => format!("Uncle {} already in the header", hash),
 			UncleParentNotInChain(ref hash) => format!("Uncle {} has a parent not in the chain", hash),
 			InvalidStateRoot(ref mis) => format!("Invalid state root in header: {}", mis),
 			InvalidGasUsed(ref mis) => format!("Invalid gas used in header: {}", mis),

ethcore/src/miner/miner.rs

@@ -88,6 +88,8 @@ pub struct MinerOptions {
 	pub reseal_on_external_tx: bool,
 	/// Reseal on receipt of new local transactions.
 	pub reseal_on_own_tx: bool,
+	/// Reseal when new uncle block has been imported.
+	pub reseal_on_uncle: bool,
 	/// Minimum period between transaction-inspired reseals.
 	pub reseal_min_period: Duration,
 	/// Maximum period between blocks (enables force sealing after that).
@@ -119,6 +121,7 @@ impl Default for MinerOptions {
 			force_sealing: false,
 			reseal_on_external_tx: false,
 			reseal_on_own_tx: true,
+			reseal_on_uncle: false,
 			tx_gas_limit: !U256::zero(),
 			tx_queue_size: 1024,
 			tx_queue_gas_limit: GasLimit::Auto,
@@ -347,7 +350,7 @@ impl Miner {
 				Some(old_block) => {
 					trace!(target: "miner", "prepare_block: Already have previous work; updating and returning");
 					// add transactions to old_block
-					old_block.reopen(&*self.engine)
+					chain.reopen_block(old_block)
 				}
 				None => {
 					// block not found - create it.
@@ -366,7 +369,6 @@ impl Miner {
 		let mut transactions_to_penalize = HashSet::new();
 		let block_number = open_block.block().fields().header.number();
 
-		// TODO Push new uncles too.
 		let mut tx_count: usize = 0;
 		let tx_total = transactions.len();
 		for tx in transactions {
@@ -1153,11 +1155,10 @@ impl MinerService for Miner {
 		})
 	}
 
-	fn chain_new_blocks(&self, chain: &MiningBlockChainClient, _imported: &[H256], _invalid: &[H256], enacted: &[H256], retracted: &[H256]) {
+	fn chain_new_blocks(&self, chain: &MiningBlockChainClient, imported: &[H256], _invalid: &[H256], enacted: &[H256], retracted: &[H256]) {
 		trace!(target: "miner", "chain_new_blocks");
 
-		// 1. We ignore blocks that were `imported` (because it means that they are not in canon-chain, and transactions
-		//	  should be still available in the queue.
+		// 1. We ignore blocks that were `imported` unless resealing on new uncles is enabled.
 		// 2. We ignore blocks that are `invalid` because it doesn't have any meaning in terms of the transactions that
 		//    are in those blocks
 
@@ -1192,7 +1193,7 @@ impl MinerService for Miner {
 			transaction_queue.remove_old(&fetch_account, time);
 		}
 
-		if enacted.len() > 0 {
+		if enacted.len() > 0 || (imported.len() > 0 && self.options.reseal_on_uncle) {
 			// --------------------------------------------------------------------------
 			// | NOTE Code below requires transaction_queue and sealing_work locks.     |
 			// | Make sure to release the locks before calling that method.             |
@@ -1312,6 +1313,7 @@ mod tests {
 				force_sealing: false,
 				reseal_on_external_tx: false,
 				reseal_on_own_tx: true,
+				reseal_on_uncle: false,
 				reseal_min_period: Duration::from_secs(5),
 				reseal_max_period: Duration::from_secs(120),
 				tx_gas_limit: !U256::zero(),

ethcore/src/verification/verification.rs

@@ -132,12 +132,17 @@ pub fn verify_block_family(header: &Header, bytes: &[u8], engine: &Engine, bc: &
 			}
 		}
 
+		let mut verified = HashSet::new();
 		for uncle in UntrustedRlp::new(bytes).at(2)?.iter().map(|rlp| rlp.as_val::<Header>()) {
 			let uncle = uncle?;
 			if excluded.contains(&uncle.hash()) {
 				return Err(From::from(BlockError::UncleInChain(uncle.hash())))
 			}
 
+			if verified.contains(&uncle.hash()) {
+				return Err(From::from(BlockError::DuplicateUncle(uncle.hash())))
+			}
+
 			// m_currentBlock.number() - uncle.number()		m_cB.n - uP.n()
 			// 1											2
 			// 2
@@ -180,6 +185,7 @@ pub fn verify_block_family(header: &Header, bytes: &[u8], engine: &Engine, bc: &
 
 			verify_parent(&uncle, &uncle_parent)?;
 			engine.verify_block_family(&uncle, &uncle_parent, Some(bytes))?;
+			verified.insert(uncle.hash());
 		}
 	}
 	Ok(())
@@ -568,6 +574,11 @@ mod tests {
 		check_fail(family_test(&create_test_block_with_data(&header, &good_transactions, &bad_uncles), engine, &bc),
 			TooManyUncles(OutOfBounds { max: Some(engine.maximum_uncle_count()), min: None, found: bad_uncles.len() }));
 
+		header = good.clone();
+		bad_uncles = vec![ good_uncle1.clone(), good_uncle1.clone() ];
+		check_fail(family_test(&create_test_block_with_data(&header, &good_transactions, &bad_uncles), engine, &bc),
+			DuplicateUncle(good_uncle1.hash()));
+
 		// TODO: some additional uncle checks
 	}
 

parity/cli/mod.rs

@@ -250,6 +250,8 @@ usage! {
 			or |c: &Config| otry!(c.mining).force_sealing.clone(),
 		flag_reseal_on_txs: String = "own",
 			or |c: &Config| otry!(c.mining).reseal_on_txs.clone(),
+		flag_reseal_on_uncle: bool = false,
+			or |c: &Config| otry!(c.mining).reseal_on_uncle.clone(),
 		flag_reseal_min_period: u64 = 2000u64,
 			or |c: &Config| otry!(c.mining).reseal_min_period.clone(),
 		flag_reseal_max_period: u64 = 120000u64,
@@ -524,6 +526,7 @@ struct Mining {
 	author: Option<String>,
 	engine_signer: Option<String>,
 	force_sealing: Option<bool>,
+	reseal_on_uncle: Option<bool>,
 	reseal_on_txs: Option<String>,
 	reseal_min_period: Option<u64>,
 	reseal_max_period: Option<u64>,
@@ -788,6 +791,7 @@ mod tests {
 			flag_reseal_on_txs: "all".into(),
 			flag_reseal_min_period: 4000u64,
 			flag_reseal_max_period: 60000u64,
+			flag_reseal_on_uncle: false,
 			flag_work_queue_size: 20usize,
 			flag_tx_gas_limit: Some("6283184".into()),
 			flag_tx_time_limit: Some(100u64),
@@ -1012,6 +1016,7 @@ mod tests {
 				engine_signer: Some("0xdeadbeefcafe0000000000000000000000000001".into()),
 				force_sealing: Some(true),
 				reseal_on_txs: Some("all".into()),
+				reseal_on_uncle: None,
 				reseal_min_period: Some(4000),
 				reseal_max_period: Some(60000),
 				work_queue_size: None,

parity/cli/usage.txt

@@ -262,6 +262,9 @@ Sealing/Mining Options:
                                    ext - reseal only on a new external transaction;
                                    all - reseal on all new transactions
                                    (default: {flag_reseal_on_txs}).
+  --reseal-on-uncle                Force the node to author new blocks when a new uncle
+                                   block is imported.
+                                   (default: {flag_reseal_on_uncle})
   --reseal-min-period MS           Specify the minimum time between reseals from
                                    incoming transactions. MS is time measured in
                                    milliseconds (default: {flag_reseal_min_period}).

parity/configuration.rs

@@ -526,6 +526,7 @@ impl Configuration {
 			force_sealing: self.args.flag_force_sealing,
 			reseal_on_external_tx: reseal.external,
 			reseal_on_own_tx: reseal.own,
+			reseal_on_uncle: self.args.flag_reseal_on_uncle,
 			tx_gas_limit: match self.args.flag_tx_gas_limit {
 				Some(ref d) => to_u256(d)?,
 				None => U256::max_value(),

rpc/src/v1/tests/eth.rs

@@ -58,6 +58,7 @@ fn miner_service(spec: &Spec, accounts: Arc<AccountProvider>) -> Arc<Miner> {
 			force_sealing: true,
 			reseal_on_external_tx: true,
 			reseal_on_own_tx: true,
+			reseal_on_uncle: false,
 			tx_queue_size: 1024,
 			tx_gas_limit: !U256::zero(),
 			tx_queue_strategy: PrioritizationStrategy::GasPriceOnly,