From b1120e635a90c9c4d873d46be2c8f04878509390 Mon Sep 17 00:00:00 2001 From: Bryan Stitt Date: Mon, 10 Apr 2023 22:29:02 -0700 Subject: [PATCH] change some error codes jsonrpc error handling --- TODO.md | 8 ++- web3_proxy/src/bin/web3_proxy_cli/main.rs | 5 +- web3_proxy/src/frontend/errors.rs | 70 +++++++++++------------ web3_proxy/src/frontend/rpc_proxy_ws.rs | 6 +- web3_proxy/src/jsonrpc.rs | 45 +++++++++++++-- web3_proxy/src/rpcs/blockchain.rs | 8 +-- 6 files changed, 89 insertions(+), 53 deletions(-) diff --git a/TODO.md b/TODO.md index 45fc30bc..5417d951 100644 --- a/TODO.md +++ b/TODO.md @@ -399,6 +399,8 @@ These are not yet ordered. There might be duplicates. We might not actually need - [x] improve "archive_needed" boolean. change to "block_depth" - [x] keep score of new_head timings for all rpcs - [x] having the whole block in /status is very verbose. trim it down +- [x] maybe we shouldn't route eth_getLogs to syncing nodes. serving queries slows down sync significantly + - change the send_best function to only include servers that are at least close to fully synced - [-] proxy mode for benchmarking all backends - [-] proxy mode for sending to multiple backends - [-] let users choose a % of reverts to log (or maybe x/second). someone like curve logging all reverts will be a BIG database very quickly @@ -416,9 +418,11 @@ These are not yet ordered. There might be duplicates. We might not actually need - then sites like curve.fi don't have to worry about their user count - it does mean we will have a harder time capacity planning from the number of keys - [ ] have the healthcheck get the block over http. if it errors, or doesn't match what the websocket says, something is wrong (likely a deadlock in the websocket code) +- [ ] has_block_data is too simple. it needs to know what kind of data is being requested + - all nodes have all blocks + - most nodes have all receipts + - only archives have old state - [ ] don't use new_head_provider anywhere except new head subscription -- [x] maybe we shouldn't route eth_getLogs to syncing nodes. serving queries slows down sync significantly - - change the send_best function to only include servers that are at least close to fully synced - [ ] enable mev protected transactions with either a /protect/ url (instead of /private/) or the database (when on /rpc/) - [-] have private transactions be enabled by a url setting rather than a setting on the key - [ ] eth_sendRawTransaction should only forward if the chain_id matches what we are running diff --git a/web3_proxy/src/bin/web3_proxy_cli/main.rs b/web3_proxy/src/bin/web3_proxy_cli/main.rs index 692324f0..06d8c22f 100644 --- a/web3_proxy/src/bin/web3_proxy_cli/main.rs +++ b/web3_proxy/src/bin/web3_proxy_cli/main.rs @@ -143,7 +143,8 @@ fn main() -> anyhow::Result<()> { vec![ "info", "ethers=debug", - "ethers_providers=warn", + // TODO: even error is too verbose for our purposes. how can we turn off this logging entirely? + "ethers_providers=error", "redis_rate_limit=debug", "web3_proxy=debug", "web3_proxy_cli=debug", @@ -229,6 +230,8 @@ fn main() -> anyhow::Result<()> { }; log::set_max_level(max_level); + + info!("RUST_LOG={}", rust_log); } info!("{}", APP_USER_AGENT); diff --git a/web3_proxy/src/frontend/errors.rs b/web3_proxy/src/frontend/errors.rs index 3d6e8164..8911e8ea 100644 --- a/web3_proxy/src/frontend/errors.rs +++ b/web3_proxy/src/frontend/errors.rs @@ -67,7 +67,9 @@ pub enum Web3ProxyError { JoinError(JoinError), #[display(fmt = "{:?}", _0)] #[error(ignore)] - JsonRpc(crate::jsonrpc::JsonRpcErrorData), + JsonRpcForwardedError(JsonRpcForwardedResponse), + #[display(fmt = "{:?}", _0)] + #[error(ignore)] MsgPackEncode(rmp_serde::encode::Error), NoBlockNumberOrHash, NoBlocksKnown, @@ -247,6 +249,7 @@ impl Web3ProxyError { ), ) } + Self::JsonRpcForwardedError(x) => (StatusCode::OK, x), Self::GasEstimateNotU256 => { warn!("GasEstimateNotU256"); ( @@ -413,24 +416,13 @@ impl Web3ProxyError { ), ) } - Self::JsonRpc(err) => { - debug!("JsonRpc err={:?}", err); - ( - StatusCode::BAD_REQUEST, - JsonRpcForwardedResponse::from_str( - "json rpc error!", - Some(StatusCode::BAD_REQUEST.as_u16().into()), - None, - ), - ) - } Self::MsgPackEncode(err) => { - debug!("MsgPackEncode Error: {}", err); + warn!("MsgPackEncode Error: {}", err); ( - StatusCode::BAD_REQUEST, + StatusCode::INTERNAL_SERVER_ERROR, JsonRpcForwardedResponse::from_str( &format!("msgpack encode error: {}", err), - Some(StatusCode::BAD_REQUEST.as_u16().into()), + Some(StatusCode::INTERNAL_SERVER_ERROR.as_u16().into()), None, ), ) @@ -438,10 +430,10 @@ impl Web3ProxyError { Self::NoBlockNumberOrHash => { warn!("NoBlockNumberOrHash"); ( - StatusCode::BAD_REQUEST, + StatusCode::INTERNAL_SERVER_ERROR, JsonRpcForwardedResponse::from_str( "Blocks here must have a number or hash", - Some(StatusCode::BAD_REQUEST.as_u16().into()), + Some(StatusCode::INTERNAL_SERVER_ERROR.as_u16().into()), None, ), ) @@ -449,10 +441,10 @@ impl Web3ProxyError { Self::NoBlocksKnown => { error!("NoBlocksKnown"); ( - StatusCode::INTERNAL_SERVER_ERROR, + StatusCode::BAD_GATEWAY, JsonRpcForwardedResponse::from_str( "no blocks known", - Some(StatusCode::INTERNAL_SERVER_ERROR.as_u16().into()), + Some(StatusCode::BAD_GATEWAY.as_u16().into()), None, ), ) @@ -460,10 +452,10 @@ impl Web3ProxyError { Self::NoConsensusHeadBlock => { error!("NoConsensusHeadBlock"); ( - StatusCode::INTERNAL_SERVER_ERROR, + StatusCode::BAD_GATEWAY, JsonRpcForwardedResponse::from_str( "no consensus head block", - Some(StatusCode::INTERNAL_SERVER_ERROR.as_u16().into()), + Some(StatusCode::BAD_GATEWAY.as_u16().into()), None, ), ) @@ -506,7 +498,7 @@ impl Web3ProxyError { } Self::NotFound => { // TODO: emit a stat? - // TODO: instead of an error, show a normal html page for 404 + // TODO: instead of an error, show a normal html page for 404? ( StatusCode::NOT_FOUND, JsonRpcForwardedResponse::from_str( @@ -528,7 +520,7 @@ impl Web3ProxyError { ) } Self::OriginRequired => { - warn!("OriginRequired"); + trace!("OriginRequired"); ( StatusCode::BAD_REQUEST, JsonRpcForwardedResponse::from_str( @@ -539,7 +531,7 @@ impl Web3ProxyError { ) } Self::OriginNotAllowed(origin) => { - warn!("OriginNotAllowed origin={}", origin); + trace!("OriginNotAllowed origin={}", origin); ( StatusCode::FORBIDDEN, JsonRpcForwardedResponse::from_string( @@ -550,7 +542,7 @@ impl Web3ProxyError { ) } Self::ParseBytesError(err) => { - warn!("ParseBytesError err={:?}", err); + trace!("ParseBytesError err={:?}", err); ( StatusCode::BAD_REQUEST, JsonRpcForwardedResponse::from_str( @@ -561,7 +553,7 @@ impl Web3ProxyError { ) } Self::ParseMsgError(err) => { - warn!("ParseMsgError err={:?}", err); + trace!("ParseMsgError err={:?}", err); ( StatusCode::BAD_REQUEST, JsonRpcForwardedResponse::from_str( @@ -572,9 +564,9 @@ impl Web3ProxyError { ) } Self::ParseAddressError => { - warn!("ParseAddressError"); + trace!("ParseAddressError"); ( - StatusCode::UNAUTHORIZED, + StatusCode::BAD_REQUEST, JsonRpcForwardedResponse::from_str( "unable to parse address", Some(StatusCode::BAD_REQUEST.as_u16().into()), @@ -775,6 +767,7 @@ impl Web3ProxyError { } Self::UserIdZero => { warn!("UserIdZero"); + // TODO: this might actually be an application error and not a BAD_REQUEST ( StatusCode::BAD_REQUEST, JsonRpcForwardedResponse::from_str( @@ -785,7 +778,7 @@ impl Web3ProxyError { ) } Self::VerificationError(err) => { - warn!("VerificationError err={:?}", err); + trace!("VerificationError err={:?}", err); ( StatusCode::BAD_REQUEST, JsonRpcForwardedResponse::from_str( @@ -818,7 +811,7 @@ impl Web3ProxyError { ) } Self::WebsocketOnly => { - warn!("WebsocketOnly"); + trace!("WebsocketOnly"); ( StatusCode::BAD_REQUEST, JsonRpcForwardedResponse::from_str( @@ -828,20 +821,23 @@ impl Web3ProxyError { ), ) } - Self::WithContext(err, msg) => { - info!("in context: {}", msg); - match err { - Some(err) => err.into_response_parts(), - None => ( + Self::WithContext(err, msg) => match err { + Some(err) => { + warn!("{:#?} w/ context {}", err, msg); + err.into_response_parts() + } + None => { + warn!("error w/ context {}", msg); + ( StatusCode::INTERNAL_SERVER_ERROR, JsonRpcForwardedResponse::from_string( msg, Some(StatusCode::INTERNAL_SERVER_ERROR.as_u16().into()), None, ), - ), + ) } - } + }, } } } diff --git a/web3_proxy/src/frontend/rpc_proxy_ws.rs b/web3_proxy/src/frontend/rpc_proxy_ws.rs index 28c9f934..dd04f0dc 100644 --- a/web3_proxy/src/frontend/rpc_proxy_ws.rs +++ b/web3_proxy/src/frontend/rpc_proxy_ws.rs @@ -4,6 +4,7 @@ use super::authorization::{ip_is_authorized, key_is_authorized, Authorization, RequestMetadata}; use super::errors::{Web3ProxyError, Web3ProxyResponse}; +use crate::jsonrpc::JsonRpcId; use crate::stats::RpcQueryStats; use crate::{ app::Web3ProxyApp, @@ -29,7 +30,6 @@ use hashbrown::HashMap; use http::StatusCode; use log::{info, trace, warn}; use serde_json::json; -use serde_json::value::to_raw_value; use std::sync::Arc; use std::{str::from_utf8_mut, sync::atomic::AtomicUsize}; use tokio::sync::{broadcast, OwnedSemaphorePermit, RwLock}; @@ -420,9 +420,7 @@ async fn handle_socket_payload( (id, response) } Err(err) => { - // TODO: move this logic somewhere else and just set id to None here - let id = - to_raw_value(&json!(None::>)).expect("None can always be a RawValue"); + let id = JsonRpcId::None.to_raw_value(); (id, Err(err.into())) } }; diff --git a/web3_proxy/src/jsonrpc.rs b/web3_proxy/src/jsonrpc.rs index 1bd70e79..78c76b0e 100644 --- a/web3_proxy/src/jsonrpc.rs +++ b/web3_proxy/src/jsonrpc.rs @@ -7,8 +7,6 @@ use serde_json::json; use serde_json::value::{to_raw_value, RawValue}; use std::fmt; -// this is used by serde -#[allow(dead_code)] fn default_jsonrpc() -> String { "2.0".to_string() } @@ -24,6 +22,45 @@ pub struct JsonRpcRequest { pub params: Option, } +#[derive(From)] +pub enum JsonRpcId { + None, + Number(u64), + String(String), +} + +impl JsonRpcId { + pub fn to_raw_value(&self) -> Box { + // TODO: is this a good way to do this? we should probably use references + match self { + Self::None => { + to_raw_value(&json!(None::>)).expect("null id should always work") + } + Self::Number(x) => { + serde_json::from_value(json!(x)).expect("number id should always work") + } + Self::String(x) => serde_json::from_str(x).expect("string id should always work"), + } + } +} + +impl JsonRpcRequest { + pub fn new( + id: JsonRpcId, + method: String, + params: Option, + ) -> anyhow::Result { + let x = Self { + jsonrpc: default_jsonrpc(), + id: id.to_raw_value(), + method, + params, + }; + + Ok(x) + } +} + impl fmt::Debug for JsonRpcRequest { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { // TODO: the default formatter takes forever to write. this is too quiet though @@ -206,9 +243,7 @@ impl JsonRpcForwardedResponse { // TODO: can we somehow get the initial request here? if we put that into a tracing span, will things slow down a ton? JsonRpcForwardedResponse { jsonrpc: "2.0".to_string(), - id: id.unwrap_or_else(|| { - to_raw_value(&json!(None::>)).expect("null id should always work") - }), + id: id.unwrap_or_else(|| JsonRpcId::None.to_raw_value()), result: None, error: Some(JsonRpcErrorData { code: code.unwrap_or(-32099), diff --git a/web3_proxy/src/rpcs/blockchain.rs b/web3_proxy/src/rpcs/blockchain.rs index 18abc471..34a09e7a 100644 --- a/web3_proxy/src/rpcs/blockchain.rs +++ b/web3_proxy/src/rpcs/blockchain.rs @@ -251,8 +251,8 @@ impl Web3Rpcs { ) .await?; - if let Some(err) = response.error { - return Err(err).web3_context("failed fetching block"); + if response.error.is_some() { + return Err(response.into()); } let block = response @@ -347,8 +347,8 @@ impl Web3Rpcs { .try_send_best_consensus_head_connection(authorization, request, None, Some(num), None) .await?; - if let Some(err) = response.error { - debug!("could not find canonical block {}: {:?}", num, err); + if response.error.is_some() { + return Err(response.into()); } let raw_block = response.result.web3_context("no cannonical block result")?;