Add some helpers for routing debug #566
Conversation
lightning/src/ln/router.rs
Outdated
| @@ -346,6 +346,58 @@ pub struct RouteHint { | |||
| pub htlc_minimum_msat: u64, | |||
| } | |||
|
|
|||
| /// Details of a channel, as returned by Router::list_graph | |||
There was a problem hiding this comment.
Why add new structs? We already store them in the message format, why not just return those?
There was a problem hiding this comment.
Right made them public, but added their identifier for each
lightning/src/ln/router.rs
Outdated
|
|
||
| /// Gets the list of announced channels/nodes, in random order. See EdgeDetails and VerticeDetails field documentation for | ||
| /// more information. | ||
| pub fn list_graph(&self) -> (Vec<EdgeDetails>, Vec<VerticeDetails>) { |
There was a problem hiding this comment.
Forcing the user into Vecs kinda sucks, maybe return an iterator (which the user can convert to a Vec if they want)?
There was a problem hiding this comment.
Split in two helpers, dunno about Iterator, we don't have collection of ChannelInfo/NodeInfo on which to iterate on, do you suggest to return HashMap/Btree in which we already store them ?
Note we already return a Vec for ChannelManager::list_channels
ariard
force-pushed
the
2020-03-router-helper
branch
2 times, most recently
from
4067c4a to
7b218dc
Compare
Next commit expose publicly structure to allow displaying network graph. Even if we already index by short_channel_id in NetworkMap, an exposed identifier makes exposition more useful.
Make ChannelInfo and its fields public.
Next commit expose publicly structure to alow displaying network graph. Even if we already index by node_id in NetworkMap, an exposed identifier makes exposition more useful.
Make NodeInfo and its fields publics
ariard
force-pushed
the
2020-03-router-helper
branch
from
7b218dc to
005975d
Compare
| @@ -1049,6 +1085,28 @@ impl Router { | |||
|
|
|||
| Err(LightningError{err: "Failed to find a path to the given destination", action: ErrorAction::IgnoreError}) | |||
| } | |||
|
|
|||
| /// Gets the list of announced channels, in random order. See ChannelInfo details field documentation for more information | |||
| pub fn list_edges(&self) -> Vec<ChannelInfo> { | |||
There was a problem hiding this comment.
Oops, I didnt mean to expose the internal representation, I meant to expose the last_update_message field (if it is Some). Sadly, thats maybe not perfect here since we sometimes dont include it, but exposing the fields as-is is also not OK, since channel info is initialized with dummy values until we receive the first channel_updates - if we're gonna expose them they need to be Option<>al.
| let network = self.network_map.read().unwrap(); | ||
| edges.reserve(network.channels.len()); | ||
| for (_, chan) in network.channels.iter() { | ||
| edges.push((*chan).clone()); |
There was a problem hiding this comment.
The whole point in using an internal representation was to avoid copying the whole thing. Just return network.channels.iter() itself. (this also means you dont have to include the short_channel_ids duplicatively in memory).
There was a problem hiding this comment.
But you can't return a reference to network here as it's under a lock and you can't be sure than lock taking is going to be successful?
There was a problem hiding this comment.
Grrrr, I forgot you can't return the lock and an interator from inside it :/. Can you remove the commit that adds the short_channel_id and just return (u64, ChannelInfo)?
|
Closing gonna be superseded by #592 |
Second commit is used to implement
list_graphin sample, quite useful to debug path finding. But maybeEdgeDetailsandVerticeDetailsexpose to much information.