From 9d97919afcb8abc104c9f1adb364d738bf61c654 Mon Sep 17 00:00:00 2001 From: teor Date: Thu, 2 Feb 2023 04:20:48 +1000 Subject: [PATCH] fix(rpc): Make RPC "incorrect parameters" error code match `zcashd` (#6066) * Move RPC method constants into their own module * Rename RPC compatibility modules to avoid confusion * Rename RPC middleware to include its new functionality * Use FutureExt::inspect() for logging, and only format on failure * Log all RPC errors at info level * Make "invalid parameters" RPC error code match `zcashd` --- zebra-rpc/src/constants.rs | 18 ++++ zebra-rpc/src/lib.rs | 2 + zebra-rpc/src/methods.rs | 8 +- zebra-rpc/src/server.rs | 11 ++- ...ility.rs => http_request_compatibility.rs} | 6 +- .../src/server/rpc_call_compatibility.rs | 96 +++++++++++++++++++ zebra-rpc/src/server/tracing_middleware.rs | 71 -------------- 7 files changed, 128 insertions(+), 84 deletions(-) create mode 100644 zebra-rpc/src/constants.rs rename zebra-rpc/src/server/{compatibility.rs => http_request_compatibility.rs} (95%) create mode 100644 zebra-rpc/src/server/rpc_call_compatibility.rs delete mode 100644 zebra-rpc/src/server/tracing_middleware.rs diff --git a/zebra-rpc/src/constants.rs b/zebra-rpc/src/constants.rs new file mode 100644 index 000000000..9d549767b --- /dev/null +++ b/zebra-rpc/src/constants.rs @@ -0,0 +1,18 @@ +//! Constants for RPC methods and server responses. + +use jsonrpc_core::ErrorCode; + +/// The RPC error code used by `zcashd` for incorrect RPC parameters. +/// +/// [`jsonrpc_core`] uses these codes: +/// +/// +/// `node-stratum-pool` mining pool library expects error code `-1` to detect available RPC methods: +/// +pub const INVALID_PARAMETERS_ERROR_CODE: ErrorCode = ErrorCode::ServerError(-1); + +/// The RPC error code used by `zcashd` for missing blocks. +/// +/// `lightwalletd` expects error code `-8` when a block is not found: +/// +pub const MISSING_BLOCK_ERROR_CODE: ErrorCode = ErrorCode::ServerError(-8); diff --git a/zebra-rpc/src/lib.rs b/zebra-rpc/src/lib.rs index 0bb4b072e..2f5dae4a0 100644 --- a/zebra-rpc/src/lib.rs +++ b/zebra-rpc/src/lib.rs @@ -5,8 +5,10 @@ #![doc(html_root_url = "https://doc.zebra.zfnd.org/zebra_rpc")] pub mod config; +pub mod constants; pub mod methods; pub mod queue; pub mod server; + #[cfg(test)] mod tests; diff --git a/zebra-rpc/src/methods.rs b/zebra-rpc/src/methods.rs index 4331383cf..454b27bc8 100644 --- a/zebra-rpc/src/methods.rs +++ b/zebra-rpc/src/methods.rs @@ -32,7 +32,7 @@ use zebra_network::constants::USER_AGENT; use zebra_node_services::mempool; use zebra_state::{HashOrHeight, OutputIndex, OutputLocation, TransactionLocation}; -use crate::queue::Queue; +use crate::{constants::MISSING_BLOCK_ERROR_CODE, queue::Queue}; #[cfg(feature = "getblocktemplate-rpcs")] pub mod get_block_template_rpcs; @@ -43,12 +43,6 @@ pub use get_block_template_rpcs::{GetBlockTemplateRpc, GetBlockTemplateRpcImpl}; #[cfg(test)] mod tests; -/// The RPC error code used by `zcashd` for missing blocks. -/// -/// `lightwalletd` expects error code `-8` when a block is not found: -/// -pub const MISSING_BLOCK_ERROR_CODE: ErrorCode = ErrorCode::ServerError(-8); - #[rpc(server)] /// RPC method signatures. pub trait Rpc { diff --git a/zebra-rpc/src/server.rs b/zebra-rpc/src/server.rs index 5c40ba584..7745e5851 100644 --- a/zebra-rpc/src/server.rs +++ b/zebra-rpc/src/server.rs @@ -25,14 +25,17 @@ use zebra_node_services::mempool; use crate::{ config::Config, methods::{Rpc, RpcImpl}, - server::{compatibility::FixHttpRequestMiddleware, tracing_middleware::TracingMiddleware}, + server::{ + http_request_compatibility::FixHttpRequestMiddleware, + rpc_call_compatibility::FixRpcResponseMiddleware, + }, }; #[cfg(feature = "getblocktemplate-rpcs")] use crate::methods::{get_block_template_rpcs, GetBlockTemplateRpc, GetBlockTemplateRpcImpl}; -pub mod compatibility; -mod tracing_middleware; +pub mod http_request_compatibility; +pub mod rpc_call_compatibility; #[cfg(test)] mod tests; @@ -124,7 +127,7 @@ impl RpcServer { // Create handler compatible with V1 and V2 RPC protocols let mut io: MetaIoHandler<(), _> = - MetaIoHandler::new(Compatibility::Both, TracingMiddleware); + MetaIoHandler::new(Compatibility::Both, FixRpcResponseMiddleware); #[cfg(feature = "getblocktemplate-rpcs")] { diff --git a/zebra-rpc/src/server/compatibility.rs b/zebra-rpc/src/server/http_request_compatibility.rs similarity index 95% rename from zebra-rpc/src/server/compatibility.rs rename to zebra-rpc/src/server/http_request_compatibility.rs index e717b794e..63f445e91 100644 --- a/zebra-rpc/src/server/compatibility.rs +++ b/zebra-rpc/src/server/http_request_compatibility.rs @@ -1,4 +1,6 @@ -//! Compatibility fixes for JSON-RPC requests. +//! Compatibility fixes for JSON-RPC HTTP requests. +//! +//! These fixes are applied at the HTTP level, before the RPC request is parsed. use futures::TryStreamExt; use hyper::{body::Bytes, Body}; @@ -7,7 +9,7 @@ use jsonrpc_http_server::RequestMiddleware; /// HTTP [`RequestMiddleware`] with compatibility workarounds. /// -/// This middleware makes the following changes to requests: +/// This middleware makes the following changes to HTTP requests: /// /// ## Remove `jsonrpc` field in JSON RPC 1.0 /// diff --git a/zebra-rpc/src/server/rpc_call_compatibility.rs b/zebra-rpc/src/server/rpc_call_compatibility.rs new file mode 100644 index 000000000..e6f44c1eb --- /dev/null +++ b/zebra-rpc/src/server/rpc_call_compatibility.rs @@ -0,0 +1,96 @@ +//! Compatibility fixes for JSON-RPC remote procedure calls. +//! +//! These fixes are applied at the JSON-RPC call level, +//! after the RPC request is parsed and split into calls. + +use std::future::Future; + +use futures::future::{Either, FutureExt}; +use jsonrpc_core::{ + middleware::Middleware, + types::{Call, Failure, Output, Response}, + BoxFuture, ErrorCode, Metadata, MethodCall, Notification, +}; + +use crate::constants::INVALID_PARAMETERS_ERROR_CODE; + +/// JSON-RPC [`Middleware`] with compatibility workarounds. +/// +/// This middleware makes the following changes to JSON-RPC calls: +/// +/// ## Make RPC framework response codes match `zcashd` +/// +/// [`jsonrpc_core`] returns specific error codes while parsing requests: +/// +/// +/// But these codes are different from `zcashd`, and some RPC clients rely on the exact code. +/// +/// ## Read-Only Functionality +/// +/// This middleware also logs unrecognized RPC requests. +pub struct FixRpcResponseMiddleware; + +impl Middleware for FixRpcResponseMiddleware { + type Future = BoxFuture>; + type CallFuture = BoxFuture>; + + fn on_call( + &self, + call: Call, + meta: M, + next: Next, + ) -> Either + where + Next: Fn(Call, M) -> NextFuture + Send + Sync, + NextFuture: Future> + Send + 'static, + { + Either::Left( + next(call.clone(), meta) + .map(|mut output| { + Self::fix_error_codes(&mut output); + output + }) + .inspect(|output| Self::log_if_error(output, call)) + .boxed(), + ) + } +} + +impl FixRpcResponseMiddleware { + /// Replace [`jsonrpc_core`] server error codes in `output` with the `zcashd` equivalents. + fn fix_error_codes(output: &mut Option) { + if let Some(Output::Failure(Failure { ref mut error, .. })) = output { + if matches!(error.code, ErrorCode::InvalidParams) { + let original_code = error.code.clone(); + + error.code = INVALID_PARAMETERS_ERROR_CODE; + tracing::debug!("Replacing RPC error: {original_code:?} with {error}"); + } + } + } + + /// Obtain a description string for a received request. + /// + /// Prints out only the method name and the received parameters. + fn call_description(call: &Call) -> String { + match call { + Call::MethodCall(MethodCall { method, params, .. }) => { + format!(r#"method = {method:?}, params = {params:?}"#) + } + Call::Notification(Notification { method, params, .. }) => { + format!(r#"notification = {method:?}, params = {params:?}"#) + } + Call::Invalid { .. } => "invalid request".to_owned(), + } + } + + /// Check RPC output and log any errors. + // + // TODO: do we want to ignore ErrorCode::ServerError(_), or log it at debug? + fn log_if_error(output: &Option, call: Call) { + if let Some(Output::Failure(Failure { error, .. })) = output { + let call_description = Self::call_description(&call); + tracing::info!("RPC error: {error} in call: {call_description}"); + } + } +} diff --git a/zebra-rpc/src/server/tracing_middleware.rs b/zebra-rpc/src/server/tracing_middleware.rs deleted file mode 100644 index c4342f6b1..000000000 --- a/zebra-rpc/src/server/tracing_middleware.rs +++ /dev/null @@ -1,71 +0,0 @@ -//! A custom middleware to trace unrecognized RPC requests. - -use std::future::Future; - -use futures::future::{Either, FutureExt}; -use jsonrpc_core::{ - middleware::Middleware, - types::{Call, Failure, Output, Response}, - BoxFuture, Error, ErrorCode, Metadata, MethodCall, Notification, -}; - -/// A custom RPC middleware that logs unrecognized RPC requests. -pub struct TracingMiddleware; - -impl Middleware for TracingMiddleware { - type Future = BoxFuture>; - type CallFuture = BoxFuture>; - - fn on_call( - &self, - call: Call, - meta: M, - next: Next, - ) -> Either - where - Next: Fn(Call, M) -> NextFuture + Send + Sync, - NextFuture: Future> + Send + 'static, - { - Either::Left( - next(call.clone(), meta) - .then(move |output| Self::log_error_if_method_not_found(output, call)) - .boxed(), - ) - } -} - -impl TracingMiddleware { - /// Obtain a description string for a received request. - /// - /// Prints out only the method name and the received parameters. - fn call_description(call: &Call) -> String { - match call { - Call::MethodCall(MethodCall { method, params, .. }) => { - format!(r#"method = {method:?}, params = {params:?}"#) - } - Call::Notification(Notification { method, params, .. }) => { - format!(r#"notification = {method:?}, params = {params:?}"#) - } - Call::Invalid { .. } => "invalid request".to_owned(), - } - } - - /// Check an RPC output and log an error if it indicates the method was not found. - async fn log_error_if_method_not_found(output: Option, call: Call) -> Option { - let call_description = Self::call_description(&call); - - if let Some(Output::Failure(Failure { - error: - Error { - code: ErrorCode::MethodNotFound, - .. - }, - .. - })) = output - { - tracing::warn!("Received unrecognized RPC request: {call_description}"); - } - - output - } -}