SecretStore: get rid of engine.signer dependency (#8173)

* SecretStore: get rid of engine.signer dependency

* SecretStore: fixed self for transact_contract

* SecretStore: fixed pending requests + 1-of-1 sessions completion

* SecretStore: fixed completion signal in 1-of-1 case

* fixed test(s)

* removed obsolete TODO && redundant statement

* ok_or -> ok_or_else

Cargo.lock

@@ -766,6 +766,7 @@ dependencies = [
  "ethcore 1.11.0",
  "ethcore-bytes 0.1.0",
  "ethcore-logger 1.11.0",
+ "ethcore-transaction 0.1.0",
  "ethcrypto 0.1.0",
  "ethereum-types 0.3.0 (registry+https://github.com/rust-lang/crates.io-index)",
  "ethkey 0.3.0",

parity/run.rs

@@ -843,6 +843,7 @@ fn execute_impl<Cr, Rr>(cmd: RunCmd, logger: Arc<RotatingLogger>, on_client_rq:
 	let secretstore_deps = secretstore::Dependencies {
 		client: client.clone(),
 		sync: sync_provider.clone(),
+		miner: miner,
 		account_provider: account_provider,
 		accounts_passwords: &passwords,
 	};

parity/secretstore.rs

@@ -20,6 +20,7 @@ use dir::default_data_path;
 use dir::helpers::replace_home;
 use ethcore::account_provider::AccountProvider;
 use ethcore::client::Client;
+use ethcore::miner::Miner;
 use ethkey::{Secret, Public};
 use ethsync::SyncProvider;
 use ethereum_types::Address;
@@ -87,6 +88,8 @@ pub struct Dependencies<'a> {
 	pub client: Arc<Client>,
 	/// Sync provider.
 	pub sync: Arc<SyncProvider>,
+	/// Miner service.
+	pub miner: Arc<Miner>,
 	/// Account provider.
 	pub account_provider: Arc<AccountProvider>,
 	/// Passed accounts passwords.
@@ -190,7 +193,7 @@ mod server {
 
 			cconf.cluster_config.nodes.insert(self_secret.public().clone(), cconf.cluster_config.listener_address.clone());
 
-			let key_server = ethcore_secretstore::start(deps.client, deps.sync, self_secret, cconf)
+			let key_server = ethcore_secretstore::start(deps.client, deps.sync, deps.miner, self_secret, cconf)
 				.map_err(|e| format!("Error starting KeyServer {}: {}", key_server_name, e))?;
 
 			Ok(KeyServer {

secret_store/Cargo.toml

@@ -24,6 +24,7 @@ tokio-proto = "0.1"
 url = "1.0"
 ethcore = { path = "../ethcore" }
 ethcore-bytes = { path = "../util/bytes" }
+ethcore-transaction = { path = "../ethcore/transaction" }
 ethereum-types = "0.3"
 ethsync = { path = "../sync" }
 kvdb = { path = "../util/kvdb" }

secret_store/src/acl_storage.rs

@@ -62,7 +62,7 @@ impl OnChainAclStorage {
 			contract: Mutex::new(CachedContract::new(trusted_client)),
 		});
 		client
-			.ok_or(Error::Internal("Constructing OnChainAclStorage without active Client".into()))?
+			.ok_or_else(|| Error::Internal("Constructing OnChainAclStorage without active Client".into()))?
 			.add_notify(acl_storage.clone());
 		Ok(acl_storage)
 	}

secret_store/src/key_server_cluster/client_sessions/generation_session.rs

@@ -326,7 +326,12 @@ impl SessionImpl {
 				self.complete_initialization(derived_point)?;
 				self.disseminate_keys()?;
 				self.verify_keys()?;
-				self.complete_generation()
+				self.complete_generation()?;
+
+				self.data.lock().state = SessionState::Finished;
+				self.completed.notify_all();
+
+				Ok(())
 			}
 		}
 	}

secret_store/src/key_server_cluster/client_sessions/signing_session_schnorr.rs

@@ -280,7 +280,7 @@ impl SessionImpl {
 			});
 			generation_session.initialize(Default::default(), Default::default(), false, 0, vec![self.core.meta.self_node_id.clone()].into_iter().collect::<BTreeSet<_>>().into())?;
 
-			debug_assert_eq!(generation_session.state(), GenerationSessionState::WaitingForGenerationConfirmation);
+			debug_assert_eq!(generation_session.state(), GenerationSessionState::Finished);
 			let joint_public_and_secret = generation_session
 				.joint_public_and_secret()
 				.expect("session key is generated before signature is computed; we are in SignatureComputing state; qed")?;

secret_store/src/key_server_cluster/cluster.rs

@@ -896,6 +896,20 @@ impl ClusterClientImpl {
 			}
 		}
 	}
