Remove unnecessary Engine method (#4184)

* remove register_account_provider

* build rpc module

* new dummy client

ethcore/src/engines/authority_round.rs

@@ -234,7 +234,7 @@ impl Engine for AuthorityRound {
 		let header = block.header();
 		let step = self.step.load(AtomicOrdering::SeqCst);
 		if self.is_step_proposer(step, header.author()) {
-			let ref ap = *self.account_provider.lock();
+			let ap = self.account_provider.lock();
 			if let Ok(signature) = ap.sign(*header.author(), self.password.read().clone(), header.bare_hash()) {
 				trace!(target: "poa", "generate_seal: Issuing a block for step {}.", step);
 				self.proposed.store(true, AtomicOrdering::SeqCst);
@@ -329,12 +329,10 @@ impl Engine for AuthorityRound {
 		self.validators.register_call_contract(client);
 	}
 
-	fn set_signer(&self, _address: Address, password: String) {
+	fn set_signer(&self, ap: Arc<AccountProvider>, address: Address, password: String) {
+		*self.account_provider.lock() = ap;	
 		*self.password.write() = Some(password);
-	}
+		debug!(target: "poa", "Setting Engine signer to {}", address);
-
-	fn register_account_provider(&self, account_provider: Arc<AccountProvider>) {
-		*self.account_provider.lock() = account_provider;
 	}
 }
 
@@ -401,13 +399,12 @@ mod tests {
 
 	#[test]
 	fn generates_seal_and_does_not_double_propose() {
-		let tap = AccountProvider::transient_provider();
+		let tap = Arc::new(AccountProvider::transient_provider());
 		let addr1 = tap.insert_account(Secret::from_slice(&"1".sha3()).unwrap(), "1").unwrap();
 		let addr2 = tap.insert_account(Secret::from_slice(&"2".sha3()).unwrap(), "2").unwrap();
 
 		let spec = Spec::new_test_round();
 		let engine = &*spec.engine;
-		engine.register_account_provider(Arc::new(tap));
 		let genesis_header = spec.genesis_header();
 		let db1 = spec.ensure_db_good(get_temp_state_db().take(), &Default::default()).unwrap();
 		let db2 = spec.ensure_db_good(get_temp_state_db().take(), &Default::default()).unwrap();
@@ -417,14 +414,14 @@ mod tests {
 		let b2 = OpenBlock::new(engine, Default::default(), false, db2, &genesis_header, last_hashes, addr2, (3141562.into(), 31415620.into()), vec![]).unwrap();
 		let b2 = b2.close_and_lock();
 
-		engine.set_signer(addr1, "1".into());
+		engine.set_signer(tap.clone(), addr1, "1".into());
 		if let Seal::Regular(seal) = engine.generate_seal(b1.block()) {
 			assert!(b1.clone().try_seal(engine, seal).is_ok());
 			// Second proposal is forbidden.
 			assert!(engine.generate_seal(b1.block()) == Seal::None);
 		}
 
-		engine.set_signer(addr2, "2".into());
+		engine.set_signer(tap, addr2, "2".into());
 		if let Seal::Regular(seal) = engine.generate_seal(b2.block()) {
 			assert!(b2.clone().try_seal(engine, seal).is_ok());
 			// Second proposal is forbidden.

ethcore/src/engines/basic_authority.rs

@@ -56,7 +56,7 @@ pub struct BasicAuthority {
 	params: CommonParams,
 	gas_limit_bound_divisor: U256,
 	builtins: BTreeMap<Address, Builtin>,
-	account_provider: Mutex<Option<Arc<AccountProvider>>>,
+	account_provider: Mutex<Arc<AccountProvider>>,
 	password: RwLock<Option<String>>,
 	validators: Box<ValidatorSet + Send + Sync>,
 }
@@ -69,7 +69,7 @@ impl BasicAuthority {
 			gas_limit_bound_divisor: our_params.gas_limit_bound_divisor,
 			builtins: builtins,
 			validators: new_validator_set(our_params.validators),
-			account_provider: Mutex::new(None),
+			account_provider: Mutex::new(Arc::new(AccountProvider::transient_provider())),
 			password: RwLock::new(None),
 		}
 	}
@@ -110,7 +110,7 @@ impl Engine for BasicAuthority {
 
 	/// Attempt to seal the block internally.
 	fn generate_seal(&self, block: &ExecutedBlock) -> Seal {
-		if let Some(ref ap) = *self.account_provider.lock() {
+		let ref ap = *self.account_provider.lock();
 		let header = block.header();
 		let author = header.author();
 		if self.validators.contains(author) {
@@ -122,9 +122,6 @@ impl Engine for BasicAuthority {
 				trace!(target: "basicauthority", "generate_seal: FAIL: accounts secret key unavailable");
 			}
 		}
-		} else {
-			trace!(target: "basicauthority", "generate_seal: FAIL: accounts not provided");
-		}
 		Seal::None
 	}
 
@@ -174,12 +171,9 @@ impl Engine for BasicAuthority {
 		self.validators.register_call_contract(client);
 	}
 
-	fn set_signer(&self, _address: Address, password: String) {
+	fn set_signer(&self, ap: Arc<AccountProvider>, _address: Address, password: String) {
 		*self.password.write() = Some(password);
-	}
+		*self.account_provider.lock() = ap;
-
-	fn register_account_provider(&self, ap: Arc<AccountProvider>) {
-		*self.account_provider.lock() = Some(ap);
 	}
 }
 
@@ -256,8 +250,7 @@ mod tests {
 
 		let spec = new_test_authority();
 		let engine = &*spec.engine;
-		engine.set_signer(addr, "".into());
+		engine.set_signer(Arc::new(tap), addr, "".into());
-		engine.register_account_provider(Arc::new(tap));
 		let genesis_header = spec.genesis_header();
 		let mut db_result = get_temp_state_db();
 		let db = spec.ensure_db_good(db_result.take(), &Default::default()).unwrap();

ethcore/src/engines/mod.rs

@@ -212,14 +212,11 @@ pub trait Engine : Sync + Send {
 	fn is_proposal(&self, _verified_header: &Header) -> bool { false }
 
 	/// Register an account which signs consensus messages.
-	fn set_signer(&self, _address: Address, _password: String) {}
+	fn set_signer(&self, _account_provider: Arc<AccountProvider>, _address: Address, _password: String) {}
 
 	/// Add Client which can be used for sealing, querying the state and sending messages.
 	fn register_client(&self, _client: Weak<Client>) {}
 
-	/// Add an account provider useful for Engines that sign stuff.
-	fn register_account_provider(&self, _account_provider: Arc<AccountProvider>) {}
-
 	/// Trigger next step of the consensus engine.
 	fn step(&self) {}
 

ethcore/src/engines/tendermint/mod.rs

@@ -94,7 +94,7 @@ pub struct Tendermint {
 	/// Vote accumulator.
 	votes: VoteCollector,
 	/// Used to sign messages and proposals.
-	account_provider: Mutex<Option<Arc<AccountProvider>>>,
+	account_provider: Mutex<Arc<AccountProvider>>,
 	/// Message for the last PoLC.
 	lock_change: RwLock<Option<ConsensusMessage>>,
 	/// Last lock round.
@@ -122,7 +122,7 @@ impl Tendermint {
 				round: AtomicUsize::new(0),
 				step: RwLock::new(Step::Propose),
 				votes: VoteCollector::new(),
-				account_provider: Mutex::new(None),
+				account_provider: Mutex::new(Arc::new(AccountProvider::transient_provider())),
 				lock_change: RwLock::new(None),
 				last_lock: AtomicUsize::new(0),
 				proposal: RwLock::new(None),
@@ -158,7 +158,7 @@ impl Tendermint {
 	}
 
 	fn generate_message(&self, block_hash: Option<BlockHash>) -> Option<Bytes> {
-		if let Some(ref ap) = *self.account_provider.lock() {
+		let ref ap = *self.account_provider.lock();
 		let h = self.height.load(AtomicOrdering::SeqCst);
 		let r = self.round.load(AtomicOrdering::SeqCst);
 		let s = self.step.read();
@@ -179,10 +179,6 @@ impl Tendermint {
 				None
 			},
 		}
-		} else {
-			warn!(target: "poa", "No AccountProvider available.");
-			None
-		}
 	}
 
 	fn generate_and_broadcast_message(&self, block_hash: Option<BlockHash>) {
@@ -420,7 +416,7 @@ impl Engine for Tendermint {
 
 	/// Attempt to seal generate a proposal seal.
 	fn generate_seal(&self, block: &ExecutedBlock) -> Seal {
-		if let Some(ref ap) = *self.account_provider.lock() {
+		let ref ap = *self.account_provider.lock();
 		let header = block.header();
 		let author = header.author();
 		// Only proposer can generate seal if None was generated.
@@ -447,10 +443,6 @@ impl Engine for Tendermint {
 			warn!(target: "poa", "generate_seal: FAIL: accounts secret key unavailable");
 			Seal::None
 		}
-		} else {
-			warn!(target: "poa", "generate_seal: FAIL: accounts not provided");
-			Seal::None
-		}
 	}
 
 	fn handle_message(&self, rlp: &[u8]) -> Result<(), Error> {
@@ -563,9 +555,12 @@ impl Engine for Tendermint {
 		Ok(())
 	}
 
-	fn set_signer(&self, address: Address, password: String) {
+	fn set_signer(&self, ap: Arc<AccountProvider>, address: Address, password: String) {
+		{
 			*self.authority.write()	= address;
 			*self.password.write() = Some(password);
+			*self.account_provider.lock() = ap;
+		}
 		self.to_step(Step::Propose);
 	}
 
@@ -651,10 +646,6 @@ impl Engine for Tendermint {
 		*self.client.write() = Some(client.clone());
 		self.validators.register_call_contract(client);
 	}
-
-	fn register_account_provider(&self, account_provider: Arc<AccountProvider>) {
-		*self.account_provider.lock() = Some(account_provider);
-	}
 }
 
 #[cfg(test)]
@@ -678,7 +669,6 @@ mod tests {
 	fn setup() -> (Spec, Arc<AccountProvider>) {
 		let tap = Arc::new(AccountProvider::transient_provider());
 		let spec = Spec::new_test_tendermint();
-		spec.engine.register_account_provider(tap.clone());
 		(spec, tap)
 	}
 
@@ -722,7 +712,7 @@ mod tests {
 
 	fn insert_and_register(tap: &Arc<AccountProvider>, engine: &Engine, acc: &str) -> Address {
 		let addr = insert_and_unlock(tap, acc);
-		engine.set_signer(addr.clone(), acc.into());
+		engine.set_signer(tap.clone(), addr.clone(), acc.into());
 		addr
 	}
 
@@ -884,7 +874,7 @@ mod tests {
 		let v0 = insert_and_register(&tap, engine.as_ref(), "0");
 		let v1 = insert_and_register(&tap, engine.as_ref(), "1");
 
-		let h = 0;
+		let h = 1;
 		let r = 0;
 
 		// Propose
@@ -915,16 +905,16 @@ mod tests {
 		use types::transaction::{Transaction, Action};
 		use client::BlockChainClient;
 
-		let client = generate_dummy_client_with_spec_and_data(Spec::new_test_tendermint, 0, 0, &[]);
-		let engine = client.engine();
 		let tap = Arc::new(AccountProvider::transient_provider());
-
 		// Accounts for signing votes.
 		let v0 = insert_and_unlock(&tap, "0");
 		let v1 = insert_and_unlock(&tap, "1");
+		let client = generate_dummy_client_with_spec_and_accounts(Spec::new_test_tendermint, Some(tap.clone()));
+		let engine = client.engine();
+
+		client.miner().set_engine_signer(v1.clone(), "1".into()).unwrap();
 
 		let notify = Arc::new(TestNotify::default());
-		engine.register_account_provider(tap.clone());
 		client.add_notify(notify.clone());
 		engine.register_client(Arc::downgrade(&client));
 
@@ -939,8 +929,6 @@ mod tests {
 		}.sign(keypair.secret(), None);
 		client.miner().import_own_transaction(client.as_ref(), transaction.into()).unwrap();
 
-		client.miner().set_engine_signer(v1.clone(), "1".into()).unwrap();
-
 		// Propose
 		let proposal = Some(client.miner().pending_block().unwrap().header.bare_hash());
 		// Propose timeout

ethcore/src/engines/validator_set/contract.rs

@@ -142,13 +142,13 @@ mod tests {
 	use client::{BlockChainClient, EngineClient};
 	use ethkey::Secret;
 	use miner::MinerService;
-	use tests::helpers::generate_dummy_client_with_spec_and_data;
+	use tests::helpers::generate_dummy_client_with_spec_and_accounts;
 	use super::super::ValidatorSet;
 	use super::ValidatorContract;
 
 	#[test]
 	fn fetches_validators() {
-		let client = generate_dummy_client_with_spec_and_data(Spec::new_validator_contract, 0, 0, &[]);
+		let client = generate_dummy_client_with_spec_and_accounts(Spec::new_validator_contract, None);
 		let vc = Arc::new(ValidatorContract::new(Address::from_str("0000000000000000000000000000000000000005").unwrap()));
 		vc.register_call_contract(Arc::downgrade(&client));
 		vc.update();
@@ -162,12 +162,7 @@ mod tests {
 		let s0 = Secret::from_slice(&"1".sha3()).unwrap();
 		let v0 = tap.insert_account(s0.clone(), "").unwrap();
 		let v1 = tap.insert_account(Secret::from_slice(&"0".sha3()).unwrap(), "").unwrap();
-		let spec_factory = || {
+		let client = generate_dummy_client_with_spec_and_accounts(Spec::new_validator_contract, Some(tap));
-			let spec = Spec::new_validator_contract();
-			spec.engine.register_account_provider(tap.clone());
-			spec
-		};
-		let client = generate_dummy_client_with_spec_and_data(spec_factory, 0, 0, &[]);
 		client.engine().register_client(Arc::downgrade(&client));
 		let validator_contract = Address::from_str("0000000000000000000000000000000000000005").unwrap();
 

ethcore/src/miner/miner.rs

@@ -757,7 +757,6 @@ impl MinerService for Miner {
 		if self.seals_internally {
 			if let Some(ref ap) = self.accounts {
 				ap.sign(address.clone(), Some(password.clone()), Default::default())?;
-			}
 				// Limit the scope of the locks.
 				{
 					let mut sealing_work = self.sealing_work.lock();
@@ -769,7 +768,8 @@ impl MinerService for Miner {
 				// | (some `Engine`s call `EngineClient.update_sealing()`)                  |.
 				// | Make sure to release the locks before calling that method.             |
 				// --------------------------------------------------------------------------
-			self.engine.set_signer(address, password);
+				self.engine.set_signer(ap.clone(), address, password);
+			}
 		}
 		Ok(())
 	}

ethcore/src/tests/helpers.rs

@@ -19,6 +19,7 @@ use io::*;
 use client::{BlockChainClient, Client, ClientConfig};
 use util::*;
 use spec::*;
+use account_provider::AccountProvider;
 use state_db::StateDB;
 use block::{OpenBlock, Drain};
 use blockchain::{BlockChain, Config as BlockChainConfig};
@@ -140,7 +141,16 @@ pub fn generate_dummy_client_with_data(block_number: u32, txs_per_block: usize,
 	generate_dummy_client_with_spec_and_data(Spec::new_null, block_number, txs_per_block, tx_gas_prices)
 }
 
+
 pub fn generate_dummy_client_with_spec_and_data<F>(get_test_spec: F, block_number: u32, txs_per_block: usize, tx_gas_prices: &[U256]) -> GuardedTempResult<Arc<Client>> where F: Fn()->Spec {
+	generate_dummy_client_with_spec_accounts_and_data(get_test_spec, None, block_number, txs_per_block, tx_gas_prices)
+}
+
+pub fn generate_dummy_client_with_spec_and_accounts<F>(get_test_spec: F, accounts: Option<Arc<AccountProvider>>) -> GuardedTempResult<Arc<Client>> where F: Fn()->Spec {
+	generate_dummy_client_with_spec_accounts_and_data(get_test_spec, accounts, 0, 0, &[])
+}
+
+pub fn generate_dummy_client_with_spec_accounts_and_data<F>(get_test_spec: F, accounts: Option<Arc<AccountProvider>>, block_number: u32, txs_per_block: usize, tx_gas_prices: &[U256]) -> GuardedTempResult<Arc<Client>> where F: Fn()->Spec {
 	let dir = RandomTempPath::new();
 	let test_spec = get_test_spec();
 	let db_config = DatabaseConfig::with_columns(::db::NUM_COLUMNS);
@@ -149,7 +159,7 @@ pub fn generate_dummy_client_with_spec_and_data<F>(get_test_spec: F, block_numbe
 		ClientConfig::default(),
 		&test_spec,
 		dir.as_path(),
-		Arc::new(Miner::with_spec(&test_spec)),
+		Arc::new(Miner::with_spec_and_accounts(&test_spec, accounts)),
 		IoChannel::disconnected(),
 		&db_config
 	).unwrap();

parity/run.rs

@@ -232,9 +232,6 @@ pub fn execute(cmd: RunCmd, can_restart: bool, logger: Arc<RotatingLogger>) -> R
 	// prepare account provider
 	let account_provider = Arc::new(prepare_account_provider(&cmd.spec, &cmd.dirs, &spec.data_dir, cmd.acc_conf, &passwords)?);
 
-	// let the Engine access the accounts
-	spec.engine.register_account_provider(account_provider.clone());
-
 	// create miner
 	let miner = Miner::new(cmd.miner_options, cmd.gas_pricer.into(), &spec, Some(account_provider.clone()));
 	miner.set_author(cmd.miner_extras.author);

rpc/src/v1/tests/eth.rs

@@ -116,7 +116,6 @@ impl EthTester {
 	fn from_spec(spec: Spec) -> Self {
 		let dir = RandomTempPath::new();
 		let account_provider = account_provider();
-		spec.engine.register_account_provider(account_provider.clone());
 		let miner_service = miner_service(&spec, account_provider.clone());
 		let snapshot_service = snapshot_service();
 

sync/src/tests/consensus.rs

@@ -57,15 +57,11 @@ fn new_tx(secret: &Secret, nonce: U256) -> PendingTransaction {
 fn authority_round() {
 	let s0 = KeyPair::from_secret_slice(&"1".sha3()).unwrap();
 	let s1 = KeyPair::from_secret_slice(&"0".sha3()).unwrap();
-	let spec_factory = || {
+	let ap = Arc::new(AccountProvider::transient_provider());
-		let spec = Spec::new_test_round();
+	ap.insert_account(s0.secret().clone(), "").unwrap();
-		let account_provider = AccountProvider::transient_provider();
+	ap.insert_account(s1.secret().clone(), "").unwrap();
-		account_provider.insert_account(s0.secret().clone(), "").unwrap();
+
-		account_provider.insert_account(s1.secret().clone(), "").unwrap();
+	let mut net = TestNet::with_spec_and_accounts(2, SyncConfig::default(), Spec::new_test_round, Some(ap));
-		spec.engine.register_account_provider(Arc::new(account_provider));
-		spec
-	};
-	let mut net = TestNet::with_spec(2, SyncConfig::default(), spec_factory);
 	let mut net = &mut *net;
 	let io_handler0: Arc<IoHandler<ClientIoMessage>> = Arc::new(TestIoHandler { client: net.peer(0).chain.clone() });
 	let io_handler1: Arc<IoHandler<ClientIoMessage>> = Arc::new(TestIoHandler { client: net.peer(1).chain.clone() });
@@ -120,15 +116,11 @@ fn authority_round() {
 fn tendermint() {
 	let s0 = KeyPair::from_secret_slice(&"1".sha3()).unwrap();
 	let s1 = KeyPair::from_secret_slice(&"0".sha3()).unwrap();
-	let spec_factory = || {
+	let ap = Arc::new(AccountProvider::transient_provider());
-		let spec = Spec::new_test_tendermint();
+	ap.insert_account(s0.secret().clone(), "").unwrap();
-		let account_provider = AccountProvider::transient_provider();
+	ap.insert_account(s1.secret().clone(), "").unwrap();
-		account_provider.insert_account(s0.secret().clone(), "").unwrap();
+
-		account_provider.insert_account(s1.secret().clone(), "").unwrap();
+	let mut net = TestNet::with_spec_and_accounts(2, SyncConfig::default(), Spec::new_test_tendermint, Some(ap));
-		spec.engine.register_account_provider(Arc::new(account_provider));
-		spec
-	};
-	let mut net = TestNet::with_spec(2, SyncConfig::default(), spec_factory);
 	let mut net = &mut *net;
 	let io_handler0: Arc<IoHandler<ClientIoMessage>> = Arc::new(TestIoHandler { client: net.peer(0).chain.clone() });
 	let io_handler1: Arc<IoHandler<ClientIoMessage>> = Arc::new(TestIoHandler { client: net.peer(1).chain.clone() });

sync/src/tests/helpers.rs

@@ -21,6 +21,7 @@ use ethcore::client::{TestBlockChainClient, BlockChainClient, Client as EthcoreC
 use ethcore::header::BlockNumber;
 use ethcore::snapshot::SnapshotService;
 use ethcore::spec::Spec;
+use ethcore::account_provider::AccountProvider;
 use ethcore::miner::Miner;
 use ethcore::db::NUM_COLUMNS;
 use sync_io::SyncIo;
@@ -262,7 +263,7 @@ impl TestNet<EthPeer<TestBlockChainClient>> {
 }
 
 impl TestNet<EthPeer<EthcoreClient>> {
-	pub fn with_spec<F>(n: usize, config: SyncConfig, spec_factory: F) -> GuardedTempResult<Self>
+	pub fn with_spec_and_accounts<F>(n: usize, config: SyncConfig, spec_factory: F, accounts: Option<Arc<AccountProvider>>) -> GuardedTempResult<Self>
 		where F: Fn() -> Spec
 	{
 		let mut net = TestNet {
@@ -282,7 +283,7 @@ impl TestNet<EthPeer<EthcoreClient>> {
 				ClientConfig::default(),
 				&spec,
 				client_dir.as_path(),
-				Arc::new(Miner::with_spec(&spec)),
+				Arc::new(Miner::with_spec_and_accounts(&spec, accounts.clone())),
 				IoChannel::disconnected(),
 				&db_config
 			).unwrap();