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 <teor@riseup.net>

Co-authored-by: teor <teor@riseup.net>

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(
+    fn get_address_tx_ids(&self, request: GetAddressTxIdsRequest)
-        &self,
+        -> BoxFuture<Result<Vec<String>>>;
-        address_strings: AddressStrings,
-        start: u32,
-        end: u32,
-    ) -> BoxFuture<Result<Vec<String>>>;
 
     /// Returns all unspent outputs for a list of addresses.
     ///
@@ -665,13 +662,11 @@ where
 
     fn get_address_tx_ids(
         &self,
-        address_strings: AddressStrings,
+        request: GetAddressTxIdsRequest,
-        start: u32,
-        end: u32,
     ) -> BoxFuture<Result<Vec<String>>> {
         let mut state = self.state.clone();
-        let start = Height(start);
+        let start = Height(request.start);
-        let end = Height(end);
+        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<String>,
+    // 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(

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");
 

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");

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<PathBuf> {
+    pub fn zebrad_state_path(&self, test_name: String) -> Option<PathBuf> {
         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<Result<ZebradConfig>> {
+    pub fn zebrad_config(&self, test_name: String) -> Option<Result<ZebradConfig>> {
         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<PathBuf> {
+    pub fn lightwalletd_state_path(&self, test_name: String) -> Option<PathBuf> {
-        env::var_os(LIGHTWALLETD_DATA_DIR_VAR).map(Into::into)
+        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.

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) {
+    // We want a zebra state dir and a lightwalletd data dir in place,
-        Some(argument) => PathBuf::from(argument),
+    // so `UpdateCachedState` can be used as our test type
-        None => {
+    let test_type = UpdateCachedState;
-            tracing::info!(
+
-                "skipped send transactions using lightwalletd test, \
+    let cached_state_path = test_type.zebrad_state_path("send_transaction_tests".to_string());
-                 set the {ZEBRA_CACHED_STATE_DIR_VAR:?} environment variable to run the test",
+    if cached_state_path.is_none() {
-            );
+        return Ok(());
-            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) =
+    let (_lightwalletd, lightwalletd_rpc_port) = spawn_lightwalletd_with_rpc_server(
-        spawn_lightwalletd_with_rpc_server(zebra_rpc_address, true)?;
+        zebra_rpc_address,
+        lightwalletd_state_path,
+        test_type,
+        true,
+    )?;
 
     let mut rpc_client = connect_to_lightwalletd(lightwalletd_rpc_port).await?;
 

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<PathBuf>,
+    test_type: LightwalletdTestType,
     wait_for_blocks: bool,
 ) -> Result<(TestChild<TempDir>, 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);
 

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) =
+    let (_lightwalletd, lightwalletd_rpc_port) = spawn_lightwalletd_with_rpc_server(
-        spawn_lightwalletd_with_rpc_server(zebra_rpc_address, false)?;
+        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 {
+    let mut transactions = rpc_client
-        address: "t3Vz22vK5z2LcKEdg16Yv4FFneEL1zg9ojd".to_string(),
+        .get_taddress_txids(TransparentAddressBlockFilter {
-        range: Some(BlockRange {
+            address: "t3Vz22vK5z2LcKEdg16Yv4FFneEL1zg9ojd".to_string(),
-            start: Some(BlockId {
+            range: Some(BlockRange {
-                height: 1,
+                start: Some(BlockId {
-                hash: vec![],
+                    height: 1,
-            }),
+                    hash: vec![],
-            end: Some(BlockId {
+                }),
-                height: 10,
+                end: Some(BlockId {
-                hash: vec![],
+                    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