splice: prevent splice going to onchaind & race prevention

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

@@ -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);

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 {

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;

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);
 }
 

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;
 
-		inflight->last_tx = tal_steal(inflight, commit->tx);
+		if (inflight->splice_locked_memonly)
+			continue;
+
+		commit = inflight_commit_sigs[i];
+
+		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);

tests/test_splicing.py

@@ -2,6 +2,7 @@ from fixtures import *  # noqa: F401,F403
 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

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);