From 75ffc50e2c2210a13677722f45f871c06d95a6a9 Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Tue, 22 Aug 2023 12:28:24 +0200 Subject: [PATCH 1/3] Simplify and fix `TestStore` .. as we don't require all that logic anymore now that we don't return an `FilesystemWriter` anymore etc. --- src/test/utils.rs | 84 +++++++++++++++++------------------------------ 1 file changed, 30 insertions(+), 54 deletions(-) diff --git a/src/test/utils.rs b/src/test/utils.rs index 31c73d288..a695bb82d 100644 --- a/src/test/utils.rs +++ b/src/test/utils.rs @@ -18,11 +18,11 @@ use rand::{thread_rng, Rng}; use std::collections::hash_map; use std::collections::HashMap; use std::env; -use std::io::{Cursor, Read, Write}; +use std::io::{self, Write}; use std::path::PathBuf; use std::str::FromStr; use std::sync::atomic::{AtomicBool, Ordering}; -use std::sync::{Arc, Mutex, RwLock}; +use std::sync::{Arc, Mutex}; use std::time::Duration; macro_rules! expect_event { @@ -42,25 +42,20 @@ macro_rules! expect_event { pub(crate) use expect_event; pub(crate) struct TestStore { - persisted_bytes: RwLock>>>>>, + persisted_bytes: Mutex>>>, did_persist: Arc, } impl TestStore { pub fn new() -> Self { - let persisted_bytes = RwLock::new(HashMap::new()); + let persisted_bytes = Mutex::new(HashMap::new()); let did_persist = Arc::new(AtomicBool::new(false)); Self { persisted_bytes, did_persist } } pub fn get_persisted_bytes(&self, namespace: &str, key: &str) -> Option> { - if let Some(outer_ref) = self.persisted_bytes.read().unwrap().get(namespace) { - if let Some(inner_ref) = outer_ref.get(key) { - let locked = inner_ref.read().unwrap(); - return Some((*locked).clone()); - } - } - None + let persisted_lock = self.persisted_bytes.lock().unwrap(); + persisted_lock.get(namespace).and_then(|e| e.get(key).cloned()) } pub fn get_and_clear_did_persist(&self) -> bool { @@ -69,46 +64,45 @@ impl TestStore { } impl KVStore for TestStore { - type Reader = TestReader; + type Reader = io::Cursor>; - fn read(&self, namespace: &str, key: &str) -> std::io::Result { - if let Some(outer_ref) = self.persisted_bytes.read().unwrap().get(namespace) { + fn read(&self, namespace: &str, key: &str) -> io::Result { + let persisted_lock = self.persisted_bytes.lock().unwrap(); + if let Some(outer_ref) = persisted_lock.get(namespace) { if let Some(inner_ref) = outer_ref.get(key) { - Ok(TestReader::new(Arc::clone(inner_ref))) + let bytes = inner_ref.clone(); + Ok(io::Cursor::new(bytes)) } else { - let msg = format!("Key not found: {}", key); - Err(std::io::Error::new(std::io::ErrorKind::NotFound, msg)) + Err(io::Error::new(io::ErrorKind::NotFound, "Key not found")) } } else { - let msg = format!("Namespace not found: {}", namespace); - Err(std::io::Error::new(std::io::ErrorKind::NotFound, msg)) + Err(io::Error::new(io::ErrorKind::NotFound, "Namespace not found")) } } - fn write(&self, namespace: &str, key: &str, buf: &[u8]) -> std::io::Result<()> { - let mut guard = self.persisted_bytes.write().unwrap(); - let outer_e = guard.entry(namespace.to_string()).or_insert(HashMap::new()); - let inner_e = outer_e.entry(key.to_string()).or_insert(Arc::new(RwLock::new(Vec::new()))); - - let mut guard = inner_e.write().unwrap(); - guard.write_all(buf)?; + fn write(&self, namespace: &str, key: &str, buf: &[u8]) -> io::Result<()> { + let mut persisted_lock = self.persisted_bytes.lock().unwrap(); + let outer_e = persisted_lock.entry(namespace.to_string()).or_insert(HashMap::new()); + let mut bytes = Vec::new(); + bytes.write_all(buf)?; + outer_e.insert(key.to_string(), bytes); self.did_persist.store(true, Ordering::SeqCst); Ok(()) } - fn remove(&self, namespace: &str, key: &str) -> std::io::Result<()> { - match self.persisted_bytes.write().unwrap().entry(namespace.to_string()) { - hash_map::Entry::Occupied(mut e) => { - self.did_persist.store(true, Ordering::SeqCst); - e.get_mut().remove(&key.to_string()); - Ok(()) - } - hash_map::Entry::Vacant(_) => Ok(()), + fn remove(&self, namespace: &str, key: &str) -> io::Result<()> { + let mut persisted_lock = self.persisted_bytes.lock().unwrap(); + if let Some(outer_ref) = persisted_lock.get_mut(namespace) { + outer_ref.remove(&key.to_string()); + self.did_persist.store(true, Ordering::SeqCst); } + + Ok(()) } - fn list(&self, namespace: &str) -> std::io::Result> { - match self.persisted_bytes.write().unwrap().entry(namespace.to_string()) { + fn list(&self, namespace: &str) -> io::Result> { + let mut persisted_lock = self.persisted_bytes.lock().unwrap(); + match persisted_lock.entry(namespace.to_string()) { hash_map::Entry::Occupied(e) => Ok(e.get().keys().cloned().collect()), hash_map::Entry::Vacant(_) => Ok(Vec::new()), } @@ -139,24 +133,6 @@ impl KVStorePersister for TestStore { } } -pub struct TestReader { - entry_ref: Arc>>, -} - -impl TestReader { - pub fn new(entry_ref: Arc>>) -> Self { - Self { entry_ref } - } -} - -impl Read for TestReader { - fn read(&mut self, buf: &mut [u8]) -> std::io::Result { - let bytes = self.entry_ref.read().unwrap().clone(); - let mut reader = Cursor::new(bytes); - reader.read(buf) - } -} - // Copied over from upstream LDK #[allow(dead_code)] pub struct TestLogger { From 89e97abfd400eee84c4071f72de7e88cde52bcbd Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Thu, 24 Aug 2023 11:01:06 +0200 Subject: [PATCH 2/3] Sort values returned from `KVStore::list` This will be mandatory on the upstreamed trait anyways and is useful to make it easiert to compare returned values. --- src/io/fs_store.rs | 2 ++ src/io/sqlite_store.rs | 1 + 2 files changed, 3 insertions(+) diff --git a/src/io/fs_store.rs b/src/io/fs_store.rs index 4eb067bf1..93141b021 100644 --- a/src/io/fs_store.rs +++ b/src/io/fs_store.rs @@ -222,6 +222,8 @@ impl KVStore for FilesystemStore { } } + keys.sort(); + Ok(keys) } } diff --git a/src/io/sqlite_store.rs b/src/io/sqlite_store.rs index 0c71d9e51..729e313ec 100644 --- a/src/io/sqlite_store.rs +++ b/src/io/sqlite_store.rs @@ -160,6 +160,7 @@ impl KVStore for SqliteStore { })?); } + keys.sort(); Ok(keys) } } From 76d2f03182f853d51e67c29f142a9e16d50e544c Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Thu, 24 Aug 2023 10:12:13 +0200 Subject: [PATCH 3/3] Refactor test code and introduce `TestSyncStore` .. which asserts that all `KVStore` implementations operatate synchronously, i.e., yield identical results given the same inputs. --- src/test/functional_tests.rs | 144 +++-------------------------------- src/test/utils.rs | 143 +++++++++++++++++++++++++++++++++- 2 files changed, 153 insertions(+), 134 deletions(-) diff --git a/src/test/functional_tests.rs b/src/test/functional_tests.rs index cbb847e5d..26a53e2cc 100644 --- a/src/test/functional_tests.rs +++ b/src/test/functional_tests.rs @@ -1,7 +1,7 @@ use crate::builder::NodeBuilder; use crate::io::KVStore; use crate::test::utils::*; -use crate::test::utils::{expect_event, random_config}; +use crate::test::utils::{expect_event, random_config, setup_two_nodes}; use crate::{Error, Event, Node, PaymentDirection, PaymentStatus}; use bitcoin::Amount; @@ -11,45 +11,14 @@ use electrsd::ElectrsD; #[test] fn channel_full_cycle() { let (bitcoind, electrsd) = setup_bitcoind_and_electrsd(); - println!("== Node A =="); - let esplora_url = format!("http://{}", electrsd.esplora_url.as_ref().unwrap()); - let config_a = random_config(); - let mut builder_a = NodeBuilder::from_config(config_a); - builder_a.set_esplora_server(esplora_url.clone()); - let node_a = builder_a.build().unwrap(); - node_a.start().unwrap(); - - println!("\n== Node B =="); - let config_b = random_config(); - let mut builder_b = NodeBuilder::from_config(config_b); - builder_b.set_esplora_server(esplora_url); - let node_b = builder_b.build().unwrap(); - node_b.start().unwrap(); - + let (node_a, node_b) = setup_two_nodes(&electrsd, false); do_channel_full_cycle(node_a, node_b, &bitcoind, &electrsd, false); } #[test] fn channel_full_cycle_0conf() { let (bitcoind, electrsd) = setup_bitcoind_and_electrsd(); - println!("== Node A =="); - let esplora_url = format!("http://{}", electrsd.esplora_url.as_ref().unwrap()); - let config_a = random_config(); - let mut builder_a = NodeBuilder::from_config(config_a); - builder_a.set_esplora_server(esplora_url.clone()); - let node_a = builder_a.build().unwrap(); - node_a.start().unwrap(); - - println!("\n== Node B =="); - let mut config_b = random_config(); - config_b.trusted_peers_0conf.push(node_a.node_id()); - - let mut builder_b = NodeBuilder::from_config(config_b); - builder_b.set_esplora_server(esplora_url.clone()); - let node_b = builder_b.build().unwrap(); - - node_b.start().unwrap(); - + let (node_a, node_b) = setup_two_nodes(&electrsd, true); do_channel_full_cycle(node_a, node_b, &bitcoind, &electrsd, true) } @@ -271,21 +240,9 @@ fn do_channel_full_cycle( #[test] fn channel_open_fails_when_funds_insufficient() { let (bitcoind, electrsd) = setup_bitcoind_and_electrsd(); - println!("== Node A =="); - let esplora_url = format!("http://{}", electrsd.esplora_url.as_ref().unwrap()); - let config_a = random_config(); - let mut builder_a = NodeBuilder::from_config(config_a); - builder_a.set_esplora_server(esplora_url.clone()); - let node_a = builder_a.build().unwrap(); - node_a.start().unwrap(); - let addr_a = node_a.new_onchain_address().unwrap(); + let (node_a, node_b) = setup_two_nodes(&electrsd, false); - println!("\n== Node B =="); - let config_b = random_config(); - let mut builder_b = NodeBuilder::from_config(config_b); - builder_b.set_esplora_server(esplora_url); - let node_b = builder_b.build().unwrap(); - node_b.start().unwrap(); + let addr_a = node_a.new_onchain_address().unwrap(); let addr_b = node_b.new_onchain_address().unwrap(); let premine_amount_sat = 100_000; @@ -329,12 +286,11 @@ fn connect_to_public_testnet_esplora() { #[test] fn start_stop_reinit() { let (bitcoind, electrsd) = setup_bitcoind_and_electrsd(); - let esplora_url = format!("http://{}", electrsd.esplora_url.as_ref().unwrap()); let config = random_config(); - let mut builder = NodeBuilder::from_config(config.clone()); - builder.set_esplora_server(esplora_url.clone()); - let node = builder.build().unwrap(); + let node = setup_node(&electrsd, config.clone()); + let expected_node_id = node.node_id(); + assert_eq!(node.start(), Err(Error::AlreadyRunning)); let funding_address = node.new_onchain_address().unwrap(); let expected_amount = Amount::from_sat(100000); @@ -342,13 +298,10 @@ fn start_stop_reinit() { premine_and_distribute_funds(&bitcoind, &electrsd, vec![funding_address], expected_amount); assert_eq!(node.total_onchain_balance_sats().unwrap(), 0); - node.start().unwrap(); - assert_eq!(node.start(), Err(Error::AlreadyRunning)); - node.sync_wallets().unwrap(); assert_eq!(node.spendable_onchain_balance_sats().unwrap(), expected_amount.to_sat()); - let log_file_symlink = format!("{}/logs/ldk_node_latest.log", config.storage_dir_path); + let log_file_symlink = format!("{}/logs/ldk_node_latest.log", config.clone().storage_dir_path); assert!(std::path::Path::new(&log_file_symlink).is_symlink()); node.stop().unwrap(); @@ -361,66 +314,9 @@ fn start_stop_reinit() { assert_eq!(node.stop(), Err(Error::NotRunning)); drop(node); - let mut new_builder = NodeBuilder::from_config(config); - new_builder.set_esplora_server(esplora_url); - let reinitialized_node = builder.build().unwrap(); - assert_eq!(reinitialized_node.node_id(), expected_node_id); - - reinitialized_node.start().unwrap(); - - assert_eq!( - reinitialized_node.spendable_onchain_balance_sats().unwrap(), - expected_amount.to_sat() - ); - - reinitialized_node.sync_wallets().unwrap(); - assert_eq!( - reinitialized_node.spendable_onchain_balance_sats().unwrap(), - expected_amount.to_sat() - ); - - reinitialized_node.stop().unwrap(); -} - -#[test] -fn start_stop_reinit_fs_store() { - let (bitcoind, electrsd) = setup_bitcoind_and_electrsd(); - let esplora_url = format!("http://{}", electrsd.esplora_url.as_ref().unwrap()); - let config = random_config(); - let mut builder = NodeBuilder::from_config(config.clone()); - builder.set_esplora_server(esplora_url.clone()); - let node = builder.build_with_fs_store().unwrap(); - let expected_node_id = node.node_id(); - - let funding_address = node.new_onchain_address().unwrap(); - let expected_amount = Amount::from_sat(100000); - - premine_and_distribute_funds(&bitcoind, &electrsd, vec![funding_address], expected_amount); - assert_eq!(node.total_onchain_balance_sats().unwrap(), 0); - - node.start().unwrap(); - assert_eq!(node.start(), Err(Error::AlreadyRunning)); - - node.sync_wallets().unwrap(); - assert_eq!(node.spendable_onchain_balance_sats().unwrap(), expected_amount.to_sat()); - - node.stop().unwrap(); - assert_eq!(node.stop(), Err(Error::NotRunning)); - - node.start().unwrap(); - assert_eq!(node.start(), Err(Error::AlreadyRunning)); - - node.stop().unwrap(); - assert_eq!(node.stop(), Err(Error::NotRunning)); - drop(node); - - let mut new_builder = NodeBuilder::from_config(config); - new_builder.set_esplora_server(esplora_url); - let reinitialized_node = builder.build_with_fs_store().unwrap(); + let reinitialized_node = setup_node(&electrsd, config); assert_eq!(reinitialized_node.node_id(), expected_node_id); - reinitialized_node.start().unwrap(); - assert_eq!( reinitialized_node.spendable_onchain_balance_sats().unwrap(), expected_amount.to_sat() @@ -438,20 +334,9 @@ fn start_stop_reinit_fs_store() { #[test] fn onchain_spend_receive() { let (bitcoind, electrsd) = setup_bitcoind_and_electrsd(); - let esplora_url = format!("http://{}", electrsd.esplora_url.as_ref().unwrap()); + let (node_a, node_b) = setup_two_nodes(&electrsd, false); - let config_a = random_config(); - let mut builder_a = NodeBuilder::from_config(config_a); - builder_a.set_esplora_server(esplora_url.clone()); - let node_a = builder_a.build().unwrap(); - node_a.start().unwrap(); let addr_a = node_a.new_onchain_address().unwrap(); - - let config_b = random_config(); - let mut builder_b = NodeBuilder::from_config(config_b); - builder_b.set_esplora_server(esplora_url); - let node_b = builder_b.build().unwrap(); - node_b.start().unwrap(); let addr_b = node_b.new_onchain_address().unwrap(); premine_and_distribute_funds( @@ -494,13 +379,8 @@ fn onchain_spend_receive() { #[test] fn sign_verify_msg() { let (_bitcoind, electrsd) = setup_bitcoind_and_electrsd(); - let esplora_url = format!("http://{}", electrsd.esplora_url.as_ref().unwrap()); let config = random_config(); - let mut builder = NodeBuilder::from_config(config.clone()); - builder.set_esplora_server(esplora_url.clone()); - let node = builder.build().unwrap(); - - node.start().unwrap(); + let node = setup_node(&electrsd, config); // Tests arbitrary message signing and later verification let msg = "OK computer".as_bytes(); diff --git a/src/test/utils.rs b/src/test/utils.rs index a695bb82d..5b63fb7ee 100644 --- a/src/test/utils.rs +++ b/src/test/utils.rs @@ -1,5 +1,6 @@ -use crate::io::KVStore; -use crate::Config; +use crate::builder::NodeBuilder; +use crate::io::{FilesystemStore, KVStore, SqliteStore}; +use crate::{Config, Node}; use lightning::util::logger::{Level, Logger, Record}; use lightning::util::persist::KVStorePersister; use lightning::util::ser::Writeable; @@ -133,6 +134,118 @@ impl KVStorePersister for TestStore { } } +// A `KVStore` impl for testing purposes that wraps all our `KVStore`s and asserts their synchronicity. +pub(crate) struct TestSyncStore { + fs_store: FilesystemStore, + sqlite_store: SqliteStore, +} + +impl TestSyncStore { + pub(crate) fn new(dest_dir: PathBuf) -> Self { + let fs_store = FilesystemStore::new(dest_dir.clone()); + let sqlite_store = SqliteStore::new(dest_dir); + Self { fs_store, sqlite_store } + } +} + +impl KVStore for TestSyncStore { + type Reader = io::Cursor>; + + fn read(&self, namespace: &str, key: &str) -> std::io::Result { + // For now, we only assert `Ok` with the `fs_reader` here, as it's too complicated to track + // the read status of both seperately, however, the `Reader` concept is going away anyways + // at which point we can assert on simply on the returned values of `KVStore::read`. + let fs_res = self.fs_store.read(namespace, key); + let sqlite_res = self.sqlite_store.read(namespace, key); + + match sqlite_res { + Ok(read) => { + assert!(fs_res.is_ok()); + Ok(read) + } + Err(e) => { + assert!(fs_res.is_err()); + assert_eq!(e.kind(), unsafe { fs_res.unwrap_err_unchecked().kind() }); + Err(e) + } + } + } + + fn write(&self, namespace: &str, key: &str, buf: &[u8]) -> std::io::Result<()> { + let fs_res = self.fs_store.write(namespace, key, buf); + let sqlite_res = self.sqlite_store.write(namespace, key, buf); + + assert!(self.list(namespace).unwrap().contains(&key.to_string())); + + match fs_res { + Ok(()) => { + assert!(sqlite_res.is_ok()); + Ok(()) + } + Err(e) => { + assert!(sqlite_res.is_err()); + Err(e) + } + } + } + + fn remove(&self, namespace: &str, key: &str) -> std::io::Result<()> { + let fs_res = self.fs_store.remove(namespace, key); + let sqlite_res = self.sqlite_store.remove(namespace, key); + + match fs_res { + Ok(()) => { + assert!(sqlite_res.is_ok()); + Ok(()) + } + Err(e) => { + assert!(sqlite_res.is_err()); + Err(e) + } + } + } + + fn list(&self, namespace: &str) -> std::io::Result> { + let fs_res = self.fs_store.list(namespace); + let sqlite_res = self.sqlite_store.list(namespace); + + match fs_res { + Ok(list) => { + assert_eq!(list, sqlite_res.unwrap()); + Ok(list) + } + Err(e) => { + assert!(sqlite_res.is_err()); + Err(e) + } + } + } +} + +impl KVStorePersister for TestSyncStore { + fn persist(&self, prefixed_key: &str, object: &W) -> std::io::Result<()> { + let msg = format!("Could not persist file for key {}.", prefixed_key); + let dest_file = PathBuf::from_str(prefixed_key).map_err(|_| { + lightning::io::Error::new(lightning::io::ErrorKind::InvalidInput, msg.clone()) + })?; + + let parent_directory = dest_file.parent().ok_or(lightning::io::Error::new( + lightning::io::ErrorKind::InvalidInput, + msg.clone(), + ))?; + let namespace = parent_directory.display().to_string(); + + let dest_without_namespace = dest_file + .strip_prefix(&namespace) + .map_err(|_| lightning::io::Error::new(lightning::io::ErrorKind::InvalidInput, msg))?; + let key = dest_without_namespace.display().to_string(); + + let data = object.encode(); + self.write(&namespace, &key, &data)?; + Ok(()) + } +} + // Copied over from upstream LDK #[allow(dead_code)] pub struct TestLogger { @@ -268,6 +381,32 @@ pub fn setup_bitcoind_and_electrsd() -> (BitcoinD, ElectrsD) { (bitcoind, electrsd) } +pub(crate) fn setup_two_nodes( + electrsd: &ElectrsD, allow_0conf: bool, +) -> (Node, Node) { + println!("== Node A =="); + let config_a = random_config(); + let node_a = setup_node(electrsd, config_a); + + println!("\n== Node B =="); + let mut config_b = random_config(); + if allow_0conf { + config_b.trusted_peers_0conf.push(node_a.node_id()); + } + let node_b = setup_node(electrsd, config_b); + (node_a, node_b) +} + +pub(crate) fn setup_node(electrsd: &ElectrsD, config: Config) -> Node { + let esplora_url = format!("http://{}", electrsd.esplora_url.as_ref().unwrap()); + let mut builder = NodeBuilder::from_config(config.clone()); + builder.set_esplora_server(esplora_url.clone()); + let test_sync_store = Arc::new(TestSyncStore::new(config.storage_dir_path.into())); + let node = builder.build_with_store(test_sync_store).unwrap(); + node.start().unwrap(); + node +} + pub fn generate_blocks_and_wait(bitcoind: &BitcoinD, electrsd: &ElectrsD, num: usize) { let cur_height = bitcoind.client.get_block_count().expect("failed to get current block height"); let address = bitcoind