From 10aa32b0f5eda6a96995558db5dc8ef22e672a46 Mon Sep 17 00:00:00 2001 From: Gav Wood Date: Mon, 27 Jun 2016 20:19:01 +0200 Subject: [PATCH] Include RPC configurability for max tx gas limit. Also Move the gas limit into the transaction queue from the miner. --- ethcore/src/miner/miner.rs | 9 ++++- ethcore/src/miner/mod.rs | 3 ++ ethcore/src/miner/transaction_queue.rs | 40 ++++++++++++++++------- parity/configuration.rs | 1 + rpc/src/v1/impls/ethcore_set.rs | 9 ++++- rpc/src/v1/tests/helpers/miner_service.rs | 6 ++++ rpc/src/v1/traits/ethcore_set.rs | 4 +++ rpc/src/v1/types/optionals.rs | 26 +++++++++++++-- 8 files changed, 82 insertions(+), 16 deletions(-) diff --git a/ethcore/src/miner/miner.rs b/ethcore/src/miner/miner.rs index 72228a959..42971850a 100644 --- a/ethcore/src/miner/miner.rs +++ b/ethcore/src/miner/miner.rs @@ -53,6 +53,8 @@ pub struct MinerOptions { /// Specify maximum amount of gas to bother considering for block insertion. /// If `None`, then no limit. pub max_tx_gas: Option, + /// 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, } @@ -64,6 +66,7 @@ impl Default for MinerOptions { reseal_on_external_tx: true, reseal_on_own_tx: true, max_tx_gas: None, + tx_queue_size: 1024, pending_set: PendingSet::AlwaysQueue, } } @@ -107,7 +110,7 @@ impl Miner { /// Creates new instance of miner pub fn new(options: MinerOptions, spec: Spec, accounts: Option>) -> Arc { Arc::new(Miner { - transaction_queue: Mutex::new(TransactionQueue::new()), + 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), @@ -396,6 +399,10 @@ impl MinerService for Miner { self.transaction_queue.lock().unwrap().set_limit(limit) } + fn set_tx_gas_limit(&self, limit: Option) { + 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() diff --git a/ethcore/src/miner/mod.rs b/ethcore/src/miner/mod.rs index f02925438..a56f881a4 100644 --- a/ethcore/src/miner/mod.rs +++ b/ethcore/src/miner/mod.rs @@ -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: Option); + /// Imports transactions to transaction queue. fn import_transactions(&self, chain: &MiningBlockChainClient, transactions: Vec, fetch_account: T) -> Vec> diff --git a/ethcore/src/miner/transaction_queue.rs b/ethcore/src/miner/transaction_queue.rs index 9159cc6b3..26ec441fe 100644 --- a/ethcore/src/miner/transaction_queue.rs +++ b/ethcore/src/miner/transaction_queue.rs @@ -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, + /// If `Some`, then the maximum amount of gas any individual transaction may use. + tx_gas_limit: Option, /// 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 @@ -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, None) } /// 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: Option) -> Self { let current = TransactionSet { by_priority: BTreeSet::new(), by_address: Table::new(), @@ -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, @@ -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: Option) { + self.tx_gas_limit = limit; + } + /// Returns current status for this queue pub fn status(&self) -> TransactionQueueStatus { TransactionQueueStatus { @@ -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 { @@ -446,10 +457,12 @@ impl TransactionQueue { try!(tx.check_low_s()); - if tx.gas > self.gas_limit { + if tx.gas > self.gas_limit || self.tx_gas_limit.map(|l| tx.gas > l).unwrap_or(false) { trace!(target: "miner", "Dropping transaction above gas limit: {:?} ({} > {})", - tx.hash(), tx.gas, self.gas_limit + tx.hash(), + tx.gas, + self.gas_limit ); return Err(Error::Transaction(TransactionError::GasLimitExceeded { @@ -463,8 +476,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 @@ -1304,7 +1322,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_limit(1, None); let (tx, tx2) = new_txs(U256::one()); let sender = tx.sender().unwrap(); let nonce = tx.nonce; @@ -1326,7 +1344,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_limit(2, None); let tx = new_tx(); let (tx1, tx2) = new_txs(U256::one()); let sender = tx1.sender().unwrap(); @@ -1347,7 +1365,7 @@ mod test { #[test] fn should_limit_future_transactions() { - let mut txq = TransactionQueue::with_limit(1); + let mut txq = TransactionQueue::with_limit(1, None); 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)); @@ -1607,7 +1625,7 @@ mod test { #[test] fn should_keep_right_order_in_future() { // given - let mut txq = TransactionQueue::with_limit(1); + let mut txq = TransactionQueue::with_limit(1, None); 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 }; diff --git a/parity/configuration.rs b/parity/configuration.rs index 501cc68cb..4469eefaf 100644 --- a/parity/configuration.rs +++ b/parity/configuration.rs @@ -89,6 +89,7 @@ impl Configuration { reseal_on_external_tx: ext, reseal_on_own_tx: own, max_tx_gas: self.args.flag_max_tx_gas.as_ref().map(|d| Self::decode_u256(d, "--max-tx-gas")), + tx_queue_size: self.args.flag_tx_queue_size, pending_set: match self.args.flag_relay_set.as_str() { "cheap" => PendingSet::AlwaysQueue, "strict" => PendingSet::AlwaysSealing, diff --git a/rpc/src/v1/impls/ethcore_set.rs b/rpc/src/v1/impls/ethcore_set.rs index 55164217c..d7624442a 100644 --- a/rpc/src/v1/impls/ethcore_set.rs +++ b/rpc/src/v1/impls/ethcore_set.rs @@ -22,7 +22,7 @@ use jsonrpc_core::*; use ethcore::miner::MinerService; use ethcore::service::SyncMessage; use v1::traits::EthcoreSet; -use v1::types::{Bytes}; +use v1::types::{OptionalValue, Bytes}; /// Ethcore-specific rpc interface for operations altering the settings. pub struct EthcoreSetClient where @@ -86,6 +86,13 @@ impl EthcoreSet for EthcoreSetClient where M: MinerService + 'static { }) } + fn set_tx_gas_limit(&self, params: Params) -> Result { + from_params::<(OptionalValue,)>(params).and_then(|(limit,)| { + take_weak!(self.miner).set_tx_gas_limit(limit.into()); + to_value(&true) + }) + } + fn add_reserved_peer(&self, params: Params) -> Result { from_params::<(String,)>(params).and_then(|(peer,)| { match take_weak!(self.net).add_reserved_peer(&peer) { diff --git a/rpc/src/v1/tests/helpers/miner_service.rs b/rpc/src/v1/tests/helpers/miner_service.rs index b87b650e1..4a9b22f42 100644 --- a/rpc/src/v1/tests/helpers/miner_service.rs +++ b/rpc/src/v1/tests/helpers/miner_service.rs @@ -43,6 +43,7 @@ pub struct TestMinerService { author: RwLock
, extra_data: RwLock, limit: RwLock, + tx_gas_limit: RwLock>, } impl Default for TestMinerService { @@ -58,6 +59,7 @@ impl Default for TestMinerService { author: RwLock::new(Address::zero()), extra_data: RwLock::new(vec![1, 2, 3, 4]), limit: RwLock::new(1024), + tx_gas_limit: RwLock::new(None), } } } @@ -99,6 +101,10 @@ impl MinerService for TestMinerService { *self.limit.write().unwrap() = limit; } + fn set_tx_gas_limit(&self, limit: Option) { + *self.tx_gas_limit.write().unwrap() = limit; + } + fn transactions_limit(&self) -> usize { *self.limit.read().unwrap() } diff --git a/rpc/src/v1/traits/ethcore_set.rs b/rpc/src/v1/traits/ethcore_set.rs index d00295036..8afbcdab9 100644 --- a/rpc/src/v1/traits/ethcore_set.rs +++ b/rpc/src/v1/traits/ethcore_set.rs @@ -40,6 +40,9 @@ pub trait EthcoreSet: Sized + Send + Sync + 'static { /// Sets the limits for transaction queue. fn set_transactions_limit(&self, _: Params) -> Result; + /// Sets the maximum amount of gas a single transaction may consume. + fn set_tx_gas_limit(&self, _: Params) -> Result; + /// Add a reserved peer. fn add_reserved_peer(&self, _: Params) -> Result; @@ -60,6 +63,7 @@ pub trait EthcoreSet: Sized + Send + Sync + 'static { delegate.add_method("ethcore_setGasCeilTarget", EthcoreSet::set_gas_ceil_target); delegate.add_method("ethcore_setExtraData", EthcoreSet::set_extra_data); delegate.add_method("ethcore_setAuthor", EthcoreSet::set_author); + delegate.add_method("ethcore_setMaxTransactionGas", EthcoreSet::set_tx_gas_limit); delegate.add_method("ethcore_setTransactionsLimit", EthcoreSet::set_transactions_limit); delegate.add_method("ethcore_addReservedPeer", EthcoreSet::add_reserved_peer); delegate.add_method("ethcore_removeReservedPeer", EthcoreSet::remove_reserved_peer); diff --git a/rpc/src/v1/types/optionals.rs b/rpc/src/v1/types/optionals.rs index 2ed272ade..5f62dc4a0 100644 --- a/rpc/src/v1/types/optionals.rs +++ b/rpc/src/v1/types/optionals.rs @@ -14,7 +14,7 @@ // You should have received a copy of the GNU General Public License // along with Parity. If not, see . -use serde::{Serialize, Serializer}; +use serde::{Serialize, Serializer, Deserialize, Deserializer}; use serde_json::Value; /// Optional value @@ -26,13 +26,22 @@ pub enum OptionalValue where T: Serialize { Null } -impl Default for OptionalValue where T: Serialize { +impl Default for OptionalValue where T: Serialize + Deserialize { fn default() -> Self { OptionalValue::Null } } -impl Serialize for OptionalValue where T: Serialize { +impl Into> for OptionalValue where T: Serialize + Deserialize { + fn into(self) -> Option { + match self { + OptionalValue::Null => None, + OptionalValue::Value(t) => Some(t), + } + } +} + +impl Serialize for OptionalValue where T: Serialize + Deserialize { fn serialize(&self, serializer: &mut S) -> Result<(), S::Error> where S: Serializer { match *self { @@ -42,6 +51,17 @@ impl Serialize for OptionalValue where T: Serialize { } } +impl Deserialize for OptionalValue where T: Serialize + Deserialize { + fn deserialize(deserializer: &mut D) -> Result, D::Error> + where D: Deserializer { + let deser_result: Result = Deserialize::deserialize(deserializer); + match deser_result { + Ok(t) => Ok(OptionalValue::Value(t)), + Err(_) => Ok(OptionalValue::Null), + } + } +} + #[cfg(test)] mod tests { use serde_json;