From f2dc091be07efe3c44b4c44ab7908c03492fb660 Mon Sep 17 00:00:00 2001 From: Vladimir Komendantskiy Date: Tue, 2 Apr 2019 16:03:44 +0100 Subject: [PATCH] RPC method parity_clearEngineSigner (#114) Add RPC method parity_clearEngineSigner Fixes https://github.com/poanetwork/parity-ethereum/issues/113 --- ethcore/src/block.rs | 2 +- ethcore/src/engines/authority_round/mod.rs | 60 ++++++++++++++++++++++ ethcore/src/engines/basic_authority.rs | 6 +++ ethcore/src/engines/mod.rs | 3 ++ ethcore/src/miner/miner.rs | 10 +++- 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 ++ 10 files changed, 100 insertions(+), 2 deletions(-) diff --git a/ethcore/src/block.rs b/ethcore/src/block.rs index c07373803f5..56cfc1c4c14 100644 --- a/ethcore/src/block.rs +++ b/ethcore/src/block.rs @@ -60,7 +60,7 @@ use types::receipt::{Receipt, TransactionOutcome}; /// It's a bit like a Vec, except that whenever a transaction is pushed, we execute it and /// maintain the system `state()`. We also archive execution receipts in preparation for later block creation. pub struct OpenBlock<'x> { - pub(crate) block: ExecutedBlock, + block: ExecutedBlock, engine: &'x EthEngine, } diff --git a/ethcore/src/engines/authority_round/mod.rs b/ethcore/src/engines/authority_round/mod.rs index 23645080767..ed901b302d9 100644 --- a/ethcore/src/engines/authority_round/mod.rs +++ b/ethcore/src/engines/authority_round/mod.rs @@ -1582,6 +1582,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 { Ok(self.signer.read() .as_ref() @@ -1720,12 +1724,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, &mut Vec::new().into_iter()) + .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(b1.header.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, &mut Vec::new().into_iter()) + .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/src/engines/basic_authority.rs b/ethcore/src/engines/basic_authority.rs index 69b8d07c526..3a50dfc8072 100644 --- a/ethcore/src/engines/basic_authority.rs +++ b/ethcore/src/engines/basic_authority.rs @@ -193,6 +193,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 { Ok(self.signer.read() .as_ref() @@ -280,5 +284,7 @@ mod tests { assert!(!engine.seals_internally().unwrap()); engine.set_signer(Box::new((Arc::new(tap), authority, "".into()))); assert!(engine.seals_internally().unwrap()); + engine.clear_signer(); + assert!(!engine.seals_internally().unwrap()); } } diff --git a/ethcore/src/engines/mod.rs b/ethcore/src/engines/mod.rs index a5322d84027..55841059b68 100644 --- a/ethcore/src/engines/mod.rs +++ b/ethcore/src/engines/mod.rs @@ -434,6 +434,9 @@ pub trait Engine: Sync + Send { /// Register a component which signs consensus messages. fn set_signer(&self, _signer: Box) {} + /// 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 { unimplemented!() } diff --git a/ethcore/src/miner/miner.rs b/ethcore/src/miner/miner.rs index eab60aa680e..fe587e1cad4 100644 --- a/ethcore/src/miner/miner.rs +++ b/ethcore/src/miner/miner.rs @@ -441,7 +441,7 @@ impl Miner { }; // Before adding from the queue to the new block, give the engine a chance to add transactions. - engine_pending = match self.engine.on_prepare_block(&open_block.block) { + engine_pending = match self.engine.on_prepare_block(&open_block) { Ok(transactions) => transactions, Err(err) => { error!(target: "miner", "Failed to prepare engine transactions for new block: {:?}. \ @@ -893,6 +893,14 @@ impl miner::MinerService for Miner { } } + fn clear_author(&self) { + self.params.write().author = Default::default(); + if self.engine.seals_internally().is_some() { + 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 fd7ab96513c..48ff5307f6b 100644 --- a/ethcore/src/miner/mod.rs +++ b/ethcore/src/miner/mod.rs @@ -133,6 +133,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 68fc212b2fb..f84933ffb33 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 b7cef6c6b8c..e82fde236a1 100644 --- a/rpc/src/v1/impls/parity_set.rs +++ b/rpc/src/v1/impls/parity_set.rs @@ -165,6 +165,11 @@ impl ParitySet for ParitySetClient where Ok(true) } + fn clear_engine_signer(&self) -> Result { + self.miner.clear_author(); + 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 b16651bd9c5..7de795a76bd 100644 --- a/rpc/src/v1/tests/helpers/miner_service.rs +++ b/rpc/src/v1/tests/helpers/miner_service.rs @@ -130,6 +130,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 c7c23387900..9c599ee2bba 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;