+
+	fn process_initialization_result<S: ClusterSession, SC: ClusterSessionCreator<S, D>, D>(result: Result<(), Error>, session: Arc<S>, sessions: &ClusterSessionsContainer<S, SC, D>) -> Result<Arc<S>, Error> {
+		match result {
+			Ok(()) if session.is_finished() => {
+				sessions.remove(&session.id());
+				Ok(session)
+			},
+			Ok(()) => Ok(session),
+			Err(error) => {
+				sessions.remove(&session.id());
+				Err(error)
+			},
+		}
+	}
 }
 
 impl ClusterClient for ClusterClientImpl {
@@ -909,13 +923,9 @@ impl ClusterClient for ClusterClientImpl {
 
 		let cluster = create_cluster_view(&self.data, true)?;
 		let session = self.data.sessions.generation_sessions.insert(cluster, self.data.self_key_pair.public().clone(), session_id, None, false, None)?;
-		match session.initialize(origin, author, false, threshold, connected_nodes.into()) {
+		Self::process_initialization_result(
-			Ok(()) => Ok(session),
+			session.initialize(origin, author, false, threshold, connected_nodes.into()),
-			Err(error) => {
+			session, &self.data.sessions.generation_sessions)
-				self.data.sessions.generation_sessions.remove(&session.id());
-				Err(error)
-			},
-		}
 	}
 
 	fn new_encryption_session(&self, session_id: SessionId, requester: Requester, common_point: Public, encrypted_point: Public) -> Result<Arc<EncryptionSession>, Error> {
@@ -924,13 +934,9 @@ impl ClusterClient for ClusterClientImpl {
 
 		let cluster = create_cluster_view(&self.data, true)?;
 		let session = self.data.sessions.encryption_sessions.insert(cluster, self.data.self_key_pair.public().clone(), session_id, None, false, None)?;
-		match session.initialize(requester, common_point, encrypted_point) {
+		Self::process_initialization_result(
-			Ok(()) => Ok(session),
+			session.initialize(requester, common_point, encrypted_point),
-			Err(error) => {
+			session, &self.data.sessions.encryption_sessions)
-				self.data.sessions.encryption_sessions.remove(&session.id());
-				Err(error)
-			},
-		}
 	}
 
 	fn new_decryption_session(&self, session_id: SessionId, origin: Option<Address>, requester: Requester, version: Option<H256>, is_shadow_decryption: bool, is_broadcast_decryption: bool) -> Result<Arc<DecryptionSession>, Error> {
@@ -954,13 +960,9 @@ impl ClusterClient for ClusterClientImpl {
 			},
 		};
 
-		match initialization_result {
+		Self::process_initialization_result(
-			Ok(()) => Ok(session),
+			initialization_result,
-			Err(error) => {
+			session, &self.data.sessions.decryption_sessions)
-				self.data.sessions.decryption_sessions.remove(&session.id());
-				Err(error)
-			},
-		}
 	}
 
 	fn new_schnorr_signing_session(&self, session_id: SessionId, requester: Requester, version: Option<H256>, message_hash: H256) -> Result<Arc<SchnorrSigningSession>, Error> {
@@ -983,13 +985,9 @@ impl ClusterClient for ClusterClientImpl {
 			},
 		};
 
-		match initialization_result {
+		Self::process_initialization_result(
-			Ok(()) => Ok(session),
+			initialization_result,
-			Err(error) => {
+			session, &self.data.sessions.schnorr_signing_sessions)
-				self.data.sessions.schnorr_signing_sessions.remove(&session.id());
-				Err(error)
-			},
-		}
 	}
 
 	fn new_ecdsa_signing_session(&self, session_id: SessionId, requester: Requester, version: Option<H256>, message_hash: H256) -> Result<Arc<EcdsaSigningSession>, Error> {
@@ -1012,13 +1010,9 @@ impl ClusterClient for ClusterClientImpl {
 			},
 		};
 
