-
Notifications
You must be signed in to change notification settings - Fork 890
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
gossipd: actually update own node announcement if needed #6454
gossipd: actually update own node announcement if needed #6454
Conversation
When an outdated own node announcement is present, it fails the nannounce_different test and also fails to kick off the forced regen timer. Changelog-Fixed: Node announcements are refreshed more reliably.
if (timestamp > self->bcast.timestamp && | ||
timestamp - self->bcast.timestamp > | ||
GOSSIP_PRUNE_INTERVAL(daemon->rstate->dev_fast_gossip_prune) / 2) | ||
goto send; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setup_force_nannounce_regen_timer runs every 24 hours and calls this with always_refresh=true, so I don't see how this helps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should, but setup_force_nannounce_regen_timer is not being called in the first place. It slips through on this return without ever initiating the timer:
if (self && self->bcast.index) {
u32 next;
bool only_missing_tlv;
if (!nannounce_different(daemon->rstate->gs, self, nannounce,
&only_missing_tlv)) {
if (always_refresh)
goto send;
return false;
}
Actually, it frees the timer prior to this point, so I'm not sure even that would cover it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then I think the right answer is simply to call setup_force_nannounce_regen_timer the first time? A bit tricky, since we're returning true on that path, but we can remove that since nobody actually uses the return value!
diff --git a/gossipd/gossip_generation.c b/gossipd/gossip_generation.c
index 4c70b1e5c..e2c38296b 100644
--- a/gossipd/gossip_generation.c
+++ b/gossipd/gossip_generation.c
@@ -251,8 +251,7 @@ static void setup_force_nannounce_regen_timer(struct daemon *daemon);
* the routing.c code like any other `node_announcement`. Such announcements
* are only accepted if there is an announced channel associated with that node
* (to prevent spam), so we only call this once we've announced a channel. */
-/* Returns true if this sent one, or has arranged to send one in future. */
-static bool update_own_node_announcement(struct daemon *daemon,
+static void update_own_node_announcement(struct daemon *daemon,
bool startup,
bool always_refresh)
{
@@ -287,7 +286,10 @@ static bool update_own_node_announcement(struct daemon *daemon,
&only_missing_tlv)) {
if (always_refresh)
goto send;
- return false;
+ /* First time? Start regen timer. */
+ if (!daemon->node_announce_regen_timer)
+ goto reset_timer;
+ return;
}
/* Missing liquidity_ad, maybe we'll get plugin callback */
@@ -302,7 +304,7 @@ static bool update_own_node_announcement(struct daemon *daemon,
time_from_sec(delay),
update_own_node_announcement_after_startup,
daemon);
- return true;
+ return;
}
/* BOLT #7:
*
@@ -324,7 +326,7 @@ static bool update_own_node_announcement(struct daemon *daemon,
time_from_sec(next - timestamp),
update_own_node_announcement_after_startup,
daemon);
- return true;
+ return;
}
}
@@ -334,8 +336,6 @@ send:
reset_timer:
/* Generate another one in 24 hours. */
setup_force_nannounce_regen_timer(daemon);
-
- return true;
}
static void update_own_node_announcement_after_startup(struct daemon *daemon)
Also as update_own_node_announcement is called nearly continuously under normal operation by maybe_send_own_node_announce, the timer should not be freed continuously - better to only free before actually refreshing.
Fixed the timer failing to initialize - arguably the bigger problem. Also had to be more judicious in freeing the previous one as this function is essentially called continuously on normal operation. Now the question becomes, if we already generated a fresh node_announcement and the old one was significantly out of date, should we go ahead and use it rather than waiting a day for the forced refresh timer to fire? I'm thinking of user experience and how there could be some confusion if the node is fired up and there's still no signs of a node announcement for the first 24 hours. I think it's reasonable to do both, but either one individually would solve the issue to some degree. |
Agreed, this is good. We want to rework this next release anyway... |
When an outdated own node announcement is present, it can fail the
nannounce_different
test and also fail to kick off the forced regen timer.An alternative solution is to manually start the
setup_force_nannounce_regen_timer
from gossip_init, however asmaybe_send_own_node_announce
is already called at init, that seems like a more appropriate event to actually update and broadcast the refreshed node announcement.Changelog-Fixed: Node announcements are refreshed more reliably.