Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(rpc): Refactor getrawtransaction & RPC error handling #9049

Open
wants to merge 39 commits into
base: main
Choose a base branch
from

Conversation

upbqdn
Copy link
Member

@upbqdn upbqdn commented Nov 21, 2024

Motivation

Close #8945.

Specifications & References

Solution

  • Fix the bugs described in bug: getrawtransaction output conflicts with zcashd output #8744. Note that zcashd stores transactions in orphaned blocks and responds to queries requesting such transactions with height to -1. Since Zebra doesn't keep orphaned blocks below the rollback limit, it now returns the error "No such mempool or blockchain transaction". If we wanted to match zcashd, we'd need to start storing orphaned blocks.
  • Fix the error codes for getrawtransaction and some other RPCs.
  • Related cleanups:
    • Refactor the error handling for RPCs.
    • Add references to the original error codes.
    • Use HexData instead of Strings in getrawtransaction.
    • Simplify prop tests and run them with arbitrary Networks.

Tests

Update and add

  • snapshot tests,
  • unit tests, and
  • prop tests

to check the new error codes.

Follow-up Work

  • None

PR Author's Checklist

  • The PR name will make sense to users.
  • The PR provides a CHANGELOG summary.
  • The solution is tested.
  • The documentation is up to date.
  • The PR has a priority label.

PR Reviewer's Checklist

  • The PR Author's checklist is complete.
  • The PR resolves the issue.

@upbqdn upbqdn added C-bug Category: This is a bug A-rpc Area: Remote Procedure Call interfaces P-Medium ⚡ rust Pull requests that update Rust code labels Nov 21, 2024
@upbqdn upbqdn self-assigned this Nov 21, 2024
@upbqdn upbqdn requested a review from a team as a code owner November 21, 2024 15:05
@upbqdn upbqdn requested review from arya2 and removed request for a team November 21, 2024 15:05
Copy link
Contributor

@arya2 arya2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking good so far, I left an optional suggestion about matching zcashd's output when a transaction is found in a side chain.

I'd like to review the changes to the get_block() method against #9006 if we can target this PR against the get-block branch (or wait for #9006 to merge and rebase off of main).

Comment on lines +60 to +78
/// A trait for mapping errors to [`jsonrpc_core::Error`].
pub(crate) trait MapError<T> {
/// Maps errors to [`jsonrpc_core::Error`] with a specific error code.
fn map_error(
self,
code: impl Into<jsonrpc_core::ErrorCode>,
) -> std::result::Result<T, jsonrpc_core::Error>;
}

/// A trait for conditionally converting a value into a `Result<T, jsonrpc_core::Error>`.
pub(crate) trait OkOrError<T> {
/// Converts the implementing type to `Result<T, jsonrpc_core::Error>`, using an error code and
/// message if conversion is to `Err`.
fn ok_or_error(
self,
code: impl Into<jsonrpc_core::ErrorCode>,
message: impl ToString,
) -> std::result::Result<T, jsonrpc_core::Error>;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error constructor methods for commonly-used error codes (or just Misc) could be nice here.

Suggested change
/// A trait for mapping errors to [`jsonrpc_core::Error`].
pub(crate) trait MapError<T> {
/// Maps errors to [`jsonrpc_core::Error`] with a specific error code.
fn map_error(
self,
code: impl Into<jsonrpc_core::ErrorCode>,
) -> std::result::Result<T, jsonrpc_core::Error>;
}
/// A trait for conditionally converting a value into a `Result<T, jsonrpc_core::Error>`.
pub(crate) trait OkOrError<T> {
/// Converts the implementing type to `Result<T, jsonrpc_core::Error>`, using an error code and
/// message if conversion is to `Err`.
fn ok_or_error(
self,
code: impl Into<jsonrpc_core::ErrorCode>,
message: impl ToString,
) -> std::result::Result<T, jsonrpc_core::Error>;
}
/// A trait for mapping errors to [`jsonrpc_core::Error`].
pub(crate) trait MapError<T>: Sized {
/// Maps errors to [`jsonrpc_core::Error`] with a specific error code.
fn map_error(
self,
code: impl Into<jsonrpc_core::ErrorCode>,
) -> std::result::Result<T, jsonrpc_core::Error>;
/// Maps errors to [`jsonrpc_core::Error`] with a [`LegacyCode::Misc`] error code.
fn map_misc_error(self) -> std::result::Result<T, jsonrpc_core::Error> {
self.map_error(LegacyCode::Misc)
}
}
/// A trait for conditionally converting a value into a `Result<T, jsonrpc_core::Error>`.
pub(crate) trait OkOrError<T>: Sized {
/// Converts the implementing type to `Result<T, jsonrpc_core::Error>`, using an error code and
/// message if conversion is to `Err`.
fn ok_or_error(
self,
code: impl Into<jsonrpc_core::ErrorCode>,
message: impl ToString,
) -> std::result::Result<T, jsonrpc_core::Error>;
/// Converts the implementing type to `Result<T, jsonrpc_core::Error>`, using a [`LegacyCode::Misc`] error code.
fn ok_or_misc_error(
self,
message: impl ToString,
) -> std::result::Result<T, jsonrpc_core::Error> {
self.ok_or_error(LegacyCode::Misc, message)
}
}

fn get_raw_transaction(
&self,
txid_hex: String,
HexData(txid): HexData,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be typed as a transaction::Hash (which implements ToHex & FromHex), though it may return the wrong error code that way.

.ready()
.and_then(|service| service.call(request))
.and_then(|service| service.call(zebra_state::ReadRequest::Transaction(txid)))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional: To match zcashd's output when a transaction is on a side chain, we either need a new request to check for transactions on side chains, or to update this request so that it checks side chains for the transaction id too and returns an enum variant indicating whether the transaction was found on the best chain or a side chain.

zebra-rpc/src/methods.rs Show resolved Hide resolved
@mpguerra mpguerra linked an issue Nov 22, 2024 that may be closed by this pull request
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rpc Area: Remote Procedure Call interfaces C-bug Category: This is a bug P-Medium ⚡ rust Pull requests that update Rust code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

getrawtransaction fixes and updates bug: getrawtransaction output conflicts with zcashd output
2 participants