-
Notifications
You must be signed in to change notification settings - Fork 37
Allow unspecified alias destinations #254
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -248,10 +248,7 @@ The example simulation file below sets up the following simulation: | |
``` | ||
|
||
|
||
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`, 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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would also like to rephrase this a bit. Activity sources must reference an |
||
|
||
### Simulation Output | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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, | ||
chuksys marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Commit message still doesn't wrap at 72 |
||
Simulation, SimulationCfg, WriteResults, | ||
}; | ||
use std::collections::HashMap; | ||
use std::fs; | ||
use std::ops::AsyncFn; | ||
use std::path::PathBuf; | ||
use std::sync::Arc; | ||
use tokio::sync::Mutex; | ||
|
@@ -159,20 +158,15 @@ pub async fn create_simulation(cli: &Cli) -> Result<Simulation, anyhow::Error> { | |
let (clients, clients_info) = get_clients(nodes).await?; | ||
// We need to be able to look up destination nodes in the graph, because we allow defined activities to send to | ||
// nodes that we do not control. To do this, we can just grab the first node in our map and perform the lookup. | ||
let get_node = async |pk: &PublicKey| -> Result<NodeInfo, LightningError> { | ||
if let Some(c) = clients.values().next() { | ||
return c.lock().await.get_node_info(pk).await; | ||
} | ||
|
||
Err(LightningError::GetNodeInfoError( | ||
"no nodes for query".to_string(), | ||
)) | ||
let graph = match clients.values().next() { | ||
Some(client) => client.lock().await.get_graph().await?, | ||
None => panic!("Graph is empty"), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Return an error rather than panicing? |
||
}; | ||
|
||
let (pk_node_map, alias_node_map) = add_node_to_maps(&clients_info).await?; | ||
|
||
let validated_activities = | ||
validate_activities(activity, pk_node_map, alias_node_map, get_node).await?; | ||
validate_activities(activity, pk_node_map, alias_node_map, graph).await?; | ||
let tasks = TaskTracker::new(); | ||
|
||
Ok(Simulation::new(cfg, clients, validated_activities, tasks)) | ||
|
@@ -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> { | ||
chuksys marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this need to be a vec? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are right. It doesn't have to be a vec anymore since we aren't allowing duplicates in the user-provided aliases. |
||
let mut pk_node_map = HashMap::new(); | ||
let mut alias_node_map = HashMap::new(); | ||
let mut alias_node_map: HashMap<String, Vec<NodeInfo>> = HashMap::new(); | ||
|
||
for node_info in nodes.values() { | ||
log::info!( | ||
|
@@ -241,7 +235,7 @@ async fn add_node_to_maps( | |
))); | ||
} | ||
|
||
alias_node_map.insert(node_info.alias.clone(), node_info.clone()); | ||
alias_node_map.insert(node_info.alias.clone(), vec![node_info.clone()]); | ||
} | ||
|
||
pk_node_map.insert(node_info.pubkey, node_info.clone()); | ||
|
@@ -255,31 +249,72 @@ async fn add_node_to_maps( | |
async fn validate_activities( | ||
activity: Vec<ActivityParser>, | ||
pk_node_map: HashMap<PublicKey, NodeInfo>, | ||
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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: let's pass in the hashmap that's ready for lookups here (like we do for the other maps) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Struct sounds good 👍 |
||
) -> Result<Vec<ActivityDefinition>, LightningError> { | ||
let mut validated_activities = vec![]; | ||
|
||
// Store graph nodes' information keyed by their alias. | ||
// 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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: unnecessary comment, can delete |
||
for node in graph.get_nodes()? { | ||
if graph_nodes_by_alias.contains_key(&node.1.alias) { | ||
if let Some(node_infos) = graph_nodes_by_alias.get(&node.1.alias) { | ||
let mut updated_node_infos = node_infos.clone(); | ||
updated_node_infos.extend(vec![node.1.clone()]); | ||
graph_nodes_by_alias.insert(node.1.alias.clone(), vec![node.1]); | ||
} | ||
} else { | ||
graph_nodes_by_alias.insert(node.1.alias.clone(), vec![node.1]); | ||
} | ||
} | ||
Comment on lines
+262
to
+272
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Get a little fancier here with
|
||
|
||
// Make all the activities identifiable by PK internally | ||
for act in activity.into_iter() { | ||
// We can only map aliases to nodes we control, so if either the source or destination alias | ||
// We can only map source aliases to nodes we control, so if the source alias | ||
// is not in alias_node_map, we fail | ||
let source = if let Some(source) = match &act.source { | ||
NodeId::PublicKey(pk) => pk_node_map.get(pk), | ||
NodeId::Alias(a) => alias_node_map.get(a), | ||
} { | ||
source.clone() | ||
} else { | ||
return Err(LightningError::ValidationError(format!( | ||
"activity source {} not found in nodes.", | ||
act.source | ||
))); | ||
let source = match &act.source { | ||
NodeId::PublicKey(pk) => { | ||
if let Some(node_info) = pk_node_map.get(pk) { | ||
node_info.clone() | ||
} else { | ||
return Err(LightningError::ValidationError(format!( | ||
"activity source {} not found in simulation nodes.", | ||
act.source | ||
))); | ||
} | ||
}, | ||
NodeId::Alias(a) => { | ||
if let Some(node_infos) = alias_node_map.get(a) { | ||
node_infos[0].clone() | ||
} else { | ||
return Err(LightningError::ValidationError(format!( | ||
"activity source {} not found in simulation nodes.", | ||
act.source | ||
))); | ||
} | ||
}, | ||
Comment on lines
+278
to
+298
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that this can be reverted if |
||
}; | ||
|
||
let destination = match &act.destination { | ||
NodeId::Alias(a) => { | ||
if let Some(info) = alias_node_map.get(a) { | ||
info.clone() | ||
if let Some(node_infos) = alias_node_map.get(a) { | ||
node_infos[0].clone() | ||
Comment on lines
+303
to
+304
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see why we need to allow multiple
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, not making |
||
} else if let Some(node_infos) = graph_nodes_by_alias.get(a) { | ||
if node_infos.len() > 1 { | ||
let pks: Vec<PublicKey> = node_infos | ||
.iter() | ||
.map(|node_info| node_info.pubkey) | ||
.collect(); | ||
return Err(LightningError::ValidationError(format!( | ||
"Multiple nodes in the graph have the same destination alias - {}. | ||
Use one of these public keys as the destination instead - {:?}", | ||
a, pks | ||
))); | ||
} | ||
node_infos[0].clone() | ||
} else { | ||
return Err(LightningError::ValidationError(format!( | ||
"unknown activity destination: {}.", | ||
|
@@ -288,10 +323,15 @@ async fn validate_activities( | |
} | ||
}, | ||
NodeId::PublicKey(pk) => { | ||
if let Some(info) = pk_node_map.get(pk) { | ||
info.clone() | ||
if let Some(node_info) = pk_node_map.get(pk) { | ||
node_info.clone() | ||
} else if let Some(node_info) = graph.get_node_by_pk(*pk)? { | ||
node_info.clone() | ||
} else { | ||
get_node_info(pk).await? | ||
return Err(LightningError::ValidationError(format!( | ||
"unknown activity destination: {}.", | ||
act.destination | ||
))); | ||
} | ||
}, | ||
}; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -284,9 +284,51 @@ impl Display for NodeInfo { | |
} | ||
} | ||
|
||
#[derive(Debug, Clone)] | ||
pub struct ChannelInfo { | ||
pub channel_id: ShortChannelID, | ||
pub capacity_msat: u64, | ||
} | ||
|
||
#[derive(Debug, Clone)] | ||
/// Graph represents the network graph of the simulated network and is useful for efficient lookups. | ||
pub struct Graph { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice, prefer this representation. |
||
// Store nodes' information keyed by their public key. | ||
nodes_by_pk: HashMap<PublicKey, NodeInfo>, | ||
// Represent channels as a mapping from ShortChannelID to ChannelInfo. | ||
#[allow(dead_code)] | ||
channels: HashMap<ShortChannelID, ChannelInfo>, | ||
Comment on lines
+299
to
+300
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove this if it's not being used? We can always add it back if we need it |
||
} | ||
|
||
impl Graph { | ||
pub fn new() -> Self { | ||
Graph { | ||
nodes_by_pk: HashMap::new(), | ||
channels: HashMap::new(), | ||
} | ||
} | ||
|
||
pub fn get_nodes(&mut self) -> Result<HashMap<PublicKey, NodeInfo>, LightningError> { | ||
Ok(self.nodes_by_pk.clone()) | ||
} | ||
Comment on lines
+311
to
+313
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
|
||
pub fn get_node_by_pk( | ||
&mut self, | ||
public_key: PublicKey, | ||
) -> Result<Option<&NodeInfo>, LightningError> { | ||
Ok(self.nodes_by_pk.get(&public_key)) | ||
} | ||
} | ||
|
||
impl Default for Graph { | ||
fn default() -> Self { | ||
Self::new() | ||
} | ||
} | ||
|
||
/// LightningNode represents the functionality that is required to execute events on a lightning node. | ||
#[async_trait] | ||
pub trait LightningNode: Send { | ||
pub trait LightningNode: Send + Sync { | ||
/// Get information about the node. | ||
fn get_info(&self) -> &NodeInfo; | ||
/// Get the network this node is running at. | ||
|
@@ -308,6 +350,8 @@ pub trait LightningNode: Send { | |
/// Lists all channels, at present only returns a vector of channel capacities in msat because no further | ||
/// information is required. | ||
async fn list_channels(&mut self) -> Result<Vec<u64>, LightningError>; | ||
/// Get the network graph from the point of view of a given node. | ||
async fn get_graph(&mut self) -> Result<Graph, LightningError>; | ||
} | ||
|
||
/// Represents an error that occurs when generating a destination for a payment. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: line wrapping incorrect