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

pyln: gossmap ... all the way #6012

Merged
merged 15 commits into from
Apr 4, 2023

Conversation

m-schmoock
Copy link
Collaborator

@m-schmoock m-schmoock commented Feb 14, 2023

I did some work on the python gossmap implementation.
Let's see how it goes...

Update: I added a lot of missing and useful functionality to operate-on/analyse gossip stores and also use that for plugins that do network analysis and optimization.

  • adds missing parsing of node_announcement metadata addresses, alias, rgb, ...
  • add a get_nodes() methods that can return filtered subsets of the graph.
  • adds a get_halfchannels() method that can be used to return an area of channels with a given distance (depth) towards or from a certain node
  • adds various predefined and parametrable filters (channel_capacity, channel_count, nodes_address_type, ...)
  • print stats of a loaded store, useful for debugging
  • optimized lookups and hashes
  • a more extensive python gossmap testcase
  • various cleanups

@m-schmoock m-schmoock changed the title chore: cleanups pyln: gossmap Feb 14, 2023
@m-schmoock m-schmoock force-pushed the chore/cleanups branch 9 times, most recently from f7db3d4 to 6544bfe Compare February 16, 2023 16:02
@m-schmoock m-schmoock marked this pull request as ready for review February 16, 2023 16:07
@m-schmoock m-schmoock force-pushed the chore/cleanups branch 2 times, most recently from d862fb1 to 4888e96 Compare February 16, 2023 17:05
Copy link
Member

@cdecker cdecker left a comment

Choose a reason for hiding this comment

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

Forgot to submit my reviews, not to mention actually finish reviewing :-) Anyway, I'll pick this up asap

contrib/pyln-client/pyln/client/gossmap.py Outdated Show resolved Hide resolved
contrib/pyln-client/pyln/client/gossmap.py Outdated Show resolved Hide resolved
Copy link
Member

@cdecker cdecker left a comment

Choose a reason for hiding this comment

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

Very nice PR, with some interesting tricks in there, thanks :-)
I found a couple minor things, but this could go forward as is.

tests/test_misc.py Outdated Show resolved Hide resolved
tests/test_misc.py Outdated Show resolved Hide resolved
tests/test_misc.py Outdated Show resolved Hide resolved
contrib/pyln-client/pyln/client/gossmap.py Show resolved Hide resolved
contrib/pyln-client/pyln/client/gossmap.py Outdated Show resolved Hide resolved
contrib/pyln-client/pyln/client/gossmap.py Outdated Show resolved Hide resolved
contrib/pyln-client/pyln/client/gossmap.py Show resolved Hide resolved
contrib/pyln-client/pyln/client/gossmap.py Show resolved Hide resolved
contrib/pyln-client/pyln/client/gossmap.py Outdated Show resolved Hide resolved
contrib/pyln-client/pyln/client/gossmap.py Show resolved Hide resolved
@m-schmoock m-schmoock force-pushed the chore/cleanups branch 5 times, most recently from 99ee3a4 to 210efa1 Compare February 17, 2023 11:59
@m-schmoock
Copy link
Collaborator Author

@cdecker Applied your suggestions (except the wait_for_mempool).
Do you have additional ideas on e.g. graph operations or path finding we could add into python gossmap?

I was thinking about adding a constraints paramter to operations such get_halfchannels that would allow us to limit the channels it uses e.g. by requiring a minimum total channel amount and such.

This can even be made in a way that when a callback/lambda is passed it applies this to all half_channels it comes across to find out if a channel should be considered or not. This way a user/programmer can descide to filter on anything it wants.

@m-schmoock
Copy link
Collaborator Author

Or maybe add some basic routing :D

Currently I'm testing this beefed up gossmap implementation on mainnet data

@m-schmoock
Copy link
Collaborator Author

m-schmoock commented Feb 17, 2023

This looks promising, when I load full mainnet data, the initial load takes some time ofc, but complex querying is good:

loaded 15173 nodes and 74856 channels.execution time: 7.362061977386475 seconds
get_halfchannels(depth=0) on my node gave 31 channels took:  8.654594421386719e-05 seconds
get_halfchannels(depth=1) on my node gave 10243 channels took:  0.022817611694335938 seconds

