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

Gossip rework part 2 #6904

Merged

Conversation

rustyrussell
Copy link
Contributor

@rustyrussell rustyrussell commented Nov 29, 2023

This builds on #6869 (the first 24 commits!). Merged into master.

This series unifies the gossip generation in lightningd. Previously channeld created and verified channel_announcement, and gossipd created channel_update and node_announcement messages. This, once complete, finally lets us remove the last traces of private gossip from gossipd.

  1. The first 10 commits generalize various tests so they don't break.
  2. The next 9 commits make sure we're passing the information we need around internally for channel_update/announcement.
  3. The next 3 commits put channel_update/announcement generation and management in lightningd, then switch it across.
  4. The next 6 commits rip out now-unused code from channeld, dualopend and gossipd.
  5. The next 9 commits re-add checking peer announcement signatures, and move node announcement generation into lightningd.
  6. The final 5 commits add a "channel is reestablished successfully" hook from channeld, and use it in various ways.

@rustyrussell rustyrussell added this to the v24.02 milestone Nov 29, 2023
@rustyrussell rustyrussell force-pushed the guilt/fix-empty-update branch 4 times, most recently from 5ffa94a to 5425571 Compare December 6, 2023 06:44
@rustyrussell rustyrussell force-pushed the guilt/fix-empty-update branch 3 times, most recently from e687a99 to 1cb04ca Compare December 13, 2023 22:56
@rustyrussell rustyrussell force-pushed the guilt/fix-empty-update branch 2 times, most recently from fec73cd to 1f1d7e6 Compare December 14, 2023 06:21
Copy link
Collaborator

@endothermicdev endothermicdev left a comment

Choose a reason for hiding this comment

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

Looks great - this was fun to review. Most of my complaints were addressed a few commits later. Also great to see all the improvements that already result from this.

lightningd/test/run-invoice-select-inchan.c Outdated Show resolved Hide resolved
lightningd/channel_control.c Show resolved Hide resolved
lightningd/gossip_generation.c Show resolved Hide resolved
lightningd/channel_gossip.h Outdated Show resolved Hide resolved
lightningd/channel_gossip.c Outdated Show resolved Hide resolved
lightningd/channel_gossip.c Outdated Show resolved Hide resolved
tests/test_gossip.py Show resolved Hide resolved
lightningd/gossip_generation.c Outdated Show resolved Hide resolved
tests/test_connection.py Outdated Show resolved Hide resolved
@rustyrussell rustyrussell force-pushed the guilt/fix-empty-update branch 3 times, most recently from 2996dc5 to aab2dd5 Compare January 24, 2024 05:52
Copy link
Collaborator

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

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

From the lnprototest failure looks like that cln will not send the announcement_signatures after the channel_ready (this is the only case where we check announcement_signatures in lnprototest). It is not wrong because the message is optional, but it is a breaking change for lnprototest

These are the incriminated log

