From 0e0aefaa4ed7c999ccb1da6a5e3b2b90d64260a2 Mon Sep 17 00:00:00 2001 From: Janito Vaqueiro Ferreira Filho Date: Tue, 8 Mar 2022 06:14:21 -0300 Subject: [PATCH] Refactor `SentTransactionHash` to be a stricter type (#3706) * Stub `sendrawtransaction` RPC method Register the RPC method, and stub an implementation that currently just panics. The method has a single `String` parameter with the hexadecimal string of the raw transaction's bytes and returns a `SentTransactionHash` wrapper type that's just a hexadecimal `String` of the sent transaction's hash. * Add mempool service instance to `RpcImpl` Use a type parameter to represent the mempool service using the interface defined by `zebra-node-services`. * Update test vector to use a mock mempool service Update the test to be compatible with the changes to `RpcImpl`. The mock mempool service is expected to not be used during the test. * Use a `tower::Buffer` for the mempool service Make it simpler to send requests to the service in a concurrent manner. * Return a `Future` from `send_raw_transaction` Make the call asynchronous. * Implement `sendrawtransaction` RPC Deserialize the transaction and send it to be queued for verification and subsequent inclusion in the mempool. * Test if mempool receives sent raw transaction Use a mock service as the mempool service and check that it receives a sent raw transaction. * Test using non-hexadecimal string parameter The method should return an error. * Test with bytes that fail deserialization Check that the method returns an invalid parameters error if the input can't be deserialized as a `Transaction`. * Test if mempool errors are forwarded to caller Mempool service errors should be sent back to the remote caller as server errors. * Test transactions rejected by the mempool service Transactions that are rejected by the mempool service should result in a server error being sent to the caller. * Improve error message Add the word "structurally" to make it clear that the issue is in the transaction's deserialization. Co-authored-by: Deirdre Connolly * Add note regarding missing `allowhighfees` param. The parameter isn't supported yet because `lightwalletd` doesn't use it. * Update the documentation to be consistent Follow the convention adopted by the `get_info` RPC method. * Implement `ToHex` and `FromHex` for `Hash` Make it easier to generate hexadecimal strings from `transaction::Hash` instances. * Use `ToHex` in `Debug` and `Display` Reduce repeated code. * Refactor to add `bytes_in_display_order` method Use it to remove repeated code and improve clarity a bit. * Use `hex::serialize` to serialize transaction hash Make the type stricter in its contents, while still serializing the transaction has as a hexadecimal string. * Simplify serialization attribute Deserialization should also use `hex::deserialize`, so using the shorter attribute makes things easier to read and more future proof. * Update zebra-chain/src/transaction/hash.rs * Remove unnecessary lifetime The anonymous lifetime is automatically inferred by the compiler. Co-authored-by: Deirdre Connolly --- Cargo.lock | 3 ++ zebra-chain/src/transaction/hash.rs | 51 +++++++++++++++++++++++++---- zebra-rpc/Cargo.toml | 2 +- zebra-rpc/src/methods.rs | 9 +++-- zebra-rpc/src/methods/tests/prop.rs | 2 +- zebrad/src/commands/start.rs | 2 +- 6 files changed, 57 insertions(+), 12 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 9c8db4f5..8e518f2f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1772,6 +1772,9 @@ name = "hex" version = "0.4.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7f24254aa9a54b5c858eaee2f5bccdb46aaf0e486a595ed5fd8f86ba55232a70" +dependencies = [ + "serde", +] [[package]] name = "hkdf" diff --git a/zebra-chain/src/transaction/hash.rs b/zebra-chain/src/transaction/hash.rs index 41298f85..fd18a56a 100644 --- a/zebra-chain/src/transaction/hash.rs +++ b/zebra-chain/src/transaction/hash.rs @@ -34,6 +34,7 @@ use std::{ #[cfg(any(test, feature = "proptest-impl"))] use proptest_derive::Arbitrary; +use hex::{FromHex, ToHex}; use serde::{Deserialize, Serialize}; use crate::serialization::{ @@ -100,20 +101,58 @@ impl From<&Hash> for [u8; 32] { } } -impl fmt::Display for Hash { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { +impl Hash { + /// Return the hash bytes in big-endian byte-order suitable for printing out byte by byte. + /// + /// Zebra displays transaction and block hashes in big-endian byte-order, + /// following the u256 convention set by Bitcoin and zcashd. + fn bytes_in_display_order(&self) -> [u8; 32] { let mut reversed_bytes = self.0; reversed_bytes.reverse(); - f.write_str(&hex::encode(&reversed_bytes)) + reversed_bytes + } +} + +impl ToHex for &Hash { + fn encode_hex>(&self) -> T { + self.bytes_in_display_order().encode_hex() + } + + fn encode_hex_upper>(&self) -> T { + self.bytes_in_display_order().encode_hex_upper() + } +} + +impl ToHex for Hash { + fn encode_hex>(&self) -> T { + (&self).encode_hex() + } + + fn encode_hex_upper>(&self) -> T { + (&self).encode_hex_upper() + } +} + +impl FromHex for Hash { + type Error = <[u8; 32] as FromHex>::Error; + + fn from_hex>(hex: T) -> Result { + let hash = <[u8; 32]>::from_hex(hex)?; + + Ok(hash.into()) + } +} + +impl fmt::Display for Hash { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + f.write_str(&self.encode_hex::()) } } impl fmt::Debug for Hash { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - let mut reversed_bytes = self.0; - reversed_bytes.reverse(); f.debug_tuple("transaction::Hash") - .field(&hex::encode(reversed_bytes)) + .field(&self.encode_hex::()) .finish() } } diff --git a/zebra-rpc/Cargo.toml b/zebra-rpc/Cargo.toml index ffb364e0..04e1c955 100644 --- a/zebra-rpc/Cargo.toml +++ b/zebra-rpc/Cargo.toml @@ -28,8 +28,8 @@ tower = "0.4.12" tracing = "0.1.31" tracing-futures = "0.2.5" +hex = { version = "0.4.3", features = ["serde"] } serde = { version = "1.0.136", features = ["serde_derive"] } -hex = "0.4.3" [dev-dependencies] proptest = "0.10.1" diff --git a/zebra-rpc/src/methods.rs b/zebra-rpc/src/methods.rs index 91b1c98e..2932ce68 100644 --- a/zebra-rpc/src/methods.rs +++ b/zebra-rpc/src/methods.rs @@ -12,7 +12,10 @@ use jsonrpc_core::{self, BoxFuture, Error, ErrorCode, Result}; use jsonrpc_derive::rpc; use tower::{buffer::Buffer, ServiceExt}; -use zebra_chain::{serialization::ZcashDeserialize, transaction::Transaction}; +use zebra_chain::{ + serialization::ZcashDeserialize, + transaction::{self, Transaction}, +}; use zebra_network::constants::USER_AGENT; use zebra_node_services::{mempool, BoxError}; @@ -156,7 +159,7 @@ where ); match &queue_results[0] { - Ok(()) => Ok(SentTransactionHash(transaction_hash.to_string())), + Ok(()) => Ok(SentTransactionHash(transaction_hash)), Err(error) => Err(Error { code: ErrorCode::ServerError(0), message: error.to_string(), @@ -186,4 +189,4 @@ pub struct GetBlockChainInfo { /// Response to a `sendrawtransaction` RPC request. /// /// A JSON string with the transaction hash in hexadecimal. -pub struct SentTransactionHash(String); +pub struct SentTransactionHash(#[serde(with = "hex")] transaction::Hash); diff --git a/zebra-rpc/src/methods/tests/prop.rs b/zebra-rpc/src/methods/tests/prop.rs index 369924e4..f67c6cb1 100644 --- a/zebra-rpc/src/methods/tests/prop.rs +++ b/zebra-rpc/src/methods/tests/prop.rs @@ -21,7 +21,7 @@ proptest! { runtime.block_on(async move { let mut mempool = MockService::build().for_prop_tests(); let rpc = RpcImpl::new("RPC test".to_owned(), Buffer::new(mempool.clone(), 1)); - let hash = SentTransactionHash(transaction.hash().to_string()); + let hash = SentTransactionHash(transaction.hash()); let transaction_bytes = transaction .zcash_serialize_to_vec() diff --git a/zebrad/src/commands/start.rs b/zebrad/src/commands/start.rs index e700dc48..35476a06 100644 --- a/zebrad/src/commands/start.rs +++ b/zebrad/src/commands/start.rs @@ -189,7 +189,7 @@ impl StartCmd { chain_tip_change, ); - let mempool_queue_checker_task_handle = mempool::QueueChecker::spawn(mempool); + let mempool_queue_checker_task_handle = mempool::QueueChecker::spawn(mempool.clone()); let tx_gossip_task_handle = tokio::spawn( mempool::gossip_mempool_transaction_id(mempool_transaction_receiver, peer_set)