gossipd: push our own gossip messages harder. #3152
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
push-our-own-gossip
branch
from
ec72bb2
to
b014a7e
Compare
trueptolemy
approved these changes
Oct 11, 2019
trueptolemy
reviewed
Oct 11, 2019
| struct peer *peer; | ||
|
|
||
| list_for_each(&daemon->peers, peer, list) | ||
| queue_peer_msg(peer, msg); |
There was a problem hiding this comment.
Sorry for my "too early" ACK.
If we call push_gossip(daemon, take(msg)), here msg is regarded as TAKEN not only in push_gossip(), but also in queue_peer_msg().
In this loop, if the parameter msg is marked as TAKEN, then the first queue_peer_msg would free it.
So, maybe msg shouldn't be declared as "TAKES" in void push_gossip(struct daemon *daemon, const u8 *msg TAKES).
Is this reason accurate?
There was a problem hiding this comment.
Yep, good catch. Will fix...
rustyrussell
force-pushed
the
push-our-own-gossip
branch
from
b014a7e
to
e677a0c
Compare
|
Rebased and fixed thinko, as diagnosed by @trueptolemy |
rustyrussell
force-pushed
the
push-our-own-gossip
branch
from
e677a0c
to
bf7c72c
Compare
|
Indeed LGTM now, but always TIMEOUT on travis-ci. |
I had a report of a 0.7.2 user whose node hadn't appeared on 1ml. Their node_announcement wasn't visible to my node, either. I suspect this is a consequence of recent version reducing the amount of gossip they send, as well as large nodes increasingly turning off gossip altogether from some peers (as we do). We should ignore timestamp filters for our own channels: the easiest way to do this is to push them out directly from gossipd (other messages are sent via the store). We change channeld to wrap the local channel_announcements: previously we just handed it to gossipd as for any other gossip message we received from our peer. Now gossipd knows to push it out, as it's local. This interferes with the logic in tests/test_misc.py::test_htlc_send_timeout which expects the node_announcement message last, so we generalize that too. [ Thanks to @trueptolmy for bugfix! ] Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
rustyrussell
force-pushed
the
push-our-own-gossip
branch
from
bf7c72c
to
80e50ba
Compare
|
ACK 80e50ba |
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.
I had a report of a 0.7.2 user whose node hadn't appeared on 1ml. Their
node_announcement wasn't visible to my node, either.
I suspect this is a consequence of recent version reducing the amount of
gossip they send, as well as large nodes increasingly turning off gossip
altogether from some peers (as we do). We should ignore timestamp filters
for our own channels: the easiest way to do this is to push them out
directly from gossipd (other messages are sent via the store).
Signed-off-by: Rusty Russell rusty@rustcorp.com.au