From d4c61171a8057f7b9fc2f845881f68d6858bb185 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Drwi=C4=99ga?= Date: Thu, 9 Mar 2017 14:56:19 +0100 Subject: [PATCH] Fix RPC errors. Implement geth-compatibility option to return correct pending nonce. --- parity/rpc_apis.rs | 1 + rpc/src/v1/helpers/errors.rs | 12 ++++++ rpc/src/v1/impls/eth.rs | 46 ++++++++++++++++------- rpc/src/v1/impls/light/eth.rs | 2 +- rpc/src/v1/impls/light/parity.rs | 2 +- rpc/src/v1/impls/parity.rs | 2 +- rpc/src/v1/tests/helpers/miner_service.rs | 17 +++++++++ rpc/src/v1/tests/mocked/eth.rs | 33 ++++++++++++++-- 8 files changed, 95 insertions(+), 20 deletions(-) diff --git a/parity/rpc_apis.rs b/parity/rpc_apis.rs index 548e8499591..469245c19e6 100644 --- a/parity/rpc_apis.rs +++ b/parity/rpc_apis.rs @@ -215,6 +215,7 @@ pub fn setup_rpc(stats: Arc, deps: Arc, apis: ApiSet) -> &deps.miner, &deps.external_miner, EthClientOptions { + pending_nonce_from_queue: deps.geth_compatibility, allow_pending_receipt_query: !deps.geth_compatibility, send_block_number_in_get_work: !deps.geth_compatibility, } diff --git a/rpc/src/v1/helpers/errors.rs b/rpc/src/v1/helpers/errors.rs index 0be3f724035..1f98b922d48 100644 --- a/rpc/src/v1/helpers/errors.rs +++ b/rpc/src/v1/helpers/errors.rs @@ -37,6 +37,7 @@ mod codes { pub const TRANSACTION_ERROR: i64 = -32010; pub const EXECUTION_ERROR: i64 = -32015; pub const EXCEPTION_ERROR: i64 = -32016; + pub const DATABASE_ERROR: i64 = -32017; pub const ACCOUNT_LOCKED: i64 = -32020; pub const PASSWORD_INVALID: i64 = -32021; pub const ACCOUNT_ERROR: i64 = -32023; @@ -100,6 +101,9 @@ pub fn account(error: &str, details: T) -> Error { } } +/// Internal error signifying a logic error in code. +/// Should not be used when function can just fail +/// because of invalid parameters or incomplete node state. pub fn internal(error: &str, data: T) -> Error { Error { code: ErrorCode::InternalError, @@ -216,6 +220,14 @@ pub fn encryption_error(error: T) -> Error { } } +pub fn database_error(error: T) -> Error { + Error { + code: ErrorCode::ServerError(codes::DATABASE_ERROR), + message: "Database error.".into(), + data: Some(Value::String(format!("{:?}", error))), + } +} + pub fn from_fetch_error(error: T) -> Error { Error { code: ErrorCode::ServerError(codes::FETCH_ERROR), diff --git a/rpc/src/v1/impls/eth.rs b/rpc/src/v1/impls/eth.rs index 6df8f5278c7..8da7063f4b3 100644 --- a/rpc/src/v1/impls/eth.rs +++ b/rpc/src/v1/impls/eth.rs @@ -58,15 +58,28 @@ const EXTRA_INFO_PROOF: &'static str = "Object exists in in blockchain (fetched /// Eth RPC options pub struct EthClientOptions { + /// Return nonce from transaction queue when pending block not available. + pub pending_nonce_from_queue: bool, /// Returns receipt from pending blocks pub allow_pending_receipt_query: bool, /// Send additional block number when asking for work pub send_block_number_in_get_work: bool, } +impl EthClientOptions { + /// Creates new default `EthClientOptions` and allows alterations + /// by provided function. + pub fn with(fun: F) -> Self { + let mut options = Self::default(); + fun(&mut options); + options + } +} + impl Default for EthClientOptions { fn default() -> Self { EthClientOptions { + pending_nonce_from_queue: false, allow_pending_receipt_query: true, send_block_number_in_get_work: true, } @@ -227,7 +240,7 @@ impl EthClient where store .note_dapp_used(dapp.clone()) .and_then(|_| store.dapp_addresses(dapp)) - .map_err(|e| errors::internal("Could not fetch accounts.", e)) + .map_err(|e| errors::account("Could not fetch accounts.", e)) } } @@ -352,18 +365,16 @@ impl Eth for EthClient where fn balance(&self, address: RpcH160, num: Trailing) -> BoxFuture { let address = address.into(); + let client = take_weakf!(self.client); let res = match num.0.clone() { BlockNumber::Pending => { - let client = take_weakf!(self.client); match take_weakf!(self.miner).balance(&*client, &address) { Some(balance) => Ok(balance.into()), - None => Err(errors::internal("Unable to load balance from database", "")) + None => Err(errors::database_error("latest balance missing")) } } id => { - let client = take_weakf!(self.client); - try_bf!(check_known(&*client, id.clone())); match client.balance(&address, id.into()) { Some(balance) => Ok(balance.into()), @@ -384,7 +395,7 @@ impl Eth for EthClient where let client = take_weakf!(self.client); match take_weakf!(self.miner).storage_at(&*client, &address, &H256::from(position)) { Some(s) => Ok(s.into()), - None => Err(errors::internal("Unable to load storage from database", "")) + None => Err(errors::database_error("latest storage missing")) } } id => { @@ -403,17 +414,26 @@ impl Eth for EthClient where fn transaction_count(&self, address: RpcH160, num: Trailing) -> BoxFuture { let address: Address = RpcH160::into(address); + let client = take_weakf!(self.client); + let miner = take_weakf!(self.miner); + let res = match num.0.clone() { + BlockNumber::Pending if self.options.pending_nonce_from_queue => { + let nonce = miner.last_nonce(&address) + .map(|n| n + 1.into()) + .or_else(|| miner.nonce(&*client, &address)); + match nonce { + Some(nonce) => Ok(nonce.into()), + None => Err(errors::database_error("latest nonce missing")) + } + } BlockNumber::Pending => { - let client = take_weakf!(self.client); - match take_weakf!(self.miner).nonce(&*client, &address) { + match miner.nonce(&*client, &address) { Some(nonce) => Ok(nonce.into()), - None => Err(errors::internal("Unable to load nonce from database", "")) + None => Err(errors::database_error("latest nonce missing")) } } id => { - let client = take_weakf!(self.client); - try_bf!(check_known(&*client, id.clone())); match client.nonce(&address, id.into()) { Some(nonce) => Ok(nonce.into()), @@ -464,7 +484,7 @@ impl Eth for EthClient where let client = take_weakf!(self.client); match take_weakf!(self.miner).code(&*client, &address) { Some(code) => Ok(code.map_or_else(Bytes::default, Bytes::new)), - None => Err(errors::internal("Unable to load code from database", "")) + None => Err(errors::database_error("latest code missing")) } } id => { @@ -597,7 +617,7 @@ impl Eth for EthClient where number: None }) } - }).unwrap_or(Err(Error::internal_error())) // no work found. + }).unwrap_or(Err(errors::internal("No work found.", ""))) } fn submit_work(&self, nonce: RpcH64, pow_hash: RpcH256, mix_hash: RpcH256) -> Result { diff --git a/rpc/src/v1/impls/light/eth.rs b/rpc/src/v1/impls/light/eth.rs index f889faf00be..9b0c92e9cc0 100644 --- a/rpc/src/v1/impls/light/eth.rs +++ b/rpc/src/v1/impls/light/eth.rs @@ -263,7 +263,7 @@ impl Eth for EthClient { let accounts = self.accounts .note_dapp_used(dapp.clone()) .and_then(|_| self.accounts.dapp_addresses(dapp)) - .map_err(|e| errors::internal("Could not fetch accounts.", e)) + .map_err(|e| errors::account("Could not fetch accounts.", e)) .map(|accs| accs.into_iter().map(Into::::into).collect()); future::done(accounts).boxed() diff --git a/rpc/src/v1/impls/light/parity.rs b/rpc/src/v1/impls/light/parity.rs index 332088dd635..1f056b89e55 100644 --- a/rpc/src/v1/impls/light/parity.rs +++ b/rpc/src/v1/impls/light/parity.rs @@ -87,7 +87,7 @@ impl Parity for ParityClient { let dapp_accounts = store .note_dapp_used(dapp.clone().into()) .and_then(|_| store.dapp_addresses(dapp.into())) - .map_err(|e| errors::internal("Could not fetch accounts.", e))? + .map_err(|e| errors::account("Could not fetch accounts.", e))? .into_iter().collect::>(); let info = store.accounts_info().map_err(|e| errors::account("Could not fetch account info.", e))?; diff --git a/rpc/src/v1/impls/parity.rs b/rpc/src/v1/impls/parity.rs index 820aa6670ff..503dfcce699 100644 --- a/rpc/src/v1/impls/parity.rs +++ b/rpc/src/v1/impls/parity.rs @@ -119,7 +119,7 @@ impl Parity for ParityClient where let dapp_accounts = store .note_dapp_used(dapp.clone().into()) .and_then(|_| store.dapp_addresses(dapp.into())) - .map_err(|e| errors::internal("Could not fetch accounts.", e))? + .map_err(|e| errors::account("Could not fetch accounts.", e))? .into_iter().collect::>(); let info = store.accounts_info().map_err(|e| errors::account("Could not fetch account info.", e))?; diff --git a/rpc/src/v1/tests/helpers/miner_service.rs b/rpc/src/v1/tests/helpers/miner_service.rs index 01dd9edc715..c667e408bc6 100644 --- a/rpc/src/v1/tests/helpers/miner_service.rs +++ b/rpc/src/v1/tests/helpers/miner_service.rs @@ -16,6 +16,7 @@ //! Test implementation of miner service. +use std::collections::hash_map::Entry; use util::{Address, H256, Bytes, U256, FixedHash, Uint}; use util::standard::*; use ethcore::error::{Error, CallError}; @@ -72,6 +73,22 @@ impl Default for TestMinerService { } } +impl TestMinerService { + /// Increments last nonce for given address. + pub fn increment_last_nonce(&self, address: Address) { + let mut last_nonces = self.last_nonces.write(); + match last_nonces.entry(address) { + Entry::Occupied(mut occupied) => { + let val = *occupied.get(); + *occupied.get_mut() = val + 1.into(); + }, + Entry::Vacant(vacant) => { + vacant.insert(0.into()); + }, + } + } +} + impl MinerService for TestMinerService { /// Returns miner's status. diff --git a/rpc/src/v1/tests/mocked/eth.rs b/rpc/src/v1/tests/mocked/eth.rs index 2432b55e70f..6603bd7220b 100644 --- a/rpc/src/v1/tests/mocked/eth.rs +++ b/rpc/src/v1/tests/mocked/eth.rs @@ -468,6 +468,32 @@ fn rpc_eth_transaction_count() { assert_eq!(EthTester::default().io.handle_request_sync(request), Some(response.to_owned())); } +#[test] +fn rpc_eth_transaction_count_next_nonce() { + let tester = EthTester::new_with_options(EthClientOptions::with(|mut options| { + options.pending_nonce_from_queue = true; + })); + tester.miner.increment_last_nonce(1.into()); + + let request1 = r#"{ + "jsonrpc": "2.0", + "method": "eth_getTransactionCount", + "params": ["0x0000000000000000000000000000000000000001", "pending"], + "id": 1 + }"#; + let response1 = r#"{"jsonrpc":"2.0","result":"0x1","id":1}"#; + assert_eq!(tester.io.handle_request_sync(request1), Some(response1.to_owned())); + + let request2 = r#"{ + "jsonrpc": "2.0", + "method": "eth_getTransactionCount", + "params": ["0x0000000000000000000000000000000000000002", "pending"], + "id": 1 + }"#; + let response2 = r#"{"jsonrpc":"2.0","result":"0x0","id":1}"#; + assert_eq!(tester.io.handle_request_sync(request2), Some(response2.to_owned())); +} + #[test] fn rpc_eth_block_transaction_count_by_hash() { let request = r#"{ @@ -1076,10 +1102,9 @@ fn rpc_get_work_returns_correct_work_package() { #[test] fn rpc_get_work_should_not_return_block_number() { - let eth_tester = EthTester::new_with_options(EthClientOptions { - allow_pending_receipt_query: true, - send_block_number_in_get_work: false, - }); + let eth_tester = EthTester::new_with_options(EthClientOptions::with(|mut options| { + options.send_block_number_in_get_work = false; + })); eth_tester.miner.set_author(Address::from_str("d46e8dd67c5d32be8058bb8eb970870f07244567").unwrap()); let request = r#"{"jsonrpc": "2.0", "method": "eth_getWork", "params": [], "id": 1}"#;