Implement caching for service transactions checker (#10088)

* Tx permission contract improvement

* Take in account zero gas price certification when doing transact_contract

* DRY in ServiceTransactionChecker

* Fix typos and regroup mod

* Introduce CertifiedAddressesCache

* Introduce refresh_cache for CertifiedAddressesCache

* Add CertifiedAddressesCache read and write on checking

* Refresh CertifiedAddressesCache on new imported block

* Separate ChainInfo trait and fix errors after merge

* Do not fire an error when service txes contract does not exist

* WIP: Shared certified addresses cache between miner and client + use HashMap instead of BTreeMap

* Refactor refresh_cache for ServiceTransactionChecker

* Refresh cache fixes

* Add cache read in check_address + log when cache is used + improve code

* Remove ChainInfo from ServiceTransaction dependencies

* DRY ServiceTransactionChecker

* Fix Client and Miner in tests

* Fix node_filter test

* Fix Client::new in add_peer_with_private_config

* WIP: Separated ChainNotify from ethcore trait and implemented ChainNotify for ServiceTransactionChecker

* Fix watcher test

* Revert "Merge branch 'master' into master"

This reverts commit 4e7371dc109d022efe3087defc33d827998ce648, reversing
changes made to bffd73e5fd58a516bbf404281b51cf26422e181e.

* Revert "Fix watcher test"

This reverts commit bffd73e5fd58a516bbf404281b51cf26422e181e.

* Revert "WIP: Separated ChainNotify from ethcore trait and implemented ChainNotify for ServiceTransactionChecker"

This reverts commit 6e73d1e61fa15dc10ffd4fab63df29eabe9c3b3a.

* Revert "Fix Client::new in add_peer_with_private_config"

This reverts commit ec610a30bee95588d58b79edcc9e43c2ff90f1ad.

* Revert "Fix node_filter test"

This reverts commit 06a4b2de86317c902f579e912b40de0b0fbf6d78.

* Revert "Fix Client and Miner in tests"

This reverts commit 51bbad330ea6e7bdfc1516208cc8705d5d11516d.

* Implement ServiceTransactionChecker in miner and delegate it to client + revert unnecessary changes

* Merge master

* Code improvements

* Merge branch 'master' of https://github.com/paritytech/parity-ethereum

# Conflicts:
#	Cargo.lock
#	ethcore/private-tx/src/lib.rs
#	ethcore/src/miner/miner.rs
#	ethcore/src/miner/pool_client.rs

ethcore/private-tx/src/lib.rs

@@ -277,13 +277,12 @@ impl Provider {
 
 	fn pool_client<'a>(&'a self, nonce_cache: &'a NonceCache, local_accounts: &'a HashSet<Address>) -> miner::pool_client::PoolClient<'a, Client> {
 		let engine = self.client.engine();
-		let refuse_service_transactions = true;
 		miner::pool_client::PoolClient::new(
 			&*self.client,
 			nonce_cache,
 			engine,
 			local_accounts,
-			refuse_service_transactions,
+			None,  // refuse_service_transactions = true
 		)
 	}
 

ethcore/src/client/client.rs