21:34:57.233Z DEBUG   gossipd: REPLY WIRE_GOSSIPD_NEW_BLOCKHEIGHT_REPLY with 0 fds
2024-01-24T21:34:57.234Z DEBUG   02c6047f9441ed7d6d3045406e95c07cd85c778e4b8cef3ca7abac09b95c709ee5-channeld-chan#1: handle_funding_depth: Setting short_channel_ids[LOCAL] to 103x1x0
2024-01-24T21:34:57.234Z DEBUG   02c6047f9441ed7d6d3045406e95c07cd85c778e4b8cef3ca7abac09b95c709ee5-channeld-chan#1: billboard: Channel ready for use.
2024-01-24T21:34:57.234Z 02c6047f9441ed7d6d3045406e95c07cd85c778e4b8cef3ca7abac09b95c709ee5-connectd: [OUT] 0103a37362839b13f61cfe82d35bd397b1264c389b245847cfb6111b38892546dc77000067000001000029f6c4b55f47473f880633f4528bf1d0fdff2a92c8284f0df4bf685413440b0e71716658891c6258df6abf32448b74fb17628bacfa03870b1683dded2db0d49e48c168d1b9cafba8cd35217f36d97fdc6627cf79c7ab5be0fb1a1daf0b3f46a716c11b72fd86f9956d6bd6ea7ea527b059c1da60a4c81bc08abd3b59e30a27b1
2024-01-24T21:34:57.385Z jsonrpc#17: [IN] 7b226a736f6e727063223a2022322e30222c20226d6574686f64223a2022676574696e666f222c2022706172616d73223a207b7d2c20226964223a2022707974
2024-01-24T21:34:57.385Z jsonrpc#17: [IN] 6573743a676574696e666f233135227d
2024-01-24T21:34:57.385Z jsonrpc#17: "pytest:getinfo#15"[IN]
2024-01-24T21:34:57.385Z jsonrpc#17: [OUT] 7b226a736f6e727063223a22322e30222c226964223a227079746573743a676574696e666f233135222c22726573756c74223a7b226964223a22303237396265363637656639646362626163353561303632393563653837306230373032396266636462326463653238643935396632383135623136663831373938222c22616c696173223a2253494c454e54535041574e2d2d3137352d67616162326464352d6d6f64646564222c22636f6c6f72223a22303237396265222c226e756d5f7065657273223a312c226e756d5f70656e64696e675f6368616e6e656c73223a302c226e756d5f6163746976655f6368616e6e656c73223a312c226e756d5f696e6163746976655f6368616e6e656c73223a302c2261646472657373223a5b5d2c2262696e64696e67223a5b7b2274797065223a2269707634222c2261646472657373223a223132372e302e302e31222c22706f7274223a34323732397d5d2c2276657273696f6e223a227632332e31312d3137352d67616162326464352d6d6f64646564222c22626c6f636b686569676874223a3131312c226e6574776f726b223a2272656774657374222c22666565735f636f6c6c65637465645f6d736174223a302c226c696768746e696e672d646972223a222f746d702f6c6e70742d636c2d636935733538766a2f6c696768746e696e67642f72656774657374222c226f75725f6665617475726573223a7b22696e6974223a223038613030303061306136396132222c226e6f6465223a223838613030303061306136396132222c226368616e6e656c223a22222c22696e766f696365223a223032303030303032303234313030227d7d7d0a0a
2024-01-24T21:34:57.943Z lightningd: "cln:estimatefees#80"[OUT]
2024-01-24T21:34:57.962Z plugin-bcli: [IN] 7b226a736f6e727063223a22322e30222c226964223a22636c6e3a657374696d61746566656573233830222c22726573756c74223a7b226665657261746573223a5b7b22626c6f636b73223a322c2266656572617465223a313030307d2c7b22626c6f636b73223a362c2266656572617465223a313030307d2c7b22626c6f636b73223a31322c2266656572617465223a313030307d2c7b22626c6f636b73223a3130302c2266656572617465223a313030307d5d2c22666565726174655f666c6f6f72223a313030307d7d0a0a
2024-01-24T21:34:58.234Z lightningd: "cln:getrawblockbyheight#81"[OUT]
2024-01-24T21:34:58.238Z plugin-bcli: [IN] 7b226a736f6e727063223a22322e30222c226964223a22636c6e3a676574726177626c6f636b6279686569676874233831222c22726573756c74223a7b22626c6f636b68617368223a6e756c6c2c22626c6f636b223a6e756c6c7d7d0a0a
2024-01-24T21:34:58.962Z lightningd: "cln:estimatefees#82"[OUT]
2024-01-24T21:34:58.980Z plugin-bcli: [IN] 7b226a736f6e727063223a22322e30222c226964223a22636c6e3a657374696d61746566656573233832222c22726573756c74223a7b226665657261746573223a5b7b22626c6f636b73223a322c2266656572617465223a313030307d2c7b22626c6f636b73223a362c2266656572617465223a313030307d2c7b22626c6f636b73223a31322c2266656572617465223a313030307d2c7b22626c6f636b73223a3130302c2266656572617465223a313030307d5d2c22666565726174655f666c6f6f72223a313030307d7d0a0a
2024-01-24T21:34:59.238Z lightningd: "cln:getrawblockbyheight#83"[OUT]
2024-01-24T21:34:59.241Z plugin-bcli: [IN] 7b226a736f6e727063223a22322e30222c226964223a22636c6e3a676574726177626c6f636b6279686569676874233833222c22726573756c74223a7b22626c6f636b68617368223a6e756c6c2c22626c6f636b223a6e756c6c7d7d0a0a
2024-01-24T21:34:59.915Z DEBUG   gossipd: seeker: no peers, waiting
2024-01-24T21:34:59.981Z lightningd: "cln:estimatefees#84"[OUT]

I am missing somethings? I need to play more with the code to provide a patch, but maybe for you it is more easy to understand why this is happening now?

@rustyrussell
Copy link
Contributor Author

I don't know: I cannot get lnprototest to run locally. It is failing on the "stop" command with:

>           self.sock.connect(str(self.path))
E           FileNotFoundError: [Errno 2] No such file or directory

../../contrib/pyln-client/pyln/client/lightning.py:239: FileNotFoundError

I will submit a PR to remove lnprototest from the tree. We don't have the resources to maintain it, and it's getting worse. It needs to be completely rewritten from scratch and I don't be doing that this year.

@rustyrussell
Copy link
Contributor Author

Rebased (again!) and removed overzealous assert.

