Code review

Author: arturgontijo

src/builder.rs

@@ -973,7 +973,7 @@ fn build_with_store_internal(
 	let persister = Arc::new(Persister::new(
 		Arc::clone(&kv_store),
 		Arc::clone(&logger),
-		10, // (?)
+		100,
 		Arc::clone(&keys_manager),
 		Arc::clone(&keys_manager),
 		Arc::clone(&tx_broadcaster),

src/io/test_utils.rs

@@ -6,8 +6,8 @@
 // accordance with one or both of these licenses.
 
 use lightning::ln::functional_test_utils::{
-	check_closed_event, connect_block, create_announced_chan_between_nodes, create_chanmon_cfgs,
-	create_dummy_block, create_network, create_node_cfgs, create_node_chanmgrs, send_payment,
+	connect_block, create_announced_chan_between_nodes, create_chanmon_cfgs, create_dummy_block,
+	create_network, create_node_cfgs, create_node_chanmgrs, send_payment, TestChanMonCfg,
 };
 use lightning::util::persist::{
 	KVStore, MonitorName, MonitorUpdatingPersister,
@@ -16,14 +16,23 @@ use lightning::util::persist::{
 
 use lightning::events::ClosureReason;
 use lightning::util::test_utils;
-use lightning::{check_added_monitors, check_closed_broadcast};
+use lightning::{check_added_monitors, check_closed_broadcast, check_closed_event};
 
 use rand::distributions::Alphanumeric;
 use rand::{thread_rng, Rng};
 
 use std::panic::RefUnwindSafe;
 use std::path::PathBuf;
 
+type TestMonitorUpdatePersister<'a, K> = MonitorUpdatingPersister<
+	&'a K,
+	&'a test_utils::TestLogger,
+	&'a test_utils::TestKeysInterface,
+	&'a test_utils::TestKeysInterface,
+	&'a test_utils::TestBroadcaster,
+	&'a test_utils::TestFeeEstimator,
+>;
+
 const EXPECTED_UPDATES_PER_PAYMENT: u64 = 5;
 
 pub(crate) fn random_storage_path() -> PathBuf {
@@ -83,6 +92,35 @@ pub(crate) fn do_read_write_remove_list_persist<K: KVStore + RefUnwindSafe>(kv_s
 	assert_eq!(listed_keys.len(), 0);
 }
 
+pub(crate) fn create_persister<'a, K: KVStore>(
+	store: &'a K, chanmon_cfg: &'a TestChanMonCfg, max_pending_updates: u64,
+) -> TestMonitorUpdatePersister<'a, K> {
+	let persister: TestMonitorUpdatePersister<'a, K> = MonitorUpdatingPersister::new(
+		store,
+		&chanmon_cfg.logger,
+		max_pending_updates,
+		&chanmon_cfg.keys_manager,
+		&chanmon_cfg.keys_manager,
+		&chanmon_cfg.tx_broadcaster,
+		&chanmon_cfg.fee_estimator,
+	);
+	return persister;
+}
+
+pub(crate) fn create_chain_monitor<'a, K: KVStore>(
+	chanmon_cfg: &'a TestChanMonCfg, persister: &'a TestMonitorUpdatePersister<'a, K>,
+) -> test_utils::TestChainMonitor<'a> {
+	let chain_mon = test_utils::TestChainMonitor::new(
+		Some(&chanmon_cfg.chain_source),
+		&chanmon_cfg.tx_broadcaster,
+		&chanmon_cfg.logger,
+		&chanmon_cfg.fee_estimator,
+		persister,
+		&chanmon_cfg.keys_manager,
+	);
+	return chain_mon;
+}
+
 // Integration-test the given KVStore implementation. Test relaying a few payments and check that
 // the persisted data is updated the appropriate number of times.
 pub(crate) fn do_test_store<K: KVStore>(store_0: &K, store_1: &K) {
@@ -93,46 +131,13 @@ pub(crate) fn do_test_store<K: KVStore>(store_0: &K, store_1: &K) {
 
 	let chanmon_cfgs = create_chanmon_cfgs(2);
 
-	let persister_0 = MonitorUpdatingPersister::new(
-		store_0,
-		&chanmon_cfgs[0].logger,
-		persister_0_max_pending_updates,
-		&chanmon_cfgs[0].keys_manager,
-		&chanmon_cfgs[0].keys_manager,
-		&chanmon_cfgs[0].tx_broadcaster,
-		&chanmon_cfgs[0].fee_estimator,
-	);
+	let persister_0 = create_persister(store_0, &chanmon_cfgs[0], persister_0_max_pending_updates);
+	let persister_1 = create_persister(store_1, &chanmon_cfgs[1], persister_1_max_pending_updates);
 
-	let persister_1 = MonitorUpdatingPersister::new(
-		store_1,
-		&chanmon_cfgs[1].logger,
-		persister_1_max_pending_updates,
-		&chanmon_cfgs[1].keys_manager,
-		&chanmon_cfgs[1].keys_manager,
-		&chanmon_cfgs[1].tx_broadcaster,
-		&chanmon_cfgs[1].fee_estimator,
-	);
+	let chain_mon_0 = create_chain_monitor(&chanmon_cfgs[0], &persister_0);
+	let chain_mon_1 = create_chain_monitor(&chanmon_cfgs[1], &persister_1);
 
 	let mut node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
-
-	let chain_mon_0 = test_utils::TestChainMonitor::new(
-		Some(&chanmon_cfgs[0].chain_source),
-		&chanmon_cfgs[0].tx_broadcaster,
-		&chanmon_cfgs[0].logger,
-		&chanmon_cfgs[0].fee_estimator,
-		&persister_0,
-		&chanmon_cfgs[0].keys_manager,
-	);
-
-	let chain_mon_1 = test_utils::TestChainMonitor::new(
-		Some(&chanmon_cfgs[1].chain_source),
-		&chanmon_cfgs[1].tx_broadcaster,
-		&chanmon_cfgs[1].logger,
-		&chanmon_cfgs[1].fee_estimator,
-		&persister_1,
-		&chanmon_cfgs[1].keys_manager,
-	);
-
 	node_cfgs[0].chain_monitor = chain_mon_0;
 	node_cfgs[1].chain_monitor = chain_mon_1;
 	let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
@@ -155,14 +160,15 @@ pub(crate) fn do_test_store<K: KVStore>(store_0: &K, store_1: &K) {
 				assert_eq!(mon.get_latest_update_id(), $expected_update_id);
 
 				let monitor_name = MonitorName::from(mon.get_funding_txo().0);
+				let store_0_updates = store_0
+					.list(
+						CHANNEL_MONITOR_UPDATE_PERSISTENCE_PRIMARY_NAMESPACE,
+						monitor_name.as_str(),
+					)
+					.unwrap()
+					.len() as u64;
 				assert_eq!(
-					store_0
-						.list(
-							CHANNEL_MONITOR_UPDATE_PERSISTENCE_PRIMARY_NAMESPACE,
-							monitor_name.as_str()
-						)
-						.unwrap()
-						.len() as u64,
+					store_0_updates,
 					mon.get_latest_update_id() % persister_0_max_pending_updates,
 					"Wrong number of updates stored in persister 0",
 				);
@@ -173,14 +179,15 @@ pub(crate) fn do_test_store<K: KVStore>(store_0: &K, store_1: &K) {
 				assert_eq!(mon.get_latest_update_id(), $expected_update_id);
 
 				let monitor_name = MonitorName::from(mon.get_funding_txo().0);
+				let store_1_updates = store_1
+					.list(
+						CHANNEL_MONITOR_UPDATE_PERSISTENCE_PRIMARY_NAMESPACE,
+						monitor_name.as_str(),
+					)
+					.unwrap()
+					.len() as u64;
 				assert_eq!(
-					store_1
-						.list(
-							CHANNEL_MONITOR_UPDATE_PERSISTENCE_PRIMARY_NAMESPACE,
-							monitor_name.as_str()
-						)
-						.unwrap()
-						.len() as u64,
+					store_1_updates,
 					mon.get_latest_update_id() % persister_1_max_pending_updates,
 					"Wrong number of updates stored in persister 1",
 				);
@@ -193,9 +200,11 @@ pub(crate) fn do_test_store<K: KVStore>(store_0: &K, store_1: &K) {
 	check_persisted_data!(0);
 
 	// Send a few payments and make sure the monitors are updated to the latest.
-	send_payment(&nodes[0], &vec![&nodes[1]][..], 8_000_000);
+	let expected_route = &[&nodes[1]][..];
+	send_payment(&nodes[0], expected_route, 8_000_000);
 	check_persisted_data!(EXPECTED_UPDATES_PER_PAYMENT);
-	send_payment(&nodes[1], &vec![&nodes[0]][..], 4_000_000);
+	let expected_route = &[&nodes[0]][..];
+	send_payment(&nodes[1], expected_route, 4_000_000);
 	check_persisted_data!(2 * EXPECTED_UPDATES_PER_PAYMENT);
 
 	// Send a few more payments to try all the alignments of max pending updates with
@@ -210,7 +219,8 @@ pub(crate) fn do_test_store<K: KVStore>(store_0: &K, store_1: &K) {
 			sender = 0;
 			receiver = 1;
 		}
-		send_payment(&nodes[sender], &vec![&nodes[receiver]][..], 21_000);
+		let expected_route = &[&nodes[receiver]][..];
+		send_payment(&nodes[sender], expected_route, 21_000);
 		check_persisted_data!(i * EXPECTED_UPDATES_PER_PAYMENT);
 	}
 
@@ -223,7 +233,7 @@ pub(crate) fn do_test_store<K: KVStore>(store_0: &K, store_1: &K) {
 	nodes[0].node.force_close_broadcasting_latest_txn(&chan_id, &node_id_1, err_msg).unwrap();
 
 	let reason = ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(true) };
-	check_closed_event(&nodes[0], 1, reason, false, &[node_id_1], 100000);
+	check_closed_event!(nodes[0], 1, reason, false, [node_id_1], 100000);
 	check_closed_broadcast!(nodes[0], true);
 	check_added_monitors!(nodes[0], 1);
 
@@ -236,7 +246,7 @@ pub(crate) fn do_test_store<K: KVStore>(store_0: &K, store_1: &K) {
 	check_closed_broadcast!(nodes[1], true);
 	let reason = ClosureReason::CommitmentTxConfirmed;
 	let node_id_0 = nodes[0].node.get_our_node_id();
-	check_closed_event(&nodes[1], 1, reason, false, &[node_id_0], 100000);
+	check_closed_event!(nodes[1], 1, reason, false, [node_id_0], 100000);
 	check_added_monitors!(nodes[1], 1);
 
 	// Make sure everything is persisted as expected after close.