Skip to content
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

Support LND channel graph format #22

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

VasanthManiVasi
Copy link

This adds support for the ChannelGraph in lnd format.

It converts the lnd format to cln format and then loads the graph

Copy link
Owner

@renepickhardt renepickhardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks very good! besides the missing fields I have only a few stylistic suggestions and tiny questions.

I also very much like how you made it backwards compatible. I was initially thinking of having also a LNDCHannel Base class from which all the others derrive (and I did not like my idea which is why I didn't support lnd yet). but I think the way it is being done here is much more elegant!

Btw does anyone know if there is a checker available that something is the correct lnd or cln json format? If it was we could basically do the check before conversion but I guess that is really unneccesary as the JSON might very well change at some point in time

pickhardtpayments/ChannelGraph.py Show resolved Hide resolved
pickhardtpayments/ChannelGraph.py Show resolved Hide resolved
pickhardtpayments/ChannelGraph.py Outdated Show resolved Hide resolved
pickhardtpayments/ChannelGraph.py Outdated Show resolved Hide resolved
pickhardtpayments/ChannelGraph.py Outdated Show resolved Hide resolved
pickhardtpayments/ChannelGraph.py Outdated Show resolved Hide resolved
pickhardtpayments/ChannelGraph.py Outdated Show resolved Hide resolved
@VasanthManiVasi
Copy link
Author

Thanks a lot for the review and suggestions!

DescribeGraph.json in LND does not seem to provide channel flags and message flags. Should it be None or set to 0 (since they're bit flags)?

For features, since they're not provided along with the RoutingPolicy of each node in an LND channel edge, we have to search for the corresponding channel node(s) by pubkey in the "nodes" field of DescribeGraph.json (which can be potentially slow as we have to search nodes everytime for each channel direction that we are adding), and then reverse map the dict to feature bits following this: https://github.com/lightningnetwork/lnd/blob/master/lnwire/features.go
Could you please share your thoughts about it?

@renepickhardt
Copy link
Owner

DescribeGraph.json in LND does not seem to provide channel flags and message flags. Should it be None or set to 0 (since they're bit flags)?

I think the python way of doing it would be None though I am pretty sure that the message flag are not of importance for us. Maybe we should just remove them from Channel? I just wrapped everything that I saw from cln without thinking. So I guess the best is to use None for now and maybe even deprecate this property.

For features, since they're not provided along with the RoutingPolicy of each node in an LND channel edge, we have to search for the corresponding channel node(s) by pubkey in the "nodes" field of DescribeGraph.json (which can be potentially slow as we have to search nodes everytime for each channel direction that we are adding), and then reverse map the dict to feature bits following this: https://github.com/lightningnetwork/lnd/blob/master/lnwire/features.go

yeah I think we could just parse the nodes from describe graph and get the features of the nodes and do it the way that you suggested. I think runtime of the import is not a concern (unless we get really slow) as that should hopefully happen only once.

@VasanthManiVasi
Copy link
Author

Yup, I think it would be better to remove channel fields that are not needed for us..
I could be wrong but I think message flags, channel flags and features might not contribute much information for pickhardt payments, so can they be safely removed from ChannelFields?

@renepickhardt
Copy link
Owner

let us keep the features. When we upgrade channels to PTLCs we might be in a situation that we can only send the payment via legacy channels or PTLC channels. While it is not clear how we upgrade I would assume a feature bit will be set so Ican forsee why this will be useful in the futre. but we can remove messages I think

@VasanthManiVasi
Copy link
Author

I've removed channel flags and added support for features, but it's very slow as it has to linearly search for nodes (to get their features) each time when adding channels.
It took 162.63 seconds for a channel graph with 13536 nodes and 74496 channels. @renepickhardt

Copy link
Owner

@renepickhardt renepickhardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can index nodes in a dict and avoid linear pass through the list for every time that you want to fetch and element.

cln_channel[LND_CLN_POLICY_MAP[key]] = val

def _find_node(pubkey, nodes_list):
for node in nodes_list:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't you initially just load all nodes into a dict with pubkey as index and then just fetch them instead of going linearly through the list everytime you need it?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 9c733e1

@VasanthManiVasi
Copy link
Author

Woops, forgot about that.. Thanks!

@VasanthManiVasi VasanthManiVasi changed the title [WIP] Support lnd format in ChannelGraph Support LND channel graph format May 19, 2022
@renepickhardt
Copy link
Owner

LGTM. Thank you very much! Need to run / test code. Not sure if I will be able to merge this week.

If anyone else wants to do a review that will be great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants