simln-lib/feature: Optional name for activity descriptions #253

Onyekachukwu-Nweke · 2025-04-14T00:00:52Z

This PR adds an optional name field to activity descriptions that is used as a logging prefix. This makes it easier to relate logs to their corresponding activity, improving debugging and monitoring of simulations with multiple activities.

Fixes #127

Changes

Added optional name field to ActivityParser and ActivityDefinition structs
Added optional name field to DefinedPaymentActivity structs and added allow deadcode due to compiler warnings; this may be an ineffective way.
Updated validate_activities() to handle named activities and auto-generate names for unnamed ones
Modified ExecutorKit to include the activity name
Modified test utils to pass name check
Added appropriate documentation

Implementation Notes

This feature is currently only available for DefinedActivity (not for random activities)
Names are ensured to be unique across all activities
Backward compatible with existing simulation configurations

Test Vector

Tested with the following simulation file:

{
  "nodes": [...],
  "activity": [
    {
      "name": "High-Value-Payments",
      "source": "alice",
      "destination": "erin",
      "interval_secs": 3,
      "amount_msat": 50000
    },
    {
      "source": "dave",
      "destination": "alice",
      "interval_secs": 2,
      "amount_msat": [1000, 3000]
    }
  ]
}

Resulting logs properly show activity names:

2025-04-13T23:54:09.953Z INFO  [sim_cli::parsing] Connected to erin - Node ID: 020d45c1c54f67ddc1c8e1026fd795111fc6641126f06f5a0e07a730700aeed1bf.
2025-04-13T23:54:09.953Z INFO  [sim_cli::parsing] Connected to alice - Node ID: 027e988bfaf8c5f103f102764c73b4e25d6eda8538a02a8c3363151a748d871243.
2025-04-13T23:54:09.953Z INFO  [sim_cli::parsing] Connected to dave - Node ID: 03cc8e752d37b6b968ed4695b4560bd7804e4f3daa6674e018967a0e481bf0050f.
2025-04-13T23:54:09.953Z INFO  [simln_lib] Running the simulation forever.
2025-04-13T23:54:10.013Z INFO  [simln_lib] Simulation is running on regtest.
2025-04-13T23:54:10.014Z INFO  [simln_lib] Simulating 2 activity on 3 nodes.
2025-04-13T23:54:10.014Z INFO  [simln_lib] Summary of results will be reported every 60s.
2025-04-13T23:54:10.014Z INFO  [simln_lib] [High-Value-Payments] Starting activity producer for alice(027e98...871243): static payment of 50000 to erin(020d45...eed1bf) every 3s.
2025-04-13T23:54:10.014Z INFO  [simln_lib] [Activity-1] Starting activity producer for dave(03cc8e...f0050f): static payment of (1000-3000) to alice(027e98...871243) every 2s.
2025-04-13T23:55:10.015Z INFO  [simln_lib] Processed 48 payments sending 1002670 msat total with 0.00% success rate.
^C2025-04-13T23:55:36.657Z INFO  [sim_cli] Shutting down simulation.
2025-04-13T23:55:36.658Z INFO  [simln_lib] All producers finished. Shutting down.

carlaKC

Thanks for the PR!

This needs a little git surgery to get the last few commits into shape, feel free to just restructure as you see fit + squash (rather than using fixups) so that we can start with a solid base.

Only major design ask is whether we actually need Option throughout

carlaKC · 2025-04-22T19:33:25Z

sim-cli/src/parsing.rs