Maybe we can improve initial load performance somehow...

@cdecker
Copy link
Member

cdecker commented Feb 17, 2023

Looking good 👍 That initial load time can likely be reduced, but profiles will likely tell us how. The C gossmap is rather clever in that it creates an index over the mmaped file, but we should likely get close to its performance, though not its memory efficiency :-)

@m-schmoock m-schmoock force-pushed the chore/cleanups branch 2 times, most recently from 74b8395 to e0f3009 Compare February 18, 2023 08:19
@m-schmoock m-schmoock marked this pull request as draft February 18, 2023 12:14
@m-schmoock
Copy link
Collaborator Author

@cdecker thanks, I applied a lot from your suggestions directly and resolved them. Some are still open, there I applied what I can and asked some questions back on others...

Comment on lines 214 to 234
def has_feature(self, bit):
return 1 << bit & self.features > 0

def has_feature_odd(self, bit):
return 2 << bit & self.features > 0

def has_features(self, bitmask):
return bitmask & self.features == bitmask
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think these functions are generally useful. Most commonly, you want all nodes which support feature_foo, you want "features & (3 << foo) != 0". Call that "has_feature()". Then you can have "has_compulsory_feature()" and "has_optional_feature()".

has_features() should almost certainly use the "has_feature" logic, too.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please fix this. This API is terrible!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes I did that now, can you re-check?

contrib/pyln-client/pyln/client/gossmap.py Outdated Show resolved Hide resolved
contrib/pyln-client/pyln/client/gossmap.py Outdated Show resolved Hide resolved
contrib/pyln-client/pyln/client/gossmap.py Outdated Show resolved Hide resolved
contrib/pyln-client/pyln/client/gossmap.py Outdated Show resolved Hide resolved
@rustyrussell
Copy link
Contributor

Oh, and needs a Changelog-Added indicating that pyln.client.gossmap has seen significant functionality improvements.

@rustyrussell
Copy link
Contributor

I rebased again (trivial, to remove conflict in test_misc.py) before I realized you still didn't:

  1. Change has_feature API to make sense.
  2. Add Changelog-Changed to highlight these significant improvements in pyln.client.gossmap.

Also, the commit message for pytest: adds skipped test_create_gossip_mesh is wonky, and has Changelog-None which is suppressing our changelog detection. (Generally, I prefer Changelog-None only to appear in the GH PR description, otherwise it'd be on almost every commit!).

@rustyrussell rustyrussell self-requested a review March 25, 2023 05:08
Copy link
Contributor

@rustyrussell rustyrussell left a comment

Choose a reason for hiding this comment

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

A little more cleanup, see comment....

This method is no longer used in cln nor in the plugins repo.

Changelog-None
This can be adapted and used to create test gossip stores.
The test is just skipped by design as it would fail on intention.
 - moves offset into GossipHeader hdr which is passed to all constuctors
 - reads .flags as u16 instead of extracting it from the .length, see 0274d88
 - adds zombie and ratelimit flag to GossipHeader
 - bytes_read start at 0 instead of 1 which is more correct,
   the one byte is then corrected for when setting the offset of new header.
 - bytes_read is increased in pull_bytes as this is the only place where
   something is read
 - use new style for various format-strings
Also caches certain __hash__ and __str__ operations,
This way graph operations can be done quicker.
also makes them acessible using bitmask functions
@m-schmoock m-schmoock force-pushed the chore/cleanups branch 2 times, most recently from 48effcb to b9e1a3a Compare March 26, 2023 10:13
Includes a lot of useful filters and statistical methods.

To see a gossip_store summary:
```
s = GossmapStats(g)
s.print_stats()
```
Changelog-Added: pyln-client: Improvements on the gossmap implementation
@rustyrussell
Copy link
Contributor

Excellent!

Ack 4d199b7

@rustyrussell rustyrussell merged commit 04ea37d into ElementsProject:master Apr 4, 2023
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.

3 participants