-		match initialization_result {
+		Self::process_initialization_result(
-			Ok(()) => Ok(session),
+			initialization_result,
-			Err(error) => {
+			session, &self.data.sessions.ecdsa_signing_sessions)
-				self.data.sessions.ecdsa_signing_sessions.remove(&session.id());
-				Err(error)
-			},
-		}
 	}
 
 	fn new_key_version_negotiation_session(&self, session_id: SessionId) -> Result<Arc<KeyVersionNegotiationSession<KeyVersionNegotiationSessionTransport>>, Error> {
@@ -1042,16 +1036,13 @@ impl ClusterClient for ClusterClientImpl {
 		let initialization_result = session.as_servers_set_change().expect("servers set change session is created; qed")
 			.initialize(new_nodes_set, old_set_signature, new_set_signature);
 
-		match initialization_result {
+		if initialization_result.is_ok() {
-			Ok(()) => {
+			self.data.connections.servers_set_change_creator_connector().set_key_servers_set_change_session(session.clone());
-				self.data.connections.servers_set_change_creator_connector().set_key_servers_set_change_session(session.clone());
-				Ok(session)
-			},
-			Err(error) => {
-				self.data.sessions.admin_sessions.remove(&session.id());
-				Err(error)
-			},
 		}
+
+		Self::process_initialization_result(
+			initialization_result,
+			session, &self.data.sessions.admin_sessions)
 	}
 
 	fn add_generation_listener(&self, listener: Arc<ClusterSessionsListener<GenerationSession>>) {

secret_store/src/key_server_set.rs

@@ -141,7 +141,7 @@ impl OnChainKeyServerSet {
 			contract: Mutex::new(CachedContract::new(trusted_client, self_key_pair, auto_migrate_enabled, key_servers)?),
 		});
 		client
-			.ok_or(Error::Internal("Constructing OnChainKeyServerSet without active Client".into()))?
+			.ok_or_else(|| Error::Internal("Constructing OnChainKeyServerSet without active Client".into()))?
 			.add_notify(key_server_set.clone());
 		Ok(key_server_set)
 	}
@@ -292,7 +292,7 @@ impl CachedContract {
 			let transaction_data = self.contract.functions().start_migration().input(migration_id);
 
 			// send transaction
-			if let Err(error) = client.transact_contract(*contract_address, transaction_data) {
+			if let Err(error) = self.client.transact_contract(*contract_address, transaction_data) {
 				warn!(target: "secretstore_net", "{}: failed to submit auto-migration start transaction: {}",
 					self.self_key_pair.public(), error);
 			} else {
@@ -314,7 +314,7 @@ impl CachedContract {
 			let transaction_data = self.contract.functions().confirm_migration().input(migration_id);
 
 			// send transaction
-			if let Err(error) = client.transact_contract(contract_address, transaction_data) {
+			if let Err(error) = self.client.transact_contract(contract_address, transaction_data) {
 				warn!(target: "secretstore_net", "{}: failed to submit auto-migration confirmation transaction: {}",
 					self.self_key_pair.public(), error);
 			} else {
@@ -551,7 +551,6 @@ fn update_number_of_confirmations<F1: Fn() -> H256, F2: Fn(H256) -> Option<u64>>
 }
 
 fn update_last_transaction_block(client: &Client, migration_id: &H256, previous_transaction: &mut Option<PreviousMigrationTransaction>) -> bool {
-	// TODO [Reliability]: add the same mechanism to the contract listener, if accepted
 	let last_block = client.block_number(BlockId::Latest).unwrap_or_default();
 	match previous_transaction.as_ref() {
 		// no previous transaction => send immideately
@@ -565,7 +564,6 @@ fn update_last_transaction_block(client: &Client, migration_id: &H256, previous_
 		//   or the transaction has been removed from the queue (and never reached any miner node)
 		// if we have restarted after sending tx => assume we have never sent it
 		Some(tx) => {
-			let last_block = client.block_number(BlockId::Latest).unwrap_or_default();
 			if tx.block > last_block || last_block - tx.block < TRANSACTION_RETRY_INTERVAL_BLOCKS {
 				return false;
 			}

secret_store/src/lib.rs

@@ -19,6 +19,7 @@ extern crate ethabi;
 extern crate ethcore;
 extern crate ethcore_bytes as bytes;
 extern crate ethcore_logger as logger;
+extern crate ethcore_transaction as transaction;
 extern crate ethcrypto;
 extern crate ethereum_types;
 extern crate ethkey;
@@ -68,6 +69,7 @@ mod trusted_client;
 
 use std::sync::Arc;
 use ethcore::client::Client;
+use ethcore::miner::Miner;
 use ethsync::SyncProvider;
 
 pub use types::all::{ServerKeyId, EncryptedDocumentKey, RequestSignature, Public,
@@ -76,8 +78,8 @@ pub use traits::{NodeKeyPair, KeyServer};
 pub use self::node_key_pair::{PlainNodeKeyPair, KeyStoreNodeKeyPair};
 
 /// Start new key server instance
-pub fn start(client: Arc<Client>, sync: Arc<SyncProvider>, self_key_pair: Arc<NodeKeyPair>, config: ServiceConfiguration) -> Result<Box<KeyServer>, Error> {
+pub fn start(client: Arc<Client>, sync: Arc<SyncProvider>, miner: Arc<Miner>, self_key_pair: Arc<NodeKeyPair>, config: ServiceConfiguration) -> Result<Box<KeyServer>, Error> {
-	let trusted_client = trusted_client::TrustedClient::new(client.clone(), sync);
+	let trusted_client = trusted_client::TrustedClient::new(self_key_pair.clone(), client.clone(), sync, miner);
 	let acl_storage: Arc<acl_storage::AclStorage> = if config.acl_check_enabled {
 			acl_storage::OnChainAclStorage::new(trusted_client.clone())?
 		} else {

secret_store/src/listener/service_contract.rs

@@ -188,7 +188,7 @@ impl OnChainServiceContract {
 		let transaction_data = prepare_tx(&*client, origin, &self.contract)?;
 
 		// send transaction
-		client.transact_contract(
+		self.client.transact_contract(
 			origin.clone(),
 			transaction_data
 		).map_err(|e| format!("{}", e))?;

secret_store/src/listener/service_contract_listener.rs

@@ -268,9 +268,9 @@ impl ServiceContractListener {
 		let pending_tasks = data.contract.read_pending_requests()
 			.filter_map(|(is_confirmed, task)| Self::filter_task(data, task)
 				.map(|t| (is_confirmed, t)));
-		for (is_confirmed, task) in pending_tasks {
+		for (is_response_required, task) in pending_tasks {
 			// only process requests, which we haven't confirmed yet
-			if is_confirmed {
+			if !is_response_required {
 				continue;
 			}
 

secret_store/src/trusted_client.rs

@@ -15,24 +15,35 @@
 // along with Parity.  If not, see <http://www.gnu.org/licenses/>.
 
 use std::sync::{Arc, Weak};
-use ethcore::client::{Client, BlockChainClient, ChainInfo};
+use bytes::Bytes;
+use ethereum_types::Address;
+use ethcore::client::{Client, BlockChainClient, ChainInfo, Nonce};
+use ethcore::miner::{Miner, MinerService};
 use ethsync::SyncProvider;
+use transaction::{Transaction, SignedTransaction, Action};
+use {Error, NodeKeyPair};
 
 #[derive(Clone)]
 /// 'Trusted' client weak reference.
 pub struct TrustedClient {
+	/// This key server node key pair.
+	self_key_pair: Arc<NodeKeyPair>,
 	/// Blockchain client.
 	client: Weak<Client>,
 	/// Sync provider.
 	sync: Weak<SyncProvider>,
+	/// Miner service.
+	miner: Weak<Miner>,
 }
 
 impl TrustedClient {
 	/// Create new trusted client.
-	pub fn new(client: Arc<Client>, sync: Arc<SyncProvider>) -> Self {
+	pub fn new(self_key_pair: Arc<NodeKeyPair>, client: Arc<Client>, sync: Arc<SyncProvider>, miner: Arc<Miner>) -> Self {
 		TrustedClient {
+			self_key_pair: self_key_pair,
 			client: Arc::downgrade(&client),
 			sync: Arc::downgrade(&sync),
+			miner: Arc::downgrade(&miner),
 		}
 	}
 
@@ -54,4 +65,25 @@ impl TrustedClient {
 	pub fn get_untrusted(&self) -> Option<Arc<Client>> {
 		self.client.upgrade()
 	}
+
+	/// Transact contract.
+	pub fn transact_contract(&self, contract: Address, tx_data: Bytes) -> Result<(), Error> {
+		let client = self.client.upgrade().ok_or_else(|| Error::Internal("cannot submit tx when client is offline".into()))?;
+		let miner = self.miner.upgrade().ok_or_else(|| Error::Internal("cannot submit tx when miner is offline".into()))?;
+		let engine = client.engine();
+		let transaction = Transaction {
+			nonce: client.latest_nonce(&self.self_key_pair.address()),
+			action: Action::Call(contract),
+			gas: miner.gas_floor_target(),
+			gas_price: miner.sensible_gas_price(),
+			value: Default::default(),
+			data: tx_data,
+		};
+		let chain_id = engine.signing_chain_id(&client.latest_env_info());
+		let signature = self.self_key_pair.sign(&transaction.hash(chain_id))?;
+		let signed = SignedTransaction::new(transaction.with_signature(signature, chain_id))?;
+		miner.import_own_transaction(&*client, signed.into())
+			.map_err(|e| Error::Internal(format!("failed to import tx: {}", e)))
+			.map(|_| ())
+	}
 }