simln-lib/feature: Add ability to run with mocked ln network #248

f3r10 · 2025-03-30T15:44:46Z

address #215

carlaKC · 2025-04-02T12:38:11Z

Is this ready for a first review pass?

f3r10 · 2025-04-02T14:38:26Z

Sure, @carlaKC, it is ready for a first review pass. I have been thinking about how to add some tests

carlaKC

A few big picture comments on how we're going to split this up, now that I've had some time to think about this (and live with these decisions on a downstream fork) I think we can cut it up a bit cleaner.

Also feel free to cherry pick carlaKC@d1afc33 for docs once this reaches the stage where you need then (removing the embarrassing rebase booger in there, kek).

carlaKC · 2025-04-02T19:12:09Z

sim-cli/src/parsing.rs

@@ -96,6 +101,27 @@ enum NodeConnection {
    Eclair(eclair::EclairConnection),
 }

+/// Data structure that is used to parse information from the simulation file, used to pair two node policies together
+/// without the other internal state that is used in our simulated network.


wdym by "without the other internal state that is used in our simulated network?"

Without all the stuff in SimulatedChannel? I don't think the parsing module needs to worry too much about noting that - we have a few structs across modules that are quite duplicated but different for the sake of separation.

carlaKC · 2025-04-02T19:17:17Z

sim-cli/src/main.rs

@@ -20,7 +20,7 @@ async fn main() -> anyhow::Result<()> {
        .init()
        .unwrap();

-    let sim = create_simulation(&cli).await?;
+    let (sim, sim_network) = create_simulation(&cli).await?;


I'm torn about whether to have all the logic about our two different run "modes" here vs hidden in create_simulation (I know I did that in my branch, but I didn't love it).

WDYT of something like this which separates validation and has two dedicated functions that assumes we've done some validation on cli:

cli.validate()?; // does all the checks on nodes and sim_network let tasks = TaskTracker::new(); let sim = if sim.network.is_none() { create_simulation(cli, tasks) } else { create_simulation_with_network(cli, tasks); } sim.run() tasks.wait

I don't like that we have some duplication in the different create network functions, but it is nice to clearly see the two cases and have more specialized functions.

(Related to comment about getting rid of new_with_sim_network)

I agree; it makes the code more concise

carlaKC · 2025-04-02T19:19:05Z

simln-lib/src/sim_node.rs

@@ -654,7 +655,7 @@ pub struct SimGraph {

    /// track all tasks spawned to process payments in the graph. Note that handling the shutdown of tasks
    /// in this tracker must be done externally.
-    tasks: TaskTracker,
+    pub tasks: TaskTracker,


If we're passing tasks into the constructor, we should have it available at our call site and then don't need to make it pub?

carlaKC · 2025-04-02T19:26:00Z

simln-lib/src/lib.rs

@@ -527,6 +528,42 @@ impl Simulation {
        }
    }

