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

@@ -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:
+/// <https://github.com/paritytech/jsonrpc/blob/609d7a6cc160742d035510fa89fb424ccf077660/core/src/types/error.rs#L25-L36>
+///
+/// `node-stratum-pool` mining pool library expects error code `-1` to detect available RPC methods:
+/// <https://github.com/s-nomp/node-stratum-pool/blob/d86ae73f8ff968d9355bb61aac05e0ebef36ccb5/lib/pool.js#L459>
+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:
+/// <https://github.com/adityapk00/lightwalletd/blob/c1bab818a683e4de69cd952317000f9bb2932274/common/common.go#L251-L254>
+pub const MISSING_BLOCK_ERROR_CODE: ErrorCode = ErrorCode::ServerError(-8);

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;

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:
-/// <https://github.com/adityapk00/lightwalletd/blob/c1bab818a683e4de69cd952317000f9bb2932274/common/common.go#L251-L254>
-pub const MISSING_BLOCK_ERROR_CODE: ErrorCode = ErrorCode::ServerError(-8);
-
 #[rpc(server)]
 /// RPC method signatures.
 pub trait Rpc {

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")]
             {

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
 ///

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:
+/// <https://docs.rs/jsonrpc-core/18.0.0/jsonrpc_core/types/error/enum.ErrorCode.html#variants>
+///
+/// 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<M: Metadata> Middleware<M> for FixRpcResponseMiddleware {
+    type Future = BoxFuture<Option<Response>>;
+    type CallFuture = BoxFuture<Option<Output>>;
+
+    fn on_call<Next, NextFuture>(
+        &self,
+        call: Call,
+        meta: M,
+        next: Next,
+    ) -> Either<Self::CallFuture, NextFuture>
+    where
+        Next: Fn(Call, M) -> NextFuture + Send + Sync,
+        NextFuture: Future<Output = Option<Output>> + 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<Output>) {
+        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<Output>, 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}");
+        }
+    }
+}

zebra-rpc/src/server/tracing_middleware.rs

@@ -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<M: Metadata> Middleware<M> for TracingMiddleware {
-    type Future = BoxFuture<Option<Response>>;
-    type CallFuture = BoxFuture<Option<Output>>;
-
-    fn on_call<Next, NextFuture>(
-        &self,
-        call: Call,
-        meta: M,
-        next: Next,
-    ) -> Either<Self::CallFuture, NextFuture>
-    where
-        Next: Fn(Call, M) -> NextFuture + Send + Sync,
-        NextFuture: Future<Output = Option<Output>> + 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<Output>, call: Call) -> Option<Output> {
-        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
-    }
-}