anchors: dont make anchorspend txs for in-chain commitments

We were seeing occassional spends of commitment tx utxos in
anchorspend txs, which was causing occasional test fails.

We shouldn't be sending anchorspends at all for onchain funding-spends.
The logic is in place for this, but the ordering of the state update to
channel was incorrect, which lead to loglines as follows
(we'd expect to see failure in ONCHAIN not NORMAL), given that the
reason=onchain...

"Peer permanent failure in CHANNELD_NORMAL: Funding transaction spent (reason=onchain)"

A quick reordering of the state change + firing of `drop_to_chain` fixes
the issue of sending the anchorspend tx, which removes the race case
of onchaind adding utxos from the commitment tx to the utxoset, where
the anchorspend construction was picking them up.

Fixes ElementsProject#7526

lightningd/onchain_control.c

@@ -1553,14 +1553,6 @@ enum watch_result onchaind_funding_spent(struct channel *channel,
 	int hsmfd;
 	enum state_change reason;
 
-	/* use REASON_ONCHAIN or closer's reason, if known */
-	reason = REASON_ONCHAIN;
-	if (channel->closer != NUM_SIDES)
-		reason = REASON_UNKNOWN;  /* will use last cause as reason */
-
-	channel_fail_permanent(channel, reason,
-			       "Funding transaction spent");
-
 	/* If we haven't posted the open event yet, post an open */
 	if (!channel->scid || !channel->remote_channel_ready) {
 		u32 blkh;
@@ -1573,6 +1565,12 @@ enum watch_result onchaind_funding_spent(struct channel *channel,
 	tal_free(channel->close_blockheight);
 	channel->close_blockheight = tal_dup(channel, u32, &blockheight);
 
+	/* use REASON_ONCHAIN or closer's reason, if known */
+	reason = REASON_ONCHAIN;
+	if (channel->closer != NUM_SIDES)
+		reason = REASON_UNKNOWN;  /* will use last cause as reason */
+
+
 	/* We could come from almost any state. */
 	/* NOTE(mschmoock) above comment is wrong, since we failed above! */
 	channel_set_state(channel,
@@ -1581,6 +1579,9 @@ enum watch_result onchaind_funding_spent(struct channel *channel,
 			  reason,
 			  tal_fmt(tmpctx, "Onchain funding spend"));
 
+	channel_fail_permanent(channel, reason,
+			       "Funding transaction spent");
+
 	hsmfd = hsm_get_client_fd(ld, &channel->peer->id,
 				  channel->dbid,
 				  HSM_PERM_SIGN_ONCHAIN_TX

tests/test_closing.py

@@ -2392,35 +2392,45 @@ def try_pay():
     assert account_balance(l2, channel_id) == 0
     assert account_balance(l1, channel_id) == 0
 
-    # Graph of coin_move events we expect
-    expected_1 = {
-        '0': [('wallet', ['deposit'], ['withdrawal'], 'A')],
-        # This is ugly, but this wallet deposit is either unspent or used
-        # in the next channel open
-        'A': [('wallet', ['deposit'], None, None), ('cid1', ['channel_open', 'opener'], ['channel_close'], 'B')],
-        'B': [('wallet', ['deposit'], None, None), ('cid1', ['htlc_timeout'], ['to_wallet'], 'C')],
-        'C': [('wallet', ['deposit'], None, None)],
-    }
+    # Graph of coin_move events we expect!
+    if anchors:
+        expected_1 = {
+            # Initial wallet deposit
+            '0': [('wallet', ['deposit'], ['withdrawal'], 'A')],
+            # Funding tx
+            'A': [('wallet', ['deposit'], None, None), ('cid1', ['channel_open', 'opener'], ['channel_close'], 'B')],
+            # Commitment tx
+            'B': [('wallet', ['deposit'], None, None), ('cid1', ['htlc_timeout'], ['to_wallet'], 'C'), ('external', ['anchor'], None, None), ('wallet', ['anchor', 'ignored'], None, None)],
+            # HTLC timeout tx
+            'C': [('wallet', ['deposit'], None, None)],
+        }
 
-    expected_2 = {
-        'A': [('cid1', ['channel_open'], ['channel_close'], 'B')],
-        'B': [('external', ['to_them'], None, None), ('external', ['htlc_timeout'], None, None)],
-    }
+        expected_2 = {
+            # Funding tx
+            'A': [('cid1', ['channel_open'], ['channel_close'], 'B')],
+            # Commitment tx
+            'B': [('external', ['to_them'], None, None), ('external', ['htlc_timeout'], None, None), ('external', ['anchor'], None, None), ('wallet', ['anchor', 'ignored'], None, None)],
+        }
+    else:
+        expected_1 = {
+            '0': [('wallet', ['deposit'], ['withdrawal'], 'A')],
+            # This is ugly, but this wallet deposit is either unspent or used
+            # in the next channel open
+            'A': [('wallet', ['deposit'], None, None), ('cid1', ['channel_open', 'opener'], ['channel_close'], 'B')],
+            'B': [('wallet', ['deposit'], None, None), ('cid1', ['htlc_timeout'], ['to_wallet'], 'C')],
+            'C': [('wallet', ['deposit'], None, None)],
+        }
 
-    if anchors:
-        expected_1['B'].append(('external', ['anchor'], None, None))
-        expected_2['B'].append(('external', ['anchor'], None, None))
-        expected_1['B'].append(('wallet', ['anchor', 'ignored'], None, None))
-        expected_2['B'].append(('wallet', ['anchor', 'ignored'], None, None))
+        expected_2 = {
+            'A': [('cid1', ['channel_open'], ['channel_close'], 'B')],
+            'B': [('external', ['to_them'], None, None), ('external', ['htlc_timeout'], None, None)],
+        }
 
-    # FIXME: Why does this fail?
-    if not anchors:
-        tags = check_utxos_channel(l1, [channel_id], expected_1)
-        check_utxos_channel(l2, [channel_id], expected_2, tags)
+    tags = check_utxos_channel(l1, [channel_id], expected_1)
+    check_utxos_channel(l2, [channel_id], expected_2, tags)
 
     # Check 'bkpr-inspect' and 'bkpr-listbalances'
-    # The wallet events aren't in the channel's events
-    del expected_1['0']
+    del expected_1['0']  # Tx '0' was the initial deposit, its not in channel's events
     expected_1['A'] = expected_1['A'][1:]
     check_inspect_channel(l1, channel_id, expected_1)