diff --git a/channeld/channeld.c b/channeld/channeld.c index 9bf708a3279a..baf630be3131 100644 --- a/channeld/channeld.c +++ b/channeld/channeld.c @@ -183,6 +183,9 @@ struct peer { /* Send tx_abort on every splice we accept, once it completes */ bool dev_splice_abort_at_tx_complete; + /* Send shutdown without waiting for a pending splice to lock */ + bool dev_shutdown_during_splice; + /* Signing second, send a warning instead of tx_signatures */ bool dev_splice_warn_instead_of_sigs; @@ -554,6 +557,10 @@ static void check_mutual_splice_locked(struct peer *peer) peer->splice_state->inflights = tal_free(peer->splice_state->inflights); peer->splice_state->count = 0; peer->splice_state->remote_locked_txid = tal_free(peer->splice_state->remote_locked_txid); + + /* A shutdown waiting for this splice can go now. */ + if (peer->send_shutdown) + start_commit_timer(peer); } static void implied_peer_splice_locked(struct peer *peer, @@ -972,6 +979,25 @@ static bool shutdown_complete(const struct peer *peer) * - MUST NOT send a `shutdown`. */ /* So we only call this after reestablish or immediately after sending commit */ +static bool shared_input_finalized(const struct peer *peer, + const struct inflight *inflight); +static bool have_they_signed_inflight(const struct peer *peer, + const struct inflight *inflight); + +/* A splice tx both sides have signed, so it can be (or has been) broadcast. */ +static bool have_signed_splice(const struct peer *peer) +{ + for (size_t i = 0; i < tal_count(peer->splice_state->inflights); i++) { + const struct inflight *inflight = peer->splice_state->inflights[i]; + if ((inflight->i_sent_sigs + && (inflight->remote_tx_sigs + || have_they_signed_inflight(peer, inflight))) + || shared_input_finalized(peer, inflight)) + return true; + } + return false; +} + static void maybe_send_shutdown(struct peer *peer) { u8 *msg; @@ -980,8 +1006,26 @@ static void maybe_send_shutdown(struct peer *peer) if (!peer->send_shutdown) return; - /* DTODO: Ensure 'shutdown' rules around splice are followed once those - * rules get settled on spec */ + /* BOLT-splice #2: + * + * A sending node: + *... + * - MUST NOT send `shutdown` if there is a splice transaction that + * isn't locked yet. + * + * We only wait for one both sides have signed: any other may never + * confirm, and waiting on it would let them stall the close (should + * it confirm anyway, the mutual close loses and lightningd closes the + * splice output instead). Once they have sent `shutdown` we reply + * anyway: a peer that has may never lock the splice, so waiting would + * hang. */ + if (have_signed_splice(peer) + && !peer->shutdown_sent[REMOTE] + && !peer->dev_shutdown_during_splice) { + peer_billboard(false, "Waiting for the splice to lock before" + " sending shutdown"); + return; + } if (peer->shutdown_wrong_funding) { tlvs = tlv_shutdown_tlvs_new(tmpctx); @@ -4529,6 +4573,18 @@ static void splice_accepter(struct peer *peer, const u8 *inmsg) " a splice while we have one pending. Did you" " mean to send SPLICE_ACK?"); + /* BOLT-splice #2: + * + * The sending node: + *... + * - MUST NOT send `splice_init` if it has previously sent `shutdown`. + * + * Nor will we take one (or an RBF) after sending ours. While ours waits + * for a splice to lock, an RBF of it is fine. */ + if (peer->shutdown_sent[LOCAL] || peer->shutdown_sent[REMOTE]) + peer_failed_warn(peer->pps, &peer->channel_id, + "Can't splice while shutting down"); + peer->splicing = splicing_new(peer); ictx = new_interactivetx_context(tmpctx, our_role, @@ -7348,7 +7404,8 @@ static void init_channel(struct peer *peer) &peer->local_alias, &peer->dev_splice_abort_at_tx_complete, &peer->dev_splice_warn_instead_of_sigs, - &peer->dev_delay_confirmed_stfu)) { + &peer->dev_delay_confirmed_stfu, + &peer->dev_shutdown_during_splice)) { master_badmsg(WIRE_CHANNELD_INIT, msg); } diff --git a/channeld/channeld_wire.csv b/channeld/channeld_wire.csv index db2cf4b987fc..225dd4a5fa9a 100644 --- a/channeld/channeld_wire.csv +++ b/channeld/channeld_wire.csv @@ -86,6 +86,7 @@ msgdata,channeld_init,scid_alias,short_channel_id, msgdata,channeld_init,dev_splice_abort_at_tx_complete,bool, msgdata,channeld_init,dev_splice_warn_instead_of_sigs,bool, msgdata,channeld_init,dev_delay_confirmed_stfu,u32, +msgdata,channeld_init,dev_shutdown_during_splice,bool, # channeld->lightningd: successfully negotated reestablishment. msgtype,channeld_reestablished,1101 diff --git a/lightningd/channel_control.c b/lightningd/channel_control.c index 1450ebc6a495..9da7743e891b 100644 --- a/lightningd/channel_control.c +++ b/lightningd/channel_control.c @@ -704,7 +704,8 @@ static void handle_splice_confirmed_signed(struct lightningd *ld, inflight->remote_tx_sigs = true; wallet_inflight_save(ld->wallet, inflight); - if (channel->state != CHANNELD_AWAITING_SPLICE) { + if (channel->state != CHANNELD_AWAITING_SPLICE + && channel->state != CHANNELD_SHUTTING_DOWN) { log_debug(channel->log, "Would broadcast splice, but state %s" " isn't CHANNELD_AWAITING_SPLICE", @@ -726,8 +727,10 @@ static enum watch_result splice_depth_cb(struct lightningd *ld, { /* Usually, we're here because we're awaiting a splice, but * we could also mutual shutdown, or that weird splice_locked_memonly - * hack... */ - if (inflight->channel->state != CHANNELD_AWAITING_SPLICE) { + * hack... channeld holds a shutdown until the splice locks, so + * keep telling it the depth. */ + if (inflight->channel->state != CHANNELD_AWAITING_SPLICE + && inflight->channel->state != CHANNELD_SHUTTING_DOWN) { log_debug(inflight->channel->log, "Splice inflight event but not" " in AWAITING_SPLICE, ending watch of txid %s", fmt_bitcoin_txid(tmpctx, &inflight->funding->outpoint.txid)); @@ -832,22 +835,26 @@ static void handle_splice_sending_sigs(struct lightningd *ld, /* We can get here because of a splice RBF or because re-signing during * or because of a splice RBF. In the latter case, we will be adding - * adding a harmless second txid watch on the inflight. */ - if (channel->state != CHANNELD_NORMAL - && channel->state != CHANNELD_AWAITING_SPLICE) { - log_unusual(channel->log, "Setting state to" - " CHANNELD_AWAITING_SPLICE but existing channel" - " state is unexpected value %s", - channel_state_str(channel->state)); - } + * adding a harmless second txid watch on the inflight. A close + * requested meanwhile stays: channeld holds the shutdown until the + * splice locks. */ + if (channel->state != CHANNELD_SHUTTING_DOWN) { + if (channel->state != CHANNELD_NORMAL + && channel->state != CHANNELD_AWAITING_SPLICE) { + log_unusual(channel->log, "Setting state to" + " CHANNELD_AWAITING_SPLICE but existing channel" + " state is unexpected value %s", + channel_state_str(channel->state)); + } - cc = splice_command_for_chan(ld, channel); - /* If matching user command found, this was a user intiated splice */ - channel_set_state(channel, - channel->state, - CHANNELD_AWAITING_SPLICE, - cc ? REASON_USER : REASON_REMOTE, - "Splice signatures sent"); + cc = splice_command_for_chan(ld, channel); + /* If matching user command found, this was a user intiated splice */ + channel_set_state(channel, + channel->state, + CHANNELD_AWAITING_SPLICE, + cc ? REASON_USER : REASON_REMOTE, + "Splice signatures sent"); + } watch_splice_inflight(ld, inflight); @@ -1266,6 +1273,7 @@ static void handle_peer_splice_locked(struct channel *channel, const u8 *msg) s64 splice_amnt; struct channel_inflight *inflight; struct bitcoin_txid locked_txid; + u32 locked_blockheight; if (!fromwire_channeld_got_splice_locked(msg, &funding_sats, &splice_amnt, @@ -1299,8 +1307,14 @@ static void handle_peer_splice_locked(struct channel *channel, const u8 *msg) /* Remember that we got the lockin */ wallet_channel_save(channel->peer->ld->wallet, channel); - /* Update the scid and tell everyone */ - change_scid(channel, *inflight->locked_scid); + /* inflight is freed below, along with the rest. */ + locked_blockheight = short_channel_id_blocknum(*inflight->locked_scid); + + /* Update the scid and tell everyone, unless we're shutting down: + * gossip already has the channel dying under its scid (see + * channel_apply_scid). */ + if (!channel_state_closing(channel->state)) + change_scid(channel, *inflight->locked_scid); /* Now watch the new funding tx */ channel_watch_funding(channel->peer->ld, channel); @@ -1310,8 +1324,7 @@ static void handle_peer_splice_locked(struct channel *channel, const u8 *msg) prev_our_msats, prev_funding_sats, &prev_funding_out, - channel->scid ? - short_channel_id_blocknum(*channel->scid) : 0, + locked_blockheight, &locked_txid, inflight); @@ -1329,7 +1342,12 @@ static void handle_peer_splice_locked(struct channel *channel, const u8 *msg) * funding_spend_watch. */ channel_watch_inflight_outs(channel->peer->ld, channel); - lockin_complete(channel, CHANNELD_AWAITING_SPLICE); + /* A shutdown was waiting for this: stay in CHANNELD_SHUTTING_DOWN, + * but record the new funding like lockin_complete would. */ + if (channel->state == CHANNELD_SHUTTING_DOWN) + channel_record_open(channel, locked_blockheight, true); + else + lockin_complete(channel, CHANNELD_AWAITING_SPLICE); } /* We were informed by channeld that channel is ready (reached mindepth) */ @@ -2102,7 +2120,8 @@ bool peer_start_channeld(struct channel *channel, *channel->alias[LOCAL], ld->dev_splice_abort_at_tx_complete, ld->dev_splice_warn_instead_of_sigs, - ld->dev_delay_confirmed_stfu); + ld->dev_delay_confirmed_stfu, + ld->dev_shutdown_during_splice); /* We don't expect a response: we are triggered by funding_depth_cb. */ subd_send_msg(channel->owner, take(initmsg)); diff --git a/lightningd/lightningd.c b/lightningd/lightningd.c index 879af2d63f11..8ea9fa8926f7 100644 --- a/lightningd/lightningd.c +++ b/lightningd/lightningd.c @@ -148,6 +148,7 @@ static struct lightningd *new_lightningd(const tal_t *ctx) ld->dev_splice_without_unified_sigs = false; ld->dev_splice_coinbase_input = false; ld->dev_splice_abort_at_tx_complete = false; + ld->dev_shutdown_during_splice = false; ld->dev_splice_rbf_any_feerate = false; ld->dev_splice_warn_instead_of_sigs = false; ld->dev_splice_abort_after_sigs = false; diff --git a/lightningd/lightningd.h b/lightningd/lightningd.h index 4f8c0f7501ef..3267158c9ef7 100644 --- a/lightningd/lightningd.h +++ b/lightningd/lightningd.h @@ -375,6 +375,10 @@ struct lightningd { * completes, so the initiator's handling of it can be tested. */ bool dev_splice_abort_at_tx_complete; + /* Send shutdown without waiting for a pending splice to lock, as + * peers without that rule do. */ + bool dev_shutdown_during_splice; + /* Let splice_init propose an RBF below 25/24 of the last feerate, so * the peer's refusal can be tested. */ bool dev_splice_rbf_any_feerate; diff --git a/lightningd/options.c b/lightningd/options.c index 9084c080b952..f6d1fd542475 100644 --- a/lightningd/options.c +++ b/lightningd/options.c @@ -917,6 +917,10 @@ static void dev_register_opts(struct lightningd *ld) opt_set_bool, &ld->dev_splice_abort_at_tx_complete, "Send tx_abort on every splice we accept, once it completes"); + clnopt_noarg("--dev-shutdown-during-splice", OPT_DEV, + opt_set_bool, + &ld->dev_shutdown_during_splice, + "Send shutdown without waiting for a pending splice to lock"); clnopt_noarg("--dev-splice-rbf-any-feerate", OPT_DEV, opt_set_bool, &ld->dev_splice_rbf_any_feerate, diff --git a/lightningd/peer_control.c b/lightningd/peer_control.c index 6dc019de660e..244c0ae1e8f7 100644 --- a/lightningd/peer_control.c +++ b/lightningd/peer_control.c @@ -3177,9 +3177,18 @@ static void setup_peer(struct peer *peer) continue; /* Normal cases where we watch funding */ + /* channeld holds a shutdown until a pending splice locks, + * so keep watching it (not dual-open candidates, which are + * not splices). */ + case CHANNELD_SHUTTING_DOWN: + channel_watch_funding(ld, channel); + list_for_each(&channel->inflights, inflight, list) + if (inflight->funding->splice_remote_funding) + watch_splice_inflight(ld, inflight); + break; + case CHANNELD_AWAITING_LOCKIN: case CHANNELD_NORMAL: - case CHANNELD_SHUTTING_DOWN: case CLOSINGD_SIGEXCHANGE: /* We still want to watch spend, to tell onchaind: */ case CLOSINGD_COMPLETE: diff --git a/tests/plugins/channeld_fakenet.c b/tests/plugins/channeld_fakenet.c index f66c122feca8..37f098bd8700 100644 --- a/tests/plugins/channeld_fakenet.c +++ b/tests/plugins/channeld_fakenet.c @@ -1086,6 +1086,7 @@ static struct channel *handle_init(struct info *info, const u8 *init_msg) bool dev_splice_abort_at_tx_complete; bool dev_splice_warn_instead_of_sigs; u32 dev_delay_confirmed_stfu; + bool dev_shutdown_during_splice; struct channel *channel; const u8 *wscript; char *err_reason; @@ -1154,7 +1155,8 @@ static struct channel *handle_init(struct info *info, const u8 *init_msg) &local_alias, &dev_splice_abort_at_tx_complete, &dev_splice_warn_instead_of_sigs, - &dev_delay_confirmed_stfu)) + &dev_delay_confirmed_stfu, + &dev_shutdown_during_splice)) abort(); status_debug("Parsed init..."); diff --git a/tests/test_splicing.py b/tests/test_splicing.py index e8cfa64a2002..84abb94b39b7 100644 --- a/tests/test_splicing.py +++ b/tests/test_splicing.py @@ -1383,7 +1383,10 @@ def test_splice_restart_watches_funding_spend(node_factory, bitcoind): def test_splice_confirms_after_mutual_close(node_factory, bitcoind): """A splice mined in place of the mutual close leaves the funds in its output, so we must close that with a commitment.""" - l1, l2 = node_factory.line_graph(2, fundamount=1000000, wait_for_announce=True) + # l2 sends shutdown with the splice unlocked, as peers without that + # rule do, and l1 replies. + l1, l2 = node_factory.line_graph(2, fundamount=1000000, wait_for_announce=True, + opts=[{}, {'dev-shutdown-during-splice': None}]) chan_id = l1.get_channel_id(l2) funds_result = l1.rpc.fundpsbt("111722sat", 0, 0, excess_as_change=True) @@ -1398,7 +1401,7 @@ def test_splice_confirms_after_mutual_close(node_factory, bitcoind): wait_for(lambda: bitcoind.rpc.getrawmempool() == [splice_txid]) # The mutual close spends the old funding, so it loses to the splice. - close_txid = only_one(l1.rpc.close(chan_id)['txids']) + close_txid = only_one(l2.rpc.close(chan_id)['txids']) for n in (l1, l2): wait_for(lambda: only_one(n.rpc.listpeerchannels()['channels'])['state'] == 'CLOSINGD_COMPLETE') assert bitcoind.rpc.getrawmempool() == [splice_txid] @@ -1432,6 +1435,78 @@ def test_splice_confirms_after_mutual_close(node_factory, bitcoind): assert bitcoind.rpc.gettxout(splice_txid, splice_out) is None +@pytest.mark.openchannel('v1') +@pytest.mark.openchannel('v2') +@unittest.skipIf(TEST_NETWORK != 'regtest', 'elementsd doesnt yet support PSBT features we need') +def test_splice_holds_shutdown_until_locked(node_factory, bitcoind, executor): + """We don't send shutdown while a signed splice is unlocked: the close + waits for it, takes an RBF meanwhile, survives a restart, and then spends + the splice output.""" + l1, l2 = node_factory.line_graph(2, fundamount=1000000, wait_for_announce=True, + opts={'may_reconnect': True}) + chan_id = l1.get_channel_id(l2) + # Deep enough that the blocks a restart steps back keep it announced. + bitcoind.generate_block(5) + sync_blockheight(bitcoind, [l1, l2]) + + def splice_out(amount): + result = l1.rpc.splice_init(chan_id, amount, l1.rpc.addpsbtoutput(100000)['psbt']) + result = l1.rpc.splice_update(chan_id, result['psbt']) + result = l1.rpc.splice_update(chan_id, result['psbt']) + assert result['commitments_secured'] is True + return l1.rpc.splice_signed(chan_id, result['psbt'])['txid'] + + splice_txid = splice_out(-105801) + for n in (l1, l2): + n.daemon.wait_for_log(r'CHANNELD_NORMAL to CHANNELD_AWAITING_SPLICE') + wait_for(lambda: bitcoind.rpc.getrawmempool() == [splice_txid]) + + def held(n): + return any('Waiting for the splice to lock before sending shutdown' in s + for s in only_one(n.rpc.listpeerchannels()['channels'])['status']) + + fut = executor.submit(l2.rpc.close, chan_id) + l2.daemon.wait_for_log(r'CHANNELD_AWAITING_SPLICE to CHANNELD_SHUTTING_DOWN') + wait_for(lambda: held(l2)) + assert only_one(l1.rpc.listpeerchannels()['channels'])['state'] == 'CHANNELD_AWAITING_SPLICE' + + # l1 can still bump the splice, and l2 stays shutting down. + rbf_txid = splice_out(-111722) + l2.daemon.wait_for_log('Watching splice inflight {}'.format(rbf_txid)) + wait_for(lambda: bitcoind.rpc.getrawmempool() == [rbf_txid]) + assert only_one(l2.rpc.listpeerchannels()['channels'])['state'] == 'CHANNELD_SHUTTING_DOWN' + wait_for(lambda: held(l2)) + + # Still waiting after a restart. + l2.restart() + fut.exception(TIMEOUT) + l2.rpc.connect(l1.info['id'], 'localhost', l1.port) + wait_for(lambda: held(l2)) + assert only_one(l2.rpc.listpeerchannels()['channels'])['state'] == 'CHANNELD_SHUTTING_DOWN' + assert not l2.daemon.is_in_log('peer_out WIRE_SHUTDOWN') + assert not l1.daemon.is_in_log('peer_in WIRE_SHUTDOWN') + + # Once it locks, the shutdown goes and the close spends the splice. + bitcoind.generate_block(6, wait_for_mempool=rbf_txid) + l2.daemon.wait_for_log('peer_out WIRE_SHUTDOWN') + for n in (l1, l2): + wait_for(lambda: only_one(n.rpc.listpeerchannels()['channels'])['state'] == 'CLOSINGD_COMPLETE') + chan = only_one(l1.rpc.listpeerchannels()['channels']) + assert chan['funding_txid'] == rbf_txid + assert chan['to_us_msat'] == 1000000000 - 111722000 + close_txid = only_one(bitcoind.rpc.getrawmempool()) + close_tx = bitcoind.rpc.getrawtransaction(close_txid, True) + assert only_one(close_tx['vin'])['txid'] == rbf_txid + + bitcoind.generate_block(1, wait_for_mempool=close_txid) + for n in (l1, l2): + n.daemon.wait_for_log(r'Resolved FUNDING_TRANSACTION/FUNDING_OUTPUT by MUTUAL_CLOSE') + moves = n.rpc.listchainmoves()['chainmoves'] + assert [m for m in moves + if m['primary_tag'] == 'channel_open' + and m['utxo'].startswith(rbf_txid)] + + @pytest.mark.openchannel('v1') @pytest.mark.openchannel('v2') @unittest.skipIf(TEST_NETWORK != 'regtest', 'elementsd doesnt yet support PSBT features we need')