Allow unspecified alias destinations #254

chuksys · 2025-04-17T22:38:54Z

Description

This PR pulls up the network graph to make it simpler to run node lookups using public keys and aliases. This new node lookup approach is used to allow unspecified aliases in activity destinations by confirming if node(s) with unspecified alias exists in the graph.

Changes

Added Graph struct to simln-lib to enable simpler node lookups.
Added get_graph function to LightningNode trait and implemented it in the different LN implementations.
Refactored activities validation to run lookup against Graph directly.
Enabled mapping of duplicate aliases in alias_node_map and nodes_by_alias.

This PR closes #227

chuksys

Dropped some notes. Open to suggestions.

simln-lib/src/sim_node.rs

simln-lib/src/eclair.rs

sim-cli/src/parsing.rs

carlaKC

Promising start, but I think that this PR has scope crept a bit. I'd really like to keep this down to just the case where we have a destination alias that isn't in our configured set of nodes. Feel free to squash and restructure as suggested, we can start with fixups when the PR's structure is in good shape.

Also think that we need to look more carefully at the design of the graph function in the LightningNode trait and move some of the more custom behavior into our validation function.

simln-lib/src/lib.rs

simln-lib/src/eclair.rs

simln-lib/src/sim_node.rs

sim-cli/src/parsing.rs

README.md

chuksys · 2025-04-23T18:38:17Z

Promising start, but I think that this PR has scope crept a bit. I'd really like to keep this down to just the case where we have a destination alias that isn't in our configured set of nodes. Feel free to squash and restructure as suggested, we can start with fixups when the PR's structure is in good shape.

Also think that we need to look more carefully at the design of the graph function in the LightningNode trait and move some of the more custom behavior into our validation function.

Thanks for the feedback. Will sort this out asap!

Switches from an async closure to direct NetworkGraph usage for activity destination node lookup. This improves efficiency and maintainability, and enables destination aliases by validating against the graph and erroring on duplicates.

carlaKC

Moving in the right direction!

carlaKC · 2025-05-01T13:47:35Z

sim-cli/src/parsing.rs

