From a3f4495df51faf2d18f37c2717a0f5cf9dfb9bb4 Mon Sep 17 00:00:00 2001 From: Onyekachukwu-Nweke Date: Sat, 12 Apr 2025 19:39:24 +0100 Subject: [PATCH 1/3] add optional name field to activities for easier logging and debugging --- Cargo.lock | 1 + sim-cli/Cargo.toml | 1 + sim-cli/src/parsing.rs | 55 +++++++++++++++++++++++++++++-- simln-lib/src/defined_activity.rs | 6 ++++ simln-lib/src/lib.rs | 10 +++++- simln-lib/src/test_utils.rs | 1 + 6 files changed, 71 insertions(+), 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index b7f74997..ecd5baf2 100755 --- a/Cargo.lock +++ b/Cargo.lock @@ -2570,6 +2570,7 @@ dependencies = [ "log", "openssl", "rand", + "regex", "serde", "serde_json", "simln-lib", diff --git a/sim-cli/Cargo.toml b/sim-cli/Cargo.toml index c45094c0..b973635e 100755 --- a/sim-cli/Cargo.toml +++ b/sim-cli/Cargo.toml @@ -27,6 +27,7 @@ futures = "0.3.30" console-subscriber = { version = "0.4.0", optional = true} tokio-util = { version = "0.7.13", features = ["rt"] } openssl = { version = "0.10", features = ["vendored"] } +regex = "1.11.1" [features] dev = ["console-subscriber"] diff --git a/sim-cli/src/parsing.rs b/sim-cli/src/parsing.rs index 84696ba7..bc941508 100755 --- a/sim-cli/src/parsing.rs +++ b/sim-cli/src/parsing.rs @@ -8,13 +8,14 @@ use simln_lib::{ ActivityDefinition, Amount, Interval, LightningError, LightningNode, NodeId, NodeInfo, Simulation, SimulationCfg, WriteResults, }; -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use std::fs; use std::ops::AsyncFn; use std::path::PathBuf; use std::sync::Arc; use tokio::sync::Mutex; use tokio_util::task::TaskTracker; +use regex::Regex; /// The default directory where the simulation files are stored and where the results will be written to. pub const DEFAULT_DATA_DIR: &str = "."; @@ -100,6 +101,9 @@ enum NodeConnection { /// [NodeId], which enables the use of public keys and aliases in the simulation description. #[derive(Debug, Clone, Serialize, Deserialize)] struct ActivityParser { + /// Optional identifier for this activity. + #[serde(default)] + pub name: Option, /// The source of the payment. #[serde(with = "serializers::serde_node_id")] pub source: NodeId, @@ -259,9 +263,55 @@ async fn validate_activities( get_node_info: impl AsyncFn(&PublicKey) -> Result, ) -> Result, LightningError> { let mut validated_activities = vec![]; + let mut activity_names = HashSet::new(); // Make all the activities identifiable by PK internally - for act in activity.into_iter() { + for (index, act) in activity.into_iter().enumerate() { + // Generate a default name if one is not provided + let name = match &act.name { + Some(name) => { + // Disallow empty names + if name.is_empty() { + return Err(LightningError::ValidationError( + "activity name cannot be empty".to_string() + )); + } + + // Disallow users from using the reserved "Activity-x" format + let reserved_pattern = Regex::new(r"^Activity-\d+$").unwrap(); + if reserved_pattern.is_match(name) { + return Err(LightningError::ValidationError(format!( + "'{}' uses a reserved name format. 'Activity-{{number}}' is reserved for system use.", + name + ))); + } + + // Check for duplicate names + if !activity_names.insert(name.clone()) { + return Err(LightningError::ValidationError(format!( + "duplicate activity name: {}", + name + ))); + } + name.clone() + }, + None => { + // Generate a unique system name + let mut counter = index; + let mut unique_name; + + loop { + unique_name = format!("Activity-{}", counter); + if activity_names.insert(unique_name.clone()) { + break; + } + counter += 1; + } + + unique_name + }, + }; + // We can only map aliases to nodes we control, so if either the source or destination alias // is not in alias_node_map, we fail let source = if let Some(source) = match &act.source { @@ -297,6 +347,7 @@ async fn validate_activities( }; validated_activities.push(ActivityDefinition { + name: Some(name), source, destination, interval_secs: act.interval_secs, diff --git a/simln-lib/src/defined_activity.rs b/simln-lib/src/defined_activity.rs index ea137609..79ed5064 100644 --- a/simln-lib/src/defined_activity.rs +++ b/simln-lib/src/defined_activity.rs @@ -7,6 +7,8 @@ use tokio::time::Duration; #[derive(Clone)] pub struct DefinedPaymentActivity { + #[allow(dead_code)] + name: Option, destination: NodeInfo, start: Option, count: Option, @@ -16,6 +18,7 @@ pub struct DefinedPaymentActivity { impl DefinedPaymentActivity { pub fn new( + name: Option, destination: NodeInfo, start: Option, count: Option, @@ -23,6 +26,7 @@ impl DefinedPaymentActivity { amount: ValueOrRange, ) -> Self { DefinedPaymentActivity { + name, destination, start, count, @@ -86,6 +90,7 @@ mod tests { #[test] fn test_defined_activity_generator() { + let name: String = "test_generator".to_string(); let node = create_nodes(1, 100000); let node = &node.first().unwrap().0; @@ -93,6 +98,7 @@ mod tests { let payment_amt = 50; let generator = DefinedPaymentActivity::new( + Option::from(name), node.clone(), None, None, diff --git a/simln-lib/src/lib.rs b/simln-lib/src/lib.rs index 304e5bef..fd3ec9d4 100755 --- a/simln-lib/src/lib.rs +++ b/simln-lib/src/lib.rs @@ -164,6 +164,8 @@ pub type Interval = ValueOrRange; /// This is constructed during activity validation and passed along to the [Simulation]. #[derive(Debug, Clone)] pub struct ActivityDefinition { + /// Optional identifier for this activity. + pub name: Option, /// The source of the payment. pub source: NodeInfo, /// The destination of the payment. @@ -500,6 +502,7 @@ pub struct WriteResults { /// ExecutorKit contains the components required to spin up an activity configured by the user, to be used to /// spin up the appropriate producers and consumers for the activity. struct ExecutorKit { + name: Option, source_info: NodeInfo, /// We use an arc mutex here because some implementations of the trait will be very expensive to clone. /// See [NetworkGraphView] for details. @@ -806,6 +809,7 @@ impl Simulation { if !self.activity.is_empty() { for description in self.activity.iter() { let activity_generator = DefinedPaymentActivity::new( + description.name.clone(), description.destination.clone(), description .start_secs @@ -816,6 +820,7 @@ impl Simulation { ); generators.push(ExecutorKit { + name: description.name.clone(), source_info: description.source.clone(), // Defined activities have very simple generators, so the traits required are implemented on // a single struct which we just cheaply clone. @@ -874,6 +879,7 @@ impl Simulation { for (node_info, capacity) in active_nodes.values() { generators.push(ExecutorKit { + name: None, source_info: node_info.clone(), network_generator: network_generator.clone(), payment_generator: Box::new( @@ -958,9 +964,11 @@ impl Simulation { let pe_sender = sender.clone(); tasks.spawn(async move { let source = executor.source_info.clone(); + let name = executor.name.as_deref().unwrap(); log::info!( - "Starting activity producer for {}: {}.", + "[{}] Starting activity producer for {}: {}.", + name, source, executor.payment_generator ); diff --git a/simln-lib/src/test_utils.rs b/simln-lib/src/test_utils.rs index 6a754747..ae45f9ad 100644 --- a/simln-lib/src/test_utils.rs +++ b/simln-lib/src/test_utils.rs @@ -212,6 +212,7 @@ pub fn create_activity( amount_msat: u64, ) -> ActivityDefinition { ActivityDefinition { + name: None, source, destination, start_secs: None, From b99ac02268ac99718f291512a7462f1c9f82be85 Mon Sep 17 00:00:00 2001 From: Onyekachukwu-Nweke Date: Mon, 28 Apr 2025 08:24:28 +0100 Subject: [PATCH 2/3] update create_activity and tests to support optional name parameter --- simln-lib/src/lib.rs | 12 ++++++++---- simln-lib/src/test_utils.rs | 3 ++- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/simln-lib/src/lib.rs b/simln-lib/src/lib.rs index fd3ec9d4..96fcebc6 100755 --- a/simln-lib/src/lib.rs +++ b/simln-lib/src/lib.rs @@ -1579,8 +1579,9 @@ mod tests { let missing_nodes = test_utils::create_nodes(1, 100_000); let missing_node = missing_nodes.first().unwrap().0.clone(); let dest_node = nodes[0].clone(); + let activity_name = None; - let activity = test_utils::create_activity(missing_node, dest_node, 1000); + let activity = test_utils::create_activity(activity_name, missing_node, dest_node, 1000); let simulation = test_utils::create_simulation(clients, vec![activity]); let result = simulation.validate_activity().await; @@ -1596,8 +1597,9 @@ mod tests { let (nodes, clients) = LightningTestNodeBuilder::new(1).build_full(); let dest_nodes = test_utils::create_nodes(1, 100_000); let dest_node = dest_nodes.first().unwrap().0.clone(); + let activity_name = None; - let activity = test_utils::create_activity(nodes[0].clone(), dest_node, 1000); + let activity = test_utils::create_activity(activity_name, nodes[0].clone(), dest_node, 1000); let simulation = test_utils::create_simulation(clients, vec![activity]); let result = simulation.validate_activity().await; @@ -1613,9 +1615,10 @@ mod tests { let (nodes, clients) = LightningTestNodeBuilder::new(1).build_full(); let dest_nodes = test_utils::create_nodes(1, 100_000); let mut dest_node = dest_nodes.first().unwrap().0.clone(); + let activity_name = None; dest_node.features.set_keysend_optional(); - let activity = test_utils::create_activity(nodes[0].clone(), dest_node, 1000); + let activity = test_utils::create_activity(activity_name, nodes[0].clone(), dest_node, 1000); let simulation = test_utils::create_simulation(clients, vec![activity]); let result = simulation.validate_activity().await; @@ -1627,8 +1630,9 @@ mod tests { #[tokio::test] async fn test_validate_zero_amount_no_valid() { let (nodes, clients) = LightningTestNodeBuilder::new(2).build_full(); + let activity_name = None; - let activity = test_utils::create_activity(nodes[0].clone(), nodes[1].clone(), 0); + let activity = test_utils::create_activity(activity_name, nodes[0].clone(), nodes[1].clone(), 0); let simulation = test_utils::create_simulation(clients, vec![activity]); let result = simulation.validate_activity().await; diff --git a/simln-lib/src/test_utils.rs b/simln-lib/src/test_utils.rs index ae45f9ad..b8fb8943 100644 --- a/simln-lib/src/test_utils.rs +++ b/simln-lib/src/test_utils.rs @@ -207,12 +207,13 @@ pub fn create_simulation( ) } pub fn create_activity( + name: Option, source: NodeInfo, destination: NodeInfo, amount_msat: u64, ) -> ActivityDefinition { ActivityDefinition { - name: None, + name, source, destination, start_secs: None, From 6f8d5c0e64ccfda9f2a2efe0c7ecf2795619a6b2 Mon Sep 17 00:00:00 2001 From: Onyekachukwu-Nweke Date: Tue, 13 May 2025 14:34:47 +0100 Subject: [PATCH 3/3] fix: improve error messages and fix regex compilation in loop - Move regex compilation outside of loop to improve performance - Improve error messages for empty and reserved activity names - Make error messages more user-friendly with actionable suggestions These changes address Clippy warnings and improve the user experience by providing clearer guidance when validation errors occur. --- sim-cli/src/parsing.rs | 10 ++++++---- simln-lib/src/defined_activity.rs | 6 +++--- simln-lib/src/lib.rs | 30 ++++++++++++++++++------------ 3 files changed, 27 insertions(+), 19 deletions(-) diff --git a/sim-cli/src/parsing.rs b/sim-cli/src/parsing.rs index bc941508..9f8809b4 100755 --- a/sim-cli/src/parsing.rs +++ b/sim-cli/src/parsing.rs @@ -2,6 +2,7 @@ use anyhow::anyhow; use bitcoin::secp256k1::PublicKey; use clap::{builder::TypedValueParser, Parser}; use log::LevelFilter; +use regex::Regex; use serde::{Deserialize, Serialize}; use simln_lib::{ cln, cln::ClnNode, eclair, eclair::EclairNode, lnd, lnd::LndNode, serializers, @@ -15,7 +16,6 @@ use std::path::PathBuf; use std::sync::Arc; use tokio::sync::Mutex; use tokio_util::task::TaskTracker; -use regex::Regex; /// The default directory where the simulation files are stored and where the results will be written to. pub const DEFAULT_DATA_DIR: &str = "."; @@ -265,6 +265,8 @@ async fn validate_activities( let mut validated_activities = vec![]; let mut activity_names = HashSet::new(); + let reserved_pattern = Regex::new(r"^Activity-\d+$").unwrap(); + // Make all the activities identifiable by PK internally for (index, act) in activity.into_iter().enumerate() { // Generate a default name if one is not provided @@ -273,15 +275,15 @@ async fn validate_activities( // Disallow empty names if name.is_empty() { return Err(LightningError::ValidationError( - "activity name cannot be empty".to_string() + "activity name cannot be an empty string, ".to_owned() + + "either remove name entirely or provide a string value", )); } // Disallow users from using the reserved "Activity-x" format - let reserved_pattern = Regex::new(r"^Activity-\d+$").unwrap(); if reserved_pattern.is_match(name) { return Err(LightningError::ValidationError(format!( - "'{}' uses a reserved name format. 'Activity-{{number}}' is reserved for system use.", + "'{}' uses a reserved name format. 'Activity-{{number}}' is reserved for system use. Please choose a different name.", name ))); } diff --git a/simln-lib/src/defined_activity.rs b/simln-lib/src/defined_activity.rs index 79ed5064..c0e2b8e1 100644 --- a/simln-lib/src/defined_activity.rs +++ b/simln-lib/src/defined_activity.rs @@ -8,7 +8,7 @@ use tokio::time::Duration; #[derive(Clone)] pub struct DefinedPaymentActivity { #[allow(dead_code)] - name: Option, + name: String, destination: NodeInfo, start: Option, count: Option, @@ -18,7 +18,7 @@ pub struct DefinedPaymentActivity { impl DefinedPaymentActivity { pub fn new( - name: Option, + name: String, destination: NodeInfo, start: Option, count: Option, @@ -98,7 +98,7 @@ mod tests { let payment_amt = 50; let generator = DefinedPaymentActivity::new( - Option::from(name), + name, node.clone(), None, None, diff --git a/simln-lib/src/lib.rs b/simln-lib/src/lib.rs index 96fcebc6..53558130 100755 --- a/simln-lib/src/lib.rs +++ b/simln-lib/src/lib.rs @@ -809,7 +809,10 @@ impl Simulation { if !self.activity.is_empty() { for description in self.activity.iter() { let activity_generator = DefinedPaymentActivity::new( - description.name.clone(), + description + .name + .clone() + .expect("Defined activity name is required"), description.destination.clone(), description .start_secs @@ -1552,7 +1555,7 @@ mod tests { let result = simulation.validate_activity().await; assert!(result.is_err()); - assert!(matches!(result, + assert!(matches!(result, Err(LightningError::ValidationError(msg)) if msg.contains("At least two nodes required"))); } @@ -1567,7 +1570,7 @@ mod tests { let result = simulation.validate_activity().await; assert!(result.is_err()); - assert!(matches!(result, + assert!(matches!(result, Err(LightningError::ValidationError(msg)) if msg.contains("must support keysend"))); } @@ -1586,7 +1589,7 @@ mod tests { let result = simulation.validate_activity().await; assert!(result.is_err()); - assert!(matches!(result, + assert!(matches!(result, Err(LightningError::ValidationError(msg)) if msg.contains("Source node not found"))); } @@ -1599,12 +1602,13 @@ mod tests { let dest_node = dest_nodes.first().unwrap().0.clone(); let activity_name = None; - let activity = test_utils::create_activity(activity_name, nodes[0].clone(), dest_node, 1000); + let activity = + test_utils::create_activity(activity_name, nodes[0].clone(), dest_node, 1000); let simulation = test_utils::create_simulation(clients, vec![activity]); let result = simulation.validate_activity().await; assert!(result.is_err()); - assert!(matches!(result, + assert!(matches!(result, Err(LightningError::ValidationError(msg)) if msg.contains("does not support keysend"))); } @@ -1618,7 +1622,8 @@ mod tests { let activity_name = None; dest_node.features.set_keysend_optional(); - let activity = test_utils::create_activity(activity_name, nodes[0].clone(), dest_node, 1000); + let activity = + test_utils::create_activity(activity_name, nodes[0].clone(), dest_node, 1000); let simulation = test_utils::create_simulation(clients, vec![activity]); let result = simulation.validate_activity().await; @@ -1632,12 +1637,13 @@ mod tests { let (nodes, clients) = LightningTestNodeBuilder::new(2).build_full(); let activity_name = None; - let activity = test_utils::create_activity(activity_name, nodes[0].clone(), nodes[1].clone(), 0); + let activity = + test_utils::create_activity(activity_name, nodes[0].clone(), nodes[1].clone(), 0); let simulation = test_utils::create_simulation(clients, vec![activity]); let result = simulation.validate_activity().await; assert!(result.is_err()); - assert!(matches!(result, + assert!(matches!(result, Err(LightningError::ValidationError(msg)) if msg.contains("zero values"))); } @@ -1651,7 +1657,7 @@ mod tests { let result = simulation.validate_node_network().await; assert!(result.is_err()); - assert!(matches!(result, + assert!(matches!(result, Err(LightningError::ValidationError(msg)) if msg.contains("we don't control any nodes"))); } @@ -1667,7 +1673,7 @@ mod tests { let result = simulation.validate_node_network().await; assert!(result.is_err()); - assert!(matches!(result, + assert!(matches!(result, Err(LightningError::ValidationError(msg)) if msg.contains("mainnet is not supported"))); } @@ -1683,7 +1689,7 @@ mod tests { let result = simulation.validate_node_network().await; assert!(result.is_err()); - assert!(matches!(result, + assert!(matches!(result, Err(LightningError::ValidationError(msg)) if msg.contains("nodes are not on the same network"))); }