@rustyrussell rustyrussell force-pushed the guilt/fix-empty-update branch 2 times, most recently from 6ff4e80 to 109a90d Compare January 30, 2024 03:40
@rustyrussell
Copy link
Contributor Author

Rebased onto #7025 which does unrelated fixes.

At this point it needs a complete rewrite to be useful, and it's just
constraining development.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We want this later, so do the prep work now.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Before this it was channeld doing it, which was tied to a particular
channel.  Create an API for lightningd to sign for any channel.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
… for our channels.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This commit is a bit messy, but it tries to do the minimal switchover.

Some tests change, so those are included here. 

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We don't have to generate these any more: lightningd does it.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We don't have to generate these any more: lightningd does it.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Lightningd now handles private channels, so we're dismantling the
gossipd infrastructure.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
…ndling.

Now lightningd just doesn't tell us about private channels, doesn't
expect us to generate channel gossip, and tells us about public channels
via the addgossip call, we don't need any of this in gossipd.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We hoise check_signed_hash_nodeid from gossipd's internals, into common/.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This is currently still done in gossipd, but we should generate it.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
It might be redundant.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Rather than take the first two from peers with committed channels, use
the most common address given by at least 2 peers, and accept the majority
from non-committed peers if there are no committed peers.

This works well with the node_announcement rework, which waits until
everyone has a chance to connect before creating the node_announcement.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
It then waits 10 more seconds (for plugins to call setlease, especially)
before it will update a node_announcement.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
In particular, we were sending `announcement_signatures` before
`channel_reestablish`; we allow this because LND used to do it, but
it's not spec compliant.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This is useful to see if we've really made progress, or just bounced
off the peer (e.g. it doesn't know the channel, or we have fallen
behind somehow).

Changelog-Added: JSON-RPC: `listpeerchannels` new field `reestablished` set once we've exchanged `channel_reestablish` messages.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
…lished.

This is a nice reflection of channel stability: in particular, worse case
ping time is 45 seconds, so we should have had some traffic.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This isuseful to find completely dead channels which are worthy of
closing.

Changelog-Added: JSON-RPC: `listpeerchannels` field `last_stable_connection` showing when we last held an established channel for a minute or more.
Changelog-Added: JSON-RPC: `listclosedchannels` field `last_stable_connection` showing when we last held an established channel for a minute or more.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
```
gossipd: MEMLEAK: 0x55e22ac0f8e8
gossipd:   label=gossipd/seeker.c:621:u8
gossipd:   alloc:
gossipd:     ccan/ccan/tal/tal.c:477 (tal_alloc_)
gossipd:     gossipd/seeker.c:621 (check_timestamps)
gossipd:     gossipd/seeker.c:645 (process_scid_probe)
gossipd:     gossipd/queries.c:849 (handle_reply_channel_range)
gossipd:     gossipd/gossipd.c:436 (handle_recv_gossip)
gossipd:     gossipd/gossipd.c:509 (connectd_req)
gossipd:     common/daemon_conn.c:35 (handle_read)
gossipd:     ccan/ccan/io/io.c:59 (next_plan)
gossipd:     ccan/ccan/io/io.c:407 (do_plan)
gossipd:     ccan/ccan/io/io.c:417 (io_ready)
gossipd:     ccan/ccan/io/poll.c:453 (io_loop)
gossipd:     gossipd/gossipd.c:950 (main)
gossipd:   parents:
gossipd:     gossipd/seeker.c:132:struct seeker
```

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We mine blocks too fast, and l3 discard the channel_announcment as too far in the future:

```
lightningd-3 2023-12-14T06:40:04.744Z DEBUG   0266e4598d1d3c415f572a8488830b60f7e744ed9235eb0b1ba93283b315c03518-gossipd: Ignoring future channel_announcment for 103x1x1 (current block 102)
```

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
…nect.

```
2024-01-29T21:26:50.9785559Z lightningd-1 2024-01-29T21:14:09.709Z DEBUG   022d223620a359a47ff7f7ac447c85c46c923da53389221a0054c11c1e3ca31d59-gossipd: Ignoring future channel_announcment for 110x1x0 (current block 109)
2024-01-29T21:26:50.9786945Z lightningd-1 2024-01-29T21:14:09.710Z UNUSUAL lightningd: Bad gossip order: could not find channel 110x1x0 for peer's channel update
```

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This caused a flake in test_gossip_lease_rates, since the peer would ignore
the node_announcement due to duplicate timestamps!

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell rustyrussell enabled auto-merge (rebase) January 31, 2024 03:47
@rustyrussell rustyrussell merged commit 1f9e977 into ElementsProject:master Jan 31, 2024
30 of 41 checks passed
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.

None yet

3 participants