Gossip cleanup part 1: Removing reliance on private gossip messages - #6869
Merged
rustyrussell merged 29 commits intoDec 13, 2023
Conversation
rustyrussell
force-pushed
the
guilt/pr-6355
branch
3 times, most recently
from
November 16, 2023 06:37
f3d9f7b to
cc0b886
Compare
rustyrussell
force-pushed
the
guilt/pr-6355
branch
2 times, most recently
from
November 29, 2023 02:26
451367c to
2b349c1
Compare
Merged
rustyrussell
force-pushed
the
guilt/pr-6355
branch
2 times, most recently
from
December 6, 2023 06:37
686164d to
af6f7a1
Compare
CI revealed one:
```
cc plugins/libplugin-pay.c
plugins/libplugin-pay.c: In function ‘payment_getroute’:
plugins/libplugin-pay.c:888:17: error: ‘errstr’ may be used uninitialized [-Werror=maybe-uninitialized]
888 | payment_fail(p, "%s", errstr);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
plugins/libplugin-pay.c:851:21: note: ‘errstr’ was declared here
851 | const char *errstr;
| ^~~~~~
cc1: all warnings being treated as errors
```
My local compiler gave another:
```
channeld/channeld.c: In function ‘resume_splice_negotiation’:
channeld/channeld.c:3734:23: error: ‘final_tx’ may be used uninitialized [-Werror=maybe-uninitialized]
3734 | msg = towire_channeld_splice_confirmed_signed(tmpctx, final_tx,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
3735 | chan_output_index);
| ~~~~~~~~~~~~~~~~~~
channeld/channeld.c:3461:28: note: ‘final_tx’ was declared here
3461 | struct bitcoin_tx *final_tx;
| ^~~~~~~~
cc1: all warnings being treated as errors
make: *** [Makefile:298: channeld/channeld.o] Error 1
```
So fix both.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Created a new canned gossmap without private channels, updated tests. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This will be used for generating route hints rather than private gossip
and stash in the database. Rusty: I added the bad gossip message so we would see unknown updates in CI, and made sure we don't send our own generated updates to lightningd. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Not just when it's a private channel. This is useful for listpeerchannels in the next patch. Most of this is renaming. It also means that source can be NULL, so move it out of the struct and put it in the message, where it logically belongs, and make it an optional field. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This is redundant if it's a public channel, but vital if it's not. Publishing unconditionally makes it easier for gossmap: we create a local modification all the time, even if redundant (and we can have the actual capacity ceiling accurate in this case, since we know it for local channels). Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> Changelog-Added: JSON-RPC: `listpeerchannels` now shows gossip update contents (even if channel unannounced).
…ent fee info. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This lets us convert one user at a time. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
…pdates. This is more thorough than the minimal one required for getroute(), including the feerates and cltv deltas. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
…suppression. We have multiple different routines here, and we want to wean them off private gossip one at a time. This converts `getroute`. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
rustyrussell
force-pushed
the
guilt/pr-6355
branch
from
December 6, 2023 23:23
af6f7a1 to
2ebae7b
Compare
endothermicdev
force-pushed
the
guilt/pr-6355
branch
2 times, most recently
from
December 11, 2023 18:45
e1c3399 to
263057c
Compare
endothermicdev
approved these changes
Dec 11, 2023
endothermicdev
left a comment
endothermicdev
left a comment
Collaborator
There was a problem hiding this comment.
I went through this pretty thoroughly but couldn't find much objectionable with it. The simplicity of the listchannels deprecation via gossmap localmods at the end makes it very clear this was the right approach.
I added a couple commits for memory allocation and test updates. Feel free to squash those if they look reasonable, otherwise cleanup part 1 looks great.
| } else if (candidate.c->scid) { | ||
| r->short_channel_id = *candidate.c->scid; | ||
| } else { | ||
| /* Haven't got remote alias yet? Can't use ut. */ |
Collaborator
There was a problem hiding this comment.
Suggested change
| /* Haven't got remote alias yet? Can't use ut. */ | |
| /* Haven't got remote alias yet? Can't use it. */ |
| channel->shutdown_wrong_funding = NULL; | ||
| channel->closing_feerate_range = NULL; | ||
| channel->private_update = NULL; | ||
| channel->peer_update = NULL; |
| sent->path = path_to_node(sent, cmd->plugin, buf, result, node_id); | ||
| tal_free(node_id); | ||
| if (!sent->path) { | ||
| return connect_direct(cmd, node_id, |
Collaborator
There was a problem hiding this comment.
This accesses node_id after a free. Copy the full pubkey struct instead?
…rivate gossip_store records. [ Includes use-after-free fix from Alex Myers <alex@endothermic.dev> ] Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This will become important when we apply localmods to the gossmap at these points. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
…ssip_store records. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We can, but I had a typo and thought we couldn't! Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We will get localmods from gossmods_from_listpeerchannels in the next commit, so we need to save routehints to add to that later. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
…e gossip_store records. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
l2 gave us a routehint, but it should have seen l1 as a dead-end. It didn't due to the presence of a channel alias, which was a bug. We're about to fix this, which breaks the test. Add a dummy node off l1. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This is wrong, but we send them for now in zeroconf! Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We temporarily use a second gossmap so we can just switch private info off for listincoming and not listchannels. Note that listchannels now uses the local alias (if no scid), so we have to change that in the routehint caller. Since we now *always* use a channel alias in hints if one exists, a test broke, so fix that. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Now listincoming sees local channels in listpeerchannels, we don't need these waits. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This breaks our tests a bit, which assumed we can always see ourselves even if we don't have a proper channel. Usually we would deprecate this first, but it's unlikely to break anyone since it's a bit obscure that this worked at all. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> Changelog-Changed: JSON-RPC: `listnodes` no longer shows private (peer) nodes: use listpeers
… private gossip. Some can only be changed once that is true, but some can be removed/amended already. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This alters a few remaining tests, as well. [ Inclused even more test fixes from Alex Myers <alex@endothermic.dev>! ] Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> Changelog-Deprecated: JSON-RPC: `listchannels` listing private channels: use listpeerchannels
In the next PR, they'll be removed, but for now all our code doesn't want them. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
…eerchannels` info. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> Changelog-Deprecated: `listchannels` no longer uses local knowledge to set `active` to false if disconnected.
We only deprecated this, we didn't actually remove it. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
rustyrussell
force-pushed
the
guilt/pr-6355
branch
from
December 13, 2023 05:36
263057c to
406e6d0
Compare
Contributor
Author
|
Thanks! I've folded your fixes in to the appropriate places, once this passes CI I'll merge. |
Collaborator
|
ACK 406e6d0 |
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
It turns out we relied on private gossip in many places, particularly in our tests.
In general, if you want a complete view of channels, you want to merge
listpeerchannels(which contains additional information on current balance, as well as whether the peer is connected) with the gossip_store.This PR does not actually stop gossipd from putting it in the gossip store, but it does stop us using it.