From 0f89768e5f124b8b71fbe256e467b913fecadf96 Mon Sep 17 00:00:00 2001
From: Vladimir Komendantskiy <komendantsky@gmail.com>
Date: Tue, 2 Apr 2019 16:03:44 +0100
Subject: [PATCH 1/8] RPC method parity_clearEngineSigner

Add RPC method parity_clearEngineSigner
Fixes https://github.com/poanetwork/parity-ethereum/issues/113
---
 ethcore/engine/src/engine.rs               |  3 ++
 ethcore/engines/authority-round/src/lib.rs | 60 ++++++++++++++++++++++
 ethcore/engines/basic-authority/src/lib.rs |  6 +++
 ethcore/src/miner/miner.rs                 |  8 +++
 ethcore/src/miner/mod.rs                   |  3 ++
 rpc/src/v1/impls/light/parity_set.rs       |  4 ++
 rpc/src/v1/impls/parity_set.rs             |  5 ++
 rpc/src/v1/tests/helpers/miner_service.rs  |  5 ++
 rpc/src/v1/traits/parity_set.rs            |  4 ++
 9 files changed, 98 insertions(+)

diff --git a/ethcore/engine/src/engine.rs b/ethcore/engine/src/engine.rs
index 815c14b3896..31476d40498 100644
--- a/ethcore/engine/src/engine.rs
+++ b/ethcore/engine/src/engine.rs
@@ -295,6 +295,9 @@ pub trait Engine: Sync + Send {
 	/// Register a component which signs consensus messages.
 	fn set_signer(&self, _signer: Box<dyn EngineSigner>) {}
 
+	/// Unregisters the engine signer address to stop signing consensus messages.
+	fn clear_signer(&self) {}
+
 	/// Sign using the EngineSigner, to be used for consensus tx signing.
 	fn sign(&self, _hash: H256) -> Result<Signature, Error> { unimplemented!() }
 
diff --git a/ethcore/engines/authority-round/src/lib.rs b/ethcore/engines/authority-round/src/lib.rs
index 202b4dbc6c4..14c396a5783 100644
--- a/ethcore/engines/authority-round/src/lib.rs
+++ b/ethcore/engines/authority-round/src/lib.rs
@@ -1626,6 +1626,10 @@ impl Engine for AuthorityRound {
 		*self.signer.write() = Some(signer);
 	}
 
+	fn clear_signer(&self) {
+		*self.signer.write() = Default::default();
+	}
+
 	fn sign(&self, hash: H256) -> Result<Signature, Error> {
 		Ok(self.signer.read()
 			.as_ref()
@@ -1773,12 +1777,68 @@ mod tests {
 
 		engine.set_signer(Box::new((tap, addr2, "2".into())));
 		if let Seal::Regular(seal) = engine.generate_seal(&b2, &genesis_header) {
+			// FIXME: This branch is unreachable because the call to `generate_seal` above always
+			// returns `Seal::None`. Meanwhile it looks as if the intention of the test writer was
+			// to receive a `Seal::Regular` here. This can be achieved similarly to how it's done in
+			// `generates_seal_iff_sealer_is_set()`, by stepping the engine and issuing a block in
+			// the new step signed by `keccak("0")`.
 			assert!(b2.clone().try_seal(engine, seal).is_ok());
 			// Second proposal is forbidden.
 			assert!(engine.generate_seal(&b2, &genesis_header) == Seal::None);
 		}
 	}
 
+	#[test]
+	fn generates_seal_iff_sealer_is_set() {
+		let tap = Arc::new(AccountProvider::transient_provider());
+		let addr1 = tap.insert_account(keccak("1").into(), &"1".into()).unwrap();
+		let spec = Spec::new_test_round();
+		let engine = &*spec.engine;
+		let genesis_header = spec.genesis_header();
+		let db1 = spec.ensure_db_good(get_temp_state_db(), &Default::default()).unwrap();
+		let last_hashes = Arc::new(vec![genesis_header.hash()]);
+		let b1 = OpenBlock::new(engine, Default::default(), false, db1, &genesis_header,
+								last_hashes.clone(), addr1, (3141562.into(), 31415620.into()),
+								vec![], false)
+			.unwrap().close_and_lock().unwrap();
+		// Not a signer. A seal cannot be generated.
+		assert!(engine.generate_seal(&b1, &genesis_header) == Seal::None);
+		// Become a signer.
+		engine.set_signer(Box::new((tap.clone(), addr1, "1".into())));
+		if let Seal::Regular(seal) = engine.generate_seal(&b1, &genesis_header) {
+			assert!(b1.clone().try_seal(engine, seal).is_ok());
+			// Second proposal is forbidden.
+			assert!(engine.generate_seal(&b1, &genesis_header) == Seal::None);
+		} else {
+			panic!("block 1 not sealed");
+		}
+		// Stop being a signer.
+		engine.clear_signer();
+		// Make a step first and then create a new block in that new step.
+		engine.step();
+		let addr2 = tap.insert_account(keccak("0").into(), &"0".into()).unwrap();
+		let mut header2 = genesis_header.clone();
+		header2.set_number(2);
+		header2.set_author(addr2);
+		header2.set_parent_hash(header2.hash());
+		let db2 = spec.ensure_db_good(get_temp_state_db(), &Default::default()).unwrap();
+		let b2 = OpenBlock::new(engine, Default::default(), false, db2, &header2,
+								last_hashes, addr2, (3141562.into(), 31415620.into()),
+								vec![], false)
+			.unwrap().close_and_lock().unwrap();
+		// Not a signer. A seal cannot be generated.
+		assert!(engine.generate_seal(&b2, &header2) == Seal::None);
+		// Become a signer once more.
+		engine.set_signer(Box::new((tap, addr2, "0".into())));
+		if let Seal::Regular(seal) = engine.generate_seal(&b2, &header2) {
+			assert!(b2.clone().try_seal(engine, seal).is_ok());
+			// Second proposal is forbidden.
+			assert!(engine.generate_seal(&b2, &header2) == Seal::None);
+		} else {
+			panic!("block 2 not sealed");
+		}
+	}
+
 	#[test]
 	fn checks_difficulty_in_generate_seal() {
 		let tap = Arc::new(AccountProvider::transient_provider());
diff --git a/ethcore/engines/basic-authority/src/lib.rs b/ethcore/engines/basic-authority/src/lib.rs
index b18283fb214..37a70272b53 100644
--- a/ethcore/engines/basic-authority/src/lib.rs
+++ b/ethcore/engines/basic-authority/src/lib.rs
@@ -198,6 +198,10 @@ impl Engine for BasicAuthority {
 		*self.signer.write() = Some(signer);
 	}
 
+	fn clear_signer(&self) {
+		*self.signer.write() = Default::default();
+	}
+
 	fn sign(&self, hash: H256) -> Result<Signature, Error> {
 		Ok(self.signer.read()
 			.as_ref()
@@ -289,5 +293,7 @@ mod tests {
 		assert_eq!(SealingState::NotReady, engine.sealing_state());
 		engine.set_signer(Box::new((Arc::new(tap), authority, "".into())));
 		assert_eq!(SealingState::Ready, engine.sealing_state());
+		engine.clear_signer();
+		assert_eq!(SealingState::NotReady, engine.sealing_state());
 	}
 }
diff --git a/ethcore/src/miner/miner.rs b/ethcore/src/miner/miner.rs
index b6a70d231e7..8dadad06f05 100644
--- a/ethcore/src/miner/miner.rs
+++ b/ethcore/src/miner/miner.rs
@@ -912,6 +912,14 @@ impl miner::MinerService for Miner {
 		}
 	}
 
+	fn clear_author(&self) {
+		self.params.write().author = Default::default();
+		if self.engine.sealing_state() == SealingState::Ready {
+			self.sealing.lock().enabled = false;
+			self.engine.clear_signer();
+		}
+	}
+
 	fn sensible_gas_price(&self) -> U256 {
 		// 10% above our minimum.
 		self.transaction_queue.current_worst_gas_price() * 110u32 / 100
diff --git a/ethcore/src/miner/mod.rs b/ethcore/src/miner/mod.rs
index 3b68359ac96..4fb030d2391 100644
--- a/ethcore/src/miner/mod.rs
+++ b/ethcore/src/miner/mod.rs
@@ -134,6 +134,9 @@ pub trait MinerService : Send + Sync {
 	/// On chains where sealing is done externally (e.g. PoW) we provide only reward beneficiary.
 	fn set_author(&self, author: Author);
 
+	/// Clears the engine signer and stops signing.
+	fn clear_author(&self);
+
 	// Transaction Pool
 
 	/// Imports transactions to transaction queue.
diff --git a/rpc/src/v1/impls/light/parity_set.rs b/rpc/src/v1/impls/light/parity_set.rs
index 8195e4ddaf3..bb2ca0aee26 100644
--- a/rpc/src/v1/impls/light/parity_set.rs
+++ b/rpc/src/v1/impls/light/parity_set.rs
@@ -75,6 +75,10 @@ impl<F: Fetch> ParitySet for ParitySetClient<F> {
 		Err(errors::light_unimplemented(None))
 	}
 
+	fn clear_engine_signer(&self) -> Result<bool> {
+		Err(errors::light_unimplemented(None))
+	}
+
 	fn set_transactions_limit(&self, _limit: usize) -> Result<bool> {
 		Err(errors::light_unimplemented(None))
 	}
diff --git a/rpc/src/v1/impls/parity_set.rs b/rpc/src/v1/impls/parity_set.rs
index 7d7b46847af..e1c99468874 100644
--- a/rpc/src/v1/impls/parity_set.rs
+++ b/rpc/src/v1/impls/parity_set.rs
@@ -166,6 +166,11 @@ impl<C, M, U, F> ParitySet for ParitySetClient<C, M, U, F> where
 		Ok(true)
 	}
 
+	fn clear_engine_signer(&self) -> Result<bool> {
+		self.miner.clear_author();
+		Ok(true)
+	}
+
 	fn add_reserved_peer(&self, peer: String) -> Result<bool> {
 		match self.net.add_reserved_peer(peer) {
 			Ok(()) => Ok(true),
diff --git a/rpc/src/v1/tests/helpers/miner_service.rs b/rpc/src/v1/tests/helpers/miner_service.rs
index df7102e15be..4194c30fe56 100644
--- a/rpc/src/v1/tests/helpers/miner_service.rs
+++ b/rpc/src/v1/tests/helpers/miner_service.rs
@@ -134,6 +134,11 @@ impl MinerService for TestMinerService {
 		}
 	}
 
+	fn clear_author(&self) -> Result<(), AccountError> {
+		*self.authoring_params.write() = Default::default();
+		Ok(())
+	}
+
 	fn set_extra_data(&self, extra_data: Bytes) {
 		self.authoring_params.write().extra_data = extra_data;
 	}
diff --git a/rpc/src/v1/traits/parity_set.rs b/rpc/src/v1/traits/parity_set.rs
index 0c328b6b313..94fe9fa386f 100644
--- a/rpc/src/v1/traits/parity_set.rs
+++ b/rpc/src/v1/traits/parity_set.rs
@@ -57,6 +57,10 @@ pub trait ParitySet {
 	#[rpc(name = "parity_setEngineSignerSecret")]
 	fn set_engine_signer_secret(&self, H256) -> Result<bool>;
 
+	/// Unsets the engine signer account address.
+	#[rpc(name = "parity_clearEngineSigner")]
+	fn clear_engine_signer(&self) -> Result<bool>;
+
 	/// Sets the limits for transaction queue.
 	#[rpc(name = "parity_setTransactionsLimit")]
 	fn set_transactions_limit(&self, usize) -> Result<bool>;

From a155fc7ab260b498ff29a09db8dc815faab2768b Mon Sep 17 00:00:00 2001
From: Vladimir Komendantskiy <komendantsky@gmail.com>
Date: Thu, 25 Jul 2019 11:20:48 +0100
Subject: [PATCH 2/8] corrected the return type of clear_author

---
 rpc/src/v1/tests/helpers/miner_service.rs | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/rpc/src/v1/tests/helpers/miner_service.rs b/rpc/src/v1/tests/helpers/miner_service.rs
index 4194c30fe56..ac23109e17f 100644
--- a/rpc/src/v1/tests/helpers/miner_service.rs
+++ b/rpc/src/v1/tests/helpers/miner_service.rs
@@ -134,9 +134,8 @@ impl MinerService for TestMinerService {
 		}
 	}
 
-	fn clear_author(&self) -> Result<(), AccountError> {
+	fn clear_author(&self) {
 		*self.authoring_params.write() = Default::default();
-		Ok(())
 	}
 
 	fn set_extra_data(&self, extra_data: Bytes) {

From b6105bedfedefdc0c1e7a3f16efa34f053612c5c Mon Sep 17 00:00:00 2001
From: Vladimir Komendantskiy <komendantsky@gmail.com>
Date: Tue, 27 Aug 2019 12:14:20 +0100
Subject: [PATCH 3/8] review comment responses and a rebase fix

---
 ethcore/engines/authority-round/src/lib.rs | 21 +++------------------
 ethcore/src/miner/miner.rs                 |  6 +++++-
 2 files changed, 8 insertions(+), 19 deletions(-)

diff --git a/ethcore/engines/authority-round/src/lib.rs b/ethcore/engines/authority-round/src/lib.rs
index 14c396a5783..305ed4be124 100644
--- a/ethcore/engines/authority-round/src/lib.rs
+++ b/ethcore/engines/authority-round/src/lib.rs
@@ -1755,36 +1755,21 @@ mod tests {
 	fn generates_seal_and_does_not_double_propose() {
 		let tap = Arc::new(AccountProvider::transient_provider());
 		let addr1 = tap.insert_account(keccak("1").into(), &"1".into()).unwrap();
-		let addr2 = tap.insert_account(keccak("2").into(), &"2".into()).unwrap();
-
 		let spec = spec::new_test_round();
 		let engine = &*spec.engine;
 		let genesis_header = spec.genesis_header();
 		let db1 = spec.ensure_db_good(get_temp_state_db(), &Default::default()).unwrap();
-		let db2 = spec.ensure_db_good(get_temp_state_db(), &Default::default()).unwrap();
 		let last_hashes = Arc::new(vec![genesis_header.hash()]);
 		let b1 = OpenBlock::new(engine, Default::default(), false, db1, &genesis_header, last_hashes.clone(), addr1, (3141562.into(), 31415620.into()), vec![], false).unwrap();
 		let b1 = b1.close_and_lock().unwrap();
-		let b2 = OpenBlock::new(engine, Default::default(), false, db2, &genesis_header, last_hashes, addr2, (3141562.into(), 31415620.into()), vec![], false).unwrap();
-		let b2 = b2.close_and_lock().unwrap();
 
 		engine.set_signer(Box::new((tap.clone(), addr1, "1".into())));
 		if let Seal::Regular(seal) = engine.generate_seal(&b1, &genesis_header) {
 			assert!(b1.clone().try_seal(engine, seal).is_ok());
 			// Second proposal is forbidden.
 			assert!(engine.generate_seal(&b1, &genesis_header) == Seal::None);
-		}
-
-		engine.set_signer(Box::new((tap, addr2, "2".into())));
-		if let Seal::Regular(seal) = engine.generate_seal(&b2, &genesis_header) {
-			// FIXME: This branch is unreachable because the call to `generate_seal` above always
-			// returns `Seal::None`. Meanwhile it looks as if the intention of the test writer was
-			// to receive a `Seal::Regular` here. This can be achieved similarly to how it's done in
-			// `generates_seal_iff_sealer_is_set()`, by stepping the engine and issuing a block in
-			// the new step signed by `keccak("0")`.
-			assert!(b2.clone().try_seal(engine, seal).is_ok());
-			// Second proposal is forbidden.
-			assert!(engine.generate_seal(&b2, &genesis_header) == Seal::None);
+		} else {
+			panic!("block 1 not sealed");
 		}
 	}
 
@@ -1792,7 +1777,7 @@ mod tests {
 	fn generates_seal_iff_sealer_is_set() {
 		let tap = Arc::new(AccountProvider::transient_provider());
 		let addr1 = tap.insert_account(keccak("1").into(), &"1".into()).unwrap();
-		let spec = Spec::new_test_round();
+		let spec = spec::new_test_round();
 		let engine = &*spec.engine;
 		let genesis_header = spec.genesis_header();
 		let db1 = spec.ensure_db_good(get_temp_state_db(), &Default::default()).unwrap();
diff --git a/ethcore/src/miner/miner.rs b/ethcore/src/miner/miner.rs
index 8dadad06f05..231f2f89c4a 100644
--- a/ethcore/src/miner/miner.rs
+++ b/ethcore/src/miner/miner.rs
@@ -913,10 +913,14 @@ impl miner::MinerService for Miner {
 	}
 
 	fn clear_author(&self) {
+		// Clear the author.
 		self.params.write().author = Default::default();
-		if self.engine.sealing_state() == SealingState::Ready {
+		if self.engine.sealing_state() != SealingState::External {
+			// Disable sealing.
 			self.sealing.lock().enabled = false;
 			self.engine.clear_signer();
+		} else {
+			warn!("Clearing the engine signer while the engine does not require one.");
 		}
 	}
 

From 538334dd1b57588af24fee38003622838c104ccb Mon Sep 17 00:00:00 2001
From: Vladimir Komendantskiy <komendantsky@gmail.com>
Date: Tue, 27 Aug 2019 12:17:16 +0100
Subject: [PATCH 4/8] removed a spurrious warning

---
 ethcore/src/miner/miner.rs | 2 --
 1 file changed, 2 deletions(-)

diff --git a/ethcore/src/miner/miner.rs b/ethcore/src/miner/miner.rs
index 231f2f89c4a..f59d2b46b07 100644
--- a/ethcore/src/miner/miner.rs
+++ b/ethcore/src/miner/miner.rs
@@ -919,8 +919,6 @@ impl miner::MinerService for Miner {
 			// Disable sealing.
 			self.sealing.lock().enabled = false;
 			self.engine.clear_signer();
-		} else {
-			warn!("Clearing the engine signer while the engine does not require one.");
 		}
 	}
 

From c513359d6b49225678d54e9621dc3620179aa8c3 Mon Sep 17 00:00:00 2001
From: Vladimir Komendantskiy <komendantsky@gmail.com>
Date: Thu, 12 Sep 2019 16:44:38 +0100
Subject: [PATCH 5/8] moved clear_signer functionality to set_signer

---
 ethcore/engine/src/engine.rs               |  5 +-
 ethcore/engines/authority-round/src/lib.rs | 58 ++++++++++------------
 ethcore/engines/basic-authority/src/lib.rs | 14 ++----
 ethcore/engines/clique/src/lib.rs          |  5 +-
 ethcore/src/miner/miner.rs                 |  4 +-
 5 files changed, 37 insertions(+), 49 deletions(-)

diff --git a/ethcore/engine/src/engine.rs b/ethcore/engine/src/engine.rs
index 31476d40498..357a184c7af 100644
--- a/ethcore/engine/src/engine.rs
+++ b/ethcore/engine/src/engine.rs
@@ -293,10 +293,7 @@ pub trait Engine: Sync + Send {
 	fn handle_message(&self, _message: &[u8]) -> Result<(), EngineError> { Err(EngineError::UnexpectedMessage) }
 
 	/// Register a component which signs consensus messages.
-	fn set_signer(&self, _signer: Box<dyn EngineSigner>) {}
-
-	/// Unregisters the engine signer address to stop signing consensus messages.
-	fn clear_signer(&self) {}
+	fn set_signer(&self, _signer: Option<Box<dyn EngineSigner>>) {}
 
 	/// Sign using the EngineSigner, to be used for consensus tx signing.
 	fn sign(&self, _hash: H256) -> Result<Signature, Error> { unimplemented!() }
diff --git a/ethcore/engines/authority-round/src/lib.rs b/ethcore/engines/authority-round/src/lib.rs
index 305ed4be124..99ee36b7826 100644
--- a/ethcore/engines/authority-round/src/lib.rs
+++ b/ethcore/engines/authority-round/src/lib.rs
@@ -1622,12 +1622,8 @@ impl Engine for AuthorityRound {
 		self.validators.register_client(client);
 	}
 
-	fn set_signer(&self, signer: Box<dyn EngineSigner>) {
-		*self.signer.write() = Some(signer);
-	}
-
-	fn clear_signer(&self) {
-		*self.signer.write() = Default::default();
+	fn set_signer(&self, signer: Option<Box<dyn EngineSigner>>) {
+		*self.signer.write() = signer;
 	}
 
 	fn sign(&self, hash: H256) -> Result<Signature, Error> {
@@ -1763,7 +1759,7 @@ mod tests {
 		let b1 = OpenBlock::new(engine, Default::default(), false, db1, &genesis_header, last_hashes.clone(), addr1, (3141562.into(), 31415620.into()), vec![], false).unwrap();
 		let b1 = b1.close_and_lock().unwrap();
 
-		engine.set_signer(Box::new((tap.clone(), addr1, "1".into())));
+		engine.set_signer(Some(Box::new((tap.clone(), addr1, "1".into()))));
 		if let Seal::Regular(seal) = engine.generate_seal(&b1, &genesis_header) {
 			assert!(b1.clone().try_seal(engine, seal).is_ok());
 			// Second proposal is forbidden.
@@ -1789,7 +1785,7 @@ mod tests {
 		// Not a signer. A seal cannot be generated.
 		assert!(engine.generate_seal(&b1, &genesis_header) == Seal::None);
 		// Become a signer.
-		engine.set_signer(Box::new((tap.clone(), addr1, "1".into())));
+		engine.set_signer(Some(Box::new((tap.clone(), addr1, "1".into()))));
 		if let Seal::Regular(seal) = engine.generate_seal(&b1, &genesis_header) {
 			assert!(b1.clone().try_seal(engine, seal).is_ok());
 			// Second proposal is forbidden.
@@ -1798,7 +1794,7 @@ mod tests {
 			panic!("block 1 not sealed");
 		}
 		// Stop being a signer.
-		engine.clear_signer();
+		engine.set_signer(None);
 		// Make a step first and then create a new block in that new step.
 		engine.step();
 		let addr2 = tap.insert_account(keccak("0").into(), &"0".into()).unwrap();
@@ -1814,7 +1810,7 @@ mod tests {
 		// Not a signer. A seal cannot be generated.
 		assert!(engine.generate_seal(&b2, &header2) == Seal::None);
 		// Become a signer once more.
-		engine.set_signer(Box::new((tap, addr2, "0".into())));
+		engine.set_signer(Some(Box::new((tap, addr2, "0".into()))));
 		if let Seal::Regular(seal) = engine.generate_seal(&b2, &header2) {
 			assert!(b2.clone().try_seal(engine, seal).is_ok());
 			// Second proposal is forbidden.
@@ -1843,13 +1839,13 @@ mod tests {
 		let b2 = OpenBlock::new(engine, Default::default(), false, db2, &genesis_header, last_hashes, addr2, (3141562.into(), 31415620.into()), vec![], false).unwrap();
 		let b2 = b2.close_and_lock().unwrap();
 
-		engine.set_signer(Box::new((tap.clone(), addr1, "1".into())));
+		engine.set_signer(Some(Box::new((tap.clone(), addr1, "1".into()))));
 		match engine.generate_seal(&b1, &genesis_header) {
 			Seal::None => panic!("wrong seal"),
 			Seal::Regular(_) => {
 				engine.step();
 
-				engine.set_signer(Box::new((tap.clone(), addr2, "0".into())));
+				engine.set_signer(Some(Box::new((tap.clone(), addr2, "0".into()))));
 				match engine.generate_seal(&b2, &genesis_header) {
 					Seal::Regular(_) => panic!("sealed despite wrong difficulty"),
 					Seal::None => {}
@@ -1967,11 +1963,11 @@ mod tests {
 		assert!(aura.verify_block_family(&header, &parent_header).is_ok());
 		assert_eq!(last_benign.load(AtomicOrdering::SeqCst), 0);
 
-		aura.set_signer(Box::new((
+		aura.set_signer(Some(Box::new((
 			Arc::new(AccountProvider::transient_provider()),
 			validator2,
 			"".into(),
-		)));
+		))));
 
 		// Do not report on steps skipped between genesis and first block.
 		header.set_number(1);
@@ -2086,7 +2082,7 @@ mod tests {
 		client.add_notify(notify.clone());
 		engine.register_client(Arc::downgrade(&client) as _);
 
-		engine.set_signer(Box::new((tap.clone(), addr1, "1".into())));
+		engine.set_signer(Some(Box::new((tap.clone(), addr1, "1".into()))));
 
 		let b1 = OpenBlock::new(engine, Default::default(), false, db1, &genesis_header, last_hashes.clone(), addr1, (3141562.into(), 31415620.into()), vec![], false).unwrap();
 		let b1 = b1.close_and_lock().unwrap();
@@ -2130,7 +2126,7 @@ mod tests {
 		let b1 = b1.close_and_lock().unwrap();
 
 		// since the block is empty it isn't sealed and we generate empty steps
-		engine.set_signer(Box::new((tap.clone(), addr1, "1".into())));
+		engine.set_signer(Some(Box::new((tap.clone(), addr1, "1".into()))));
 		assert_eq!(engine.generate_seal(&b1, &genesis_header), Seal::None);
 		engine.step();
 
@@ -2147,9 +2143,9 @@ mod tests {
 		let b2 = b2.close_and_lock().unwrap();
 
 		// we will now seal a block with 1tx and include the accumulated empty step message
-		engine.set_signer(Box::new((tap.clone(), addr2, "0".into())));
+		engine.set_signer(Some(Box::new((tap.clone(), addr2, "0".into()))));
 		if let Seal::Regular(seal) = engine.generate_seal(&b2, &genesis_header) {
-			engine.set_signer(Box::new((tap.clone(), addr1, "1".into())));
+			engine.set_signer(Some(Box::new((tap.clone(), addr1, "1".into()))));
 			let empty_step2 = sealed_empty_step(engine, 2, &genesis_header.hash());
 			let empty_steps = ::rlp::encode_list(&vec![empty_step2]);
 
@@ -2183,14 +2179,14 @@ mod tests {
 		let b1 = b1.close_and_lock().unwrap();
 
 		// since the block is empty it isn't sealed and we generate empty steps
-		engine.set_signer(Box::new((tap.clone(), addr1, "1".into())));
+		engine.set_signer(Some(Box::new((tap.clone(), addr1, "1".into()))));
 		assert_eq!(engine.generate_seal(&b1, &genesis_header), Seal::None);
 		engine.step();
 
 		// step 3
 		let b2 = OpenBlock::new(engine, Default::default(), false, db2, &genesis_header, last_hashes.clone(), addr2, (3141562.into(), 31415620.into()), vec![], false).unwrap();
 		let b2 = b2.close_and_lock().unwrap();
-		engine.set_signer(Box::new((tap.clone(), addr2, "0".into())));
+		engine.set_signer(Some(Box::new((tap.clone(), addr2, "0".into()))));
 		assert_eq!(engine.generate_seal(&b2, &genesis_header), Seal::None);
 		engine.step();
 
@@ -2199,10 +2195,10 @@ mod tests {
 		let b3 = OpenBlock::new(engine, Default::default(), false, db3, &genesis_header, last_hashes.clone(), addr1, (3141562.into(), 31415620.into()), vec![], false).unwrap();
 		let b3 = b3.close_and_lock().unwrap();
 
-		engine.set_signer(Box::new((tap.clone(), addr1, "1".into())));
+		engine.set_signer(Some(Box::new((tap.clone(), addr1, "1".into()))));
 		if let Seal::Regular(seal) = engine.generate_seal(&b3, &genesis_header) {
 			let empty_step2 = sealed_empty_step(engine, 2, &genesis_header.hash());
-			engine.set_signer(Box::new((tap.clone(), addr2, "0".into())));
+			engine.set_signer(Some(Box::new((tap.clone(), addr2, "0".into()))));
 			let empty_step3 = sealed_empty_step(engine, 3, &genesis_header.hash());
 
 			let empty_steps = ::rlp::encode_list(&vec![empty_step2, empty_step3]);
@@ -2233,7 +2229,7 @@ mod tests {
 		let b1 = b1.close_and_lock().unwrap();
 
 		// since the block is empty it isn't sealed and we generate empty steps
-		engine.set_signer(Box::new((tap.clone(), addr1, "1".into())));
+		engine.set_signer(Some(Box::new((tap.clone(), addr1, "1".into()))));
 		assert_eq!(engine.generate_seal(&b1, &genesis_header), Seal::None);
 		engine.step();
 
@@ -2287,7 +2283,7 @@ mod tests {
 		);
 
 		// empty step with valid signature from incorrect proposer for step
-		engine.set_signer(Box::new((tap.clone(), addr1, "1".into())));
+		engine.set_signer(Some(Box::new((tap.clone(), addr1, "1".into()))));
 		let empty_steps = vec![sealed_empty_step(engine, 1, &parent_header.hash())];
 		set_empty_steps_seal(&mut header, 2, &signature, &empty_steps);
 
@@ -2297,9 +2293,9 @@ mod tests {
 		);
 
 		// valid empty steps
-		engine.set_signer(Box::new((tap.clone(), addr1, "1".into())));
+		engine.set_signer(Some(Box::new((tap.clone(), addr1, "1".into()))));
 		let empty_step2 = sealed_empty_step(engine, 2, &parent_header.hash());
-		engine.set_signer(Box::new((tap.clone(), addr2, "0".into())));
+		engine.set_signer(Some(Box::new((tap.clone(), addr2, "0".into()))));
 		let empty_step3 = sealed_empty_step(engine, 3, &parent_header.hash());
 
 		let empty_steps = vec![empty_step2, empty_step3];
@@ -2343,7 +2339,7 @@ mod tests {
 		let b1 = b1.close_and_lock().unwrap();
 
 		// since the block is empty it isn't sealed and we generate empty steps
-		engine.set_signer(Box::new((tap.clone(), addr1, "1".into())));
+		engine.set_signer(Some(Box::new((tap.clone(), addr1, "1".into()))));
 		assert_eq!(engine.generate_seal(&b1, &genesis_header), Seal::None);
 		engine.step();
 
@@ -2380,7 +2376,7 @@ mod tests {
 		let engine = &*spec.engine;
 
 		let addr1 = accounts[0];
-		engine.set_signer(Box::new((tap.clone(), addr1, "1".into())));
+		engine.set_signer(Some(Box::new((tap.clone(), addr1, "1".into()))));
 
 		let mut header: Header = Header::default();
 		let empty_step = empty_step(engine, 1, &header.parent_hash());
@@ -2461,7 +2457,7 @@ mod tests {
 		header.set_author(accounts[0]);
 
 		// when
-		engine.set_signer(Box::new((tap.clone(), accounts[1], "0".into())));
+		engine.set_signer(Some(Box::new((tap.clone(), accounts[1], "0".into()))));
 		let empty_steps = vec![
 			sealed_empty_step(&*engine, 1, &parent.hash()),
 			sealed_empty_step(&*engine, 1, &parent.hash()),
@@ -2498,9 +2494,9 @@ mod tests {
 		header.set_author(accounts[0]);
 
 		// when
-		engine.set_signer(Box::new((tap.clone(), accounts[1], "0".into())));
+		engine.set_signer(Some(Box::new((tap.clone(), accounts[1], "0".into()))));
 		let es1 = sealed_empty_step(&*engine, 1, &parent.hash());
-		engine.set_signer(Box::new((tap.clone(), accounts[0], "1".into())));
+		engine.set_signer(Some(Box::new((tap.clone(), accounts[0], "1".into()))));
 		let es2 = sealed_empty_step(&*engine, 2, &parent.hash());
 
 		let mut empty_steps = vec![es2, es1];
diff --git a/ethcore/engines/basic-authority/src/lib.rs b/ethcore/engines/basic-authority/src/lib.rs
index 37a70272b53..bae548781b4 100644
--- a/ethcore/engines/basic-authority/src/lib.rs
+++ b/ethcore/engines/basic-authority/src/lib.rs
@@ -194,12 +194,8 @@ impl Engine for BasicAuthority {
 		}
 	}
 
-	fn set_signer(&self, signer: Box<dyn EngineSigner>) {
-		*self.signer.write() = Some(signer);
-	}
-
-	fn clear_signer(&self) {
-		*self.signer.write() = Default::default();
+	fn set_signer(&self, signer: Option<Box<dyn EngineSigner>>) {
+		*self.signer.write() = signer;
 	}
 
 	fn sign(&self, hash: H256) -> Result<Signature, Error> {
@@ -273,7 +269,7 @@ mod tests {
 
 		let spec = new_test_authority();
 		let engine = &*spec.engine;
-		engine.set_signer(Box::new((Arc::new(tap), addr, "".into())));
+		engine.set_signer(Some(Box::new((Arc::new(tap), addr, "".into()))));
 		let genesis_header = spec.genesis_header();
 		let db = spec.ensure_db_good(get_temp_state_db(), &Default::default()).unwrap();
 		let last_hashes = Arc::new(vec![genesis_header.hash()]);
@@ -291,9 +287,9 @@ mod tests {
 
 		let engine = new_test_authority().engine;
 		assert_eq!(SealingState::NotReady, engine.sealing_state());
-		engine.set_signer(Box::new((Arc::new(tap), authority, "".into())));
+		engine.set_signer(Some(Box::new((Arc::new(tap), authority, "".into()))));
 		assert_eq!(SealingState::Ready, engine.sealing_state());
-		engine.clear_signer();
+		engine.set_signer(None);
 		assert_eq!(SealingState::NotReady, engine.sealing_state());
 	}
 }
diff --git a/ethcore/engines/clique/src/lib.rs b/ethcore/engines/clique/src/lib.rs
index 05cb5d2621c..d5343360b5f 100644
--- a/ethcore/engines/clique/src/lib.rs
+++ b/ethcore/engines/clique/src/lib.rs
@@ -762,9 +762,8 @@ impl Engine for Clique {
 		}
 	}
 
-	fn set_signer(&self, signer: Box<dyn EngineSigner>) {
-		trace!(target: "engine", "set_signer: {}", signer.address());
-		*self.signer.write() = Some(signer);
+	fn set_signer(&self, signer: Option<Box<dyn EngineSigner>>) {
+		*self.signer.write() = signer;
 	}
 
 	fn register_client(&self, client: Weak<dyn EngineClient>) {
diff --git a/ethcore/src/miner/miner.rs b/ethcore/src/miner/miner.rs
index f59d2b46b07..e870cf74e85 100644
--- a/ethcore/src/miner/miner.rs
+++ b/ethcore/src/miner/miner.rs
@@ -905,7 +905,7 @@ impl miner::MinerService for Miner {
 				// | (some `Engine`s call `EngineClient.update_sealing()`)                  |
 				// | Make sure to release the locks before calling that method.             |
 				// --------------------------------------------------------------------------
-				self.engine.set_signer(signer);
+				self.engine.set_signer(Some(signer));
 			} else {
 				warn!("Setting an EngineSigner while Engine does not require one.");
 			}
@@ -918,7 +918,7 @@ impl miner::MinerService for Miner {
 		if self.engine.sealing_state() != SealingState::External {
 			// Disable sealing.
 			self.sealing.lock().enabled = false;
-			self.engine.clear_signer();
+			self.engine.set_signer(None);
 		}
 	}
 

From e798a093d206877d80bf7714e0b57fa8331d3272 Mon Sep 17 00:00:00 2001
From: Andreas Fackler <AndreasFackler@gmx.de>
Date: Tue, 8 Oct 2019 12:48:01 +0200
Subject: [PATCH 6/8] Merge clear_author into MinerService::set_author.

---
 ethcore/src/miner/miner.rs                | 57 +++++++++++++----------
 ethcore/src/miner/mod.rs                  |  5 +-
 rpc/src/v1/impls/parity_set.rs            |  2 +-
 rpc/src/v1/tests/helpers/miner_service.rs | 15 +++---
 4 files changed, 41 insertions(+), 38 deletions(-)

diff --git a/ethcore/src/miner/miner.rs b/ethcore/src/miner/miner.rs
index e870cf74e85..d97b1170266 100644
--- a/ethcore/src/miner/miner.rs
+++ b/ethcore/src/miner/miner.rs
@@ -893,32 +893,39 @@ impl miner::MinerService for Miner {
 		self.params.write().extra_data = extra_data;
 	}
 
-	fn set_author(&self, author: Author) {
-		self.params.write().author = author.address();
-
-		if let Author::Sealer(signer) = author {
-			if self.engine.sealing_state() != SealingState::External {
-				// Enable sealing
-				self.sealing.lock().enabled = true;
-				// --------------------------------------------------------------------------
-				// | NOTE Code below may require author and sealing locks                   |
-				// | (some `Engine`s call `EngineClient.update_sealing()`)                  |
-				// | Make sure to release the locks before calling that method.             |
-				// --------------------------------------------------------------------------
-				self.engine.set_signer(Some(signer));
-			} else {
-				warn!("Setting an EngineSigner while Engine does not require one.");
+	fn set_author<T: Into<Option<Author>>>(&self, author: T) {
+		let author_opt = author.into();
+		self.params.write().author = author_opt.as_ref().map(Author::address).unwrap_or_default();
+
+		match author_opt {
+			Some(Author::Sealer(signer)) => {
+				if self.engine.sealing_state() != SealingState::External {
+					// Enable sealing
+					self.sealing.lock().enabled = true;
+					// --------------------------------------------------------------------------
+					// | NOTE Code below may require author and sealing locks                   |
+					// | (some `Engine`s call `EngineClient.update_sealing()`)                  |
+					// | Make sure to release the locks before calling that method.             |
+					// --------------------------------------------------------------------------
+					self.engine.set_signer(Some(signer));
+				} else {
+					warn!("Setting an EngineSigner while Engine does not require one.");
+				}
+			}
+			Some(Author::External(_address)) => (),
+			None => {
+				// Clear the author.
+				if self.engine.sealing_state() != SealingState::External {
+					// Disable sealing.
+					self.sealing.lock().enabled = false;
+					// --------------------------------------------------------------------------
+					// | NOTE Code below may require author and sealing locks                   |
+					// | (some `Engine`s call `EngineClient.update_sealing()`)                  |
+					// | Make sure to release the locks before calling that method.             |
+					// --------------------------------------------------------------------------
+					self.engine.set_signer(None);
+				}
 			}
-		}
-	}
-
-	fn clear_author(&self) {
-		// Clear the author.
-		self.params.write().author = Default::default();
-		if self.engine.sealing_state() != SealingState::External {
-			// Disable sealing.
-			self.sealing.lock().enabled = false;
-			self.engine.set_signer(None);
 		}
 	}
 
diff --git a/ethcore/src/miner/mod.rs b/ethcore/src/miner/mod.rs
index 4fb030d2391..9961f12af80 100644
--- a/ethcore/src/miner/mod.rs
+++ b/ethcore/src/miner/mod.rs
@@ -132,10 +132,7 @@ pub trait MinerService : Send + Sync {
 	/// Set info necessary to sign consensus messages and block authoring.
 	///
 	/// On chains where sealing is done externally (e.g. PoW) we provide only reward beneficiary.
-	fn set_author(&self, author: Author);
-
-	/// Clears the engine signer and stops signing.
-	fn clear_author(&self);
+	fn set_author<T: Into<Option<Author>>>(&self, author: T);
 
 	// Transaction Pool
 
diff --git a/rpc/src/v1/impls/parity_set.rs b/rpc/src/v1/impls/parity_set.rs
index e1c99468874..0db3c99ca27 100644
--- a/rpc/src/v1/impls/parity_set.rs
+++ b/rpc/src/v1/impls/parity_set.rs
@@ -167,7 +167,7 @@ impl<C, M, U, F> ParitySet for ParitySetClient<C, M, U, F> where
 	}
 
 	fn clear_engine_signer(&self) -> Result<bool> {
-		self.miner.clear_author();
+		self.miner.set_author(None);
 		Ok(true)
 	}
 
diff --git a/rpc/src/v1/tests/helpers/miner_service.rs b/rpc/src/v1/tests/helpers/miner_service.rs
index ac23109e17f..0af99e1227c 100644
--- a/rpc/src/v1/tests/helpers/miner_service.rs
+++ b/rpc/src/v1/tests/helpers/miner_service.rs
@@ -127,17 +127,16 @@ impl MinerService for TestMinerService {
 		self.authoring_params.read().clone()
 	}
 
-	fn set_author(&self, author: miner::Author) {
-		self.authoring_params.write().author = author.address();
-		if let miner::Author::Sealer(signer) = author {
-			*self.signer.write() = Some(signer);
+	fn set_author<T: Into<Option<miner::Author>>>(&self, author: T) {
+		let author_opt = author.into();
+		self.authoring_params.write().author = author_opt.as_ref().map(miner::Author::address).unwrap_or_default();
+		match author_opt {
+			Some(miner::Author::Sealer(signer)) => *self.signer.write() = Some(signer),
+			Some(miner::Author::External(_addr)) => (),
+			None => *self.signer.write() = None,
 		}
 	}
 
-	fn clear_author(&self) {
-		*self.authoring_params.write() = Default::default();
-	}
-
 	fn set_extra_data(&self, extra_data: Bytes) {
 		self.authoring_params.write().extra_data = extra_data;
 	}

From 869c3b3a1403f7ed3b4883c2e22bd1720ad34202 Mon Sep 17 00:00:00 2001
From: Andreas Fackler <AndreasFackler@gmx.de>
Date: Tue, 8 Oct 2019 16:21:18 +0200
Subject: [PATCH 7/8] Add trace logs to Clique::set_signer.

---
 ethcore/engines/clique/src/lib.rs | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/ethcore/engines/clique/src/lib.rs b/ethcore/engines/clique/src/lib.rs
index d5343360b5f..b7d83e709f7 100644
--- a/ethcore/engines/clique/src/lib.rs
+++ b/ethcore/engines/clique/src/lib.rs
@@ -763,6 +763,11 @@ impl Engine for Clique {
 	}
 
 	fn set_signer(&self, signer: Option<Box<dyn EngineSigner>>) {
+		if let Some(signer) = signer.as_ref() {
+			trace!(target: "engine", "set_signer: {:?}", signer.address());
+		} else if let Some(signer) = &*self.signer.read() {
+			trace!(target: "engine", "set_signer: cleared; previous signer: {:?})", signer.address());
+		}
 		*self.signer.write() = signer;
 	}
 

From 2249974e3e812fe6fc0dfd2bf79c829ecbf12263 Mon Sep 17 00:00:00 2001
From: Andreas Fackler <AndreasFackler@gmx.de>
Date: Tue, 8 Oct 2019 18:18:42 +0200
Subject: [PATCH 8/8] Clique: Don't lock signer multiple times.

---
 ethcore/engines/clique/src/lib.rs | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/ethcore/engines/clique/src/lib.rs b/ethcore/engines/clique/src/lib.rs
index b7d83e709f7..ac8ed4b86d2 100644
--- a/ethcore/engines/clique/src/lib.rs
+++ b/ethcore/engines/clique/src/lib.rs
@@ -763,12 +763,13 @@ impl Engine for Clique {
 	}
 
 	fn set_signer(&self, signer: Option<Box<dyn EngineSigner>>) {
+		let mut current_signer = self.signer.write();
 		if let Some(signer) = signer.as_ref() {
 			trace!(target: "engine", "set_signer: {:?}", signer.address());
-		} else if let Some(signer) = &*self.signer.read() {
+		} else if let Some(signer) = &*current_signer {
 			trace!(target: "engine", "set_signer: cleared; previous signer: {:?})", signer.address());
 		}
-		*self.signer.write() = signer;
+		*current_signer = signer;
 	}
 
 	fn register_client(&self, client: Weak<dyn EngineClient>) {