+    pub async fn new_with_sim_network(


On second look, this seems like a bit of a layering violation to have Simulation be aware of SimGraph stuff - it should just care about getting some arbitrary set of lightning nodes.

If we have a dedicated create_simulation_with_network in the parsing module, we can just create the graph with SimGraph::new and then use ln_node_from_graph to get the nodes to pass into Simulation::new.

The nice side effect of passing tasks in is that we don't actually need the graph to wait on anymore, we can just tasks.wait at the end of the sim.

carlaKC

Looking really nice with the last refactor!

carlaKC · 2025-04-08T17:11:20Z

sim-cli/src/main.rs

+    let sim_path = read_sim_path(cli.data_dir.clone(), cli.sim_file.clone()).await?;
+    let sim_params = serde_json::from_str(&std::fs::read_to_string(sim_path)?).map_err(|e| {
+        anyhow!(
+            "Could not deserialize node connection data or activity description from simulation file (line {}, col {}, err: {}).",
+            e.line(),
+            e.column(),
+            e.to_string()
+        )
+        })?;
+
+    cli.validate(&sim_params)?;


nittynit: move all of this + Cli::parse into a helper to keep main thin

carlaKC · 2025-04-08T17:12:09Z

sim-cli/src/parsing.rs

@@ -22,7 +27,7 @@ pub const DEFAULT_DATA_DIR: &str = ".";
 /// The default simulation file to be used by the simulator.
 pub const DEFAULT_SIM_FILE: &str = "sim.json";

-/// The default expected payment amount for the simulation, around ~$10 at the time of writing.
+/// The default expected payment amount for the simulation, around ~$10 at the time of w.


acciental change

carlaKC · 2025-04-08T17:12:36Z

sim-cli/src/parsing.rs

@@ -81,9 +86,31 @@ pub struct Cli {
    pub fix_seed: Option<u64>,
 }

+impl Cli {
+    pub fn validate(&self, sim_params: &SimParams) -> Result<(), anyhow::Error> {
+        // Validate that nodes and sim_graph are exclusively set, and setup node clients from the populated field.


"and setup node clients from the populated field." out of date now

sim-cli/src/parsing.rs

carlaKC · 2025-04-08T17:15:52Z

sim-cli/src/parsing.rs

+type NodeMapping =
+    Result<(HashMap<PublicKey, NodeInfo>, HashMap<String, NodeInfo>), LightningError>;


Maybe a struct rather than a type? little more readable IMO

carlaKC · 2025-04-08T17:19:45Z

sim-cli/src/parsing.rs

+    for sim_channel in sim_network {
+        nodes_info.insert(
+            sim_channel.node_1.pubkey,
+            node_info(sim_channel.node_1.pubkey),


Is there a way around using this internal function? Rather iter through channels and go via the API?

Parsing shouldn't really have to know the underlying details of how we create mocked node info.

carlaKC · 2025-04-08T17:24:14Z

sim-cli/src/parsing.rs

+    let (pk_node_map, alias_node_map) = add_node_to_maps(&nodes_info)?;
+    let validated_activities = validate_activities(
+        activity.to_vec(),
+        pk_node_map,
+        alias_node_map,
+        get_node_info,
+    )
+    .await?;


This duplication (here in the other simulation creation function) makes me think that we don't have the right API for Simulation::new().

Maybe we should be passing activities into simulation.run() instead of requiring that we have them on hand when the simulation is created? Although we would then need access to the nodes to validate our activities (which is something I'm certain belongs here, not in the simulator), so tradeoffs.

WDYT?

Yes, I agree @carlaKC that those lines should be abstracted away.
I think passing activities into simulation.run() is a great idea, but maybe the validation could be done in main.rs

let (mut sim, nodes_info) = if sim_params.sim_network.is_empty() { create_simulation(&cli, &sim_params, tasks.clone()).await? } else { create_simulation_with_network(&cli, &sim_params, tasks.clone()).await? }; let sim2 = sim.clone(); let validated_activities = get_validated_activities(&sim.nodes, nodes_info, sim_params.activity).await?; sim.run(validated_activities).await?;

pub async fn run( &mut self, activities: Vec<ActivityDefinition>, ) -> Result<(), SimulationError> { self.internal_run().await?; self.activity = activities; // Close our TaskTracker and wait for any background tasks // spawned during internal_run to complete. self.tasks.close(); self.tasks.wait().await; Ok(()) }

get_validated_activities would be a function located in parsing.rs

This looks good!

I believe that we can get rid of self.activity on Simulation completely if we pass activities into Simulation.run?

ie, not sure why we need self.activity = activities;?

carlaKC · 2025-04-14T18:08:30Z

sim-cli/src/main.rs

@@ -29,6 +37,7 @@ async fn main() -> anyhow::Result<()> {
    })?;

    sim.run().await?;
+    tasks.wait().await;


wait is covered in sim.run?

Unaddressed, believe that this can be removed?

carlaKC · 2025-04-14T18:13:39Z

sim-cli/src/parsing.rs

@@ -140,23 +188,94 @@ impl TryFrom<&Cli> for SimulationCfg {
    }
 }

+struct NodeMapping(HashMap<PublicKey, NodeInfo>, HashMap<String, NodeInfo>);


let's name these fields? Original motivation to ask for the struct was to make this more easily readable

carlaKC · 2025-04-14T18:14:14Z

sim-cli/src/parsing.rs

+    // Copy all simulated channels into a read-only routing graph, allowing to pathfind for
+    // individual payments without locking th simulation graph (this is a duplication of the channels, but the performance tradeoff is worthwhile for concurrent pathfinding).


nit: line wrapping off here

carlaKC · 2025-04-14T18:14:43Z

sim-cli/src/parsing.rs

+            .map_err(|e| SimulationError::SimulatedNetworkError(format!("{:?}", e)))?,
+    );
+
+    let nodes = ln_node_from_graph(simulation_graph.clone(), routing_graph).await;


nit: inline nodes in Simulation::new

carlaKC · 2025-04-14T18:16:07Z

sim-cli/src/parsing.rs

@@ -362,3 +485,17 @@ fn mkdir(dir: PathBuf) -> anyhow::Result<PathBuf> {
    fs::create_dir_all(&dir)?;
    Ok(dir)
 }
+
+pub async fn parse_cli() -> anyhow::Result<(Cli, SimParams)> {


slight personal preference to call Cli::parse at the call site and pass in &cli but not strongly held opinion

carlaKC · 2025-04-14T18:17:44Z

sim-cli/src/parsing.rs

+                "Simulation file cannot contain {} nodes and {} sim_graph entries, simulation can only be run with real 
+            or simulated nodes not both.", sim_params.nodes.len(), sim_params.sim_network.len(),
+        ));
+        }


nit: formatting looks odd here?

rustfmt doesn't work well inside of macros, maybe you can manually clean this up a bit?

carlaKC · 2025-04-14T18:21:56Z

sim-cli/src/parsing.rs

+        let (pubkey, node_info) = channel.create_simulated_node();
+        nodes_info.insert(pubkey, node_info);


I believe we have to return node info both node_1 and node_2 here?

If we have a simulated channel with:

node_1: { pubkey: A } node_2: {pubkey: B }

And then we want to send payments:

"activity": [ { "source": "B", "destination": "A", "interval_secs": 1, "amount_msat": 2000 } ]

As we have this now, we won't be able to look up the source node in our map?

carlaKC · 2025-04-14T18:25:12Z

Shaping up really well! I think that we can make the validation changes discussed here and then this will be good to go.

If possible, let's do the refactor to move validation in a commit before this one (just add a new one before it, rather than fixup).

f3r10 · 2025-04-16T16:53:10Z

OK, @carlaKC. I think I was able to insert the abstraction of the validation of activities commit before all others. (I have not done this process before. sweat_smile: I hope I did it right.) Let me know if it is okay.

carlaKC

Nice job on the git surgery!

Some comments seem to be unaddressed since last review (eg). GH sometimes hides them after a push, so worth another look.

carlaKC · 2025-04-17T20:58:10Z

sim-cli/src/main.rs

@@ -29,6 +37,7 @@ async fn main() -> anyhow::Result<()> {
    })?;

    sim.run().await?;
+    tasks.wait().await;


Unaddressed, believe that this can be removed?

carlaKC · 2025-04-17T21:01:27Z

simln-lib/src/lib.rs

-    nodes: HashMap<PublicKey, Arc<Mutex<dyn LightningNode>>>,
-    /// The activity that are to be executed on the node.
-    activity: Vec<ActivityDefinition>,
+    pub nodes: HashMap<PublicKey, Arc<Mutex<dyn LightningNode>>>,


Don't love that we have to make this pub

Now that activity validation has been DRY-ed up, I think that we can have an in-between approach where the create_simulation... functions return them. Rather than exposing the client info to main just so we can validate the activites.

Patch here for suggestion, I think it hits a good middle ground.

carlaKC

This is getting really close, just a few cleanups left. Tested and works like a charm with the mainnet graph 🙌

Feel free to squash and fix up the last few nits. Would also like to cherry pick this docs commit + fixup so that we have a write up of how to run this.

carlaKC · 2025-04-22T18:07:12Z

sim-cli/src/parsing.rs

+    let validated_activities =
+        validate_activities(activity.to_vec(), pk_node_map, alias_node_map, get_node).await?;
+    Ok(validated_activities)


nit: can inline to Ok(validate_activities(...)), intermediate var doesn't add much

carlaKC · 2025-04-22T18:08:31Z

sim-cli/src/parsing.rs

+/// Data structure that is used to parse information from the simulation file. It is used to
+/// create a mocked network


nit: comments should end with .

carlaKC · 2025-04-22T18:08:56Z

sim-cli/src/parsing.rs

+struct NodeMapping {
+    ///Tracks nodes-info by pubkey
+    pk_node_map: HashMap<PublicKey, NodeInfo>,
+    ///Tracks nodes-info by aliases
+    alias_node_map: HashMap<String, NodeInfo>,
+}


nit: missing space after // and needs full stop or delete the comment, I think the field names are quite self-explanatory

carlaKC · 2025-04-22T18:10:49Z

sim-cli/src/parsing.rs

@@ -290,7 +412,7 @@ async fn validate_activities(
    Ok(validated_activities)
 }

-async fn read_sim_path(data_dir: PathBuf, sim_file: PathBuf) -> anyhow::Result<PathBuf> {
+pub async fn read_sim_path(data_dir: PathBuf, sim_file: PathBuf) -> anyhow::Result<PathBuf> {


Why does this function need to be made pub?

carlaKC · 2025-04-22T18:16:43Z

simln-lib/src/sim_node.rs

@@ -269,6 +270,14 @@ impl ChannelState {
    }
 }

+///Represents the necessary simulated nodes in order to create a simulation network


missing space after ///

carlaKC · 2025-04-22T18:17:25Z

simln-lib/src/sim_node.rs

@@ -269,6 +270,14 @@ impl ChannelState {
    }
 }

+///Represents the necessary simulated nodes in order to create a simulation network
+pub struct SimulatedNodes {
+    pub pubkey_node_1: PublicKey,


The pubkey fields are unnecessary here, we have this in NodeInfo

carlaKC · 2025-04-22T18:17:47Z

simln-lib/src/sim_node.rs

+    pub fn create_simulated_nodes(&self) -> SimulatedNodes {
+        SimulatedNodes {
+            pubkey_node_1: self.node_1.policy.pubkey,
+            node_1_info: node_info(self.node_1.policy.pubkey),


I think that this can just return (NodeInfo, NodeInfo) because we don't need the pubkeys

carlaKC · 2025-04-22T18:18:08Z

simln-lib/src/sim_node.rs

@@ -488,7 +506,7 @@ impl<'a, T: SimNetwork> SimNode<'a, T> {
 }

 /// Produces the node info for a mocked node, filling in the features that the simulator requires.
-fn node_info(pubkey: PublicKey) -> NodeInfo {
+pub fn node_info(pubkey: PublicKey) -> NodeInfo {


This does not need to be public

simln-lib/src/test_utils.rs

f3r10 · 2025-04-24T15:45:54Z

@carlaKC The squashing process that is necessary to do should not include the first commit, right? (7f5c6b3)

carlaKC

tACK, go ahead and squash 🎉

I'm going to hold off on merging this until we can cut 2.4, so that we don't ship this totally new feature in a point release. Will merge once that's done 👌

carlaKC · 2025-04-25T20:59:40Z

README.md

+* Liquidity checks: HTLCs are only forwarded if the node has sufficient
+  liquidity in the mocked channel.
+
+Not included: 


In a followup: we should also mention that we don't have a concept of expiry/force closes in the simulator

f3r10 force-pushed the feature_add_ability_to_run_with_mocked_ln_network branch from 6795897 to 3d2685b Compare March 30, 2025 15:48

carlaKC mentioned this pull request Apr 2, 2025

sim-lib: add simulated clock to speed up simulations #242

Open

f3r10 force-pushed the feature_add_ability_to_run_with_mocked_ln_network branch from 3d2685b to 140f087 Compare April 2, 2025 18:07

carlaKC reviewed Apr 2, 2025

View reviewed changes

f3r10 marked this pull request as ready for review April 8, 2025 02:28

f3r10 requested a review from carlaKC April 8, 2025 02:28

carlaKC reviewed Apr 8, 2025

View reviewed changes

f3r10 force-pushed the feature_add_ability_to_run_with_mocked_ln_network branch 2 times, most recently from 5e2151e to b09ac15 Compare April 14, 2025 15:48

f3r10 requested a review from carlaKC April 14, 2025 15:50

carlaKC reviewed Apr 14, 2025

View reviewed changes

simln-lib/refactor: abstract activities validation

7f5c6b3

f3r10 force-pushed the feature_add_ability_to_run_with_mocked_ln_network branch from cac73aa to c57a299 Compare April 16, 2025 16:49

f3r10 requested a review from carlaKC April 16, 2025 16:53

carlaKC reviewed Apr 17, 2025

View reviewed changes

f3r10 requested a review from carlaKC April 21, 2025 21:24

carlaKC reviewed Apr 22, 2025

View reviewed changes

f3r10 requested a review from carlaKC April 24, 2025 15:55

carlaKC approved these changes Apr 25, 2025

View reviewed changes

simln-lib/feature: Add ability to run with mocked ln network

0c91cf7

f3r10 force-pushed the feature_add_ability_to_run_with_mocked_ln_network branch from 349e09c to 0c91cf7 Compare April 28, 2025 15:18

		type NodeMapping =
		Result<(HashMap<PublicKey, NodeInfo>, HashMap<String, NodeInfo>), LightningError>;

		// Copy all simulated channels into a read-only routing graph, allowing to pathfind for
		// individual payments without locking th simulation graph (this is a duplication of the channels, but the performance tradeoff is worthwhile for concurrent pathfinding).

		let (pubkey, node_info) = channel.create_simulated_node();
		nodes_info.insert(pubkey, node_info);

		/// Data structure that is used to parse information from the simulation file. It is used to
		/// create a mocked network

simln-lib/feature: Add ability to run with mocked ln network #248

Are you sure you want to change the base?

simln-lib/feature: Add ability to run with mocked ln network #248

Conversation

f3r10 commented Mar 30, 2025

carlaKC commented Apr 2, 2025

f3r10 commented Apr 2, 2025

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

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

f3r10 Apr 11, 2025 • edited Loading

Choose a reason for hiding this comment

carlaKC Apr 14, 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

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 14, 2025

f3r10 commented Apr 16, 2025

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

f3r10 commented Apr 24, 2025 • edited Loading

carlaKC left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

f3r10 Apr 11, 2025 •

edited

Loading

carlaKC Apr 14, 2025 •

edited

Loading

f3r10 commented Apr 24, 2025 •

edited

Loading