Skip to content
This repository was archived by the owner on Nov 6, 2020. It is now read-only.

Allow configuration of when to reseal blocks. #1460

Merged
merged 14 commits into from
Jun 28, 2016
Merged
7 changes: 4 additions & 3 deletions ethcore/src/client/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,8 @@ impl<V> Client<V> where V: Verifier {
let mut state_db = journaldb::new(
&append_path(&path, "state"),
config.pruning,
state_db_config);
state_db_config
);

if state_db.is_empty() && spec.ensure_db_good(state_db.as_hashdb_mut()) {
state_db.commit(0, &spec.genesis_header().hash(), None).expect("Error commiting genesis state to state DB");
Expand Down Expand Up @@ -800,8 +801,8 @@ impl<V> BlockChainClient for Client<V> where V: Verifier {
}
}

fn all_transactions(&self) -> Vec<SignedTransaction> {
self.miner.all_transactions()
fn pending_transactions(&self) -> Vec<SignedTransaction> {
self.miner.pending_transactions()
}
}

Expand Down
2 changes: 1 addition & 1 deletion ethcore/src/client/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@ pub trait BlockChainClient : Sync + Send {
fn queue_transactions(&self, transactions: Vec<Bytes>);

/// list all transactions
fn all_transactions(&self) -> Vec<SignedTransaction>;
fn pending_transactions(&self) -> Vec<SignedTransaction>;

/// Get the gas price distribution.
fn gas_price_statistics(&self, sample_size: usize, distribution_size: usize) -> Result<Vec<U256>, ()> {
Expand Down
4 changes: 2 additions & 2 deletions ethcore/src/client/test_client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -500,7 +500,7 @@ impl BlockChainClient for TestBlockChainClient {
self.import_transactions(tx);
}

fn all_transactions(&self) -> Vec<SignedTransaction> {
self.miner.all_transactions()
fn pending_transactions(&self) -> Vec<SignedTransaction> {
self.miner.pending_transactions()
}
}
117 changes: 86 additions & 31 deletions ethcore/src/miner/miner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,14 +29,56 @@ use spec::Spec;
use engine::Engine;
use miner::{MinerService, MinerStatus, TransactionQueue, AccountDetails, TransactionImportResult, TransactionOrigin};

/// Different possible definitions for pending transaction set.
#[derive(Debug)]
pub enum PendingSet {
/// Always just the transactions in the queue. These have had only cheap checks.
AlwaysQueue,
/// Always just the transactions in the sealing block. These have had full checks but
/// may be empty if the node is not actively mining or has force_sealing enabled.
AlwaysSealing,
/// Try the sealing block, but if it is not currently sealing, fallback to the queue.
SealingOrElseQueue,
}

/// Configures the behaviour of the miner.
#[derive(Debug)]
pub struct MinerOptions {
/// Force the miner to reseal, even when nobody has asked for work.
pub force_sealing: bool,
/// Reseal on receipt of new external transactions.
pub reseal_on_external_tx: bool,
/// Reseal on receipt of new local transactions.
pub reseal_on_own_tx: bool,
/// Maximum amount of gas to bother considering for block insertion.
pub max_tx_gas: U256,
/// Maximum size of the transaction queue.
pub tx_queue_size: usize,
/// Whether we should fallback to providing all the queue's transactions or just pending.
pub pending_set: PendingSet,
}

impl Default for MinerOptions {
fn default() -> Self {
MinerOptions {
force_sealing: false,
reseal_on_external_tx: true,
reseal_on_own_tx: true,
max_tx_gas: !U256::zero(),
tx_queue_size: 1024,
pending_set: PendingSet::AlwaysQueue,
}
}
}

/// Keeps track of transactions using priority queue and holds currently mined block.
pub struct Miner {
// NOTE [ToDr] When locking always lock in this order!
transaction_queue: Mutex<TransactionQueue>,
sealing_work: Mutex<UsingQueue<ClosedBlock>>,

// for sealing...
force_sealing: bool,
options: MinerOptions,
sealing_enabled: AtomicBool,
sealing_block_last_request: Mutex<u64>,
gas_range_target: RwLock<(U256, U256)>,
Expand All @@ -52,7 +94,7 @@ impl Miner {
pub fn with_spec(spec: Spec) -> Miner {
Miner {
transaction_queue: Mutex::new(TransactionQueue::new()),
force_sealing: false,
options: Default::default(),
sealing_enabled: AtomicBool::new(false),
sealing_block_last_request: Mutex::new(0),
sealing_work: Mutex::new(UsingQueue::new(5)),
Expand All @@ -65,11 +107,11 @@ impl Miner {
}

/// Creates new instance of miner
pub fn new(force_sealing: bool, spec: Spec, accounts: Option<Arc<AccountProvider>>) -> Arc<Miner> {
pub fn new(options: MinerOptions, spec: Spec, accounts: Option<Arc<AccountProvider>>) -> Arc<Miner> {
Arc::new(Miner {
transaction_queue: Mutex::new(TransactionQueue::new()),
force_sealing: force_sealing,
sealing_enabled: AtomicBool::new(force_sealing),
transaction_queue: Mutex::new(TransactionQueue::with_limits(options.tx_queue_size, options.max_tx_gas)),
sealing_enabled: AtomicBool::new(options.force_sealing),
options: options,
sealing_block_last_request: Mutex::new(0),
sealing_work: Mutex::new(UsingQueue::new(5)),
gas_range_target: RwLock::new((U256::zero(), U256::zero())),
Expand Down Expand Up @@ -356,6 +398,10 @@ impl MinerService for Miner {
self.transaction_queue.lock().unwrap().set_limit(limit)
}

fn set_tx_gas_limit(&self, limit: U256) {
self.transaction_queue.lock().unwrap().set_tx_gas_limit(limit)
}

/// Get the author that we will seal blocks as.
fn author(&self) -> Address {
*self.author.read().unwrap()
Expand Down Expand Up @@ -385,7 +431,7 @@ impl MinerService for Miner {
.map(|tx| transaction_queue.add(tx, &fetch_account, TransactionOrigin::External))
.collect()
};
if !results.is_empty() {
if !results.is_empty() && self.options.reseal_on_external_tx {
self.update_sealing(chain);
}
results
Expand Down Expand Up @@ -420,7 +466,7 @@ impl MinerService for Miner {
import
};

if imported.is_ok() {
if imported.is_ok() && self.options.reseal_on_own_tx {
// Make sure to do it after transaction is imported and lock is droped.
// We need to create pending block and enable sealing
let prepared = self.enable_and_prepare_sealing(chain);
Expand All @@ -434,39 +480,48 @@ impl MinerService for Miner {
imported
}

fn pending_transactions_hashes(&self) -> Vec<H256> {
fn all_transactions(&self) -> Vec<SignedTransaction> {
let queue = self.transaction_queue.lock().unwrap();
match (self.sealing_enabled.load(atomic::Ordering::Relaxed), self.sealing_work.lock().unwrap().peek_last_ref()) {
(true, Some(pending)) => pending.transactions().iter().map(|t| t.hash()).collect(),
_ => {
queue.pending_hashes()
}
}
queue.top_transactions()
}

fn transaction(&self, hash: &H256) -> Option<SignedTransaction> {
fn pending_transactions(&self) -> Vec<SignedTransaction> {
let queue = self.transaction_queue.lock().unwrap();
match (self.sealing_enabled.load(atomic::Ordering::Relaxed), self.sealing_work.lock().unwrap().peek_last_ref()) {
(true, Some(pending)) => pending.transactions().iter().find(|t| &t.hash() == hash).cloned(),
_ => {
queue.find(hash)
}
let sw = self.sealing_work.lock().unwrap();
// TODO: should only use the sealing_work when it's current (it could be an old block)
let sealing_set = match self.sealing_enabled.load(atomic::Ordering::Relaxed) {
true => sw.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().clone()),
}
}

fn all_transactions(&self) -> Vec<SignedTransaction> {
fn pending_transactions_hashes(&self) -> Vec<H256> {
let queue = self.transaction_queue.lock().unwrap();
queue.top_transactions()
let sw = self.sealing_work.lock().unwrap();
let sealing_set = match self.sealing_enabled.load(atomic::Ordering::Relaxed) {
true => sw.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()),
}
}

fn pending_transactions(&self) -> Vec<SignedTransaction> {
fn transaction(&self, hash: &H256) -> Option<SignedTransaction> {
let queue = self.transaction_queue.lock().unwrap();
// TODO: should only use the sealing_work when it's current (it could be an old block)
match (self.sealing_enabled.load(atomic::Ordering::Relaxed), self.sealing_work.lock().unwrap().peek_last_ref()) {
(true, Some(pending)) => pending.transactions().clone(),
_ => {
queue.top_transactions()
}
let sw = self.sealing_work.lock().unwrap();
Copy link
Contributor Author

@gavofyork gavofyork Jun 27, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

anyone know if this is the correct order for the lock? (it's the same as before, so should be good...)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks correct

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

let sealing_set = match self.sealing_enabled.load(atomic::Ordering::Relaxed) {
true => sw.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()),
}
}

Expand Down Expand Up @@ -494,7 +549,7 @@ impl MinerService for Miner {
let current_no = chain.chain_info().best_block_number;
let has_local_transactions = self.transaction_queue.lock().unwrap().has_local_pending_transactions();
let last_request = *self.sealing_block_last_request.lock().unwrap();
let should_disable_sealing = !self.force_sealing
let should_disable_sealing = !self.options.force_sealing
&& !has_local_transactions
&& current_no > last_request
&& current_no - last_request > SEALING_TIMEOUT_IN_BLOCKS;
Expand Down
5 changes: 4 additions & 1 deletion ethcore/src/miner/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ mod external;
mod transaction_queue;

pub use self::transaction_queue::{TransactionQueue, AccountDetails, TransactionImportResult, TransactionOrigin};
pub use self::miner::{Miner};
pub use self::miner::{Miner, MinerOptions, PendingSet};
pub use self::external::{ExternalMiner, ExternalMinerService};

use std::collections::BTreeMap;
Expand Down Expand Up @@ -101,6 +101,9 @@ pub trait MinerService : Send + Sync {
/// Set maximal number of transactions kept in the queue (both current and future).
fn set_transactions_limit(&self, limit: usize);

/// Set maximum amount of gas allowed for any single transaction to mine.
fn set_tx_gas_limit(&self, limit: U256);

/// Imports transactions to transaction queue.
fn import_transactions<T>(&self, chain: &MiningBlockChainClient, transactions: Vec<SignedTransaction>, fetch_account: T) ->
Vec<Result<TransactionImportResult, Error>>
Expand Down
43 changes: 31 additions & 12 deletions ethcore/src/miner/transaction_queue.rs
Original file line number Diff line number Diff line change
Expand Up @@ -334,6 +334,8 @@ const GAS_LIMIT_HYSTERESIS: usize = 10; // %
pub struct TransactionQueue {
/// Gas Price threshold for transactions that can be imported to this queue (defaults to 0)
minimal_gas_price: U256,
/// The maximum amount of gas any individual transaction may use.
tx_gas_limit: U256,
/// Current gas limit (block gas limit * factor). Transactions above the limit will not be accepted (default to !0)
gas_limit: U256,
/// Priority queue for transactions that can go to block
Expand All @@ -355,11 +357,11 @@ impl Default for TransactionQueue {
impl TransactionQueue {
/// Creates new instance of this Queue
pub fn new() -> Self {
Self::with_limit(1024)
Self::with_limits(1024, !U256::zero())
}

/// Create new instance of this Queue with specified limits
pub fn with_limit(limit: usize) -> Self {
pub fn with_limits(limit: usize, tx_gas_limit: U256) -> Self {
Copy link
Collaborator

@tomusdrw tomusdrw Jun 28, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry to complain again, but maybe removing it from constructor would be good too? No strong feelings though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's changed by setter anyway, other stuff like minimal_gas_price,gas_limit,etc is not here also (because there are sensible defaults / it's unbounded without negative consequences)

let current = TransactionSet {
by_priority: BTreeSet::new(),
by_address: Table::new(),
Expand All @@ -374,6 +376,7 @@ impl TransactionQueue {

TransactionQueue {
minimal_gas_price: U256::zero(),
tx_gas_limit: tx_gas_limit,
gas_limit: !U256::zero(),
current: current,
future: future,
Expand Down Expand Up @@ -418,6 +421,12 @@ impl TransactionQueue {
};
}

/// Set the new limit for the amount of gas any individual transaction may have.
/// Any transaction already imported to the queue is not affected.
pub fn set_tx_gas_limit(&mut self, limit: U256) {
self.tx_gas_limit = limit;
}

/// Returns current status for this queue
pub fn status(&self) -> TransactionQueueStatus {
TransactionQueueStatus {
Expand All @@ -435,7 +444,9 @@ impl TransactionQueue {
if tx.gas_price < self.minimal_gas_price {
trace!(target: "miner",
"Dropping transaction below minimal gas price threshold: {:?} (gp: {} < {})",
tx.hash(), tx.gas_price, self.minimal_gas_price
tx.hash(),
tx.gas_price,
self.minimal_gas_price
);

return Err(Error::Transaction(TransactionError::InsufficientGasPrice {
Expand All @@ -446,10 +457,13 @@ impl TransactionQueue {

try!(tx.check_low_s());

if tx.gas > self.gas_limit {
if tx.gas > self.gas_limit || tx.gas > self.tx_gas_limit {
trace!(target: "miner",
"Dropping transaction above gas limit: {:?} ({} > {})",
tx.hash(), tx.gas, self.gas_limit
"Dropping transaction above gas limit: {:?} ({} > min({}, {}))",
tx.hash(),
tx.gas,
self.gas_limit,
self.tx_gas_limit
);

return Err(Error::Transaction(TransactionError::GasLimitExceeded {
Expand All @@ -463,8 +477,13 @@ impl TransactionQueue {

let cost = vtx.transaction.value + vtx.transaction.gas_price * vtx.transaction.gas;
if client_account.balance < cost {
trace!(target: "miner", "Dropping transaction without sufficient balance: {:?} ({} < {})",
vtx.hash(), client_account.balance, cost);
trace!(target: "miner",
"Dropping transaction without sufficient balance: {:?} ({} < {})",
vtx.hash(),
client_account.balance,
cost
);

return Err(Error::Transaction(TransactionError::InsufficientBalance {
cost: cost,
balance: client_account.balance
Expand Down Expand Up @@ -1288,7 +1307,7 @@ mod test {
#[test]
fn should_drop_old_transactions_when_hitting_the_limit() {
// given
let mut txq = TransactionQueue::with_limit(1);
let mut txq = TransactionQueue::with_limits(1, !U256::zero());
let (tx, tx2) = new_txs(U256::one());
let sender = tx.sender().unwrap();
let nonce = tx.nonce;
Expand All @@ -1310,7 +1329,7 @@ mod test {
#[test]
fn should_return_correct_nonces_when_dropped_because_of_limit() {
// given
let mut txq = TransactionQueue::with_limit(2);
let mut txq = TransactionQueue::with_limits(2, !U256::zero());
let tx = new_tx();
let (tx1, tx2) = new_txs(U256::one());
let sender = tx1.sender().unwrap();
Expand All @@ -1331,7 +1350,7 @@ mod test {

#[test]
fn should_limit_future_transactions() {
let mut txq = TransactionQueue::with_limit(1);
let mut txq = TransactionQueue::with_limits(1, !U256::zero());
txq.current.set_limit(10);
let (tx1, tx2) = new_txs_with_gas_price_diff(U256::from(4), U256::from(1));
let (tx3, tx4) = new_txs_with_gas_price_diff(U256::from(4), U256::from(2));
Expand Down Expand Up @@ -1591,7 +1610,7 @@ mod test {
#[test]
fn should_keep_right_order_in_future() {
// given
let mut txq = TransactionQueue::with_limit(1);
let mut txq = TransactionQueue::with_limits(1, !U256::zero());
let (tx1, tx2) = new_txs(U256::from(1));
let prev_nonce = |a: &Address| AccountDetails { nonce: default_nonce(a).nonce - U256::one(), balance:
default_nonce(a).balance };
Expand Down
Loading