From d8aee68ba85861d7aa1216c596f18a8ff1c73b8d Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Wed, 10 Apr 2019 17:01:29 +0930 Subject: [PATCH] gossipd: handle duplicate nodes from unverified channel_announces properly. If we have a channel_announcement, we catch any node_announcement for either end while we validate the channel_announcement. But if we have multiple channel_announcements and the first one failed to verify, it would remove this catch, meaning we'd discard following node_announcements even though there was a pending channel_announcement. The answer is to use a simple reference count, and as a further optimization, only place the `pending_node_announce` if there's no node already. We also move the process_pending_node_announcement() calls lower down, so *any* new channel creation checks it. This is more robust, and will prove useful for the next patch, where we can use the same mechanism to handle node_announcements on channel_announcements which are verified, but don't yet have a channel_update. Signed-off-by: Rusty Russell --- gossipd/routing.c | 67 +++++++++++++++++++++++++++++++++++++---------- 1 file changed, 53 insertions(+), 14 deletions(-) diff --git a/gossipd/routing.c b/gossipd/routing.c index db8d41d87..9702a9946 100644 --- a/gossipd/routing.c +++ b/gossipd/routing.c @@ -50,7 +50,9 @@ struct pending_cannouncement { }; struct pending_node_announce { + struct routing_state *rstate; struct node_id nodeid; + size_t refcount; u8 *node_announcement; u32 timestamp; }; @@ -240,7 +242,8 @@ static struct node *new_node(struct routing_state *rstate, return n; } -/* We've received a channel_announce for a channel attached to this node */ +/* We've received a channel_announce for a channel attached to this node: + * otherwise it's in the map only because it's a peer, or us. */ static bool node_has_public_channels(struct node *node) { struct chan_map_iter i; @@ -791,13 +794,49 @@ static u8 *check_channel_announcement(const tal_t *ctx, return NULL; } -static void add_pending_node_announcement(struct routing_state *rstate, struct node_id *nodeid) +/* We allow node announcements for this node if it doesn't otherwise exist, so + * we can process them once it does exist (a channel_announce is being + * validated right now). + * + * If we attach one, remove it on destruction of @ctx. + */ +static void del_pending_node_announcement(const tal_t *ctx UNUSED, + struct pending_node_announce *pna) +{ + if (--pna->refcount == 0) { + pending_node_map_del(pna->rstate->pending_node_map, pna); + tal_free(pna); + } +} + +static void catch_node_announcement(const tal_t *ctx, + struct routing_state *rstate, + struct node_id *nodeid) { - struct pending_node_announce *pna = tal(rstate, struct pending_node_announce); - pna->nodeid = *nodeid; - pna->node_announcement = NULL; - pna->timestamp = 0; - pending_node_map_add(rstate->pending_node_map, pna); + struct pending_node_announce *pna; + struct node *node; + + /* No need if we already know about the node. We might, however, only + * know about it because it's a peer (maybe with private or + * not-yet-announced channels), so check for that too. */ + node = get_node(rstate, nodeid); + if (node && node_has_public_channels(node)) + return; + + /* We can have multiple channels announced at same time for nodes; + * but we can only have one of these in the map. */ + pna = pending_node_map_get(rstate->pending_node_map, nodeid); + if (!pna) { + pna = tal(rstate, struct pending_node_announce); + pna->rstate = rstate; + pna->nodeid = *nodeid; + pna->node_announcement = NULL; + pna->timestamp = 0; + pna->refcount = 0; + pending_node_map_add(rstate->pending_node_map, pna); + } + pna->refcount++; + tal_add_destructor2(ctx, del_pending_node_announcement, pna); } static void process_pending_node_announcement(struct routing_state *rstate, @@ -821,8 +860,6 @@ static void process_pending_node_announcement(struct routing_state *rstate, tal_hex(tmpctx, pna->node_announcement), sanitize_error(tmpctx, err, NULL)); } - pending_node_map_del(rstate->pending_node_map, pna); - tal_free(pna); } static struct pending_cannouncement * @@ -927,6 +964,11 @@ bool routing_add_channel_announcement(struct routing_state *rstate, chan->half[i].bcast.index); } + /* If we were waiting for these nodes to appear (or gain a + public channel), process node_announcements now */ + process_pending_node_announcement(rstate, &chan->nodes[0]->id); + process_pending_node_announcement(rstate, &chan->nodes[1]->id); + return true; } @@ -1061,8 +1103,8 @@ u8 *handle_channel_announcement(struct routing_state *rstate, /* Add both endpoints to the pending_node_map so we can stash * node_announcements while we wait for the txout check */ - add_pending_node_announcement(rstate, &pending->node_id_1); - add_pending_node_announcement(rstate, &pending->node_id_2); + catch_node_announcement(pending, rstate, &pending->node_id_1); + catch_node_announcement(pending, rstate, &pending->node_id_2); list_add_tail(&rstate->pending_cannouncement, &pending->list); tal_add_destructor2(pending, destroy_pending_cannouncement, rstate); @@ -1176,9 +1218,6 @@ void handle_pending_cannouncement(struct routing_state *rstate, process_pending_channel_update(rstate, scid, pending->updates[0]); process_pending_channel_update(rstate, scid, pending->updates[1]); - process_pending_node_announcement(rstate, &pending->node_id_1); - process_pending_node_announcement(rstate, &pending->node_id_2); - tal_free(pending); }