From 93fbbb9aaf161f21471050a2a3257f820c029a73 Mon Sep 17 00:00:00 2001 From: Vladimir Komendantskiy Date: Wed, 9 Oct 2019 13:42:51 +0100 Subject: [PATCH] RPC method for clearing the engine signer (#10920) * RPC method parity_clearEngineSigner Add RPC method parity_clearEngineSigner Fixes https://github.com/poanetwork/parity-ethereum/issues/113 * corrected the return type of clear_author * review comment responses and a rebase fix * removed a spurrious warning * moved clear_signer functionality to set_signer * Merge clear_author into MinerService::set_author. * Add trace logs to Clique::set_signer. * Clique: Don't lock signer multiple times. --- ethcore/engine/src/engine.rs | 2 +- ethcore/engines/authority-round/src/lib.rs | 105 ++++++++++++++------- ethcore/engines/basic-authority/src/lib.rs | 10 +- ethcore/engines/clique/src/lib.rs | 11 ++- ethcore/src/miner/miner.rs | 45 ++++++--- ethcore/src/miner/mod.rs | 2 +- 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 | 11 ++- rpc/src/v1/traits/parity_set.rs | 4 + 10 files changed, 140 insertions(+), 59 deletions(-) diff --git a/ethcore/engine/src/engine.rs b/ethcore/engine/src/engine.rs index 815c14b38..357a184c7 100644 --- a/ethcore/engine/src/engine.rs +++ b/ethcore/engine/src/engine.rs @@ -293,7 +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) {} + fn set_signer(&self, _signer: Option>) {} /// Sign using the EngineSigner, to be used for consensus tx signing. fn sign(&self, _hash: H256) -> Result { unimplemented!() } diff --git a/ethcore/engines/authority-round/src/lib.rs b/ethcore/engines/authority-round/src/lib.rs index 202b4dbc6..99ee36b78 100644 --- a/ethcore/engines/authority-round/src/lib.rs +++ b/ethcore/engines/authority-round/src/lib.rs @@ -1622,8 +1622,8 @@ impl Engine for AuthorityRound { self.validators.register_client(client); } - fn set_signer(&self, signer: Box) { - *self.signer.write() = Some(signer); + fn set_signer(&self, signer: Option>) { + *self.signer.write() = signer; } fn sign(&self, hash: H256) -> Result { @@ -1751,31 +1751,72 @@ 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()))); + 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. assert!(engine.generate_seal(&b1, &genesis_header) == Seal::None); + } else { + panic!("block 1 not sealed"); } + } - engine.set_signer(Box::new((tap, addr2, "2".into()))); - if let Seal::Regular(seal) = engine.generate_seal(&b2, &genesis_header) { + #[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(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. + assert!(engine.generate_seal(&b1, &genesis_header) == Seal::None); + } else { + panic!("block 1 not sealed"); + } + // Stop being a 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(); + 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(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. - assert!(engine.generate_seal(&b2, &genesis_header) == Seal::None); + assert!(engine.generate_seal(&b2, &header2) == Seal::None); + } else { + panic!("block 2 not sealed"); } } @@ -1798,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 => {} @@ -1922,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); @@ -2041,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(); @@ -2085,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(); @@ -2102,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]); @@ -2138,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(); @@ -2154,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]); @@ -2188,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(); @@ -2242,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); @@ -2252,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]; @@ -2298,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(); @@ -2335,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()); @@ -2416,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()), @@ -2453,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 b18283fb2..bae548781 100644 --- a/ethcore/engines/basic-authority/src/lib.rs +++ b/ethcore/engines/basic-authority/src/lib.rs @@ -194,8 +194,8 @@ impl Engine for BasicAuthority { } } - fn set_signer(&self, signer: Box) { - *self.signer.write() = Some(signer); + fn set_signer(&self, signer: Option>) { + *self.signer.write() = signer; } fn sign(&self, hash: H256) -> Result { @@ -269,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()]); @@ -287,7 +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.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 05cb5d262..ac8ed4b86 100644 --- a/ethcore/engines/clique/src/lib.rs +++ b/ethcore/engines/clique/src/lib.rs @@ -762,9 +762,14 @@ impl Engine for Clique { } } - fn set_signer(&self, signer: Box) { - trace!(target: "engine", "set_signer: {}", signer.address()); - *self.signer.write() = Some(signer); + fn set_signer(&self, signer: Option>) { + 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) = &*current_signer { + trace!(target: "engine", "set_signer: cleared; previous signer: {:?})", signer.address()); + } + *current_signer = signer; } fn register_client(&self, client: Weak) { diff --git a/ethcore/src/miner/miner.rs b/ethcore/src/miner/miner.rs index b6a70d231..d97b11702 100644 --- a/ethcore/src/miner/miner.rs +++ b/ethcore/src/miner/miner.rs @@ -893,21 +893,38 @@ impl miner::MinerService for Miner { self.params.write().extra_data = extra_data; } - fn set_author(&self, author: Author) { - self.params.write().author = author.address(); + fn set_author>>(&self, author: T) { + let author_opt = author.into(); + self.params.write().author = author_opt.as_ref().map(Author::address).unwrap_or_default(); - 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(signer); - } else { - warn!("Setting an EngineSigner while Engine does not require one."); + 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); + } } } } diff --git a/ethcore/src/miner/mod.rs b/ethcore/src/miner/mod.rs index 3b68359ac..9961f12af 100644 --- a/ethcore/src/miner/mod.rs +++ b/ethcore/src/miner/mod.rs @@ -132,7 +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); + fn set_author>>(&self, author: T); // Transaction Pool diff --git a/rpc/src/v1/impls/light/parity_set.rs b/rpc/src/v1/impls/light/parity_set.rs index 8195e4dda..bb2ca0aee 100644 --- a/rpc/src/v1/impls/light/parity_set.rs +++ b/rpc/src/v1/impls/light/parity_set.rs @@ -75,6 +75,10 @@ impl ParitySet for ParitySetClient { Err(errors::light_unimplemented(None)) } + fn clear_engine_signer(&self) -> Result { + Err(errors::light_unimplemented(None)) + } + fn set_transactions_limit(&self, _limit: usize) -> Result { Err(errors::light_unimplemented(None)) } diff --git a/rpc/src/v1/impls/parity_set.rs b/rpc/src/v1/impls/parity_set.rs index 7d7b46847..0db3c99ca 100644 --- a/rpc/src/v1/impls/parity_set.rs +++ b/rpc/src/v1/impls/parity_set.rs @@ -166,6 +166,11 @@ impl ParitySet for ParitySetClient where Ok(true) } + fn clear_engine_signer(&self) -> Result { + self.miner.set_author(None); + Ok(true) + } + fn add_reserved_peer(&self, peer: String) -> Result { 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 df7102e15..0af99e122 100644 --- a/rpc/src/v1/tests/helpers/miner_service.rs +++ b/rpc/src/v1/tests/helpers/miner_service.rs @@ -127,10 +127,13 @@ 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>>(&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, } } diff --git a/rpc/src/v1/traits/parity_set.rs b/rpc/src/v1/traits/parity_set.rs index 0c328b6b3..94fe9fa38 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; + /// Unsets the engine signer account address. + #[rpc(name = "parity_clearEngineSigner")] + fn clear_engine_signer(&self) -> Result; + /// Sets the limits for transaction queue. #[rpc(name = "parity_setTransactionsLimit")] fn set_transactions_limit(&self, usize) -> Result;