From d0e81001bc7e614053a540fc32cf4d4667956850 Mon Sep 17 00:00:00 2001 From: Alfredo Garcia Date: Wed, 4 May 2022 22:08:27 -0300 Subject: [PATCH] fix(rpc): Use a structure for parameters of getaddresstxids (#4264) * add info to lightwalletd_state_path() * fix getaddresstxids rpc * normalize more the env vars in lightwalletd tests * extra logging Co-authored-by: teor Co-authored-by: teor --- zebra-rpc/src/methods.rs | 35 ++++++---- zebra-rpc/src/methods/tests/vectors.rs | 32 +++++++-- zebrad/tests/acceptance.rs | 15 ++-- zebrad/tests/common/lightwalletd.rs | 22 ++++-- .../lightwalletd/send_transaction_test.rs | 41 ++++++----- .../tests/common/lightwalletd/wallet_grpc.rs | 9 ++- .../common/lightwalletd/wallet_grpc_test.rs | 68 +++++++++---------- 7 files changed, 133 insertions(+), 89 deletions(-) diff --git a/zebra-rpc/src/methods.rs b/zebra-rpc/src/methods.rs index 218756539..bbae1b8fc 100644 --- a/zebra-rpc/src/methods.rs +++ b/zebra-rpc/src/methods.rs @@ -179,6 +179,7 @@ pub trait Rpc { /// /// # Parameters /// + /// A [`GetAddressTxIdsRequest`] struct with the following named fields: /// - `addresses`: (json array of string, required) The addresses to get transactions from. /// - `start`: (numeric, required) The lower height to start looking for transactions (inclusive). /// - `end`: (numeric, required) The top height to stop looking for transactions (inclusive). @@ -188,12 +189,8 @@ pub trait Rpc { /// Only the multi-argument format is used by lightwalletd and this is what we currently support: /// https://github.com/zcash/lightwalletd/blob/631bb16404e3d8b045e74a7c5489db626790b2f6/common/common.go#L97-L102 #[rpc(name = "getaddresstxids")] - fn get_address_tx_ids( - &self, - address_strings: AddressStrings, - start: u32, - end: u32, - ) -> BoxFuture>>; + fn get_address_tx_ids(&self, request: GetAddressTxIdsRequest) + -> BoxFuture>>; /// Returns all unspent outputs for a list of addresses. /// @@ -665,13 +662,11 @@ where fn get_address_tx_ids( &self, - address_strings: AddressStrings, - start: u32, - end: u32, + request: GetAddressTxIdsRequest, ) -> BoxFuture>> { let mut state = self.state.clone(); - let start = Height(start); - let end = Height(end); + let start = Height(request.start); + let end = Height(request.end); let chain_height = self.latest_chain_tip.best_tip_height().ok_or(Error { code: ErrorCode::ServerError(0), @@ -683,7 +678,10 @@ where // height range checks check_height_range(start, end, chain_height?)?; - let valid_addresses = address_strings.valid_addresses()?; + let valid_addresses = AddressStrings { + addresses: request.addresses, + } + .valid_addresses()?; let request = zebra_state::ReadRequest::TransactionIdsByAddresses { addresses: valid_addresses, @@ -967,6 +965,19 @@ pub struct GetAddressUtxos { height: Height, } +/// A struct to use as parameter of the `getaddresstxids`. +/// +/// See the notes for the [`Rpc::get_address_tx_ids` method]. +#[derive(Clone, Debug, Eq, PartialEq, serde::Deserialize)] +pub struct GetAddressTxIdsRequest { + // A list of addresses to get transactions from. + addresses: Vec, + // The height to start looking for transactions. + start: u32, + // The height to end looking for transactions. + end: u32, +} + impl GetRawTransaction { /// Converts `tx` and `height` into a new `GetRawTransaction` in the `verbose` format. fn from_transaction( diff --git a/zebra-rpc/src/methods/tests/vectors.rs b/zebra-rpc/src/methods/tests/vectors.rs index aafa8640f..f82f07a9a 100644 --- a/zebra-rpc/src/methods/tests/vectors.rs +++ b/zebra-rpc/src/methods/tests/vectors.rs @@ -337,14 +337,18 @@ async fn rpc_getaddresstxids_invalid_arguments() { let start: u32 = 1; let end: u32 = 2; let error = rpc - .get_address_tx_ids(AddressStrings::new(addresses), start, end) + .get_address_tx_ids(GetAddressTxIdsRequest { + addresses: addresses.clone(), + start, + end, + }) .await .unwrap_err(); assert_eq!( error.message, format!( "invalid address \"{}\": parse error: t-addr decoding error", - address + address.clone() ) ); @@ -356,7 +360,11 @@ async fn rpc_getaddresstxids_invalid_arguments() { let start: u32 = 2; let end: u32 = 1; let error = rpc - .get_address_tx_ids(AddressStrings::new(addresses.clone()), start, end) + .get_address_tx_ids(GetAddressTxIdsRequest { + addresses: addresses.clone(), + start, + end, + }) .await .unwrap_err(); assert_eq!( @@ -368,7 +376,11 @@ async fn rpc_getaddresstxids_invalid_arguments() { let start: u32 = 0; let end: u32 = 1; let error = rpc - .get_address_tx_ids(AddressStrings::new(addresses.clone()), start, end) + .get_address_tx_ids(GetAddressTxIdsRequest { + addresses: addresses.clone(), + start, + end, + }) .await .unwrap_err(); assert_eq!( @@ -380,7 +392,11 @@ async fn rpc_getaddresstxids_invalid_arguments() { let start: u32 = 1; let end: u32 = 11; let error = rpc - .get_address_tx_ids(AddressStrings::new(addresses), start, end) + .get_address_tx_ids(GetAddressTxIdsRequest { + addresses, + start, + end, + }) .await .unwrap_err(); assert_eq!( @@ -459,7 +475,11 @@ async fn rpc_getaddresstxids_response_with( // call the method with valid arguments let addresses = vec![address.to_string()]; let response = rpc - .get_address_tx_ids(AddressStrings::new(addresses), *range.start(), *range.end()) + .get_address_tx_ids(GetAddressTxIdsRequest { + addresses, + start: *range.start(), + end: *range.end(), + }) .await .expect("arguments are valid so no error can happen here"); diff --git a/zebrad/tests/acceptance.rs b/zebrad/tests/acceptance.rs index 9128b9681..a08535185 100644 --- a/zebrad/tests/acceptance.rs +++ b/zebrad/tests/acceptance.rs @@ -49,7 +49,6 @@ use common::{ lightwalletd::{ random_known_rpc_port_config, zebra_skip_lightwalletd_tests, LightWalletdTestDirExt, LightwalletdTestType::{self, *}, - LIGHTWALLETD_DATA_DIR_VAR, }, sync::{ create_cached_database_height, sync_until, MempoolBehavior, LARGE_CHECKPOINT_TEST_HEIGHT, @@ -1083,7 +1082,9 @@ fn lightwalletd_integration_test(test_type: LightwalletdTestType) -> Result<()> // Write a configuration that has RPC listen_addr set. // If the state path env var is set, use it in the config. - let config = if let Some(config) = test_type.zebrad_config() { + let config = if let Some(config) = + test_type.zebrad_config("lightwalletd_integration_test".to_string()) + { config? } else { return Ok(()); @@ -1093,14 +1094,10 @@ fn lightwalletd_integration_test(test_type: LightwalletdTestType) -> Result<()> // - LaunchWithEmptyState: ignore the state directory // - FullSyncFromGenesis: use it if available, timeout if it is already populated // - UpdateCachedState: skip the test if it is not available, timeout if it is not populated - let lightwalletd_state_path = test_type.lightwalletd_state_path(); + let lightwalletd_state_path = + test_type.lightwalletd_state_path("lightwalletd_integration_test".to_string()); if test_type.needs_lightwalletd_cached_state() && lightwalletd_state_path.is_none() { - tracing::info!( - "skipped {test_type:?} lightwalletd test, \ - set the {LIGHTWALLETD_DATA_DIR_VAR:?} environment variable to run the test", - ); - return Ok(()); } @@ -1479,7 +1476,7 @@ async fn fully_synced_rpc_test() -> Result<()> { }; // Handle the Zebra state directory - let cached_state_path = test_type.zebrad_state_path(); + let cached_state_path = test_type.zebrad_state_path("fully_synced_rpc_test".to_string()); if cached_state_path.is_none() { tracing::info!("skipping fully synced zebrad RPC test"); diff --git a/zebrad/tests/common/lightwalletd.rs b/zebrad/tests/common/lightwalletd.rs index 6b57bd000..e7ec71f7f 100644 --- a/zebrad/tests/common/lightwalletd.rs +++ b/zebrad/tests/common/lightwalletd.rs @@ -266,12 +266,12 @@ impl LightwalletdTestType { } /// Returns the Zebra state path for this test, if set. - pub fn zebrad_state_path(&self) -> Option { + pub fn zebrad_state_path(&self, test_name: String) -> Option { match env::var_os(ZEBRA_CACHED_STATE_DIR_VAR) { Some(path) => Some(path.into()), None => { tracing::info!( - "skipped {self:?} lightwalletd test, \ + "skipped {test_name:?} {self:?} lightwalletd test, \ set the {ZEBRA_CACHED_STATE_DIR_VAR:?} environment variable to run the test", ); @@ -284,12 +284,12 @@ impl LightwalletdTestType { /// /// Returns `None` if the test should be skipped, /// and `Some(Err(_))` if the config could not be created. - pub fn zebrad_config(&self) -> Option> { + pub fn zebrad_config(&self, test_name: String) -> Option> { if !self.needs_zebra_cached_state() { return Some(random_known_rpc_port_config()); } - let zebra_state_path = self.zebrad_state_path()?; + let zebra_state_path = self.zebrad_state_path(test_name)?; let mut config = match random_known_rpc_port_config() { Ok(config) => config, @@ -305,8 +305,18 @@ impl LightwalletdTestType { } /// Returns the lightwalletd state path for this test, if set. - pub fn lightwalletd_state_path(&self) -> Option { - env::var_os(LIGHTWALLETD_DATA_DIR_VAR).map(Into::into) + pub fn lightwalletd_state_path(&self, test_name: String) -> Option { + match env::var_os(LIGHTWALLETD_DATA_DIR_VAR) { + Some(path) => Some(path.into()), + None => { + tracing::info!( + "skipped {test_name:?} {self:?} lightwalletd test, \ + set the {LIGHTWALLETD_DATA_DIR_VAR:?} environment variable to run the test", + ); + + None + } + } } /// Returns the `zebrad` timeout for this test type. diff --git a/zebrad/tests/common/lightwalletd/send_transaction_test.rs b/zebrad/tests/common/lightwalletd/send_transaction_test.rs index b409a53d6..d20ae587c 100644 --- a/zebrad/tests/common/lightwalletd/send_transaction_test.rs +++ b/zebrad/tests/common/lightwalletd/send_transaction_test.rs @@ -12,7 +12,6 @@ //! already been seen in a block. use std::{ - env, path::{Path, PathBuf}, sync::Arc, }; @@ -31,12 +30,14 @@ use zebra_state::HashOrHeight; use crate::common::{ cached_state::{ copy_state_directory, load_tip_height_from_state_directory, - start_state_service_with_cache_dir, ZEBRA_CACHED_STATE_DIR_VAR, + start_state_service_with_cache_dir, }, launch::spawn_zebrad_for_rpc_without_initial_peers, lightwalletd::{ wallet_grpc::{self, connect_to_lightwalletd, spawn_lightwalletd_with_rpc_server}, - zebra_skip_lightwalletd_tests, LIGHTWALLETD_TEST_TIMEOUT, + zebra_skip_lightwalletd_tests, + LightwalletdTestType::UpdateCachedState, + LIGHTWALLETD_TEST_TIMEOUT, }, sync::perform_full_sync_starting_from, }; @@ -50,21 +51,25 @@ pub async fn run() -> Result<()> { return Ok(()); } - let cached_state_path = match env::var_os(ZEBRA_CACHED_STATE_DIR_VAR) { - Some(argument) => PathBuf::from(argument), - None => { - tracing::info!( - "skipped send transactions using lightwalletd test, \ - set the {ZEBRA_CACHED_STATE_DIR_VAR:?} environment variable to run the test", - ); - return Ok(()); - } - }; + // We want a zebra state dir and a lightwalletd data dir in place, + // so `UpdateCachedState` can be used as our test type + let test_type = UpdateCachedState; + + let cached_state_path = test_type.zebrad_state_path("send_transaction_tests".to_string()); + if cached_state_path.is_none() { + return Ok(()); + } + + let lightwalletd_state_path = + test_type.lightwalletd_state_path("send_transaction_tests".to_string()); + if lightwalletd_state_path.is_none() { + return Ok(()); + } let network = Network::Mainnet; let (transactions, partial_sync_path) = - load_transactions_from_a_future_block(network, cached_state_path).await?; + load_transactions_from_a_future_block(network, cached_state_path.unwrap()).await?; let (_zebrad, zebra_rpc_address) = spawn_zebrad_for_rpc_without_initial_peers( Network::Mainnet, @@ -72,8 +77,12 @@ pub async fn run() -> Result<()> { LIGHTWALLETD_TEST_TIMEOUT, )?; - let (_lightwalletd, lightwalletd_rpc_port) = - spawn_lightwalletd_with_rpc_server(zebra_rpc_address, true)?; + let (_lightwalletd, lightwalletd_rpc_port) = spawn_lightwalletd_with_rpc_server( + zebra_rpc_address, + lightwalletd_state_path, + test_type, + true, + )?; let mut rpc_client = connect_to_lightwalletd(lightwalletd_rpc_port).await?; diff --git a/zebrad/tests/common/lightwalletd/wallet_grpc.rs b/zebrad/tests/common/lightwalletd/wallet_grpc.rs index d8d1eda2e..5ed23c8a7 100644 --- a/zebrad/tests/common/lightwalletd/wallet_grpc.rs +++ b/zebrad/tests/common/lightwalletd/wallet_grpc.rs @@ -1,6 +1,6 @@ //! Lightwalletd gRPC interface and utility functions. -use std::{env, net::SocketAddr}; +use std::{env, net::SocketAddr, path::PathBuf}; use tempfile::TempDir; @@ -21,11 +21,10 @@ pub type LightwalletdRpcClient = /// Returns the lightwalletd instance and the port number that it is listening for RPC connections. pub fn spawn_lightwalletd_with_rpc_server( zebrad_rpc_address: SocketAddr, + lightwalletd_state_path: Option, + test_type: LightwalletdTestType, wait_for_blocks: bool, ) -> Result<(TestChild, u16)> { - // We're using cached Zebra state here, so this test type is the most similar - let test_type = LightwalletdTestType::UpdateCachedState; - let lightwalletd_dir = testdir()?.with_lightwalletd_config(zebrad_rpc_address)?; let lightwalletd_rpc_port = random_known_port(); @@ -37,7 +36,7 @@ pub fn spawn_lightwalletd_with_rpc_server( test_type.lightwalletd_failure_messages(); let mut lightwalletd = lightwalletd_dir - .spawn_lightwalletd_child(test_type.lightwalletd_state_path(), arguments)? + .spawn_lightwalletd_child(lightwalletd_state_path, arguments)? .with_timeout(test_type.lightwalletd_timeout()) .with_failure_regex_iter(lightwalletd_failure_messages, lightwalletd_ignore_messages); diff --git a/zebrad/tests/common/lightwalletd/wallet_grpc_test.rs b/zebrad/tests/common/lightwalletd/wallet_grpc_test.rs index 311c7a5aa..49435bbb3 100644 --- a/zebrad/tests/common/lightwalletd/wallet_grpc_test.rs +++ b/zebrad/tests/common/lightwalletd/wallet_grpc_test.rs @@ -13,7 +13,7 @@ //! - `GetTransaction`: Covered. //! - `SendTransaction`: Not covered and it will never will, it has its own test. //! -//! - `GetTaddressTxids`: Not covered, need #4216 to be fixed first. +//! - `GetTaddressTxids`: Covered. //! - `GetTaddressBalance`: Covered. //! - `GetTaddressBalanceStream`: Not covered. //! @@ -42,11 +42,12 @@ use crate::common::{ lightwalletd::{ wallet_grpc::{ connect_to_lightwalletd, spawn_lightwalletd_with_rpc_server, AddressList, BlockId, - BlockRange, ChainSpec, Empty, GetAddressUtxosArg, TxFilter, + BlockRange, ChainSpec, Empty, GetAddressUtxosArg, TransparentAddressBlockFilter, + TxFilter, }, zebra_skip_lightwalletd_tests, LightwalletdTestType::UpdateCachedState, - LIGHTWALLETD_DATA_DIR_VAR, LIGHTWALLETD_TEST_TIMEOUT, ZEBRA_CACHED_STATE_DIR_VAR, + LIGHTWALLETD_TEST_TIMEOUT, }, }; @@ -64,24 +65,14 @@ pub async fn run() -> Result<()> { let test_type = UpdateCachedState; // Require to have a `ZEBRA_CACHED_STATE_DIR` in place - let zebrad_state_path = test_type.zebrad_state_path(); + let zebrad_state_path = test_type.zebrad_state_path("wallet_grpc_test".to_string()); if zebrad_state_path.is_none() { - tracing::info!( - "skipped {test_type:?} lightwalletd test, \ - set the {ZEBRA_CACHED_STATE_DIR_VAR:?} environment variable to run the test", - ); - return Ok(()); } // Require to have a `LIGHTWALLETD_DATA_DIR` in place - let lightwalletd_state_path = test_type.lightwalletd_state_path(); + let lightwalletd_state_path = test_type.lightwalletd_state_path("wallet_grpc_test".to_string()); if lightwalletd_state_path.is_none() { - tracing::info!( - "skipped {test_type:?} lightwalletd test, \ - set the {LIGHTWALLETD_DATA_DIR_VAR:?} environment variable to run the test", - ); - return Ok(()); } @@ -96,8 +87,12 @@ pub async fn run() -> Result<()> { )?; // Launch lightwalletd - let (_lightwalletd, lightwalletd_rpc_port) = - spawn_lightwalletd_with_rpc_server(zebra_rpc_address, false)?; + let (_lightwalletd, lightwalletd_rpc_port) = spawn_lightwalletd_with_rpc_server( + zebra_rpc_address, + lightwalletd_state_path, + test_type, + false, + )?; // Give lightwalletd a few seconds to open its grpc port before connecting to it tokio::time::sleep(std::time::Duration::from_secs(3)).await; @@ -172,29 +167,32 @@ pub async fn run() -> Result<()> { // Check the height of transactions is 1 as expected assert_eq!(transaction.height, 1); - // TODO: Add after #4216 - // Currently, this call fails with: - // 0: status: Unknown, message: "-32602: Invalid params: invalid length 1, expected a tuple of size 3.", details: [], metadata: MetadataMap { headers: {"content-type": "application/grpc"} } - - /* // Call `GetTaddressTxids` with a founders reward address that we know exists and have transactions in the first // few blocks of the mainnet - let transactions = rpc_client.get_taddress_txids(TransparentAddressBlockFilter { - address: "t3Vz22vK5z2LcKEdg16Yv4FFneEL1zg9ojd".to_string(), - range: Some(BlockRange { - start: Some(BlockId { - height: 1, - hash: vec![], - }), - end: Some(BlockId { - height: 10, - hash: vec![], + let mut transactions = rpc_client + .get_taddress_txids(TransparentAddressBlockFilter { + address: "t3Vz22vK5z2LcKEdg16Yv4FFneEL1zg9ojd".to_string(), + range: Some(BlockRange { + start: Some(BlockId { + height: 1, + hash: vec![], + }), + end: Some(BlockId { + height: 10, + hash: vec![], + }), }), }) - }).await?.into_inner(); + .await? + .into_inner(); - dbg!(transactions); - */ + let mut counter = 0; + while let Some(_transaction) = transactions.message().await? { + counter += 1; + } + + // For the provided address in the first 10 blocks there are 10 transactions in the mainnet + assert_eq!(10, counter); // Call `GetTaddressBalance` with the ZF funding stream address let balance = rpc_client