From 8e866ee5512cad7e733c42b7b8dfad87a5482a69 Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Mon, 11 Feb 2019 18:08:12 +0100 Subject: [PATCH] Revive parity_setMinGasPrice RPC call (#10294) * Add function to update minimum gas price * Update TestMinerService to handle min_gas_price changes * Place minimum gas price test behind feature flag * Update check for fixed gas pricer to be more explicit * Use errors::unsupported instead of errors::request_rejected * Add test that fails to set minimum gas price * Fix test that should fail when setting new gas price * Put dev dependencies behind feature flag * Fix deadlock in set_minimal_gas_price() * Update RPC tests with mocked error response * Remove unnecessary cfg flag * Remove duplicate crate imports --- Cargo.lock | 2 + ethcore/Cargo.toml | 2 + ethcore/src/lib.rs | 6 ++ ethcore/src/miner/miner.rs | 82 +++++++++++++++++++++++ ethcore/src/miner/mod.rs | 4 ++ miner/src/gas_pricer.rs | 2 +- rpc/src/v1/impls/parity_set.rs | 8 ++- rpc/src/v1/tests/helpers/miner_service.rs | 17 +++++ rpc/src/v1/tests/mocked/parity_set.rs | 20 +++++- 9 files changed, 138 insertions(+), 5 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 1868d889c..6e1f6bbc9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -707,6 +707,7 @@ dependencies = [ "ethjson 0.1.0", "ethkey 0.3.0", "evm 0.1.0", + "fetch 0.1.0", "hashdb 0.3.0 (registry+https://github.com/rust-lang/crates.io-index)", "heapsize 0.4.2 (git+https://github.com/cheme/heapsize.git?branch=ec-macfix)", "itertools 0.5.10 (registry+https://github.com/rust-lang/crates.io-index)", @@ -728,6 +729,7 @@ dependencies = [ "parity-bytes 0.1.0 (registry+https://github.com/rust-lang/crates.io-index)", "parity-crypto 0.3.0 (registry+https://github.com/rust-lang/crates.io-index)", "parity-machine 0.1.0", + "parity-runtime 0.1.0", "parity-snappy 0.1.0 (registry+https://github.com/rust-lang/crates.io-index)", "parking_lot 0.7.0 (registry+https://github.com/rust-lang/crates.io-index)", "patricia-trie 0.3.0 (registry+https://github.com/rust-lang/crates.io-index)", diff --git a/ethcore/Cargo.toml b/ethcore/Cargo.toml index bd030c174..b61749844 100644 --- a/ethcore/Cargo.toml +++ b/ethcore/Cargo.toml @@ -76,7 +76,9 @@ blooms-db = { path = "../util/blooms-db" } criterion = "0.2" env_logger = "0.5" ethcore-accounts = { path = "../accounts" } +fetch = { path = "../util/fetch" } kvdb-rocksdb = "0.1.3" +parity-runtime = { path = "../util/runtime" } rlp_compress = { path = "../util/rlp-compress" } tempdir = "0.3" trie-standardmap = "0.1" diff --git a/ethcore/src/lib.rs b/ethcore/src/lib.rs index 633254683..0fe2e0db1 100644 --- a/ethcore/src/lib.rs +++ b/ethcore/src/lib.rs @@ -143,6 +143,12 @@ extern crate serde_derive; #[cfg_attr(test, macro_use)] extern crate evm; +#[cfg(all(test, feature = "price-info"))] +extern crate fetch; + +#[cfg(all(test, feature = "price-info"))] +extern crate parity_runtime; + pub mod block; pub mod builtin; pub mod client; diff --git a/ethcore/src/miner/miner.rs b/ethcore/src/miner/miner.rs index 82e0e4bb0..7d6bcbe49 100644 --- a/ethcore/src/miner/miner.rs +++ b/ethcore/src/miner/miner.rs @@ -873,6 +873,32 @@ impl miner::MinerService for Miner { self.params.read().gas_range_target.0 / 5 } + fn set_minimal_gas_price(&self, new_price: U256) -> Result { + match *self.gas_pricer.lock() { + // Binding the gas pricer to `gp` here to prevent + // a deadlock when calling recalibrate() + ref mut gp @ GasPricer::Fixed(_) => { + trace!(target: "miner", "minimal_gas_price: recalibrating fixed..."); + *gp = GasPricer::new_fixed(new_price); + + let txq = self.transaction_queue.clone(); + let mut options = self.options.pool_verification_options.clone(); + gp.recalibrate(move |gas_price| { + debug!(target: "miner", "minimal_gas_price: Got gas price! {}", gas_price); + options.minimal_gas_price = gas_price; + txq.set_verifier_options(options); + }); + + Ok(true) + }, + #[cfg(feature = "price-info")] + GasPricer::Calibrated(_) => { + let error_msg = "Can't update fixed gas price while automatic gas calibration is enabled."; + return Err(error_msg); + }, + } + } + fn import_external_transactions( &self, chain: &C, @@ -1654,4 +1680,60 @@ mod tests { assert!(miner.is_currently_sealing()); } + + #[test] + fn should_set_new_minimum_gas_price() { + // Creates a new GasPricer::Fixed behind the scenes + let miner = Miner::new_for_tests(&Spec::new_test(), None); + + let expected_minimum_gas_price: U256 = 0x1337.into(); + miner.set_minimal_gas_price(expected_minimum_gas_price).unwrap(); + + let txq_options = miner.transaction_queue.status().options; + let current_minimum_gas_price = txq_options.minimal_gas_price; + + assert!(current_minimum_gas_price == expected_minimum_gas_price); + } + + #[cfg(feature = "price-info")] + fn dynamic_gas_pricer() -> GasPricer { + use std::time::Duration; + use parity_runtime::Executor; + use fetch::Client as FetchClient; + use ethcore_miner::gas_price_calibrator::{GasPriceCalibrator, GasPriceCalibratorOptions}; + + // Don't really care about any of these settings since + // the gas pricer is never actually going to be used + let fetch = FetchClient::new(1).unwrap(); + let p = Executor::new_sync(); + + GasPricer::new_calibrated( + GasPriceCalibrator::new( + GasPriceCalibratorOptions { + usd_per_tx: 0.0, + recalibration_period: Duration::from_secs(0), + }, + fetch, + p, + ) + ) + } + + #[test] + #[cfg(feature = "price-info")] + fn should_fail_to_set_new_minimum_gas_price() { + // We get a fixed gas pricer by default, need to change that + let miner = Miner::new_for_tests(&Spec::new_test(), None); + let calibrated_gas_pricer = dynamic_gas_pricer(); + *miner.gas_pricer.lock() = calibrated_gas_pricer; + + let expected_minimum_gas_price: U256 = 0x1337.into(); + let result = miner.set_minimal_gas_price(expected_minimum_gas_price); + assert!(result.is_err()); + + let received_error_msg = result.unwrap_err(); + let expected_error_msg = "Can't update fixed gas price while automatic gas calibration is enabled."; + + assert!(received_error_msg == expected_error_msg); + } } diff --git a/ethcore/src/miner/mod.rs b/ethcore/src/miner/mod.rs index 5156b6040..fd7ab9651 100644 --- a/ethcore/src/miner/mod.rs +++ b/ethcore/src/miner/mod.rs @@ -205,4 +205,8 @@ pub trait MinerService : Send + Sync { /// Suggested gas limit. fn sensible_gas_limit(&self) -> U256; + + /// Set a new minimum gas limit. + /// Will not work if dynamic gas calibration is set. + fn set_minimal_gas_price(&self, gas_price: U256) -> Result; } diff --git a/miner/src/gas_pricer.rs b/miner/src/gas_pricer.rs index 0f851a9f8..c4e04442f 100644 --- a/miner/src/gas_pricer.rs +++ b/miner/src/gas_pricer.rs @@ -45,7 +45,7 @@ impl GasPricer { /// Recalibrate current gas price. pub fn recalibrate(&mut self, set_price: F) { match *self { - GasPricer::Fixed(ref max) => set_price(max.clone()), + GasPricer::Fixed(ref curr) => set_price(curr.clone()), #[cfg(feature = "price-info")] GasPricer::Calibrated(ref mut cal) => cal.recalibrate(set_price), } diff --git a/rpc/src/v1/impls/parity_set.rs b/rpc/src/v1/impls/parity_set.rs index fc839d621..3e0d26974 100644 --- a/rpc/src/v1/impls/parity_set.rs +++ b/rpc/src/v1/impls/parity_set.rs @@ -118,9 +118,11 @@ impl ParitySet for ParitySetClient where F: Fetch + 'static, { - fn set_min_gas_price(&self, _gas_price: U256) -> Result { - warn!("setMinGasPrice is deprecated. Ignoring request."); - Ok(false) + fn set_min_gas_price(&self, gas_price: U256) -> Result { + match self.miner.set_minimal_gas_price(gas_price.into()) { + Ok(success) => Ok(success), + Err(e) => Err(errors::unsupported(e, None)), + } } fn set_transactions_limit(&self, _limit: usize) -> Result { diff --git a/rpc/src/v1/tests/helpers/miner_service.rs b/rpc/src/v1/tests/helpers/miner_service.rs index f9b649fa6..77618c853 100644 --- a/rpc/src/v1/tests/helpers/miner_service.rs +++ b/rpc/src/v1/tests/helpers/miner_service.rs @@ -49,6 +49,8 @@ pub struct TestMinerService { pub pending_receipts: Mutex>, /// Next nonces. pub next_nonces: RwLock>, + /// Minimum gas price + pub min_gas_price: RwLock>, /// Signer (if any) pub signer: RwLock>>, @@ -63,6 +65,7 @@ impl Default for TestMinerService { local_transactions: Default::default(), pending_receipts: Default::default(), next_nonces: Default::default(), + min_gas_price: RwLock::new(Some(0.into())), authoring_params: RwLock::new(AuthoringParams { author: Address::zero(), gas_range_target: (12345.into(), 54321.into()), @@ -279,4 +282,18 @@ impl MinerService for TestMinerService { fn sensible_gas_limit(&self) -> U256 { 0x5208.into() } + + fn set_minimal_gas_price(&self, gas_price: U256) -> Result { + let mut new_price = self.min_gas_price.write(); + match *new_price { + Some(ref mut v) => { + *v = gas_price; + Ok(true) + }, + None => { + let error_msg = "Can't update fixed gas price while automatic gas calibration is enabled."; + Err(error_msg) + }, + } + } } diff --git a/rpc/src/v1/tests/mocked/parity_set.rs b/rpc/src/v1/tests/mocked/parity_set.rs index 13473fbdf..25c13fb1c 100644 --- a/rpc/src/v1/tests/mocked/parity_set.rs +++ b/rpc/src/v1/tests/mocked/parity_set.rs @@ -112,7 +112,25 @@ fn rpc_parity_set_min_gas_price() { io.extend_with(parity_set_client(&client, &miner, &updater, &network).to_delegate()); let request = r#"{"jsonrpc": "2.0", "method": "parity_setMinGasPrice", "params":["0xcd1722f3947def4cf144679da39c4c32bdc35681"], "id": 1}"#; - let response = r#"{"jsonrpc":"2.0","result":false,"id":1}"#; + let response = r#"{"jsonrpc":"2.0","result":true,"id":1}"#; + + assert_eq!(io.handle_request_sync(request), Some(response.to_owned())); +} + +#[test] +fn rpc_parity_set_min_gas_price_with_automated_calibration_enabled() { + let miner = miner_service(); + *miner.min_gas_price.write() = None; + + let client = client_service(); + let network = network_service(); + let updater = updater_service(); + + let mut io = IoHandler::new(); + io.extend_with(parity_set_client(&client, &miner, &updater, &network).to_delegate()); + + let request = r#"{"jsonrpc": "2.0", "method": "parity_setMinGasPrice", "params":["0xdeadbeef"], "id": 1}"#; + let response = r#"{"jsonrpc":"2.0","error":{"code":-32000,"message":"Can't update fixed gas price while automatic gas calibration is enabled."},"id":1}"#; assert_eq!(io.handle_request_sync(request), Some(response.to_owned())); }