From c67f1f92a845dd067be310899eb99ca23a369989 Mon Sep 17 00:00:00 2001 From: Dusty Daemon Date: Tue, 15 Aug 2023 22:58:53 -0400 Subject: [PATCH] splice: prevent splice going to onchaind & race prevention MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Don’t send the funding spend to onchaind if we detect it in inflights (aka. a splice). While we already prevented onchaind_funding_spent from being called directly, the call to wallet_channeltxs_add meant onchaind_funding_spent would be called *anyway* on restart. This is now fixed. Additionally there was a potential for a race problem depending on the firing order of the channel depth and and funding spent events. Instead of requiring these events fire in a specific order, we make a special “memory only” inflight object to prevent the race regardless of firing order. Changelog-Fixed: Splice: bugfix for restart related race condition interacting with adversarial close detection. --- lightningd/channel.c | 1 + lightningd/channel.h | 10 ++++++++++ lightningd/channel_control.c | 26 ++++++++++++++++++++++---- lightningd/peer_control.c | 14 +++++++++++--- lightningd/peer_htlcs.c | 13 ++++++++++--- tests/test_splicing.py | 5 +++++ wallet/wallet.c | 3 ++- 7 files changed, 61 insertions(+), 11 deletions(-) diff --git a/lightningd/channel.c b/lightningd/channel.c index d830aca527bc..60d1c19cd48e 100644 --- a/lightningd/channel.c +++ b/lightningd/channel.c @@ -176,6 +176,7 @@ new_inflight(struct channel *channel, inflight->lease_amt = lease_amt; inflight->i_am_initiator = i_am_initiator; + inflight->splice_locked_memonly = false; list_add_tail(&channel->inflights, &inflight->list); tal_add_destructor(inflight, destroy_inflight); diff --git a/lightningd/channel.h b/lightningd/channel.h index cb3d6eeb9249..33ad7d9ef38b 100644 --- a/lightningd/channel.h +++ b/lightningd/channel.h @@ -63,6 +63,16 @@ struct channel_inflight { /* Did I initate this splice attempt? */ bool i_am_initiator; + + /* Note: This field is not stored in the database. + * + * After splice_locked, we need a way to stop the chain watchers from + * thinking the old channel was spent. + * + * Leaving the inflight in memory-only with splice_locked true leaves + * moves the responsiblity of cleaning up the inflight to the watcher, + * avoiding any potential race conditions. */ + bool splice_locked_memonly; }; struct open_attempt { diff --git a/lightningd/channel_control.c b/lightningd/channel_control.c index ab698564b068..1d03efcad41a 100644 --- a/lightningd/channel_control.c +++ b/lightningd/channel_control.c @@ -645,8 +645,10 @@ static void handle_update_inflight(struct lightningd *ld, struct bitcoin_txid, &txid)); - if (last_tx) - inflight->last_tx = tal_steal(inflight, last_tx); + if (last_tx) { + tal_free(inflight->last_tx); + inflight->last_tx = clone_bitcoin_tx(inflight, last_tx); + } if (last_sig) inflight->last_sig = *last_sig; @@ -825,10 +827,21 @@ 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); - /* Empty out the inflights */ log_debug(channel->log, "lightningd, splice_locked clearing inflights"); + + /* Take out the successful inflight from the list temporarily */ + list_del(&inflight->list); + wallet_channel_clear_inflights(channel->peer->ld->wallet, channel); + /* Put the successful inflight back in as a memory-only object. + * peer_control's funding_spent function will pick this up and clean up + * our inflight. + * + * This prevents any potential race conditions between us and them. */ + inflight->splice_locked_memonly = true; + list_add_tail(&channel->inflights, &inflight->list); + lockin_complete(channel, CHANNELD_AWAITING_SPLICE); } @@ -1394,7 +1407,12 @@ bool peer_start_channeld(struct channel *channel, inflights = tal_arr(tmpctx, struct inflight *, 0); list_for_each(&channel->inflights, inflight, list) { - struct inflight *infcopy = tal(inflights, struct inflight); + struct inflight *infcopy; + + if (inflight->splice_locked_memonly) + continue; + + infcopy = tal(inflights, struct inflight); infcopy->outpoint = inflight->funding->outpoint; infcopy->amnt = inflight->funding->total_funds; diff --git a/lightningd/peer_control.c b/lightningd/peer_control.c index eef640bdf927..7df4d5ee5c56 100644 --- a/lightningd/peer_control.c +++ b/lightningd/peer_control.c @@ -1953,18 +1953,26 @@ static enum watch_result funding_spent(struct channel *channel, bitcoin_txid(tx, &txid); - wallet_channeltxs_add(channel->peer->ld->wallet, channel, - WIRE_ONCHAIND_INIT, &txid, 0, block->height); - /* If we're doing a splice, we expect the funding transaction to be * spent, so don't freak out and just keep watching in that case */ list_for_each(&channel->inflights, inflight, list) { if (bitcoin_txid_eq(&txid, &inflight->funding->outpoint.txid)) { + /* splice_locked is a special flag that indicates this + * is a memory-only inflight acting as a race condition + * safeguard. When we see this, it is our responsability + * to clean up this memory-only inflight. */ + if (inflight->splice_locked_memonly) { + tal_free(inflight); + return DELETE_WATCH; + } return KEEP_WATCHING; } } + wallet_channeltxs_add(channel->peer->ld->wallet, channel, + WIRE_ONCHAIND_INIT, &txid, 0, block->height); + return onchaind_funding_spent(channel, tx, block->height); } diff --git a/lightningd/peer_htlcs.c b/lightningd/peer_htlcs.c index 242f29de9cb3..ee04751f0300 100644 --- a/lightningd/peer_htlcs.c +++ b/lightningd/peer_htlcs.c @@ -2320,7 +2320,8 @@ void peer_got_commitsig(struct channel *channel, const u8 *msg) i = 0; list_for_each(&channel->inflights, inflight, list) { - i++; + if (!inflight->splice_locked_memonly) + i++; } if (i != tal_count(inflight_commit_sigs)) { channel_internal_error(channel, "Got commitsig with incorrect " @@ -2381,9 +2382,15 @@ void peer_got_commitsig(struct channel *channel, const u8 *msg) /* Now append htlc sigs for inflights */ i = 0; list_for_each(&channel->inflights, inflight, list) { - struct commitsig *commit = inflight_commit_sigs[i]; + struct commitsig *commit; + + if (inflight->splice_locked_memonly) + continue; + + commit = inflight_commit_sigs[i]; - inflight->last_tx = tal_steal(inflight, commit->tx); + tal_free(inflight->last_tx); + inflight->last_tx = clone_bitcoin_tx(inflight, commit->tx); inflight->last_tx->chainparams = chainparams; inflight->last_sig = commit->commit_signature; wallet_inflight_save(ld->wallet, inflight); diff --git a/tests/test_splicing.py b/tests/test_splicing.py index f59017454ee4..959e4d4dc614 100644 --- a/tests/test_splicing.py +++ b/tests/test_splicing.py @@ -2,6 +2,7 @@ from utils import TEST_NETWORK import pytest import unittest +import time @pytest.mark.openchannel('v1') @@ -34,3 +35,7 @@ def test_splice(node_factory, bitcoind): inv = l2.rpc.invoice(10**2, '3', 'no_3') l1.rpc.pay(inv['bolt11']) + + # Check that the splice doesn't generate a unilateral close transaction + time.sleep(5) + assert l1.db_query("SELECT count(*) as c FROM channeltxs;")[0]['c'] == 0 diff --git a/wallet/wallet.c b/wallet/wallet.c index ff9f448992fd..6275f29fc4f2 100644 --- a/wallet/wallet.c +++ b/wallet/wallet.c @@ -2323,7 +2323,8 @@ void wallet_channel_save(struct wallet *w, struct channel *chan) /* Update the inflights also */ struct channel_inflight *inflight; list_for_each(&chan->inflights, inflight, list) - wallet_inflight_save(w, inflight); + if (!inflight->splice_locked_memonly) + wallet_inflight_save(w, inflight); db_bind_talarr(stmt, last_sent_commit); db_bind_u64(stmt, chan->dbid);