diff --git a/lightningd/channel_control.c b/lightningd/channel_control.c index 9735e72a6699..7e488233f263 100644 --- a/lightningd/channel_control.c +++ b/lightningd/channel_control.c @@ -1360,7 +1360,7 @@ static void peer_channeld_reestablished(struct channel *channel, const u8* msg) "bad channeld_reestablished %s", tal_hex(channel, msg)); - channel_gossip_channel_reestablished(channel, announcement_sigs_requested); + channel_gossip_channel_reestablished(channel); } void channel_fallen_behind(struct channel *channel) @@ -1976,7 +1976,7 @@ bool peer_start_channeld(struct channel *channel, /* "Reestablished" if we've just opened. */ if (!reconnected) - channel_gossip_channel_reestablished(channel, false); + channel_gossip_channel_reestablished(channel); /* FIXME: DTODO: Use a pointer to a txid instead of zero'ing one out. */ memset(&txid, 0, sizeof(txid)); diff --git a/lightningd/channel_gossip.c b/lightningd/channel_gossip.c index cb5a42d83489..ea6ac3e03faf 100644 --- a/lightningd/channel_gossip.c +++ b/lightningd/channel_gossip.c @@ -698,8 +698,13 @@ static void send_channel_announce_sigs(struct channel *channel) const u8 *ca, *msg; /* Wait until we've exchanged reestablish messages */ - if (!channel->reestablished) + if (!channel->reestablished) { log_debug(channel->log, "channel_gossip: not sending channel_announcement_sigs until reestablished"); + return; + } + + if (cg->sent_sigs) + return; ca = create_channel_announcement(tmpctx, channel, *channel->scid, NULL, NULL, NULL, NULL); @@ -714,13 +719,17 @@ static void send_channel_announce_sigs(struct channel *channel) /* Double-check that HSM gave valid signatures. */ sha256_double(&hash, ca + offset, tal_count(ca) - offset); - if (!check_signed_hash(&hash, &local_node_sig, &ld->our_pubkey)) + if (!check_signed_hash(&hash, &local_node_sig, &ld->our_pubkey)) { channel_internal_error(channel, "HSM returned an invalid node signature"); + return; + } - if (!check_signed_hash(&hash, &local_bitcoin_sig, &channel->local_funding_pubkey)) + if (!check_signed_hash(&hash, &local_bitcoin_sig, &channel->local_funding_pubkey)) { channel_internal_error(channel, "HSM returned an invalid bitcoin signature"); + return; + } msg = towire_announcement_signatures(NULL, &channel->cid, *channel->scid, @@ -729,16 +738,6 @@ static void send_channel_announce_sigs(struct channel *channel) cg->sent_sigs = true; } -static void send_channel_announce_sigs_once(struct channel *channel) -{ - struct channel_gossip *cg = channel->channel_gossip; - - if (cg->sent_sigs) - return; - - send_channel_announce_sigs(channel); -} - /* Sends channel_announcement */ static void send_channel_announcement(struct channel *channel) { @@ -830,7 +829,7 @@ static void set_gossip_state(struct channel *channel, case CGOSSIP_ANNOUNCED: /* In case this snuck up on us (fast confirmations), * make sure we sent sigs */ - send_channel_announce_sigs_once(channel); + send_channel_announce_sigs(channel); /* BOLT #7: * A recipient node: @@ -898,7 +897,7 @@ static void update_gossip_state(struct channel *channel) return; case CGOSSIP_WAITING_FOR_MATCHING_PEER_SIGS: case CGOSSIP_WAITING_FOR_ANNOUNCE_DEPTH: - send_channel_announce_sigs_once(channel); + send_channel_announce_sigs(channel); /* fall thru */ case CGOSSIP_WAITING_FOR_SCID: case CGOSSIP_PRIVATE: @@ -1006,7 +1005,7 @@ void channel_gossip_got_announcement_sigs(struct channel *channel, send_our_sigs: /* This only works once, so we won't spam them. */ - send_channel_announce_sigs_once(channel); + send_channel_announce_sigs(channel); } /* Short channel id changed (splice, or reorg). */ @@ -1202,8 +1201,7 @@ static void channel_reestablished_stable(struct channel *channel) } /* Peer has connected and successfully reestablished channel. */ -void channel_gossip_channel_reestablished(struct channel *channel, - bool announcement_sigs_requested) +void channel_gossip_channel_reestablished(struct channel *channel) { channel->reestablished = true; tal_free(channel->stable_conn_timer); @@ -1221,9 +1219,6 @@ void channel_gossip_channel_reestablished(struct channel *channel, /* We can re-xmit sigs once per reconnect */ channel->channel_gossip->sent_sigs = false; - if (announcement_sigs_requested) - send_channel_announce_sigs(channel); - /* BOLT #7: * - Upon reconnection (once the above timing requirements have * been met): @@ -1244,7 +1239,7 @@ void channel_gossip_channel_reestablished(struct channel *channel, check_channel_gossip(channel); return; case CGOSSIP_WAITING_FOR_MATCHING_PEER_SIGS: - send_channel_announce_sigs_once(channel); + send_channel_announce_sigs(channel); /* fall thru */ case CGOSSIP_PRIVATE: case CGOSSIP_WAITING_FOR_ANNOUNCE_DEPTH: diff --git a/lightningd/channel_gossip.h b/lightningd/channel_gossip.h index b6fae02a204e..6b8e4e203262 100644 --- a/lightningd/channel_gossip.h +++ b/lightningd/channel_gossip.h @@ -43,8 +43,7 @@ void channel_gossip_update_from_gossipd(struct channel *channel, void channel_gossip_init_done(struct lightningd *ld); /* Peer has connected and successfully reestablished channel. */ -void channel_gossip_channel_reestablished(struct channel *channel, - bool announcement_sigs_requested); +void channel_gossip_channel_reestablished(struct channel *channel); /* Peer has disconnected */ void channel_gossip_channel_disconnect(struct channel *channel); diff --git a/tests/test_gossip.py b/tests/test_gossip.py index 30f94174ded9..c550d56c0004 100644 --- a/tests/test_gossip.py +++ b/tests/test_gossip.py @@ -116,6 +116,70 @@ def count_active(node): wait_for(lambda: count_active(l2) == 2) +@unittest.skipIf(os.getenv('TEST_DB_PROVIDER', 'sqlite3') != 'sqlite3', "sqlite3-specific DB manip") +def test_reestablish_announcement_sigs(node_factory): + """Regression test: announcement_signatures must wait until + channel reestablishment has completed after reconnect. + + A bug in send_channel_announce_sigs() caused it to unconditionally + send announcement_signatures on reconnect (missing early returns), + before the peers had exchanged channel_reestablish. + See: https://github.com/ElementsProject/lightning/issues/8978 + """ + opts = {'may_reconnect': True, 'dev-no-reconnect': None} + l1, l2 = node_factory.line_graph(2, wait_for_announce=True, opts=opts) + + def first_log_index(node, needle, start): + node.daemon.logs_catchup() + for idx, line in enumerate(node.daemon.logs[start:], start): + if needle in line: + return idx + return None + + # Make l2 rely on its stored announcement state, and make l1 ask for + # announcement_signatures again on reconnect. + l2.stop() + gs_path = os.path.join(l2.daemon.lightning_dir, TEST_NETWORK, 'gossip_store') + os.unlink(gs_path) + l2.start() + wait_for(lambda: l2.rpc.listchannels()['channels'] != []) + + l1.db_manip("UPDATE channels SET remote_ann_node_sig=NULL, remote_ann_bitcoin_sig=NULL") + gs_path = os.path.join(l1.daemon.lightning_dir, TEST_NETWORK, 'gossip_store') + os.unlink(gs_path) + l1.restart() + + l1.daemon.logs_catchup() + l2.daemon.logs_catchup() + l1_start = len(l1.daemon.logs) + l2_start = len(l2.daemon.logs) + l1.daemon.logsearch_start = l1_start + l2.daemon.logsearch_start = l2_start + + l1.connect(l2) + + l1.daemon.wait_for_log('channel_gossip: reestablished') + l2.daemon.wait_for_log('channel_gossip: reestablished') + + l1.daemon.wait_for_logs(['peer_out WIRE_ANNOUNCEMENT_SIGNATURES', + 'peer_in WIRE_ANNOUNCEMENT_SIGNATURES']) + l2.daemon.wait_for_log('peer_out WIRE_ANNOUNCEMENT_SIGNATURES') + + l1_reestablished = first_log_index(l1, 'channel_gossip: reestablished', l1_start) + l2_reestablished = first_log_index(l2, 'channel_gossip: reestablished', l2_start) + l1_announce = first_log_index(l1, 'peer_out WIRE_ANNOUNCEMENT_SIGNATURES', l1_start) + l2_announce = first_log_index(l2, 'peer_out WIRE_ANNOUNCEMENT_SIGNATURES', l2_start) + + assert l1_reestablished is not None + assert l2_reestablished is not None + assert l1_announce > l1_reestablished + assert l2_announce > l2_reestablished + + l1msgs = [l.split()[4] for l in l1.daemon.logs[l1_start:] if 'WIRE_ANNOUNCEMENT_SIGNATURES' in l] + assert sorted(l1msgs) == ['peer_in', 'peer_out'] + assert len([l for l in l2.daemon.logs[l2_start:] if 'peer_out WIRE_ANNOUNCEMENT_SIGNATURES' in l]) == 1 + + def test_announce_address(node_factory, bitcoind): """Make sure our announcements are well formed."""