Fix gossip code when channels are marked dying #7057
Merged
Conversation
This file contains 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
rustyrussell
force-pushed
the
guilt/fix-dead-gossip
branch
2 times, most recently
from
1ec6143
to
6ca04bb
Compare
cdecker
force-pushed
the
guilt/fix-dead-gossip
branch
2 times, most recently
from
9f49ef6
to
8b10f13
Compare
|
Looks like there are still some And here is what I believe the same use-after-free from the pov of Very detailed, but almost bludgeoning devs with verbosity xD As far as I can see the |
rustyrussell
force-pushed
the
guilt/fix-dead-gossip
branch
from
8b10f13
to
0c96a3b
Compare
|
Yep, trivial use-after-free! Obvious fix, may or may not prevent that broken bad gossip msg... |
rustyrussell
force-pushed
the
guilt/fix-dead-gossip
branch
from
0c96a3b
to
9b47e74
Compare
|
Found one place I missed, added final patch with much debug info, so we will get more info if there's another failure in CI. |
rustyrussell
force-pushed
the
guilt/fix-dead-gossip
branch
3 times, most recently
from
8e0c37a
to
4de1a2b
Compare
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
It prints a message to stderr, but actually it's fine with this version: ``` dump-gossipstore: UNKNOWN GOSSIP minor VERSION 14 (expected 12) ``` Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
And fix up gossip_store backwards comment! Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We want to be able to clear them, and fetch them. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
…ing. This avoids us gossiping about nodes which don't have live channels. Interstingly, we previously tested that we *did* gossip such node announcements, and now we fix that test. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
…channel! Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We can update dying channels, though it seems weird! We accept gossip about them, we just don't propagate it. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We accept node_announcements on dying channels, but make sure we set the dying flag it channels are alll dying. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
…l_announcements. We make sure a node_announcement is preceeded by at least one channel_announcement, but dying ones don't count (as they are not broadcast!). Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
…move last channel. Normally, channels are marked dying, the 12 blocks later, removed. But for local channels, we can access any spliced channel already, so we remove them immediately from our local gossip. This left a hole in our logic, if that channel was the last one keeping a node_announcement alive. Solution is to unify with the "moved node_announcement" path. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
rustyrussell
force-pushed
the
guilt/fix-dead-gossip
branch
from
709425c
to
1590e65
Compare
…yet. This happens if: 1. The peer sets a timestamp filter to non-zero, and 2. We have a channel_announcement without a channel_update. The timestamp is 0 as a placeholder as part of the recent gossip rework (we used to hold these channel_announcement in memory, which was complex). But this means we won't send it in this case, and if we later send the channel_update, CI will complain about 'Bad gossip order'. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
rustyrussell
force-pushed
the
guilt/fix-dead-gossip
branch
from
1590e65
to
90cfc18
Compare
|
I found it! Our timestamp filtering, which we still kind of support, was omitting channels-without-an-update yet, which is a narrow window. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The "bad gossip" flakes were caused by the fact that:
dyingwhich means we don't propagate it.I added a dev sanity check which helped test all of these!