From d2b4e09e2773a09464ae0f4d2248fa4224c508b6 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Mon, 23 Apr 2018 19:38:02 +0930 Subject: [PATCH] lightningd: re-allow closing negotiation when CLOSINGD_COMPLETE d822ba1ee accidentally removed this case, which is important: if the other side didn't get our final matching closing_signed, it will reconnect and try again. We consider the channel no longer "active" and thus ignore it, and get upset when it send the `channel_reestablish` message. We could just consider CLOSINGD_COMPLETE to be active, but then we'd have to wait for the closing transaction to be mined before we'd allow another connection. We can't special case it when the peer reconnects, because there could be (in theory) multiple channels for that peer in CLOSINGD_COMPLETE, and we don't know which one to reestablish. So, we need to catch this when they send the reestablish, and hand that msg to closingd to do negotiation again. We already have code to note that we're in CLOSINGD_COMPLETE and thus ignore any result it gives us. Signed-off-by: Rusty Russell --- closingd/closing.c | 31 ++++++++++++++++++++----------- closingd/closing_wire.csv | 2 ++ lightningd/channel_control.c | 3 ++- lightningd/closing_control.c | 8 +++++--- lightningd/closing_control.h | 5 +++-- lightningd/peer_control.c | 13 ++++++++++++- wallet/test/run-wallet.c | 5 +++-- 7 files changed, 47 insertions(+), 20 deletions(-) diff --git a/closingd/closing.c b/closingd/closing.c index fc7caaab7..d0b75f855 100644 --- a/closingd/closing.c +++ b/closingd/closing.c @@ -81,7 +81,8 @@ static void do_reconnect(struct crypto_state *cs, u64 gossip_index, const struct channel_id *channel_id, const u64 next_index[NUM_SIDES], - u64 revocations_received) + u64 revocations_received, + const u8 *channel_reestablish) { u8 *msg; struct channel_id their_channel_id; @@ -104,20 +105,25 @@ static void do_reconnect(struct crypto_state *cs, if (!sync_crypto_write(cs, PEER_FD, take(msg))) peer_failed_connection_lost(); - /* Wait for them to say something interesting */ - while ((msg = read_peer_msg(tmpctx, cs, gossip_index, channel_id, - sync_crypto_write_arg, - status_fail_io, - NULL)) == NULL) + /* They might have already send reestablish, which triggered us */ + while (!channel_reestablish) { clean_tmpctx(); - if (!fromwire_channel_reestablish(msg, &their_channel_id, + /* Wait for them to say something interesting */ + channel_reestablish + = read_peer_msg(tmpctx, cs, gossip_index, channel_id, + sync_crypto_write_arg, + status_fail_io, + NULL); + } + + if (!fromwire_channel_reestablish(channel_reestablish, &their_channel_id, &next_local_commitment_number, &next_remote_revocation_number)) { peer_failed(cs, gossip_index, channel_id, "bad reestablish msg: %s %s", - wire_type_name(fromwire_peektype(msg)), - tal_hex(tmpctx, msg)); + wire_type_name(fromwire_peektype(channel_reestablish)), + tal_hex(tmpctx, channel_reestablish)); } status_trace("Got reestablish commit=%"PRIu64" revoke=%"PRIu64, next_local_commitment_number, @@ -443,6 +449,7 @@ int main(int argc, char *argv[]) u64 gossip_index; enum side whose_turn; bool deprecated_api; + u8 *channel_reestablish; subdaemon_setup(argc, argv); @@ -466,7 +473,8 @@ int main(int argc, char *argv[]) &next_index[LOCAL], &next_index[REMOTE], &revocations_received, - &deprecated_api)) + &deprecated_api, + &channel_reestablish)) master_badmsg(WIRE_CLOSING_INIT, msg); status_trace("satoshi_out = %"PRIu64"/%"PRIu64, @@ -483,7 +491,8 @@ int main(int argc, char *argv[]) if (reconnected) do_reconnect(&cs, gossip_index, &channel_id, - next_index, revocations_received); + next_index, revocations_received, + channel_reestablish); peer_billboard(true, "Negotiating closing fee between %"PRIu64 " and %"PRIu64" satoshi (ideal %"PRIu64")", diff --git a/closingd/closing_wire.csv b/closingd/closing_wire.csv index 239423897..21e935012 100644 --- a/closingd/closing_wire.csv +++ b/closingd/closing_wire.csv @@ -26,6 +26,8 @@ closing_init,,next_index_remote,u64 closing_init,,revocations_received,u64 # This means we allow closing negotiations out of bounds. closing_init,,deprecated_api,bool +closing_init,,channel_reestablish_len,u16 +closing_init,,channel_reestablish,channel_reestablish_len*u8 # We received an offer, save signature. closing_received_signature,2002 diff --git a/lightningd/channel_control.c b/lightningd/channel_control.c index c3413f9cd..87ad60256 100644 --- a/lightningd/channel_control.c +++ b/lightningd/channel_control.c @@ -114,7 +114,8 @@ static void peer_start_closingd_after_shutdown(struct channel *channel, } /* This sets channel->owner, closes down channeld. */ - peer_start_closingd(channel, &cs, gossip_index, fds[0], fds[1], false); + peer_start_closingd(channel, &cs, gossip_index, fds[0], fds[1], + false, NULL); channel_set_state(channel, CHANNELD_SHUTTING_DOWN, CLOSINGD_SIGEXCHANGE); } diff --git a/lightningd/closing_control.c b/lightningd/closing_control.c index e6296ec35..225c888b4 100644 --- a/lightningd/closing_control.c +++ b/lightningd/closing_control.c @@ -121,10 +121,11 @@ static unsigned closing_msg(struct subd *sd, const u8 *msg, const int *fds UNUSE } void peer_start_closingd(struct channel *channel, - struct crypto_state *cs, + const struct crypto_state *cs, u64 gossip_index, int peer_fd, int gossip_fd, - bool reconnected) + bool reconnected, + const u8 *channel_reestablish) { u8 *initmsg; u64 minfee, startfee, feelimit; @@ -204,7 +205,8 @@ void peer_start_closingd(struct channel *channel, channel->next_index[LOCAL], channel->next_index[REMOTE], num_revocations, - deprecated_apis); + deprecated_apis, + channel_reestablish); /* We don't expect a response: it will give us feedback on * signatures sent and received, then closing_complete. */ diff --git a/lightningd/closing_control.h b/lightningd/closing_control.h index a5661e5ff..56b25b7ff 100644 --- a/lightningd/closing_control.h +++ b/lightningd/closing_control.h @@ -7,9 +7,10 @@ struct channel_id; struct crypto_state; void peer_start_closingd(struct channel *channel, - struct crypto_state *cs, + const struct crypto_state *cs, u64 gossip_index, int peer_fd, int gossip_fd, - bool reconnected); + bool reconnected, + const u8 *channel_reestablish); #endif /* LIGHTNING_LIGHTNINGD_CLOSING_CONTROL_H */ diff --git a/lightningd/peer_control.c b/lightningd/peer_control.c index de493cb44..fd8ea36ab 100644 --- a/lightningd/peer_control.c +++ b/lightningd/peer_control.c @@ -502,7 +502,7 @@ void peer_connected(struct lightningd *ld, const u8 *msg, channel->peer->addr = addr; peer_start_closingd(channel, &cs, gossip_index, peer_fd, gossip_fd, - true); + true, NULL); goto connected; } abort(); @@ -590,6 +590,17 @@ void peer_sent_nongossip(struct lightningd *ld, error = channel->error; goto send_error; } + + /* Reestablish for a now-closed channel? They might have + * missed final update, so do the closing negotiation dance + * again. */ + if (fromwire_peektype(in_msg) == WIRE_CHANNEL_REESTABLISH + && channel + && channel->state == CLOSINGD_COMPLETE) { + peer_start_closingd(channel, cs, gossip_index, + peer_fd, gossip_fd, true, in_msg); + return; + } } /* Weird request. */ diff --git a/wallet/test/run-wallet.c b/wallet/test/run-wallet.c index 12ef48c1d..469f38f17 100644 --- a/wallet/test/run-wallet.c +++ b/wallet/test/run-wallet.c @@ -333,10 +333,11 @@ bool peer_start_channeld(struct channel *channel UNNEEDED, { fprintf(stderr, "peer_start_channeld called!\n"); abort(); } /* Generated stub for peer_start_closingd */ void peer_start_closingd(struct channel *channel UNNEEDED, - struct crypto_state *cs UNNEEDED, + const struct crypto_state *cs UNNEEDED, u64 gossip_index UNNEEDED, int peer_fd UNNEEDED, int gossip_fd UNNEEDED, - bool reconnected UNNEEDED) + bool reconnected UNNEEDED, + const u8 *channel_reestablish UNNEEDED) { fprintf(stderr, "peer_start_closingd called!\n"); abort(); } /* Generated stub for sanitize_error */ char *sanitize_error(const tal_t *ctx UNNEEDED, const u8 *errmsg UNNEEDED,