Fix RPC errors. Implement geth-compatibility option to return correct pending nonce. (#4837)

parity/rpc_apis.rs

@@ -215,6 +215,7 @@ pub fn setup_rpc(stats: Arc<RpcStats>, deps: Arc<Dependencies>, 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,
 					}

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<T: fmt::Debug>(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<T: fmt::Debug>(error: &str, data: T) -> Error {
 	Error {
 		code: ErrorCode::InternalError,
@@ -216,6 +220,14 @@ pub fn encryption_error<T: fmt::Debug>(error: T) -> Error {
 	}
 }
 
+pub fn database_error<T: fmt::Debug>(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<T: fmt::Debug>(error: T) -> Error {
 	Error {
 		code: ErrorCode::ServerError(codes::FETCH_ERROR),

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<F: Fn(&mut Self)>(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<C, SN: ?Sized, S: ?Sized, M, EM> EthClient<C, SN, S, M, EM> 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<C, SN: ?Sized, S: ?Sized, M, EM> Eth for EthClient<C, SN, S, M, EM> where
 
 	fn balance(&self, address: RpcH160, num: Trailing<BlockNumber>) -> BoxFuture<RpcU256, Error> {
 		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<C, SN: ?Sized, S: ?Sized, M, EM> Eth for EthClient<C, SN, S, M, EM> 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<C, SN: ?Sized, S: ?Sized, M, EM> Eth for EthClient<C, SN, S, M, EM> where
 
 	fn transaction_count(&self, address: RpcH160, num: Trailing<BlockNumber>) -> BoxFuture<RpcU256, Error> {
 		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 => {
-				let client = take_weakf!(self.client);
-				match take_weakf!(self.miner).nonce(&*client, &address) {
+			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::internal("Unable to load nonce from database", ""))
+					None => Err(errors::database_error("latest nonce missing"))
+				}
+			}
+			BlockNumber::Pending => {
+				match miner.nonce(&*client, &address) {
+					Some(nonce) => Ok(nonce.into()),
+					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<C, SN: ?Sized, S: ?Sized, M, EM> Eth for EthClient<C, SN, S, M, EM> 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<C, SN: ?Sized, S: ?Sized, M, EM> Eth for EthClient<C, SN, S, M, EM> 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<bool, Error> {

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::<RpcH160>::into).collect());
 
 		future::done(accounts).boxed()

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::<HashSet<_>>();
 
 		let info = store.accounts_info().map_err(|e| errors::account("Could not fetch account info.", e))?;

rpc/src/v1/impls/parity.rs

@@ -119,7 +119,7 @@ impl<C, M, S: ?Sized, U> Parity for ParityClient<C, M, S, U> 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::<HashSet<_>>();
 
 		let info = store.accounts_info().map_err(|e| errors::account("Could not fetch account info.", e))?;

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.

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}"#;