fix(test): Wait for zebrad and lightwalletd to reach the tip in tests…

…, to improve test coverage (#5164)

* Add RPC timing to zcash-rpc-diff

* Use transaction hash index for verbose block requests, rather than block data

* check if we are at tip for lightwallet wallet tests

* move function

* Apply suggestions from code review

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

* Combine the lightwalletd sync and gRPC test APIs

* Rewrite the gRPC and full sync tests for the new APIs

* Make zebra_rpc_address optional because only some tests need it

* Check for the zebrad RPC port to open in the right place

* Do the quick lightwalletd integration tests first in the sequential test function

* Ignore the lightwalletd cached state env var in tests that don't want it

* Don't replace the state path in RPC tests

* Enable IO (and timers) on the tip check tokio runtime

* Stop waiting for sync if either waiter thread errors or panics

* Try to speed up slow lightwalletd full syncs

* Don't wait for the tip in send transaction tests, and try to speed up full lightwalletd syncs

* Remove redundant is_lightwalletd_finished store

Co-authored-by: Arya <aryasolhi@gmail.com>

* Fix unused variable error

* Actually create the lightwalletd cached state

* Fix lwd cache check logic

Co-authored-by: teor <teor@riseup.net>
Co-authored-by: Arya <aryasolhi@gmail.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

zebrad/tests/common/launch.rs

@@ -14,6 +14,7 @@ use std::{
 
 use color_eyre::eyre::Result;
 use indexmap::IndexSet;
+use tempfile::TempDir;
 
 use zebra_chain::parameters::Network;
 use zebra_test::{
@@ -24,7 +25,8 @@ use zebra_test::{
 use zebrad::config::ZebradConfig;
 
 use crate::common::{
-    lightwalletd::{random_known_rpc_port_config, LightwalletdTestType},
+    config::testdir,
+    lightwalletd::{zebra_skip_lightwalletd_tests, LightwalletdTestType},
     sync::FINISH_PARTIAL_SYNC_TIMEOUT,
 };
 
@@ -195,43 +197,79 @@ where
     }
 }
 
-/// Spawns a zebrad instance to interact with lightwalletd, but without an internet connection.
+/// Spawns a zebrad instance on `network` to test lightwalletd with `test_type`.
 ///
-/// This prevents it from downloading blocks. Instead, the `zebra_directory` parameter allows
-/// providing an initial state to the zebrad instance.
+/// If `use_internet_connection` is `false` then spawn, but without any peers.
+/// This prevents it from downloading blocks. Instead, use the `ZEBRA_CACHED_STATE_DIR`
+/// environmental variable to provide an initial state to the zebrad instance.
+///
+/// Returns:
+/// - `Ok(Some(zebrad, zebra_rpc_address))` on success,
+/// - `Ok(None)` if the test doesn't have the required network or cached state, and
+/// - `Err(_)` if spawning zebrad fails.
+///
+/// `zebra_rpc_address` is `None` if the test type doesn't need an RPC port.
 #[tracing::instrument]
-pub fn spawn_zebrad_for_rpc_without_initial_peers<P: ZebradTestDirExt + std::fmt::Debug>(
+pub fn spawn_zebrad_for_rpc<S: AsRef<str> + std::fmt::Debug>(
     network: Network,
-    zebra_directory: P,
+    test_name: S,
     test_type: LightwalletdTestType,
-    debug_skip_parameter_preload: bool,
-) -> Result<(TestChild<P>, SocketAddr)> {
-    // This is what we recommend our users configure.
-    let mut config = random_known_rpc_port_config(true)
-        .expect("Failed to create a config file with a known RPC listener port");
-
-    config.state.ephemeral = false;
-    config.network.initial_mainnet_peers = IndexSet::new();
-    config.network.initial_testnet_peers = IndexSet::new();
+    use_internet_connection: bool,
+) -> Result<Option<(TestChild<TempDir>, Option<SocketAddr>)>> {
+    let test_name = test_name.as_ref();
+
+    // Skip the test unless the user specifically asked for it
+    if !can_spawn_zebrad_for_rpc(test_name, test_type) {
+        return Ok(None);
+    }
+
+    // Get the zebrad config
+    let mut config = test_type
+        .zebrad_config(test_name)
+        .expect("already checked config")?;
+
+    // TODO: move this into zebrad_config()
     config.network.network = network;
-    config.mempool.debug_enable_at_height = Some(0);
-    config.consensus.debug_skip_parameter_preload = debug_skip_parameter_preload;
+    if !use_internet_connection {
+        config.network.initial_mainnet_peers = IndexSet::new();
+        config.network.initial_testnet_peers = IndexSet::new();
+
+        config.mempool.debug_enable_at_height = Some(0);
+    }
 
     let (zebrad_failure_messages, zebrad_ignore_messages) = test_type.zebrad_failure_messages();
 
-    let mut zebrad = zebra_directory
-        .with_config(&mut config)?
+    // Writes a configuration that has RPC listen_addr set (if needed).
+    // If the state path env var is set, uses it in the config.
+    let zebrad = testdir()?
+        .with_exact_config(&config)?
         .spawn_child(args!["start"])?
         .bypass_test_capture(true)
         .with_timeout(test_type.zebrad_timeout())
         .with_failure_regex_iter(zebrad_failure_messages, zebrad_ignore_messages);
 
-    let rpc_address = config.rpc.listen_addr.unwrap();
+    Ok(Some((zebrad, config.rpc.listen_addr)))
+}
 
-    zebrad.expect_stdout_line_matches("activating mempool")?;
-    zebrad.expect_stdout_line_matches(&format!("Opened RPC endpoint at {}", rpc_address))?;
+/// Returns `true` if a zebrad test for `test_type` has everything it needs to run.
+#[tracing::instrument]
+pub fn can_spawn_zebrad_for_rpc<S: AsRef<str> + std::fmt::Debug>(
+    test_name: S,
+    test_type: LightwalletdTestType,
+) -> bool {
+    if zebra_test::net::zebra_skip_network_tests() {
+        return false;
+    }
+
+    // Skip the test unless the user specifically asked for it
+    //
+    // TODO: pass test_type to zebra_skip_lightwalletd_tests() and check for lightwalletd launch in there
+    if test_type.launches_lightwalletd() && zebra_skip_lightwalletd_tests() {
+        return false;
+    }
 
-    Ok((zebrad, rpc_address))
+    // Check if we have any necessary cached states for the zebrad config
+    test_type.zebrad_config(test_name).is_some()
 }
 
 /// Panics if `$pred` is false, with an error report containing:

zebrad/tests/common/lightwalletd.rs

@@ -12,7 +12,11 @@ use std::{
     time::Duration,
 };
 
+use tempfile::TempDir;
+
+use zebra_chain::parameters::Network::{self, *};
 use zebra_test::{
+    args,
     command::{Arguments, TestChild, TestDirExt, NO_MATCHES_REGEX_ITER},
     net::random_known_port,
     prelude::*,
@@ -21,7 +25,7 @@ use zebrad::config::ZebradConfig;
 
 use super::{
     cached_state::ZEBRA_CACHED_STATE_DIR,
-    config::default_test_config,
+    config::{default_test_config, testdir},
     failure_messages::{
         LIGHTWALLETD_EMPTY_ZEBRA_STATE_IGNORE_MESSAGES, LIGHTWALLETD_FAILURE_MESSAGES,
         PROCESS_FAILURE_MESSAGES, ZEBRA_FAILURE_MESSAGES,
@@ -37,6 +41,8 @@ use LightwalletdTestType::*;
 #[cfg(feature = "lightwalletd-grpc-tests")]
 pub mod send_transaction_test;
 #[cfg(feature = "lightwalletd-grpc-tests")]
+pub mod sync;
+#[cfg(feature = "lightwalletd-grpc-tests")]
 pub mod wallet_grpc;
 #[cfg(feature = "lightwalletd-grpc-tests")]
 pub mod wallet_grpc_test;
@@ -106,6 +112,80 @@ pub fn random_known_rpc_port_config(parallel_cpu_threads: bool) -> Result<Zebrad
     Ok(config)
 }
 
+/// Spawns a lightwalletd instance on `network`, connected to `zebrad_rpc_address`,
+/// with its gRPC server functionality enabled.
+///
+/// Expects cached state based on the `test_type`. Use the `LIGHTWALLETD_DATA_DIR`
+/// environmental variable to provide an initial state to the lightwalletd instance.
+///
+/// Returns:
+/// - `Ok(Some(lightwalletd, lightwalletd_rpc_port))` on success,
+/// - `Ok(None)` if the test doesn't have the required network or cached state, and
+/// - `Err(_)` if spawning lightwalletd fails.
+#[tracing::instrument]
+pub fn spawn_lightwalletd_for_rpc<S: AsRef<str> + std::fmt::Debug>(
+    network: Network,
+    test_name: S,
+    test_type: LightwalletdTestType,
+    zebrad_rpc_address: SocketAddr,
+) -> Result<Option<(TestChild<TempDir>, u16)>> {
+    assert_eq!(network, Mainnet, "this test only supports Mainnet for now");
+
+    let test_name = test_name.as_ref();
+
+    // Skip the test unless the user specifically asked for it
+    if !can_spawn_lightwalletd_for_rpc(test_name, test_type) {
+        return Ok(None);
+    }
+
+    let lightwalletd_state_path = test_type.lightwalletd_state_path(test_name);
+    let lightwalletd_dir = testdir()?.with_lightwalletd_config(zebrad_rpc_address)?;
+
+    let lightwalletd_rpc_port = random_known_port();
+    let lightwalletd_rpc_address = format!("127.0.0.1:{lightwalletd_rpc_port}");
+
+    let arguments = args!["--grpc-bind-addr": lightwalletd_rpc_address];
+
+    let (lightwalletd_failure_messages, lightwalletd_ignore_messages) =
+        test_type.lightwalletd_failure_messages();
+
+    let mut lightwalletd = lightwalletd_dir
+        .spawn_lightwalletd_child(lightwalletd_state_path, arguments)?
+        .with_timeout(test_type.lightwalletd_timeout())
+        .with_failure_regex_iter(lightwalletd_failure_messages, lightwalletd_ignore_messages);
+
+    // Wait until `lightwalletd` has launched.
+    // This log happens very quickly, so it is ok to block for a short while here.
+    lightwalletd.expect_stdout_line_matches(regex::escape("Starting gRPC server"))?;
+
+    Ok(Some((lightwalletd, lightwalletd_rpc_port)))
+}
+
+/// Returns `true` if a lightwalletd test for `test_type` has everything it needs to run.
+#[tracing::instrument]
+pub fn can_spawn_lightwalletd_for_rpc<S: AsRef<str> + std::fmt::Debug>(
+    test_name: S,
+    test_type: LightwalletdTestType,
+) -> bool {
+    if zebra_test::net::zebra_skip_network_tests() {
+        return false;
+    }
+
+    // Skip the test unless the user specifically asked for it
+    //
+    // TODO: pass test_type to zebra_skip_lightwalletd_tests() and check for lightwalletd launch in there
+    if test_type.launches_lightwalletd() && zebra_skip_lightwalletd_tests() {
+        return false;
+    }
+
+    let lightwalletd_state_path = test_type.lightwalletd_state_path(test_name);
+    if test_type.needs_lightwalletd_cached_state() && lightwalletd_state_path.is_none() {
+        return false;
+    }
+
+    true
+}
+
 /// Extension trait for methods on `tempfile::TempDir` for using it as a test
 /// directory for `zebrad`.
 pub trait LightWalletdTestDirExt: ZebradTestDirExt
@@ -234,6 +314,9 @@ pub enum LightwalletdTestType {
     /// Do a full sync from an empty lightwalletd state.
     ///
     /// This test requires a cached Zebra state.
+    //
+    // Only used with `--features=lightwalletd-grpc-tests`.
+    #[allow(dead_code)]
     FullSyncFromGenesis {
         /// Should the test allow a cached lightwalletd state?
         ///
@@ -258,6 +341,10 @@ pub enum LightwalletdTestType {
 impl LightwalletdTestType {
     /// Does this test need a Zebra cached state?
     pub fn needs_zebra_cached_state(&self) -> bool {
+        // Handle the Zebra state directory based on the test type:
+        // - LaunchWithEmptyState: ignore the state directory
+        // - FullSyncFromGenesis, UpdateCachedState, UpdateZebraCachedStateNoRpc:
+        //   skip the test if it is not available
         match self {
             LaunchWithEmptyState => false,
             FullSyncFromGenesis { .. } | UpdateCachedState | UpdateZebraCachedStateNoRpc => true,
@@ -274,6 +361,10 @@ impl LightwalletdTestType {
 
     /// Does this test need a `lightwalletd` cached state?
     pub fn needs_lightwalletd_cached_state(&self) -> bool {
+        // Handle the lightwalletd state directory based on the test type:
+        // - LaunchWithEmptyState, UpdateZebraCachedStateNoRpc: ignore the state directory
+        // - FullSyncFromGenesis: use it if available, timeout if it is already populated
+        // - UpdateCachedState: skip the test if it is not available
         match self {
             LaunchWithEmptyState | FullSyncFromGenesis { .. } | UpdateZebraCachedStateNoRpc => {
                 false
@@ -293,12 +384,22 @@ impl LightwalletdTestType {
         }
     }
 
+    /// Can this test create a new `LIGHTWALLETD_DATA_DIR` cached state?
+    pub fn can_create_lightwalletd_cached_state(&self) -> bool {
+        match self {
+            LaunchWithEmptyState => false,
+            FullSyncFromGenesis { .. } | UpdateCachedState => true,
+            UpdateZebraCachedStateNoRpc => false,
+        }
+    }
+
     /// Returns the Zebra state path for this test, if set.
     #[allow(clippy::print_stderr)]
-    pub fn zebrad_state_path(&self, test_name: String) -> Option<PathBuf> {
+    pub fn zebrad_state_path<S: AsRef<str>>(&self, test_name: S) -> Option<PathBuf> {
         match env::var_os(ZEBRA_CACHED_STATE_DIR) {
             Some(path) => Some(path.into()),
             None => {
+                let test_name = test_name.as_ref();
                 eprintln!(
                     "skipped {test_name:?} {self:?} lightwalletd test, \
                      set the {ZEBRA_CACHED_STATE_DIR:?} environment variable to run the test",
@@ -313,7 +414,7 @@ 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, test_name: String) -> Option<Result<ZebradConfig>> {
+    pub fn zebrad_config<S: AsRef<str>>(&self, test_name: S) -> Option<Result<ZebradConfig>> {
         let config = if self.launches_lightwalletd() {
             // This is what we recommend our users configure.
             random_known_rpc_port_config(true)
@@ -330,6 +431,12 @@ impl LightwalletdTestType {
         // except when we're doing the quick empty state test
         config.consensus.debug_skip_parameter_preload = !self.needs_zebra_cached_state();
 
+        // We want to run multi-threaded RPCs, if we're using them
+        if self.launches_lightwalletd() {
+            // Automatically runs one thread per available CPU core
+            config.rpc.parallel_cpu_threads = 0;
+        }
+
         if !self.needs_zebra_cached_state() {
             return Some(Ok(config));
         }
@@ -346,8 +453,14 @@ impl LightwalletdTestType {
     }
 
     /// Returns the `lightwalletd` state path for this test, if set, and if allowed for this test.
-    pub fn lightwalletd_state_path(&self, test_name: String) -> Option<PathBuf> {
-        if !self.launches_lightwalletd() {
+    pub fn lightwalletd_state_path<S: AsRef<str>>(&self, test_name: S) -> Option<PathBuf> {
+        let test_name = test_name.as_ref();
+
+        // Can this test type use a lwd cached state, or create/update one?
+        let use_or_create_lwd_cache =
+            self.allow_lightwalletd_cached_state() || self.can_create_lightwalletd_cached_state();
+
+        if !self.launches_lightwalletd() || !use_or_create_lwd_cache {
             tracing::info!(
                 "running {test_name:?} {self:?} lightwalletd test, \
                  ignoring any cached state in the {LIGHTWALLETD_DATA_DIR:?} environment variable",