-        ))
+    let graph = match clients.values().next() {
+        Some(client) => client.lock().await.get_graph().await?,
+        None => panic!("Graph is empty"),


Return an error rather than panicing?

carlaKC · 2025-05-01T13:48:56Z

sim-cli/src/parsing.rs

@@ -5,12 +5,11 @@ use log::LevelFilter;
 use serde::{Deserialize, Serialize};
 use simln_lib::{
    cln, cln::ClnNode, eclair, eclair::EclairNode, lnd, lnd::LndNode, serializers,
-    ActivityDefinition, Amount, Interval, LightningError, LightningNode, NodeId, NodeInfo,
+    ActivityDefinition, Amount, Graph, Interval, LightningError, LightningNode, NodeId, NodeInfo,


Commit message still doesn't wrap at 72

carlaKC · 2025-05-01T13:49:04Z

sim-cli/src/parsing.rs

@@ -215,9 +209,9 @@ async fn get_clients(
 /// validation.
 async fn add_node_to_maps(
    nodes: &HashMap<PublicKey, NodeInfo>,
-) -> Result<(HashMap<PublicKey, NodeInfo>, HashMap<String, NodeInfo>), LightningError> {
+) -> Result<(HashMap<PublicKey, NodeInfo>, HashMap<String, Vec<NodeInfo>>), LightningError> {


Does this need to be a vec?

You are right. It doesn't have to be a vec anymore since we aren't allowing duplicates in the user-provided aliases.

carlaKC · 2025-05-01T13:50:29Z

sim-cli/src/parsing.rs

+    // An alias can be mapped to multiple nodes because it is not a unique identifier.
+    let mut graph_nodes_by_alias: HashMap<String, Vec<NodeInfo>> = HashMap::new();
+
+    // sort graph nodes by alias for easy lookups.


nit: unnecessary comment, can delete

carlaKC · 2025-05-01T13:51:00Z

sim-cli/src/parsing.rs

-    alias_node_map: HashMap<String, NodeInfo>,
-    get_node_info: impl AsyncFn(&PublicKey) -> Result<NodeInfo, LightningError>,
+    alias_node_map: HashMap<String, Vec<NodeInfo>>,
+    mut graph: Graph,


nit: let's pass in the hashmap that's ready for lookups here (like we do for the other maps)

This would likely require passing in 2 hashmaps - one keyed by pk and the other by alias; we'd have 5 params in total. If this is fine by you then let's do it (or maybe use a struct?).

Struct sounds good 👍

carlaKC · 2025-05-01T13:57:33Z

simln-lib/src/sim_node.rs

@@ -441,6 +442,8 @@ trait SimNetwork: Send + Sync {

    /// Looks up a node in the simulated network and a list of its channel capacities.
    async fn lookup_node(&self, node: &PublicKey) -> Result<(NodeInfo, Vec<u64>), LightningError>;
+    /// fetches all nodes in the simulated network


nit: capitals for docs, applies in a few places

carlaKC · 2025-05-01T13:59:23Z

simln-lib/src/lib.rs

+    pub fn get_nodes(&mut self) -> Result<HashMap<PublicKey, NodeInfo>, LightningError> {
+        Ok(self.nodes_by_pk.clone())
+    }


This seems like an unnecessary clone to me (for a data struct that could have around 16k entries in it).

I think it's okay to make nodes_by_pk pub and then callers can borrow rather than needing to copy all this data + have specialized getters.

carlaKC · 2025-05-01T14:03:42Z

sim-cli/src/parsing.rs

+                if let Some(node_infos) = alias_node_map.get(a) {
+                    node_infos[0].clone()


I don't see why we need to allow multiple node_infos in our alias_node_map when that's a map of the aliases that the user specified and we validated to make sure are unique?

graph_nodes_by_alias makes sense as a vec, I like the impl, but we don't need to change both.

Yes, not making alias_node_map structured as a vec of node_infos will fix this.

carlaKC · 2025-05-01T14:04:35Z

README.md

-described above). Activity sources and destinations may reference the 
-`id` defined in `nodes`, but destinations that are not listed in `nodes` 
-*must* provide a valid public key.
+Nodes can be identified by their public key or an id string (as described above). Activity sources and destinations may reference the `id` defined in `nodes`. However, if multiple destination nodes have the same id string (alias), valid public keys *must* be used to identify the nodes.


nit: line wrapping incorrect

carlaKC · 2025-05-01T14:11:21Z

README.md

-described above). Activity sources and destinations may reference the 
-`id` defined in `nodes`, but destinations that are not listed in `nodes` 
-*must* provide a valid public key.
+Nodes can be identified by their public key or an id string (as described above). Activity sources and destinations may reference the `id` defined in `nodes`. However, if multiple destination nodes have the same id string (alias), valid public keys *must* be used to identify the nodes.


Would also like to rephrase this a bit.

Activity sources must reference an id defined in nodes, because the simulator can
only send payments from nodes that it controls. Destinations may reference either an
id defined in nodes or provide a pubkey or alias of a node in the public network.
If the alias provided is not unique in the public network, a pubkey must be used
to identify the node.

chuksys commented Apr 17, 2025

View reviewed changes

simln-lib/src/sim_node.rs Outdated Show resolved Hide resolved

simln-lib/src/eclair.rs Outdated Show resolved Hide resolved

sim-cli/src/parsing.rs Outdated Show resolved Hide resolved

chuksys marked this pull request as ready for review April 22, 2025 01:25

carlaKC requested changes Apr 22, 2025

View reviewed changes

chuksys added 2 commits April 30, 2025 20:36

sim-ln/docs: Update README for activity alias usage

9fe6a15

chuksys force-pushed the allow-unspecified-alias-destinations branch from 3b865f6 to 9fe6a15 Compare April 30, 2025 19:42

chuksys requested a review from carlaKC April 30, 2025 20:41

carlaKC requested changes May 1, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow unspecified alias destinations #254

Allow unspecified alias destinations #254

chuksys commented Apr 17, 2025

chuksys left a comment

carlaKC left a comment •

edited

Loading

chuksys commented Apr 23, 2025

carlaKC left a comment

carlaKC May 1, 2025

carlaKC May 1, 2025

carlaKC May 1, 2025

chuksys May 2, 2025

carlaKC May 1, 2025

carlaKC May 1, 2025

chuksys May 2, 2025

carlaKC May 2, 2025

carlaKC May 1, 2025

carlaKC May 1, 2025

carlaKC May 1, 2025

chuksys May 2, 2025

carlaKC May 1, 2025

carlaKC May 1, 2025

		if let Some(node_infos) = alias_node_map.get(a) {
		node_infos[0].clone()

Allow unspecified alias destinations #254

Are you sure you want to change the base?

Allow unspecified alias destinations #254

Conversation

chuksys commented Apr 17, 2025

Description

Changes

chuksys left a comment

Choose a reason for hiding this comment

carlaKC left a comment • edited Loading

Choose a reason for hiding this comment

chuksys commented Apr 23, 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

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 left a comment •

edited

Loading