From 6b667a08f7e0d912dd5296860228f95be02df630 Mon Sep 17 00:00:00 2001 From: zancas Date: Wed, 15 Jul 2026 23:34:55 -0700 Subject: [PATCH] fix: surface node rejections instead of masking them as internal errors zainod 0.6.0 reported every zebrad `sendrawtransaction` rejection to wallets as the fixed string "InternalServerError: error receiving data from backing node", where the 0.6.0-rc.1 baseline surfaced the node's actual rejection (for example "RPC Error (code: -25): failed to validate tx: ..."). Issue zingolabs/zaino#1404 reports the regression. Two cooperating faults produced the masking. First, `RpcRequestError::UnexpectedErrorResponse` carried the node's typed `RpcError` in a plain boxed field without `#[source]`, so thiserror severed the `source()` chain at that link and the downcast-walk recovery that zaino-serve's JSON-RPC surface performs could never find the rejection. Second, the gRPC surface's `From for tonic::Status` used only `ChainIndexError`'s own message, which `ChainIndexError::backing_validator` sets to a generic wrapper string. This commit restores attribution at both severing points. The `UnexpectedErrorResponse` payload is now `#[source]`, which heals the chain for all eight `impl_rpc_error_passthrough!` RPC types on every serving surface. The tonic::Status conversion now walks the source chain for a typed `RpcError` and, when one is present, reports it instead of the wrapper message. Following the test-first order for a tracked bug, three regression tests were written and confirmed red before the fix: a unit test pinning that `UnexpectedErrorResponse` exposes its payload via `source()`, a unit test driving the exact production error chain (RpcError through BlockchainSourceError and ChainIndexError into tonic::Status) and asserting the rejection reason and legacy code -25 survive, and an end-to-end mockchain test through `IndexReader::send_raw_transaction`. To support the last, `MockchainSource::send_raw_transaction` now returns a validator-shaped rejection instead of `unimplemented!`, standing in for a rejecting zebrad. The existing invalid-hex regression test's source-chain walk is extracted into a shared helper both tests use. Closes zingolabs/zaino#1404. Co-Authored-By: Claude Fable 5 --- .../zaino-fetch/src/jsonrpsee/connector.rs | 39 ++++++++++- .../chain_index/source/mockchain_source.rs | 17 ++++- .../src/chain_index/tests/mockchain_tests.rs | 62 ++++++++++++++---- packages/zaino-state/src/error.rs | 64 ++++++++++++++++++- 4 files changed, 165 insertions(+), 17 deletions(-) diff --git a/packages/zaino-fetch/src/jsonrpsee/connector.rs b/packages/zaino-fetch/src/jsonrpsee/connector.rs index b807dbcce..98fa8ac8b 100644 --- a/packages/zaino-fetch/src/jsonrpsee/connector.rs +++ b/packages/zaino-fetch/src/jsonrpsee/connector.rs @@ -184,8 +184,14 @@ pub enum RpcRequestError { /// wasn't accounted for as a MethodError. This means that either /// Zaino has not yet accounted for the possibilty of this error, /// or the Node returned an undocumented/malformed error response. + /// + /// The payload is `#[source]` so the node's typed rejection (usually an + /// [`RpcError`]) stays reachable through `source()` chains: the serving + /// surfaces recover rejection codes and messages by downcast-walking those + /// chains, and severing the link here masks every unmapped node rejection + /// as a generic internal error (zingolabs/zaino#1404). #[error("unexpected error response from server: {0}")] - UnexpectedErrorResponse(Box), + UnexpectedErrorResponse(#[source] Box), } /// JsonRpSee Client config data. @@ -1070,6 +1076,37 @@ pub async fn test_node_and_return_url( mod tests { use super::*; + /// zaino-serve attributes node rejections by downcast-walking `source()` + /// chains for the typed [`RpcError`] (see + /// `sendrawtransaction_error_object_from_indexer_error` in + /// `zaino-serve/src/rpc/jsonrpc/service.rs`). `UnexpectedErrorResponse` is + /// the variant that carries a node's structured rejection whenever no + /// method-specific mapping exists (every `impl_rpc_error_passthrough!` + /// type, including `sendrawtransaction`'s), so its boxed payload must stay + /// reachable through `source()`. Without the link, every such rejection is + /// masked as a generic internal error (zingolabs/zaino#1404). + #[test] + fn unexpected_error_response_exposes_rpc_error_via_source() { + use crate::jsonrpsee::response::SendTransactionError; + + let rejection = RpcError::new_from_legacycode( + zebra_rpc::server::error::LegacyCode::Verify, + "failed to validate tx: transparent input not found", + ); + let error: RpcRequestError = + RpcRequestError::UnexpectedErrorResponse(Box::new(rejection)); + + let source = std::error::Error::source(&error) + .expect("the boxed node rejection must be exposed via source()"); + let rpc_error = source + .downcast_ref::() + .expect("the source must downcast to the typed RpcError"); + assert_eq!( + rpc_error.code, + zebra_rpc::server::error::LegacyCode::Verify as i64 + ); + } + #[test] fn test_resolve_address_wraps_common_function() { // Verify the wrapper correctly converts io::Error to TransportError diff --git a/packages/zaino-state/src/chain_index/source/mockchain_source.rs b/packages/zaino-state/src/chain_index/source/mockchain_source.rs index a6bc290e8..76045b9ad 100644 --- a/packages/zaino-state/src/chain_index/source/mockchain_source.rs +++ b/packages/zaino-state/src/chain_index/source/mockchain_source.rs @@ -807,8 +807,21 @@ impl BlockchainSource for MockchainSource { &self, _raw_transaction_hex: String, ) -> BlockchainSourceResult { - // The mock chain has no mempool to accept submissions. - unimplemented!("MockchainSource cannot serve send_raw_transaction") + // The mock chain has no mempool to accept submissions, so every one is + // rejected — shaped exactly like a validator rejection (a typed + // `RpcError` inside `RpcRequestError::UnexpectedErrorResponse`) so tests + // can exercise the full rejection-attribution path a real zebrad + // rejection travels (zingolabs/zaino#1404). + Err(BlockchainSourceError::unrecoverable( + zaino_fetch::jsonrpsee::connector::RpcRequestError::< + zaino_fetch::jsonrpsee::response::SendTransactionError, + >::UnexpectedErrorResponse(Box::new( + zaino_fetch::jsonrpsee::connector::RpcError::new_from_legacycode( + zebra_rpc::server::error::LegacyCode::Verify, + "MockchainSource rejects all transaction submissions (static chain, no mempool)", + ), + )), + )) } async fn get_treestate_by_id( diff --git a/packages/zaino-state/src/chain_index/tests/mockchain_tests.rs b/packages/zaino-state/src/chain_index/tests/mockchain_tests.rs index 2ee2de790..a35156a86 100644 --- a/packages/zaino-state/src/chain_index/tests/mockchain_tests.rs +++ b/packages/zaino-state/src/chain_index/tests/mockchain_tests.rs @@ -1265,6 +1265,18 @@ async fn chaintip_update_subscriber_absent_without_tip_stream() { ); } +/// The code of the first typed [`RpcError`](zaino_fetch::jsonrpsee::connector::RpcError) +/// in `error`'s `source()` chain, if any — the same walk zaino-serve's +/// `sendrawtransaction_error_object_from_indexer_error` performs to recover +/// zcashd legacy error codes. +fn rpc_error_code_in_source_chain(error: &(dyn std::error::Error + 'static)) -> Option { + std::iter::successors(Some(error), |source| source.source()).find_map(|source| { + source + .downcast_ref::() + .map(|rpc_error| rpc_error.code) + }) +} + /// `sendrawtransaction` rejections must carry zcashd's legacy error code: /// zaino-serve forwards the code by downcast-walking the `source()` chain for /// the typed `RpcError` (`sendrawtransaction_error_object_from_indexer_error`), @@ -1273,7 +1285,8 @@ async fn chaintip_update_subscriber_absent_without_tip_stream() { /// /// Invalid hex fails in local validation before the source is consulted, so /// this also pins that the rejection happens without a validator round-trip -/// (the mock's `send_raw_transaction` would panic if reached). +/// (the mock's `send_raw_transaction` returns its own distinct rejection, code +/// `-25`, so reaching it would fail the `-8` assertion below). #[tokio::test(flavor = "multi_thread")] async fn send_raw_transaction_invalid_hex_keeps_legacy_error_code() { let (_blocks, _indexer, index_reader, _mockchain) = @@ -1284,20 +1297,43 @@ async fn send_raw_transaction_invalid_hex_keeps_legacy_error_code() { .await .expect_err("invalid hex must be rejected"); - let mut current: Option<&(dyn std::error::Error + 'static)> = Some(&error); - let mut rpc_error_code = None; - while let Some(source_error) = current { - if let Some(rpc_error) = - source_error.downcast_ref::() - { - rpc_error_code = Some(rpc_error.code); - break; - } - current = source_error.source(); - } assert_eq!( - rpc_error_code, + rpc_error_code_in_source_chain(&error), Some(zebra_rpc::server::error::LegacyCode::InvalidParameter as i64), "the typed RpcError (legacy code -8) must stay reachable via the source() chain" ); } + +/// A node rejection of `sendrawtransaction` must stay attributable end to end +/// (zingolabs/zaino#1404): the typed `RpcError` reachable via `source()` for +/// zaino-serve's JSON-RPC error-code recovery walk, and the rejection text +/// present in the wallet-visible `tonic::Status` on the gRPC surface. zainod +/// 0.6.0 masked both as the fixed string "InternalServerError: error receiving +/// data from backing node". The mockchain source stands in for zebrad by +/// rejecting every submission with a validator-shaped `-25` error. +#[tokio::test(flavor = "multi_thread")] +async fn send_raw_transaction_node_rejection_stays_attributable() { + let (_blocks, _indexer, index_reader, _mockchain) = + load_test_vectors_and_sync_chain_index(MockchainMode::Static).await; + + // Valid hex, so local validation passes and the (rejecting) source is consulted. + let error = index_reader + .send_raw_transaction("deadbeef".to_string()) + .await + .expect_err("the mockchain source rejects every submission"); + + assert_eq!( + rpc_error_code_in_source_chain(&error), + Some(zebra_rpc::server::error::LegacyCode::Verify as i64), + "the node's typed RpcError (legacy code -25) must stay reachable via the source() chain" + ); + + let status = tonic::Status::from(crate::error::NodeBackedIndexerServiceError::from(error)); + assert!( + status + .message() + .contains("rejects all transaction submissions"), + "wallet-visible status must carry the node's rejection reason, got: {:?}", + status.message() + ); +} diff --git a/packages/zaino-state/src/error.rs b/packages/zaino-state/src/error.rs index b0eb87b31..ca4b06490 100644 --- a/packages/zaino-state/src/error.rs +++ b/packages/zaino-state/src/error.rs @@ -114,6 +114,17 @@ impl From for NodeBackedIndexerServiceError { } } +/// The first typed JSON-RPC error in `error`'s `source()` chain, if any — the +/// node's structured rejection that a generic wrapper message otherwise hides. +/// The gRPC counterpart of zaino-serve's JSON-RPC error-code recovery walk +/// (`sendrawtransaction_error_object_from_indexer_error`). +fn rpc_error_in_source_chain<'error>( + error: &'error (dyn std::error::Error + 'static), +) -> Option<&'error zaino_fetch::jsonrpsee::connector::RpcError> { + std::iter::successors(Some(error), |source| source.source()) + .find_map(|source| source.downcast_ref()) +} + #[allow(deprecated)] impl From for tonic::Status { fn from(error: NodeBackedIndexerServiceError) -> Self { @@ -130,7 +141,16 @@ impl From for tonic::Status { tonic::Status::internal(format!("RPC error: {err:?}")) } NodeBackedIndexerServiceError::ChainIndexError(err) => match err.kind { - ChainIndexErrorKind::InternalServerError => tonic::Status::internal(err.message), + // A `ChainIndexError`'s own message may be a generic wrapper (e.g. + // `backing_validator`'s fixed string), so recover the node's typed + // rejection from the source chain when one is present: wallets + // must be able to tell "the node rejected this, and here is why" + // apart from "the indexer's connection to the node broke" + // (zingolabs/zaino#1404). + ChainIndexErrorKind::InternalServerError => match rpc_error_in_source_chain(&err) { + Some(rpc_error) => tonic::Status::internal(rpc_error.to_string()), + None => tonic::Status::internal(err.message), + }, ChainIndexErrorKind::InvalidSnapshot => { tonic::Status::failed_precondition(err.message) } @@ -642,3 +662,45 @@ impl From for ChainIndexError { } } } + +#[cfg(test)] +mod tonic_status_from_service_error { + use super::*; + use crate::chain_index::source::BlockchainSourceError; + use zaino_fetch::jsonrpsee::connector::{RpcError, RpcRequestError}; + use zaino_fetch::jsonrpsee::response::SendTransactionError; + use zebra_rpc::server::error::LegacyCode; + + /// A zebrad `sendrawtransaction` rejection reaches the gRPC surface as + /// `RpcError` → `RpcRequestError::UnexpectedErrorResponse` → + /// `BlockchainSourceError::unrecoverable` → `ChainIndexError::backing_validator` + /// → [`NodeBackedIndexerServiceError`] → [`tonic::Status`]. The wallet-visible + /// status must still name the node's rejection reason and its legacy error + /// code; zainod 0.6.0 masked both as the fixed string "InternalServerError: + /// error receiving data from backing node" (zingolabs/zaino#1404). + #[test] + fn send_rejection_survives_conversion_to_tonic_status() { + let rejection = RpcError::new_from_legacycode( + LegacyCode::Verify, + "failed to validate tx: transparent input not found", + ); + let request_error: RpcRequestError = + RpcRequestError::UnexpectedErrorResponse(Box::new(rejection)); + let source_error = BlockchainSourceError::unrecoverable(request_error); + let index_error = ChainIndexError::backing_validator(source_error); + + let status = tonic::Status::from(NodeBackedIndexerServiceError::from(index_error)); + + assert_eq!(status.code(), tonic::Code::Internal); + assert!( + status.message().contains("failed to validate tx"), + "wallet-visible status must carry the node's rejection reason, got: {:?}", + status.message() + ); + assert!( + status.message().contains("-25"), + "wallet-visible status must carry the legacy error code, got: {:?}", + status.message() + ); + } +}