@@ -25,7 +25,6 @@ use blockchain::{BlockReceipts, BlockChain, BlockChainDB, BlockProvider, TreeRou
 use bytes::Bytes;
 use call_contract::{CallContract, RegistryInfo};
 use ethcore_miner::pool::VerifiedTransaction;
-use ethcore_miner::service_transaction_checker::ServiceTransactionChecker;
 use ethereum_types::{H256, Address, U256};
 use evm::Schedule;
 use hash::keccak;
@@ -2157,10 +2156,14 @@ impl BlockChainClient for Client {
 
 	fn transact_contract(&self, address: Address, data: Bytes) -> Result<(), transaction::Error> {
 		let authoring_params = self.importer.miner.authoring_params();
-		let service_transaction_checker = ServiceTransactionChecker::default();
-		let gas_price = match service_transaction_checker.check_address(self, authoring_params.author) {
-			Ok(true) => U256::zero(),
-			_ => self.importer.miner.sensible_gas_price(),
+		let service_transaction_checker = self.importer.miner.service_transaction_checker();
+		let gas_price = if let Some(checker) = service_transaction_checker {
+			match checker.check_address(self, authoring_params.author) {
+				Ok(true) => U256::zero(),
+				_ => self.importer.miner.sensible_gas_price(),
+			}
+		} else {
+			self.importer.miner.sensible_gas_price()
 		};
 		let transaction = transaction::Transaction {
 			nonce: self.latest_nonce(&authoring_params.author),

ethcore/src/miner/miner.rs

@@ -25,6 +25,7 @@ use call_contract::CallContract;
 use ethcore_miner::gas_pricer::GasPricer;
 use ethcore_miner::local_accounts::LocalAccounts;
 use ethcore_miner::pool::{self, TransactionQueue, VerifiedTransaction, QueueStatus, PrioritizationStrategy};
+use ethcore_miner::service_transaction_checker::ServiceTransactionChecker;
 #[cfg(feature = "work-notify")]
 use ethcore_miner::work_notify::NotifyWork;
 use ethereum_types::{H256, U256, Address};
@@ -246,6 +247,7 @@ pub struct Miner {
 	engine: Arc<EthEngine>,
 	accounts: Arc<LocalAccounts>,
 	io_channel: RwLock<Option<IoChannel<ClientIoMessage>>>,
+	service_transaction_checker: Option<ServiceTransactionChecker>,
 }
 
 impl Miner {
@@ -272,6 +274,7 @@ impl Miner {
 		let verifier_options = options.pool_verification_options.clone();
 		let tx_queue_strategy = options.tx_queue_strategy;
 		let nonce_cache_size = cmp::max(4096, limits.max_count / 4);
+		let refuse_service_transactions = options.refuse_service_transactions;
 
 		Miner {
 			sealing: Mutex::new(SealingWork {
@@ -292,6 +295,11 @@ impl Miner {
 			accounts: Arc::new(accounts),
 			engine: spec.engine.clone(),
 			io_channel: RwLock::new(None),
+			service_transaction_checker: if refuse_service_transactions {
+				None
+			} else {
+				Some(ServiceTransactionChecker::default())
+			},
 		}
 	}
 
@@ -350,6 +358,11 @@ impl Miner {
 		});
 	}
 
+	/// Returns ServiceTransactionChecker
+	pub fn service_transaction_checker(&self) -> Option<ServiceTransactionChecker> {
+		self.service_transaction_checker.clone()
+	}
+
 	/// Retrieves an existing pending block iff it's not older than given block number.
 	///
 	/// NOTE: This will not prepare a new pending block if it's not existing.
@@ -377,7 +390,7 @@ impl Miner {
 			&self.nonce_cache,
 			&*self.engine,
 			&*self.accounts,
-			self.options.refuse_service_transactions,
+			self.service_transaction_checker.as_ref(),
 		)
 	}
 
@@ -1283,15 +1296,15 @@ impl miner::MinerService for Miner {
 				let nonce_cache = self.nonce_cache.clone();
 				let engine = self.engine.clone();
 				let accounts = self.accounts.clone();
-				let refuse_service_transactions = self.options.refuse_service_transactions;
+				let service_transaction_checker = self.service_transaction_checker.clone();
 
 				let cull = move |chain: &::client::Client| {
 					let client = PoolClient::new(
 						chain,
 						&nonce_cache,
 						&*engine,
 						&*accounts,
-						refuse_service_transactions,
+						service_transaction_checker.as_ref(),
 					);
 					queue.cull(client);
 				};
@@ -1303,6 +1316,17 @@ impl miner::MinerService for Miner {
 				self.transaction_queue.cull(client);
 			}
 		}
+		if let Some(ref service_transaction_checker) = self.service_transaction_checker {
+			match service_transaction_checker.refresh_cache(chain) {
+				Ok(true) => {
+					trace!(target: "client", "Service transaction cache was refreshed successfully");
+				},
+				Ok(false) => {
+					trace!(target: "client", "Registrar or/and service transactions contract does not exist");
+				},
+				Err(e) => error!(target: "client", "Error occurred while refreshing service transaction cache: {}", e)
+			};
+		};
 	}
 
 	fn pending_state(&self, latest_block_number: BlockNumber) -> Option<Self::State> {

ethcore/src/miner/pool_client.rs

@@ -75,7 +75,7 @@ pub struct PoolClient<'a, C: 'a> {
 	engine: &'a EthEngine,
 	accounts: &'a LocalAccounts,
 	best_block_header: Header,
-	service_transaction_checker: Option<ServiceTransactionChecker>,
+	service_transaction_checker: Option<&'a ServiceTransactionChecker>,
 }
 
 impl<'a, C: 'a> Clone for PoolClient<'a, C> {
@@ -100,7 +100,7 @@ impl<'a, C: 'a> PoolClient<'a, C> where
 		cache: &'a NonceCache,
 		engine: &'a EthEngine,
 		accounts: &'a LocalAccounts,
-		refuse_service_transactions: bool,
+		service_transaction_checker: Option<&'a ServiceTransactionChecker>,
 	) -> Self {
 		let best_block_header = chain.best_block_header();
 		PoolClient {
@@ -109,11 +109,7 @@ impl<'a, C: 'a> PoolClient<'a, C> where
 			engine,
 			accounts,
 			best_block_header,
-			service_transaction_checker: if refuse_service_transactions {
-				None
-			} else {
-				Some(Default::default())
-			},
+			service_transaction_checker,
 		}
 	}
 

miner/src/service_transaction_checker.rs

@@ -16,21 +16,28 @@
 
 //! A service transactions contract checker.
 
-use call_contract::{CallContract, RegistryInfo};
+use std::collections::HashMap;
+use std::mem;
+use std::sync::Arc;
+use call_contract::{RegistryInfo, CallContract};
 use types::ids::BlockId;
 use types::transaction::SignedTransaction;
 use ethabi::FunctionOutputDecoder;
 use ethereum_types::Address;
+use parking_lot::RwLock;
 
 use_contract!(service_transaction, "res/contracts/service_transaction.json");
 
 const SERVICE_TRANSACTION_CONTRACT_REGISTRY_NAME: &'static str = "service_transaction_checker";
 
 /// Service transactions checker.
 #[derive(Default, Clone)]
-pub struct ServiceTransactionChecker;
+pub struct ServiceTransactionChecker {
+	certified_addresses_cache: Arc<RwLock<HashMap<Address, bool>>>
+}
 
 impl ServiceTransactionChecker {
+
 	/// Checks if given address in tx is whitelisted to send service transactions.
 	pub fn check<C: CallContract + RegistryInfo>(&self, client: &C, tx: &SignedTransaction) -> Result<bool, String> {
 		let sender = tx.sender();
@@ -44,9 +51,42 @@ impl ServiceTransactionChecker {
 
 	/// Checks if given address is whitelisted to send service transactions.
 	pub fn check_address<C: CallContract + RegistryInfo>(&self, client: &C, sender: Address) -> Result<bool, String> {
+		trace!(target: "txqueue", "Checking service transaction checker contract from {}", sender);
+		if let Some(allowed) = self.certified_addresses_cache.try_read().as_ref().and_then(|c| c.get(&sender)) {
+			return Ok(*allowed);
+		}
 		let contract_address = client.registry_address(SERVICE_TRANSACTION_CONTRACT_REGISTRY_NAME.to_owned(), BlockId::Latest)
 			.ok_or_else(|| "contract is not configured")?;
-		trace!(target: "txqueue", "Checking service transaction checker contract from {}", sender);
+		self.call_contract(client, contract_address, sender).and_then(|allowed| {
+			if let Some(mut cache) = self.certified_addresses_cache.try_write() {
+				cache.insert(sender, allowed);
+			};
+			Ok(allowed)
+		})
+	}
+
+	/// Refresh certified addresses cache
+	pub fn refresh_cache<C: CallContract + RegistryInfo>(&self, client: &C) -> Result<bool, String> {
+		trace!(target: "txqueue", "Refreshing certified addresses cache");
+		// replace the cache with an empty list,
+		// since it's not recent it won't be used anyway.
+		let cache = mem::replace(&mut *self.certified_addresses_cache.write(), HashMap::default());
+
+		if let Some(contract_address) = client.registry_address(SERVICE_TRANSACTION_CONTRACT_REGISTRY_NAME.to_owned(), BlockId::Latest) {
+			let addresses: Vec<_> = cache.keys().collect();
+			let mut cache: HashMap<Address, bool> = HashMap::default();
+			for address in addresses {
+				let allowed = self.call_contract(client, contract_address, *address)?;
+				cache.insert(*address, allowed);
+			}
+			mem::replace(&mut *self.certified_addresses_cache.write(),  cache);
+			Ok(true)
+		} else {
+			Ok(false)
+		}
+	}
+
+	fn call_contract<C: CallContract + RegistryInfo>(&self, client: &C, contract_address: Address, sender: Address) -> Result<bool, String> {
 		let (data, decoder) = service_transaction::functions::certified::call(sender);
 		let value = client.call_contract(BlockId::Latest, contract_address, data)?;
 		decoder.decode(&value).map_err(|e| e.to_string())