From aefa8d5f5975e00da024550eb51e65bd3cae6534 Mon Sep 17 00:00:00 2001 From: David Date: Fri, 11 Oct 2019 15:54:36 +0200 Subject: [PATCH] Change how RPCs eth_call and eth_estimateGas handle "Pending" (#11127) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Change how RPCs eth_call and eth_estimateGas handle "Pending" Before this PR we would return a rather confusing error when calling `eth_call` and `eth_estimateGas` with `"Pending"`, e.g.: ``` {"jsonrpc":"2.0","error":{"code":-32000,"message":"This request is not supported because your node is running with state pruning. Run with --pruning=archive."},"id":"e237678f6648ed12ff05a74933d06d17"} ``` In reality what is going on is that users often use `"Pending"` when they really mean `"Latest"` (e.g. MyCrypto…) and when the block in question is not actually pending. This changes our behaviour for these two RPC calls to fall back to `"Latest"` when the query with `"Pending"` fails. Note that we already behave this way for many other RPCs: - eth_call (after this PR) - eth_estimateGas (after this PR) - eth_getBalance - eth_getCode - eth_getStorageAt Closes https://github.com/paritytech/parity-ethereum/issues/10096 * Fetch jsonrpc from git * No real need to wait for new jsonrpc * Add tests for calling eth_call/eth_estimateGas with "Pending" * Fix a todo, add another * Change client.latest_state to return the best header as well so we avoid potential data races and do less work * Impl review suggestions * Update rpc/src/v1/impls/eth.rs Co-Authored-By: Niklas Adolfsson * Review grumbles * update docs --- ethcore/client-traits/src/lib.rs | 4 +- ethcore/src/client/client.rs | 22 +++--- ethcore/src/miner/mod.rs | 7 +- ethcore/src/test_helpers/test_client.rs | 4 +- ethcore/src/tests/client.rs | 1 + rpc/src/v1/impls/eth.rs | 87 ++++++++++++++--------- rpc/src/v1/tests/helpers/miner_service.rs | 4 +- rpc/src/v1/tests/mocked/eth.rs | 74 +++++++++++++++++++ 8 files changed, 153 insertions(+), 50 deletions(-) diff --git a/ethcore/client-traits/src/lib.rs b/ethcore/client-traits/src/lib.rs index 6d14fc3a4..f592ef6cf 100644 --- a/ethcore/client-traits/src/lib.rs +++ b/ethcore/client-traits/src/lib.rs @@ -412,8 +412,8 @@ pub trait StateClient { /// Type representing chain state type State: StateInfo; - /// Get a copy of the best block's state. - fn latest_state(&self) -> Self::State; + /// Get a copy of the best block's state and header. + fn latest_state_and_header(&self) -> (Self::State, Header); /// Attempt to get a copy of a specific block's final state. /// diff --git a/ethcore/src/client/client.rs b/ethcore/src/client/client.rs index 8d762155b..04e7074f1 100644 --- a/ethcore/src/client/client.rs +++ b/ethcore/src/client/client.rs @@ -1033,15 +1033,16 @@ impl Client { } /// Get a copy of the best block's state. - pub fn latest_state(&self) -> State { + pub fn latest_state_and_header(&self) -> (State, Header) { let header = self.best_block_header(); - State::from_existing( + let state = State::from_existing( self.state_db.read().boxed_clone_canon(&header.hash()), *header.state_root(), self.engine.account_start_nonce(header.number()), self.factories.clone() ) - .expect("State root of best block header always valid.") + .expect("State root of best block header always valid."); + (state, header) } /// Attempt to get a copy of a specific block's final state. @@ -1051,9 +1052,9 @@ impl Client { /// is unknown. pub fn state_at(&self, id: BlockId) -> Option> { // fast path for latest state. - match id.clone() { - BlockId::Latest => return Some(self.latest_state()), - _ => {}, + if let BlockId::Latest = id { + let (state, _) = self.latest_state_and_header(); + return Some(state) } let block_number = match self.block_number(id) { @@ -1087,8 +1088,9 @@ impl Client { } /// Get a copy of the best block's state. - pub fn state(&self) -> Box { - Box::new(self.latest_state()) as Box<_> + pub fn state(&self) -> impl StateInfo { + let (state, _) = self.latest_state_and_header(); + state } /// Get info on the cache. @@ -1476,8 +1478,8 @@ impl ImportBlock for Client { impl StateClient for Client { type State = State<::state_db::StateDB>; - fn latest_state(&self) -> Self::State { - Client::latest_state(self) + fn latest_state_and_header(&self) -> (Self::State, Header) { + Client::latest_state_and_header(self) } fn state_at(&self, id: BlockId) -> Option { diff --git a/ethcore/src/miner/mod.rs b/ethcore/src/miner/mod.rs index 9961f12af..04cfd00ca 100644 --- a/ethcore/src/miner/mod.rs +++ b/ethcore/src/miner/mod.rs @@ -45,13 +45,16 @@ use types::{ receipt::RichReceipt, }; -use block::SealedBlock; use call_contract::CallContract; use registrar::RegistrarClient; -use client::{BlockProducer, SealedBlockImporter}; use client_traits::{BlockChain, ChainInfo, AccountData, Nonce, ScheduleInfo}; use account_state::state::StateInfo; +use crate::{ + block::SealedBlock, + client::{BlockProducer, SealedBlockImporter}, +}; + /// Provides methods to verify incoming external transactions pub trait TransactionVerifierClient: Send + Sync // Required for ServiceTransactionChecker diff --git a/ethcore/src/test_helpers/test_client.rs b/ethcore/src/test_helpers/test_client.rs index e7b579d65..0409e8249 100644 --- a/ethcore/src/test_helpers/test_client.rs +++ b/ethcore/src/test_helpers/test_client.rs @@ -644,8 +644,8 @@ impl StateInfo for TestState { impl StateClient for TestBlockChainClient { type State = TestState; - fn latest_state(&self) -> Self::State { - TestState + fn latest_state_and_header(&self) -> (Self::State, Header) { + (TestState, self.best_block_header()) } fn state_at(&self, _id: BlockId) -> Option { diff --git a/ethcore/src/tests/client.rs b/ethcore/src/tests/client.rs index 4d99a6bea..3fb946e01 100644 --- a/ethcore/src/tests/client.rs +++ b/ethcore/src/tests/client.rs @@ -17,6 +17,7 @@ use std::str::{FromStr, from_utf8}; use std::sync::Arc; +use account_state::state::StateInfo; use ethereum_types::{U256, Address}; use ethkey::KeyPair; use hash::keccak; diff --git a/rpc/src/v1/impls/eth.rs b/rpc/src/v1/impls/eth.rs index 9ba85b940..5093c72e2 100644 --- a/rpc/src/v1/impls/eth.rs +++ b/rpc/src/v1/impls/eth.rs @@ -37,6 +37,7 @@ use types::{ BlockNumber as EthBlockNumber, client_types::StateResult, encoded, + header::Header, ids::{BlockId, TransactionId, UncleId}, filter::Filter as EthcoreFilter, transaction::{SignedTransaction, LocalizedTransaction}, @@ -182,10 +183,11 @@ pub fn base_logs (client: &C, miner: &M, filter: F Box::new(future::ok(logs)) } -impl EthClient where +impl EthClient where C: miner::BlockChainClient + BlockChainClient + StateClient + Call + EngineInfo, SN: SnapshotService, S: SyncProvider, + T: StateInfo + 'static, M: MinerService, EM: ExternalMinerService { @@ -439,6 +441,10 @@ impl EthClient StateOrBlock { match number { BlockNumber::Hash { hash, .. } => BlockId::Hash(hash).into(), @@ -451,11 +457,33 @@ impl EthClient) - .unwrap_or(Box::new(self.client.latest_state()) as Box) + .unwrap_or_else(|| { + warn!("Asked for best pending state, but none found. Falling back to latest state"); + let (state, _) = self.client.latest_state_and_header(); + Box::new(state) as Box + }) .into() } } } + + /// Get the state and header of best pending block. On failure, fall back to the best imported + /// blocks state&header. + fn pending_state_and_header_with_fallback(&self) -> (T, Header) { + let best_block_number = self.client.chain_info().best_block_number; + let (maybe_state, maybe_header) = + self.miner.pending_state(best_block_number).map_or_else(|| (None, None),|s| { + (Some(s), self.miner.pending_block_header(best_block_number)) + }); + + match (maybe_state, maybe_header) { + (Some(state), Some(header)) => (state, header), + _ => { + warn!("Falling back to \"Latest\""); + self.client.latest_state_and_header() + } + } + } } pub fn pending_logs(miner: &M, best_block: EthBlockNumber, filter: &EthcoreFilter) -> Vec where M: MinerService { @@ -647,12 +675,13 @@ impl Eth for EthClient< let num = num.unwrap_or_default(); try_bf!(check_known(&*self.client, num.clone())); - let res = match self.client.storage_at(&address, &BigEndianHash::from_uint(&position), self.get_state(num)) { - Some(s) => Ok(s), - None => Err(errors::state_pruned()), - }; + let storage = self.client.storage_at( + &address, + &BigEndianHash::from_uint(&position), + self.get_state(num) + ).ok_or_else(errors::state_pruned); - Box::new(future::done(res)) + Box::new(future::done(storage)) } fn transaction_count(&self, address: H160, num: Option) -> BoxFuture { @@ -937,27 +966,27 @@ impl Eth for EthClient< let num = num.unwrap_or_default(); try_bf!(check_known(&*self.client, num.clone())); - let (mut state, header) = if num == BlockNumber::Pending { - let info = self.client.chain_info(); - let state = try_bf!(self.miner.pending_state(info.best_block_number).ok_or_else(errors::state_pruned)); - let header = try_bf!(self.miner.pending_block_header(info.best_block_number).ok_or_else(errors::state_pruned)); + let (mut state, header) = + if num == BlockNumber::Pending { + self.pending_state_and_header_with_fallback() + } else { + let id = match num { + BlockNumber::Hash { hash, .. } => BlockId::Hash(hash), + BlockNumber::Num(num) => BlockId::Number(num), + BlockNumber::Earliest => BlockId::Earliest, + BlockNumber::Latest => BlockId::Latest, + BlockNumber::Pending => unreachable!(), // Already covered + }; - (state, header) - } else { - let id = match num { - BlockNumber::Hash { hash, .. } => BlockId::Hash(hash), - BlockNumber::Num(num) => BlockId::Number(num), - BlockNumber::Earliest => BlockId::Earliest, - BlockNumber::Latest => BlockId::Latest, - BlockNumber::Pending => unreachable!(), // Already covered + let state = try_bf!(self.client.state_at(id).ok_or_else(errors::state_pruned)); + let header = try_bf!( + self.client.block_header(id).ok_or_else(errors::state_pruned) + .and_then(|h| h.decode().map_err(errors::decode)) + ); + + (state, header) }; - let state = try_bf!(self.client.state_at(id).ok_or_else(errors::state_pruned)); - let header = try_bf!(self.client.block_header(id).ok_or_else(errors::state_pruned).and_then(|h| h.decode().map_err(errors::decode))); - - (state, header) - }; - let result = self.client.call(&signed, Default::default(), &mut state, &header); Box::new(future::done(result @@ -978,13 +1007,7 @@ impl Eth for EthClient< let num = num.unwrap_or_default(); let (state, header) = if num == BlockNumber::Pending { - let info = self.client.chain_info(); - let state = try_bf!(self.miner.pending_state(info.best_block_number) - .ok_or_else(errors::state_pruned)); - let header = try_bf!(self.miner.pending_block_header(info.best_block_number) - .ok_or_else(errors::state_pruned)); - - (state, header) + self.pending_state_and_header_with_fallback() } else { let id = match num { BlockNumber::Hash { hash, .. } => BlockId::Hash(hash), diff --git a/rpc/src/v1/tests/helpers/miner_service.rs b/rpc/src/v1/tests/helpers/miner_service.rs index 0af99e122..7d2edcbbf 100644 --- a/rpc/src/v1/tests/helpers/miner_service.rs +++ b/rpc/src/v1/tests/helpers/miner_service.rs @@ -93,8 +93,8 @@ impl StateClient for TestMinerService { // State will not be used by test client anyway, since all methods that accept state are mocked type State = TestState; - fn latest_state(&self) -> Self::State { - TestState + fn latest_state_and_header(&self) -> (Self::State, Header) { + (TestState, Header::default()) } fn state_at(&self, _id: BlockId) -> Option { diff --git a/rpc/src/v1/tests/mocked/eth.rs b/rpc/src/v1/tests/mocked/eth.rs index ff83c05b0..bb6cb6b67 100644 --- a/rpc/src/v1/tests/mocked/eth.rs +++ b/rpc/src/v1/tests/mocked/eth.rs @@ -663,6 +663,43 @@ fn rpc_eth_call_latest() { assert_eq!(tester.io.handle_request_sync(request), Some(response.to_owned())); } +#[test] +fn rpc_eth_call_pending() { + let tester = EthTester::default(); + tester.client.set_execution_result(Ok(Executed { + exception: None, + gas: U256::zero(), + gas_used: U256::from(0xff30), + refunded: U256::from(0x5), + cumulative_gas_used: U256::zero(), + logs: vec![], + contracts_created: vec![], + output: vec![0x12, 0x34, 0xff], + trace: vec![], + vm_trace: None, + state_diff: None, + })); + + let request = r#"{ + "jsonrpc": "2.0", + "method": "eth_call", + "params": [{ + "from": "0xb60e8dd61c5d32be8058bb8eb970870f07233155", + "to": "0xd46e8dd67c5d32be8058bb8eb970870f07244567", + "gas": "0x76c0", + "gasPrice": "0x9184e72a000", + "value": "0x9184e72a", + "data": "0xd46e8dd67c5d32be8d46e8dd67c5d32be8058bb8eb970870f072445675058bb8eb970870f072445675" + }, + "pending"], + "id": 1 + }"#; + // Falls back to "Latest" and gives the same result. + let response = r#"{"jsonrpc":"2.0","result":"0x1234ff","id":1}"#; + + assert_eq!(tester.io.handle_request_sync(request), Some(response.to_owned())); +} + #[test] fn rpc_eth_call() { let tester = EthTester::default(); @@ -770,6 +807,43 @@ fn rpc_eth_estimate_gas() { assert_eq!(tester.io.handle_request_sync(request), Some(response.to_owned())); } +#[test] +fn rpc_eth_estimate_gas_pending() { + let tester = EthTester::default(); + tester.client.set_execution_result(Ok(Executed { + exception: None, + gas: U256::zero(), + gas_used: U256::from(0xff30), + refunded: U256::from(0x5), + cumulative_gas_used: U256::zero(), + logs: vec![], + contracts_created: vec![], + output: vec![0x12, 0x34, 0xff], + trace: vec![], + vm_trace: None, + state_diff: None, + })); + + let request = r#"{ + "jsonrpc": "2.0", + "method": "eth_estimateGas", + "params": [{ + "from": "0xb60e8dd61c5d32be8058bb8eb970870f07233155", + "to": "0xd46e8dd67c5d32be8058bb8eb970870f07244567", + "gas": "0x76c0", + "gasPrice": "0x9184e72a000", + "value": "0x9184e72a", + "data": "0xd46e8dd67c5d32be8d46e8dd67c5d32be8058bb8eb970870f072445675058bb8eb970870f072445675" + }, + "pending"], + "id": 1 + }"#; + // Falls back to "Latest" so the result is the same + let response = r#"{"jsonrpc":"2.0","result":"0x5208","id":1}"#; + + assert_eq!(tester.io.handle_request_sync(request), Some(response.to_owned())); +} + #[test] fn rpc_eth_estimate_gas_default_block() { let tester = EthTester::default();