From 26faef082dc1a1da688311a0a39ec6c1bf7431ff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Drwi=C4=99ga?= Date: Fri, 7 Oct 2016 10:42:10 +0200 Subject: [PATCH 1/2] Using pending block only if not old Conflicts: ethcore/src/miner/miner.rs rpc/src/v1/impls/eth.rs rpc/src/v1/impls/eth_filter.rs --- ethcore/src/client/client.rs | 2 +- ethcore/src/client/test_client.rs | 2 +- ethcore/src/miner/miner.rs | 179 +++++++++++++++------- ethcore/src/miner/mod.rs | 11 +- rpc/src/v1/impls/eth.rs | 14 +- rpc/src/v1/impls/eth_filter.rs | 12 +- rpc/src/v1/tests/eth.rs | 3 +- rpc/src/v1/tests/helpers/miner_service.rs | 13 +- 8 files changed, 153 insertions(+), 83 deletions(-) diff --git a/ethcore/src/client/client.rs b/ethcore/src/client/client.rs index 98c8384f0e6..192bff1c8f4 100644 --- a/ethcore/src/client/client.rs +++ b/ethcore/src/client/client.rs @@ -1036,7 +1036,7 @@ impl BlockChainClient for Client { } fn pending_transactions(&self) -> Vec { - self.miner.pending_transactions() + self.miner.pending_transactions(self.chain.best_block_number()) } } diff --git a/ethcore/src/client/test_client.rs b/ethcore/src/client/test_client.rs index 132e44e0436..d6df9836937 100644 --- a/ethcore/src/client/test_client.rs +++ b/ethcore/src/client/test_client.rs @@ -558,6 +558,6 @@ impl BlockChainClient for TestBlockChainClient { } fn pending_transactions(&self) -> Vec { - self.miner.pending_transactions() + self.miner.pending_transactions(self.chain_info().best_block_number) } } diff --git a/ethcore/src/miner/miner.rs b/ethcore/src/miner/miner.rs index 8f5fef1a5ba..12b0b014d9f 100644 --- a/ethcore/src/miner/miner.rs +++ b/ethcore/src/miner/miner.rs @@ -25,6 +25,7 @@ use state::State; use client::{MiningBlockChainClient, Executive, Executed, EnvInfo, TransactOptions, BlockID, CallAnalytics}; use executive::contract_address; use block::{ClosedBlock, IsBlock, Block}; +use header::BlockNumber; use error::*; use transaction::{Action, SignedTransaction}; use receipt::{Receipt, RichReceipt}; @@ -449,6 +450,21 @@ impl Miner { /// Are we allowed to do a non-mandatory reseal? fn tx_reseal_allowed(&self) -> bool { Instant::now() > *self.next_allowed_reseal.lock() } + + fn from_pending_block(&self, latest_block_number: BlockNumber, from_chain: F, map_block: G) -> H + where F: Fn() -> H, G: Fn(&ClosedBlock) -> H { + let sealing_work = self.sealing_work.lock(); + sealing_work.queue.peek_last_ref().map_or_else( + || from_chain(), + |b| { + if b.block().header().number() > latest_block_number { + map_block(b) + } else { + from_chain() + } + } + ) + } } const SEALING_TIMEOUT_IN_BLOCKS : u64 = 5; @@ -521,29 +537,35 @@ impl MinerService for Miner { } fn balance(&self, chain: &MiningBlockChainClient, address: &Address) -> U256 { - let sealing_work = self.sealing_work.lock(); - sealing_work.queue.peek_last_ref().map_or_else( + self.from_pending_block( + chain.chain_info().best_block_number, || chain.latest_balance(address), |b| b.block().fields().state.balance(address) ) } fn storage_at(&self, chain: &MiningBlockChainClient, address: &Address, position: &H256) -> H256 { - let sealing_work = self.sealing_work.lock(); - sealing_work.queue.peek_last_ref().map_or_else( + self.from_pending_block( + chain.chain_info().best_block_number, || chain.latest_storage_at(address, position), |b| b.block().fields().state.storage_at(address, position) ) } fn nonce(&self, chain: &MiningBlockChainClient, address: &Address) -> U256 { - let sealing_work = self.sealing_work.lock(); - sealing_work.queue.peek_last_ref().map_or_else(|| chain.latest_nonce(address), |b| b.block().fields().state.nonce(address)) + self.from_pending_block( + chain.chain_info().best_block_number, + || chain.latest_nonce(address), + |b| b.block().fields().state.nonce(address) + ) } fn code(&self, chain: &MiningBlockChainClient, address: &Address) -> Option { - let sealing_work = self.sealing_work.lock(); - sealing_work.queue.peek_last_ref().map_or_else(|| chain.latest_code(address), |b| b.block().fields().state.code(address).map(|c| (*c).clone())) + self.from_pending_block( + chain.chain_info().best_block_number, + || chain.latest_code(address), + |b| b.block().fields().state.code(address).map(|c| (*c).clone()) + ) } fn set_author(&self, author: Address) { @@ -688,50 +710,74 @@ impl MinerService for Miner { queue.top_transactions() } - fn pending_transactions(&self) -> Vec { + fn pending_transactions(&self, best_block: BlockNumber) -> Vec { let queue = self.transaction_queue.lock(); - let sw = self.sealing_work.lock(); - // TODO: should only use the sealing_work when it's current (it could be an old block) - let sealing_set = match sw.enabled { - true => sw.queue.peek_last_ref(), - false => None, - }; - match (&self.options.pending_set, sealing_set) { - (&PendingSet::AlwaysQueue, _) | (&PendingSet::SealingOrElseQueue, None) => queue.top_transactions(), - (_, sealing) => sealing.map_or_else(Vec::new, |s| s.transactions().to_owned()), + match self.options.pending_set { + PendingSet::AlwaysQueue => queue.top_transactions(), + PendingSet::SealingOrElseQueue => { + self.from_pending_block( + best_block, + || queue.top_transactions(), + |sealing| sealing.transactions().to_owned() + ) + }, + PendingSet::AlwaysSealing => { + self.from_pending_block( + best_block, + || vec![], + |sealing| sealing.transactions().to_owned() + ) + }, } } - fn pending_transactions_hashes(&self) -> Vec { + fn pending_transactions_hashes(&self, best_block: BlockNumber) -> Vec { let queue = self.transaction_queue.lock(); - let sw = self.sealing_work.lock(); - let sealing_set = match sw.enabled { - true => sw.queue.peek_last_ref(), - false => None, - }; - match (&self.options.pending_set, sealing_set) { - (&PendingSet::AlwaysQueue, _) | (&PendingSet::SealingOrElseQueue, None) => queue.pending_hashes(), - (_, sealing) => sealing.map_or_else(Vec::new, |s| s.transactions().iter().map(|t| t.hash()).collect()), + match self.options.pending_set { + PendingSet::AlwaysQueue => queue.pending_hashes(), + PendingSet::SealingOrElseQueue => { + self.from_pending_block( + best_block, + || queue.pending_hashes(), + |sealing| sealing.transactions().iter().map(|t| t.hash()).collect() + ) + }, + PendingSet::AlwaysSealing => { + self.from_pending_block( + best_block, + || vec![], + |sealing| sealing.transactions().iter().map(|t| t.hash()).collect() + ) + }, } } - fn transaction(&self, hash: &H256) -> Option { + fn transaction(&self, best_block: BlockNumber, hash: &H256) -> Option { let queue = self.transaction_queue.lock(); - let sw = self.sealing_work.lock(); - let sealing_set = match sw.enabled { - true => sw.queue.peek_last_ref(), - false => None, - }; - match (&self.options.pending_set, sealing_set) { - (&PendingSet::AlwaysQueue, _) | (&PendingSet::SealingOrElseQueue, None) => queue.find(hash), - (_, sealing) => sealing.and_then(|s| s.transactions().iter().find(|t| &t.hash() == hash).cloned()), + match self.options.pending_set { + PendingSet::AlwaysQueue => queue.find(hash), + PendingSet::SealingOrElseQueue => { + self.from_pending_block( + best_block, + || queue.find(hash), + |sealing| sealing.transactions().iter().find(|t| &t.hash() == hash).cloned() + ) + }, + PendingSet::AlwaysSealing => { + self.from_pending_block( + best_block, + || None, + |sealing| sealing.transactions().iter().find(|t| &t.hash() == hash).cloned() + ) + }, } } - fn pending_receipt(&self, hash: &H256) -> Option { - let sealing_work = self.sealing_work.lock(); - match (sealing_work.enabled, sealing_work.queue.peek_last_ref()) { - (true, Some(pending)) => { + fn pending_receipt(&self, best_block: BlockNumber, hash: &H256) -> Option { + self.from_pending_block( + best_block, + || None, + |pending| { let txs = pending.transactions(); txs.iter() .map(|t| t.hash()) @@ -752,15 +798,15 @@ impl MinerService for Miner { logs: receipt.logs.clone(), } }) - }, - _ => None - } + } + ) } - fn pending_receipts(&self) -> BTreeMap { - let sealing_work = self.sealing_work.lock(); - match (sealing_work.enabled, sealing_work.queue.peek_last_ref()) { - (true, Some(pending)) => { + fn pending_receipts(&self, best_block: BlockNumber) -> BTreeMap { + self.from_pending_block( + best_block, + || BTreeMap::new(), + |pending| { let hashes = pending.transactions() .iter() .map(|t| t.hash()); @@ -768,9 +814,8 @@ impl MinerService for Miner { let receipts = pending.receipts().iter().cloned(); hashes.zip(receipts).collect() - }, - _ => BTreeMap::new() - } + } + ) } fn last_nonce(&self, address: &Address) -> Option { @@ -1001,20 +1046,38 @@ mod tests { nonce: U256::zero(), }.sign(keypair.secret()) }; - + let best_block = 0; // when let res = miner.import_own_transaction(&client, transaction); // then assert_eq!(res.unwrap(), TransactionImportResult::Current); assert_eq!(miner.all_transactions().len(), 1); - assert_eq!(miner.pending_transactions().len(), 1); - assert_eq!(miner.pending_transactions_hashes().len(), 1); - assert_eq!(miner.pending_receipts().len(), 1); + assert_eq!(miner.pending_transactions(best_block).len(), 1); + assert_eq!(miner.pending_transactions_hashes(best_block).len(), 1); + assert_eq!(miner.pending_receipts(best_block).len(), 1); // This method will let us know if pending block was created (before calling that method) assert_eq!(miner.enable_and_prepare_sealing(&client), false); } + #[test] + fn should_not_use_pending_block_if_best_block_is_higher() { + // given + let client = TestBlockChainClient::default(); + let miner = miner(); + let transaction = transaction(); + let best_block = 10; + // when + let res = miner.import_own_transaction(&client, transaction); + + // then + assert_eq!(res.unwrap(), TransactionImportResult::Current); + assert_eq!(miner.all_transactions().len(), 1); + assert_eq!(miner.pending_transactions(best_block).len(), 0); + assert_eq!(miner.pending_transactions_hashes(best_block).len(), 0); + assert_eq!(miner.pending_receipts(best_block).len(), 0); + } + #[test] fn should_import_external_transaction() { // given @@ -1031,16 +1094,16 @@ mod tests { nonce: U256::zero(), }.sign(keypair.secret()) }; - + let best_block = 0; // when let res = miner.import_external_transactions(&client, vec![transaction]).pop().unwrap(); // then assert_eq!(res.unwrap(), TransactionImportResult::Current); assert_eq!(miner.all_transactions().len(), 1); - assert_eq!(miner.pending_transactions_hashes().len(), 0); - assert_eq!(miner.pending_transactions().len(), 0); - assert_eq!(miner.pending_receipts().len(), 0); + assert_eq!(miner.pending_transactions_hashes(best_block).len(), 0); + assert_eq!(miner.pending_transactions(best_block).len(), 0); + assert_eq!(miner.pending_receipts(best_block).len(), 0); // This method will let us know if pending block was created (before calling that method) assert_eq!(miner.enable_and_prepare_sealing(&client), true); } diff --git a/ethcore/src/miner/mod.rs b/ethcore/src/miner/mod.rs index 040ea233730..ad00d9f7dd3 100644 --- a/ethcore/src/miner/mod.rs +++ b/ethcore/src/miner/mod.rs @@ -56,6 +56,7 @@ use std::collections::BTreeMap; use util::{H256, U256, Address, Bytes}; use client::{MiningBlockChainClient, Executed, CallAnalytics}; use block::ClosedBlock; +use header::BlockNumber; use receipt::{RichReceipt, Receipt}; use error::{Error, CallError}; use transaction::SignedTransaction; @@ -115,7 +116,7 @@ pub trait MinerService : Send + Sync { Result; /// Returns hashes of transactions currently in pending - fn pending_transactions_hashes(&self) -> Vec; + fn pending_transactions_hashes(&self, best_block: BlockNumber) -> Vec; /// Removes all transactions from the queue and restart mining operation. fn clear_and_reset(&self, chain: &MiningBlockChainClient); @@ -135,19 +136,19 @@ pub trait MinerService : Send + Sync { where F: FnOnce(&ClosedBlock) -> T, Self: Sized; /// Query pending transactions for hash. - fn transaction(&self, hash: &H256) -> Option; + fn transaction(&self, best_block: BlockNumber, hash: &H256) -> Option; /// Get a list of all transactions. fn all_transactions(&self) -> Vec; /// Get a list of all pending transactions. - fn pending_transactions(&self) -> Vec; + fn pending_transactions(&self, best_block: BlockNumber) -> Vec; /// Get a list of all pending receipts. - fn pending_receipts(&self) -> BTreeMap; + fn pending_receipts(&self, best_block: BlockNumber) -> BTreeMap; /// Get a particular reciept. - fn pending_receipt(&self, hash: &H256) -> Option; + fn pending_receipt(&self, best_block: BlockNumber, hash: &H256) -> Option; /// Returns highest transaction nonce for given address. fn last_nonce(&self, address: &Address) -> Option; diff --git a/rpc/src/v1/impls/eth.rs b/rpc/src/v1/impls/eth.rs index 5aced9d0c86..f4235af023d 100644 --- a/rpc/src/v1/impls/eth.rs +++ b/rpc/src/v1/impls/eth.rs @@ -32,7 +32,7 @@ use util::rlp::{encode, decode, UntrustedRlp, View}; use util::{FromHex, Mutex}; use ethcore::account_provider::AccountProvider; use ethcore::client::{MiningBlockChainClient, BlockID, TransactionID, UncleID}; -use ethcore::header::Header as BlockHeader; +use ethcore::header::{Header as BlockHeader, BlockNumber as EthBlockNumber}; use ethcore::block::IsBlock; use ethcore::views::*; use ethcore::ethereum::Ethash; @@ -193,8 +193,8 @@ impl EthClient where } } -pub fn pending_logs(miner: &M, filter: &EthcoreFilter) -> Vec where M: MinerService { - let receipts = miner.pending_receipts(); +pub fn pending_logs(miner: &M, best_block: EthBlockNumber, filter: &EthcoreFilter) -> Vec where M: MinerService { + let receipts = miner.pending_receipts(best_block); let pending_logs = receipts.into_iter() .flat_map(|(hash, r)| r.logs.into_iter().map(|l| (hash.clone(), l)).collect::>()) @@ -437,7 +437,7 @@ impl Eth for EthClient where let miner = take_weak!(self.miner); let client = take_weak!(self.client); let maybe_tx = client.transaction(TransactionID::Hash(hash)).map(Transaction::from) - .or_else(|| miner.transaction(&hash).map(Transaction::from)); + .or_else(|| miner.transaction(client.chain_info().best_block_number, &hash).map(Transaction::from)); match maybe_tx { Some(t) => to_value(&t), None => Ok(Value::Null), @@ -462,8 +462,9 @@ impl Eth for EthClient where from_params::<(RpcH256,)>(params) .and_then(|(hash,)| { let miner = take_weak!(self.miner); + let best_block = take_weak!(self.client).chain_info().best_block_number; let hash: H256 = hash.into(); - match (miner.pending_receipt(&hash), self.options.allow_pending_receipt_query) { + match (miner.pending_receipt(best_block, &hash), self.options.allow_pending_receipt_query) { (Some(receipt), true) => to_value(&Receipt::from(receipt)), _ => { let client = take_weak!(self.client); @@ -509,7 +510,8 @@ impl Eth for EthClient where .collect::>(); if include_pending { - let pending = pending_logs(&*take_weak!(self.miner), &filter); + let best_block = take_weak!(self.client).chain_info().best_block_number; + let pending = pending_logs(&*take_weak!(self.miner), best_block, &filter); logs.extend(pending); } diff --git a/rpc/src/v1/impls/eth_filter.rs b/rpc/src/v1/impls/eth_filter.rs index 973e23e0bd1..bcde74608b7 100644 --- a/rpc/src/v1/impls/eth_filter.rs +++ b/rpc/src/v1/impls/eth_filter.rs @@ -88,7 +88,8 @@ impl EthFilter for EthFilterClient where try!(expect_no_params(params)); let mut polls = self.polls.lock(); - let pending_transactions = take_weak!(self.miner).pending_transactions_hashes(); + let best_block = take_weak!(self.client).chain_info().best_block_number; + let pending_transactions = take_weak!(self.miner).pending_transactions_hashes(best_block); let id = polls.create_poll(PollFilter::PendingTransaction(pending_transactions)); to_value(&RpcU256::from(id)) @@ -118,7 +119,8 @@ impl EthFilter for EthFilterClient where }, PollFilter::PendingTransaction(ref mut previous_hashes) => { // get hashes of pending transactions - let current_hashes = take_weak!(self.miner).pending_transactions_hashes(); + let current_number = client.chain_info().best_block_number; + let current_hashes = take_weak!(self.miner).pending_transactions_hashes(current_number); let new_hashes = { @@ -159,7 +161,7 @@ impl EthFilter for EthFilterClient where // additionally retrieve pending logs if include_pending { - let pending_logs = pending_logs(&*take_weak!(self.miner), &filter); + let pending_logs = pending_logs(&*take_weak!(self.miner), current_number, &filter); // remove logs about which client was already notified about let new_pending_logs: Vec<_> = pending_logs.iter() @@ -177,7 +179,6 @@ impl EthFilter for EthFilterClient where // save the number of the next block as a first block from which // we want to get logs *block_number = current_number + 1; - to_value(&logs) } } @@ -200,7 +201,8 @@ impl EthFilter for EthFilterClient where .collect::>(); if include_pending { - logs.extend(pending_logs(&*take_weak!(self.miner), &filter)); + let best_block = take_weak!(self.client).chain_info().best_block_number; + logs.extend(pending_logs(&*take_weak!(self.miner), best_block, &filter)); } to_value(&logs) diff --git a/rpc/src/v1/tests/eth.rs b/rpc/src/v1/tests/eth.rs index 7c99e9a6e6d..ebb855fbeb7 100644 --- a/rpc/src/v1/tests/eth.rs +++ b/rpc/src/v1/tests/eth.rs @@ -25,7 +25,7 @@ use ethcore::spec::{Genesis, Spec}; use ethcore::block::Block; use ethcore::views::BlockView; use ethcore::ethereum; -use ethcore::miner::{MinerOptions, GasPricer, MinerService, ExternalMiner, Miner, PendingSet}; +use ethcore::miner::{MinerOptions, GasPricer, MinerService, ExternalMiner, Miner, PendingSet, PrioritizationStrategy}; use ethcore::account_provider::AccountProvider; use devtools::RandomTempPath; use util::Hashable; @@ -58,6 +58,7 @@ fn miner_service(spec: &Spec, accounts: Arc) -> Arc { reseal_on_external_tx: true, reseal_on_own_tx: true, tx_queue_size: 1024, + tx_queue_strategy: PrioritizationStrategy::GasPriceOnly, tx_gas_limit: !U256::zero(), pending_set: PendingSet::SealingOrElseQueue, reseal_min_period: Duration::from_secs(0), diff --git a/rpc/src/v1/tests/helpers/miner_service.rs b/rpc/src/v1/tests/helpers/miner_service.rs index ddc0b057b14..0787f2102fd 100644 --- a/rpc/src/v1/tests/helpers/miner_service.rs +++ b/rpc/src/v1/tests/helpers/miner_service.rs @@ -21,6 +21,7 @@ use util::standard::*; use ethcore::error::{Error, CallError}; use ethcore::client::{MiningBlockChainClient, Executed, CallAnalytics}; use ethcore::block::{ClosedBlock, IsBlock}; +use ethcore::header::BlockNumber; use ethcore::transaction::SignedTransaction; use ethcore::receipt::{Receipt, RichReceipt}; use ethcore::miner::{MinerService, MinerStatus, TransactionImportResult}; @@ -162,7 +163,7 @@ impl MinerService for TestMinerService { } /// Returns hashes of transactions currently in pending - fn pending_transactions_hashes(&self) -> Vec { + fn pending_transactions_hashes(&self, _best_block: BlockNumber) -> Vec { vec![] } @@ -186,7 +187,7 @@ impl MinerService for TestMinerService { Some(f(&open_block.close())) } - fn transaction(&self, hash: &H256) -> Option { + fn transaction(&self, _best_block: BlockNumber, hash: &H256) -> Option { self.pending_transactions.lock().get(hash).cloned() } @@ -194,13 +195,13 @@ impl MinerService for TestMinerService { self.pending_transactions.lock().values().cloned().collect() } - fn pending_transactions(&self) -> Vec { + fn pending_transactions(&self, _best_block: BlockNumber) -> Vec { self.pending_transactions.lock().values().cloned().collect() } - fn pending_receipt(&self, hash: &H256) -> Option { + fn pending_receipt(&self, _best_block: BlockNumber, hash: &H256) -> Option { // Not much point implementing this since the logic is complex and the only thing it relies on is pending_receipts, which is already tested. - self.pending_receipts().get(hash).map(|r| + self.pending_receipts(0).get(hash).map(|r| RichReceipt { transaction_hash: Default::default(), transaction_index: Default::default(), @@ -212,7 +213,7 @@ impl MinerService for TestMinerService { ) } - fn pending_receipts(&self) -> BTreeMap { + fn pending_receipts(&self, _best_block: BlockNumber) -> BTreeMap { self.pending_receipts.lock().clone() } From 352e685e9c80a59146a90bf7729af421fcaa3e5e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Drwi=C4=99ga?= Date: Fri, 7 Oct 2016 12:19:11 +0200 Subject: [PATCH 2/2] Fixing compilation issues --- ethcore/src/miner/miner.rs | 12 +++++++++++- ethcore/src/miner/transaction_queue.rs | 2 +- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/ethcore/src/miner/miner.rs b/ethcore/src/miner/miner.rs index 12b0b014d9f..2e5c5465e6d 100644 --- a/ethcore/src/miner/miner.rs +++ b/ethcore/src/miner/miner.rs @@ -1065,7 +1065,17 @@ mod tests { // given let client = TestBlockChainClient::default(); let miner = miner(); - let transaction = transaction(); + let transaction = { + let keypair = KeyPair::create().unwrap(); + Transaction { + action: Action::Create, + value: U256::zero(), + data: "3331600055".from_hex().unwrap(), + gas: U256::from(100_000), + gas_price: U256::zero(), + nonce: U256::zero(), + }.sign(keypair.secret()) + }; let best_block = 10; // when let res = miner.import_own_transaction(&client, transaction); diff --git a/ethcore/src/miner/transaction_queue.rs b/ethcore/src/miner/transaction_queue.rs index 4ace64662c9..05e495647ad 100644 --- a/ethcore/src/miner/transaction_queue.rs +++ b/ethcore/src/miner/transaction_queue.rs @@ -1457,7 +1457,7 @@ mod test { fn should_penalize_transactions_from_sender_in_future() { // given let prev_nonce = |a: &Address| AccountDetails{ nonce: default_nonce(a).nonce - U256::one(), balance: !U256::zero() }; - let mut txq = TransactionQueue::new(); + let mut txq = TransactionQueue::default(); // txa, txb - slightly bigger gas price to have consistent ordering let (txa, txb) = new_txs(U256::from(1)); let (tx1, tx2) = new_txs_with_higher_gas_price(U256::from(3));