From 7b2afdfc8c0b5469d4201c5d749b64ede8420942 Mon Sep 17 00:00:00 2001 From: Vladyslav Lupashevskyi Date: Sun, 31 Mar 2019 11:39:38 +0300 Subject: [PATCH] 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 | 3 +- ethcore/src/client/client.rs | 13 ++++--- ethcore/src/miner/miner.rs | 30 ++++++++++++++-- ethcore/src/miner/pool_client.rs | 10 ++---- miner/src/service_transaction_checker.rs | 46 ++++++++++++++++++++++-- 5 files changed, 82 insertions(+), 20 deletions(-) diff --git a/ethcore/private-tx/src/lib.rs b/ethcore/private-tx/src/lib.rs index 64381b497..d487b4d83 100644 --- a/ethcore/private-tx/src/lib.rs +++ b/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
) -> 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 ) } diff --git a/ethcore/src/client/client.rs b/ethcore/src/client/client.rs index 1662256f4..0b07d7103 100644 --- a/ethcore/src/client/client.rs +++ b/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), diff --git a/ethcore/src/miner/miner.rs b/ethcore/src/miner/miner.rs index adaeff087..d9d4570a7 100644 --- a/ethcore/src/miner/miner.rs +++ b/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, accounts: Arc, io_channel: RwLock>>, + service_transaction_checker: Option, } 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 { + 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,7 +1296,7 @@ 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( @@ -1291,7 +1304,7 @@ impl miner::MinerService for Miner { &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 { diff --git a/ethcore/src/miner/pool_client.rs b/ethcore/src/miner/pool_client.rs index df364d446..60e93dee8 100644 --- a/ethcore/src/miner/pool_client.rs +++ b/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, + 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, } } diff --git a/miner/src/service_transaction_checker.rs b/miner/src/service_transaction_checker.rs index c9b3b4b23..56e65c8b8 100644 --- a/miner/src/service_transaction_checker.rs +++ b/miner/src/service_transaction_checker.rs @@ -16,11 +16,15 @@ //! 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"); @@ -28,9 +32,12 @@ const SERVICE_TRANSACTION_CONTRACT_REGISTRY_NAME: &'static str = "service_transa /// Service transactions checker. #[derive(Default, Clone)] -pub struct ServiceTransactionChecker; +pub struct ServiceTransactionChecker { + certified_addresses_cache: Arc>> +} impl ServiceTransactionChecker { + /// Checks if given address in tx is whitelisted to send service transactions. pub fn check(&self, client: &C, tx: &SignedTransaction) -> Result { let sender = tx.sender(); @@ -44,9 +51,42 @@ impl ServiceTransactionChecker { /// Checks if given address is whitelisted to send service transactions. pub fn check_address(&self, client: &C, sender: Address) -> Result { + 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(&self, client: &C) -> Result { + 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 = 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(&self, client: &C, contract_address: Address, sender: Address) -> Result { 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())