+    for (index, act) in activity.into_iter().enumerate() {
+        // Generate a default name if one is not provided
+        let name = match &act.name {
+            Some(name) if !name.is_empty() => {


I think we can afford to be stricter and disallow an empty string - really can't see somebody having much purpose for name: ""

Okay, that could be done

carlaKC · 2025-04-22T19:37:02Z

sim-cli/src/parsing.rs

+                while !activity_names.insert(unique_name.clone()) {
+                    unique_name = format!("{}-{}", default_name, counter);
+                    counter += 1;
+                }


This validation has an ordering condition - right?

If we've got an explicit name Activity-x before this, we'll correctly validate

If we've got an explicit name Activity-xafter this, we'll end up failing the above check on a user-provided check

I think that we can afford to be strict and just disallow explicit names with the internally reserved format Activity-x - I feel like all (sane) users can live with that. Simplifies this, and then we just need to add a regex above to check the explicit names (I think we can just match Activity-{any number})

Yeah, I didn't think it that way, I would add that condition to it

Thanks for the heads up

carlaKC · 2025-04-22T19:38:09Z

simln-lib/src/lib.rs

@@ -164,6 +164,8 @@ pub type Interval = ValueOrRange<u16>;
 /// This is constructed during activity validation and passed along to the [Simulation].
 #[derive(Debug, Clone)]
 pub struct ActivityDefinition {
+    /// Optional name/identifier for this activity


nit: here and above, name/identifier isn't helpful - since we're requiring that these be unique I think we can just say identifier in our comments.

Should also terminate comments with . - applies in various places for this PR

sim-cli/src/parsing.rs

carlaKC · 2025-04-22T19:39:46Z

simln-lib/src/defined_activity.rs

@@ -7,6 +7,7 @@ use tokio::time::Duration;

 #[derive(Clone)]
 pub struct DefinedPaymentActivity {
+    name: Option<String>,


If we're going to always fill this in if the end user provides it, so we want this to be Option?

Yeah, and also another reason for the Option is for it not to break the system for backward compatibility

I agree that this should be optional in ActivityParser, but DefinedPaymentActivity is an internal struct unrelated to parsing user values? It can be an Option when we parse it and required when we use it interanally.

Also shouldn't have #[allow(dead_code)] here?

carlaKC · 2025-04-22T19:41:06Z

simln-lib/src/defined_activity.rs

+            "[{:?}] static payment of {} to {} every {}s",
+            self.name, self.amount, self.destination, self.wait


I don't think we need an option here, but if I'm wrong this needs cleanup:

This is going to print something ugly like Some(name), rather inline a quick decomposing of the option.
if let Some(name) = self.name { name } else { "" }

carlaKC · 2025-04-22T19:41:30Z

simln-lib/src/defined_activity.rs

@@ -39,8 +39,8 @@ impl fmt::Display for DefinedPaymentActivity {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        write!(
            f,
-            "[{:?}] static payment of {} to {} every {}s",
-            self.name, self.amount, self.destination, self.wait
+            "static payment of {} to {} every {}s",


Name added in the commit before and removed here?

Yeah, I found out it wasn't where I needed it to be in

sim-cli/src/parsing.rs

simln-lib/src/test_utils.rs

Onyekachukwu-Nweke · 2025-04-28T07:27:38Z

Thanks for the PR!

This needs a little git surgery to get the last few commits into shape, feel free to just restructure as you see fit + squash (rather than using fixups) so that we can start with a solid base.

Only major design ask is whether we actually need Option throughout

Thank you for the feedback, I have implemented the changes needed.

carlaKC · 2025-04-28T14:00:52Z

simln-lib/src/defined_activity.rs

@@ -7,6 +7,7 @@ use tokio::time::Duration;

 #[derive(Clone)]
 pub struct DefinedPaymentActivity {
+    name: Option<String>,


I agree that this should be optional in ActivityParser, but DefinedPaymentActivity is an internal struct unrelated to parsing user values? It can be an Option when we parse it and required when we use it interanally.

Also shouldn't have #[allow(dead_code)] here?

carlaKC · 2025-04-28T14:02:53Z

sim-cli/src/parsing.rs

+                // Disallow empty names
+                if name.is_empty() {
+                    return Err(LightningError::ValidationError(
+                        "activity name cannot be empty".to_string()


nit: let's make this a little more helpful?

"activity name cannot be an empty string, either remove name entirely or provide a string value"

carlaKC · 2025-04-28T14:03:17Z

sim-cli/src/parsing.rs

+                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.",


likewise - slightly more helpful error names, add a prompt to pick another name here

carlaKC · 2025-04-28T14:05:57Z

simln-lib/src/lib.rs

@@ -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();


Having something be an Option then unwraping it later effectively means that it isn't actually an Option?

If somebody were using sim-ln as a library, this would make their code panic if they didn't provide a name which is seemingly optional based on our public API.

carlaKC · 2025-04-28T14:06:32Z

simln-lib/src/lib.rs


                log::info!(
-                    "Starting activity producer for {}: {}.",
+                    "[{}] Starting activity producer for {}: {}.",


nit: log name elsewhere as well?

carlaKC · 2025-04-28T14:07:16Z

simln-lib/src/test_utils.rs

@@ -207,12 +207,13 @@ pub fn create_simulation(
    )
 }
 pub fn create_activity(
+    name: Option<String>,


Unclear to me what value this commit is adding?

carlaKC · 2025-04-28T14:08:10Z

CI unhappy - see contribution guidelines!

carlaKC reviewed Apr 22, 2025

View reviewed changes

Onyekachukwu-Nweke added 2 commits April 28, 2025 08:14

add optional name field to activities for easier logging and debugging

a3f4495

update create_activity and tests to support optional name parameter

b99ac02

Onyekachukwu-Nweke force-pushed the feature/add-optional-activity-names branch from 0401324 to b99ac02 Compare April 28, 2025 07:24

Onyekachukwu-Nweke requested a review from carlaKC April 28, 2025 07:27

carlaKC requested changes Apr 28, 2025

View reviewed changes

This was referenced May 6, 2025

Feature Request: Fully Deterministic produce_events #243

Open

Feature: Optional name for activity descriptions #127

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

simln-lib/feature: Optional name for activity descriptions #253

simln-lib/feature: Optional name for activity descriptions #253

Onyekachukwu-Nweke commented Apr 14, 2025

carlaKC left a comment

carlaKC Apr 22, 2025

Onyekachukwu-Nweke Apr 26, 2025

carlaKC Apr 22, 2025

Onyekachukwu-Nweke Apr 26, 2025

carlaKC Apr 22, 2025

carlaKC Apr 22, 2025

Onyekachukwu-Nweke Apr 26, 2025 •

edited

Loading

carlaKC Apr 28, 2025

carlaKC Apr 22, 2025

carlaKC Apr 22, 2025

Onyekachukwu-Nweke Apr 27, 2025

Onyekachukwu-Nweke commented Apr 28, 2025

carlaKC Apr 28, 2025

carlaKC Apr 28, 2025

carlaKC Apr 28, 2025

carlaKC Apr 28, 2025

carlaKC Apr 28, 2025

carlaKC Apr 28, 2025

carlaKC commented Apr 28, 2025

		"[{:?}] static payment of {} to {} every {}s",
		self.name, self.amount, self.destination, self.wait

simln-lib/feature: Optional name for activity descriptions #253

Are you sure you want to change the base?

simln-lib/feature: Optional name for activity descriptions #253

Conversation

Onyekachukwu-Nweke commented Apr 14, 2025

Changes

Implementation Notes

Test Vector

carlaKC left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Onyekachukwu-Nweke Apr 26, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Onyekachukwu-Nweke commented Apr 28, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlaKC commented Apr 28, 2025

Onyekachukwu-Nweke Apr 26, 2025 •

edited

Loading