From 981ffb83f7c98fa91872520f87f0d1b01f264527 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 17 May 2018 14:38:11 +0930 Subject: [PATCH] channeld: don't send updates for 0:0:0. Some paths (eg reconnect) were unconditionally sending a channel_update. valgrind wasn't catching it because we unmarshal short_channel_ids[LOCAL] as all-zeroes, so it's technically "initialized". Create a wrapper to do this, and change the 'bool disabled' flag to be the explicit disable flag value for clarity. Signed-off-by: Rusty Russell --- channeld/channel.c | 53 +++++++++++++++++++++++++++++----------------- 1 file changed, 34 insertions(+), 19 deletions(-) diff --git a/channeld/channel.c b/channeld/channel.c index 3da6b0b79..294471e21 100644 --- a/channeld/channel.c +++ b/channeld/channel.c @@ -299,12 +299,16 @@ static void enqueue_peer_msg(struct peer *peer, const u8 *msg TAKES) } static u8 *create_channel_update(const tal_t *ctx, - struct peer *peer, bool disabled) + struct peer *peer, + int disable_flag) { u32 timestamp = time_now().ts.tv_sec; u16 flags; u8 *cupdate, *msg; + /* We must have a channel id to send */ + assert(peer->short_channel_ids[LOCAL].u64); + /* Identical timestamps will be ignored. */ if (timestamp <= peer->last_update_timestamp) timestamp = peer->last_update_timestamp + 1; @@ -314,7 +318,7 @@ static u8 *create_channel_update(const tal_t *ctx, secp256k1_ecdsa_signature *sig = talz(tmpctx, secp256k1_ecdsa_signature); - flags = peer->channel_direction | (disabled << 1); + flags = peer->channel_direction | disable_flag; cupdate = towire_channel_update( tmpctx, sig, &peer->chain_hash, &peer->short_channel_ids[LOCAL], timestamp, flags, @@ -336,6 +340,25 @@ static u8 *create_channel_update(const tal_t *ctx, return cupdate; } +/* Create and send channel_update to gossipd (and maybe peer) */ +static void send_channel_update(struct peer *peer, bool peer_too, + int disable_flag) +{ + u8 *msg; + + assert(disable_flag == 0 || disable_flag == ROUTING_FLAGS_DISABLED); + + /* If we don't have funding_locked both sides, we can't have told + * gossipd or created update. */ + if (!peer->funding_locked[LOCAL] || !peer->funding_locked[REMOTE]) + return; + + msg = create_channel_update(tmpctx, peer, disable_flag); + if (peer_too) + enqueue_peer_msg(peer, msg); + wire_sync_write(GOSSIP_FD, take(msg)); +} + /** * Add a channel locally and send a channel update to the peer * @@ -358,7 +381,7 @@ static void send_temporary_announcement(struct peer *peer) /* Tell the other side what parameters we expect should they route * through us */ - msg = create_channel_update(tmpctx, peer, false); + msg = create_channel_update(tmpctx, peer, 0); enqueue_peer_msg(peer, msg); msg = towire_gossip_local_add_channel(NULL, @@ -529,7 +552,7 @@ static void announce_channel(struct peer *peer) check_short_ids_match(peer); cannounce = create_channel_announcement(tmpctx, peer); - cupdate = create_channel_update(tmpctx, peer, false); + cupdate = create_channel_update(tmpctx, peer, 0); wire_sync_write(GOSSIP_FD, cannounce); wire_sync_write(GOSSIP_FD, cupdate); @@ -737,9 +760,7 @@ static void maybe_send_shutdown(struct peer *peer) /* Send a disable channel_update so others don't try to route * over us */ - msg = create_channel_update(NULL, peer, true); - wire_sync_write(GOSSIP_FD, msg); - enqueue_peer_msg(peer, take(msg)); + send_channel_update(peer, true, ROUTING_FLAGS_DISABLED); msg = towire_shutdown(NULL, &peer->channel_id, peer->final_scriptpubkey); enqueue_peer_msg(peer, take(msg)); @@ -1505,10 +1526,10 @@ static void handle_pong(struct peer *peer, const u8 *pong) static void handle_peer_shutdown(struct peer *peer, const u8 *shutdown) { struct channel_id channel_id; - u8 *scriptpubkey, *msg; + u8 *scriptpubkey; - msg = create_channel_update(NULL, peer, true); - wire_sync_write(GOSSIP_FD, take(msg)); + /* Disable the channel. */ + send_channel_update(peer, false, ROUTING_FLAGS_DISABLED); if (!fromwire_shutdown(peer, shutdown, &channel_id, &scriptpubkey)) peer_failed(&peer->cs, @@ -1616,11 +1637,7 @@ static void peer_in(struct peer *peer, const u8 *msg) static void peer_conn_broken(struct peer *peer) { /* If we have signatures, send an update to say we're disabled. */ - if (peer->have_sigs[LOCAL] && peer->have_sigs[REMOTE]) { - u8 *cupdate = create_channel_update(NULL, peer, true); - - wire_sync_write(GOSSIP_FD, take(cupdate)); - } + send_channel_update(peer, false, ROUTING_FLAGS_DISABLED); peer_failed_connection_lost(); } @@ -1748,7 +1765,7 @@ static void peer_reconnect(struct peer *peer) bool retransmit_revoke_and_ack; struct htlc_map_iter it; const struct htlc *htlc; - u8 *msg, *cupdate; + u8 *msg; /* BOLT #2: * @@ -1910,9 +1927,7 @@ static void peer_reconnect(struct peer *peer) /* Reenable channel by sending a channel_update without the * disable flag */ - cupdate = create_channel_update(NULL, peer, false); - wire_sync_write(GOSSIP_FD, cupdate); - enqueue_peer_msg(peer, take(cupdate)); + send_channel_update(peer, true, 0); /* Corner case: we will get upset with them if they send * commitment_signed with no changes. But it could be that we sent a