From 1b3a9be416cebda0b0b928451786be24f6d37c74 Mon Sep 17 00:00:00 2001 From: niftynei Date: Wed, 14 Oct 2020 20:24:08 -0500 Subject: [PATCH] df, channeld: cleanup how psbt signalling works We used to send our tx_sigs before we got to channeld existing. We changed how this worked so that multifundchannel could live, but failed to clean up the logic of what "having a psbt around" means wrt channeld and messaging our peer. The general idea is that we want to send `tx_signatures` to our peer on reconnect until they've sent us `funding_locked`. Note that it's an error to - send `funding_locked` without having sent `tx_signatures` - send `tx_signatures` after sending `funding_locked` We use the 'finalized' state of the peer's inputs/outputs to help signal where we are in receiving their sigs -- but this doesn't work at all for opens where the peer doesn't contribute inputs at all. This isn't really a huge deal, but it does mean that if we receive a peer's `tx_sigs` more than once (will happen for a reconnect before `funding_locked`), then we'll issue a notification about receiving their sigs multiple times. /shrug --- channeld/channeld.c | 66 ++++++++++++++++++++++++++++++++------------- 1 file changed, 48 insertions(+), 18 deletions(-) diff --git a/channeld/channeld.c b/channeld/channeld.c index 28e3ac62b..50f4b2602 100644 --- a/channeld/channeld.c +++ b/channeld/channeld.c @@ -518,6 +518,20 @@ static void announce_channel(struct peer *peer) send_channel_update(peer, 0); } +#if EXPERIMENTAL_FEATURES +static enum tx_role our_tx_role(const struct peer *peer) +{ + return peer->channel->opener == LOCAL ? + TX_INITIATOR : TX_ACCEPTER; +} +#endif + +static enum tx_role their_tx_role(const struct peer *peer) +{ + return peer->channel->opener == LOCAL ? + TX_ACCEPTER : TX_INITIATOR; +} + static void channel_announcement_negotiate(struct peer *peer) { /* Don't do any announcement work if we're shutting down */ @@ -598,6 +612,13 @@ static void handle_peer_funding_locked(struct peer *peer, const u8 *msg) if (peer->shutdown_sent[LOCAL]) return; + if (peer->psbt && !psbt_side_finalized(peer->psbt, + their_tx_role(peer))) + peer_failed(peer->pps, + &peer->channel_id, + "Rcvd `funding_locked` from peer but " + "have not received `tx_signatures`"); + peer->old_remote_per_commit = peer->remote_per_commit; if (!fromwire_funding_locked(msg, &chanid, &peer->remote_per_commit)) @@ -614,6 +635,7 @@ static void handle_peer_funding_locked(struct peer *peer, const u8 *msg) &peer->channel_id)); peer->funding_locked[REMOTE] = true; + peer->psbt = tal_free(peer->psbt); wire_sync_write(MASTER_FD, take(towire_channeld_got_funding_locked(NULL, &peer->remote_per_commit))); @@ -2043,8 +2065,7 @@ static void handle_tx_sigs(struct peer *peer, const u8 *msg) const struct witness_stack **ws; size_t j = 0; - enum tx_role role = peer->channel->opener == REMOTE - ? TX_INITIATOR : TX_ACCEPTER; + enum tx_role their_role = their_tx_role(peer); if (!fromwire_tx_signatures(tmpctx, msg, &cid, &txid, cast_const3( @@ -2080,6 +2101,15 @@ static void handle_tx_sigs(struct peer *peer, const u8 *msg) return; } + /* This check only works if they've got inputs we need sigs for. + * In the case where they send duplicate tx_sigs but have no + * sigs, we'll end up re-notifying */ + if (tal_count(ws) && psbt_side_finalized(peer->psbt, their_role)) { + status_info("Got duplicate WIRE_TX_SIGNATURES, " + "already have their sigs. Ignoring"); + return; + } + /* We put the PSBT + sigs all together */ for (size_t i = 0; i < peer->psbt->num_inputs; i++) { struct wally_psbt_input *in = @@ -2094,7 +2124,7 @@ static void handle_tx_sigs(struct peer *peer, const u8 *msg) peer->psbt)); return; } - if (in_serial % 2 != role) + if (in_serial % 2 != their_role) continue; if (j == tal_count(ws)) @@ -2111,8 +2141,6 @@ static void handle_tx_sigs(struct peer *peer, const u8 *msg) * as soon as we've got our sigs */ wire_sync_write(MASTER_FD, take(towire_channeld_funding_sigs(NULL, peer->psbt))); - - peer->psbt = tal_free(peer->psbt); } #endif /* EXPERIMENTAL_FEATURES */ @@ -2773,9 +2801,8 @@ static void peer_reconnect(struct peer *peer, #if EXPERIMENTAL_FEATURES /* Send our tx_sigs again */ - enum tx_role role = peer->channel->opener == LOCAL - ? TX_INITIATOR : TX_ACCEPTER; - if (peer->psbt && psbt_side_finalized(peer->psbt, role) + if (peer->psbt && psbt_side_finalized(peer->psbt, + our_tx_role(peer)) && !peer->funding_locked[REMOTE]) sync_crypto_write(peer->pps, take(psbt_to_tx_sigs_msg(NULL, peer->channel, @@ -2973,13 +3000,6 @@ static void handle_funding_depth(struct peer *peer, const u8 *msg) &depth)) master_badmsg(WIRE_CHANNELD_FUNDING_DEPTH, msg); - /* We were waiting for them to send us their - * `tx_signatures`, but they never did. As a - * result we'll still have the psbt */ - if (peer->psbt) { - return; - } - /* Too late, we're shutting down! */ if (peer->shutdown_sent[LOCAL]) return; @@ -2988,6 +3008,18 @@ static void handle_funding_depth(struct peer *peer, const u8 *msg) peer->depth_togo = peer->channel->minimum_depth - depth; } else { + /* We were waiting for them to send us their + * `tx_signatures`, but they never did. As a + * result we'll still have the psbt */ + if (peer->psbt && !psbt_side_finalized(peer->psbt, + their_tx_role(peer))) { + peer_failed(peer->pps, &peer->channel_id, + "Funding tx reached funding depth %d " + "but we haven't received peer's " + "tx_signatures", + depth); + } + peer->depth_togo = 0; assert(scid); @@ -3571,11 +3603,9 @@ static void init_channel(struct peer *peer) sync_crypto_write(peer->pps, take(fwd_msg)); #if EXPERIMENTAL_FEATURES - enum tx_role role; - role = opener == LOCAL ? TX_INITIATOR : TX_ACCEPTER; /* peer_reconnect does this if needed */ if (!reconnected && peer->psbt && - psbt_side_finalized(peer->psbt, role)) + psbt_side_finalized(peer->psbt, our_tx_role(peer))) sync_crypto_write(peer->pps, take(psbt_to_tx_sigs_msg(NULL, peer->channel, peer->psbt)));