lightningd: mutual close no longer falls back to broadcasting the commitment

closingd/closingd.c

@@ -215,10 +215,12 @@ static void send_offer(struct per_peer_state *pps,
 	peer_write(pps, take(msg));
 }
 
-static void tell_master_their_offer(const struct bitcoin_signature *their_sig,
+/* Returns false if master says we must not agree to this offer. */
+static bool tell_master_their_offer(const struct bitcoin_signature *their_sig,
 				    const struct bitcoin_tx *tx,
 				    struct bitcoin_txid *tx_id)
 {
+	bool acceptable;
 	u8 *msg = towire_closingd_received_signature(NULL, their_sig, tx);
 	if (!wire_sync_write(REQ_FD, take(msg)))
 		status_failed(STATUS_FAIL_MASTER_IO,
@@ -227,9 +229,11 @@ static void tell_master_their_offer(const struct bitcoin_signature *their_sig,
 
 	/* Wait for master to ack, to make sure it's in db. */
 	msg = wire_sync_read(NULL, REQ_FD);
-	if (!fromwire_closingd_received_signature_reply(msg, tx_id))
+	if (!fromwire_closingd_received_signature_reply(msg, tx_id,
+							&acceptable))
 		master_badmsg(WIRE_CLOSINGD_RECEIVED_SIGNATURE_REPLY, msg);
 	tal_free(msg);
+	return acceptable;
 }
 
 /* Returns fee they offered. */
@@ -384,7 +388,17 @@ receive_offer(struct per_peer_state *pps,
 	/* Master sorts out what is best offer, we just tell it any above min */
 	if (amount_sat_greater_eq(received_fee, min_fee_to_accept)) {
 		status_debug("...offer is reasonable");
-		tell_master_their_offer(&their_sig, tx, closing_txid);
+		/* Our own closing_signed for this round has usually gone
+		 * out by now (the opener sends first), so if their fee
+		 * matched ours they hold both signatures and can broadcast
+		 * the close whatever we do here.  Refusing only keeps us
+		 * from recording the close as agreed.  lightningd checks
+		 * the fee against the same bounds we negotiate within, so
+		 * this is not expected to fire. */
+		if (!tell_master_their_offer(&their_sig, tx, closing_txid))
+			peer_failed_warn(pps, channel_id,
+					 "Closing fee %s is outside our fee limits",
+					 fmt_amount_sat(tmpctx, received_fee));
 	}
 
 	return received_fee;

closingd/closingd_wire.csv

@@ -44,6 +44,8 @@ msgdata,closingd_received_signature,tx,bitcoin_tx,
 
 msgtype,closingd_received_signature_reply,2102
 msgdata,closingd_received_signature_reply,closing_txid,bitcoin_txid,
+# Whether we may agree to this offer at all.
+msgdata,closingd_received_signature_reply,acceptable,bool,
 
 # Negotiations complete, we're exiting.
 msgtype,closingd_complete,2004

lightningd/closing_control.c

@@ -211,13 +211,62 @@ static u32 calc_max_close_feerate(struct lightningd *ld,
 	return max_feerate;
 }
 
+/* The fee closingd negotiated is what it took off our output (we only
+ * bound the fee when we are the opener, and the opener pays it), and
+ * closingd bounded it at the weight of the closing transaction before
+ * any fee came off, with our output still present.  The transaction
+ * itself pays more than that whenever the outputs' msat remainders were
+ * rounded away or the fee trimmed our output as dust: neither is a fee
+ * we chose, so neither counts against our maximum.  *weight is the
+ * transaction's weight on entry and the weight closingd bounded the fee
+ * at on return. */
+static bool negotiated_close_fee(const struct channel *channel,
+				 const struct bitcoin_tx *tx,
+				 struct amount_sat *fee,
+				 u64 *weight)
+{
+	struct amount_sat ours = amount_msat_to_sat_round_down(channel->our_msat);
+	const u8 *our_script = channel->shutdown_scriptpubkey[LOCAL];
+	struct amount_sat out_amt;
+
+	for (size_t i = 0; i < tx->wtx->num_outputs; i++) {
+		const struct wally_tx_output *out = &tx->wtx->outputs[i];
+		const u8 *script = tal_dup_arr(tmpctx, u8,
+					       out->script, out->script_len, 0);
+		if (!scripteq(script, our_script))
+			continue;
+		out_amt = bitcoin_tx_output_get_amount_sat(tx, i);
+		if (!amount_sat_sub(fee, ours, out_amt)) {
+			/* closingd built the tx from this same balance, so
+			 * this cannot underflow; count the whole fee if it
+			 * does. */
+			log_broken(channel->log,
+				   "Closing tx output %zu pays us %s,"
+				   " more than our balance %s",
+				   i, fmt_amount_sat(tmpctx, out_amt),
+				   fmt_amount_sat(tmpctx, ours));
+			return false;
+		}
+		return true;
+	}
+
+	/* Our output was trimmed.  closingd drops it once the fee leaves
+	 * it below the dust limit, so the fee is at least our balance less
+	 * that, and the weight closingd bounded it at included the
+	 * output. */
+	if (!amount_sat_sub(fee, ours, channel->our_config.dust_limit))
+		*fee = AMOUNT_SAT(0);
+	*weight += bitcoin_tx_output_weight(tal_bytelen(our_script));
+	return true;
+}
+
 /* Assess whether a proposed closing fee is acceptable. */
 static bool closing_fee_is_acceptable(struct lightningd *ld,
 				      struct channel *channel,
 				      const struct bitcoin_tx *tx)
 {
-	struct amount_sat fee, last_fee;
-	u64 weight;
+	struct amount_sat fee, last_fee, negotiated;
+	u64 weight, negotiated_weight;
 
 	/* Calculate actual fee (adds in eliminated outputs) */
 	fee = calc_tx_fee(channel->funding_sats, tx);
@@ -234,12 +283,21 @@ static bool closing_fee_is_acceptable(struct lightningd *ld,
 		  fmt_amount_sat(tmpctx, last_fee),
 		  weight);
 
+	if (ld->dev_reject_closing_fee) {
+		log_debug(channel->log, "... dev-reject-closing-fee");
+		return false;
+	}
+
 	if (!channel->ignore_fee_limits && !ld->config.ignore_fee_limits) {
 		struct amount_sat min_fee, max_fee;
 		u32 min_feerate, max_feerate;
 
 		/* If we don't have a feerate estimate, this gives feerate_floor */
 		min_feerate = feerate_min(ld, NULL);
+		/* A feerange given to `close` is what closingd negotiated
+		 * within; its minimum is our floor too. */
+		if (channel->closing_feerate_range)
+			min_feerate = channel->closing_feerate_range[0];
 		max_feerate = calc_max_close_feerate(ld, channel);
 
 		min_fee = amount_tx_fee(min_feerate, weight);
@@ -250,12 +308,19 @@ static bool closing_fee_is_acceptable(struct lightningd *ld,
 				  weight, min_feerate);
 			return false;
 		}
-		max_fee = amount_tx_fee(max_feerate, weight);
-		if (channel->opener == LOCAL && amount_sat_less(max_fee, fee)) {
-			log_debug(channel->log, "... That's above our max %s"
-				  " for weight %"PRIu64" at feerate %u",
+		negotiated_weight = weight;
+		if (!negotiated_close_fee(channel, tx, &negotiated,
+					  &negotiated_weight))
+			negotiated = fee;
+		max_fee = amount_tx_fee(max_feerate, negotiated_weight);
+		if (channel->opener == LOCAL
+		    && amount_sat_less(max_fee, negotiated)) {
+			log_debug(channel->log, "... Negotiated fee %s is above"
+				  " our max %s for weight %"PRIu64
+				  " at feerate %u",
+				  fmt_amount_sat(tmpctx, negotiated),
 				  fmt_amount_sat(tmpctx, max_fee),
-				  weight, max_feerate);
+				  negotiated_weight, max_feerate);
 			return false;
 		}
 	}
@@ -274,6 +339,7 @@ static void peer_received_closing_signature(struct channel *channel,
 	struct bitcoin_txid tx_id;
 	struct lightningd *ld = channel->peer->ld;
 	u8 *funding_wscript;
+	bool acceptable;
 
 	if (!fromwire_closingd_received_signature(msg, msg, &sig, &tx)) {
 		channel_internal_error(channel,
@@ -302,17 +368,23 @@ static void peer_received_closing_signature(struct channel *channel,
 		return;
 	}
 
-	if (closing_fee_is_acceptable(ld, channel, tx)) {
+	acceptable = closing_fee_is_acceptable(ld, channel, tx);
+	if (acceptable) {
 		channel_set_last_tx(channel, tx, &sig);
 		wallet_channel_save(ld->wallet, channel);
-	}
-
-
-	// Send back the txid so we can update the billboard on selection.
+	} else
+		log_unusual(channel->log,
+			    "Rejecting peer's closing fee offer:"
+			    " closingd must not agree to it");
+
+	/* Send back the txid so closingd can update the billboard, and
+	 * whether it may agree to this offer at all.  Without the verdict
+	 * a rejected offer would still complete the close, and last_tx,
+	 * still the commitment, would be broadcast as the mutual close. */
 	bitcoin_txid(channel->last_tx, &tx_id);
-	/* OK, you can continue now. */
 	subd_send_msg(channel->owner,
-		      take(towire_closingd_received_signature_reply(channel, &tx_id)));
+		      take(towire_closingd_received_signature_reply(channel, &tx_id,
+								    acceptable)));
 }
 
 static void peer_closing_complete(struct channel *channel, const u8 *msg)

lightningd/lightningd.c

@@ -131,6 +131,7 @@ static struct lightningd *new_lightningd(const tal_t *ctx)
 	ld->dev_throttle_gossip = false;
 	ld->dev_suppress_gossip = false;
 	ld->dev_fast_reconnect = false;
+	ld->dev_reject_closing_fee = false;
 	ld->dev_force_privkey = NULL;
 	ld->dev_force_bip32_seed = NULL;
 	ld->dev_force_channel_secrets = NULL;

lightningd/lightningd.h

@@ -322,6 +322,9 @@ struct lightningd {
 	/* Speedup reconnect delay, for testing. */
 	bool dev_fast_reconnect;
 
+	/* Reject every closing fee the peer offers. */
+	bool dev_reject_closing_fee;
+
 	/* This is the forced private key for the node. */
 	struct privkey *dev_force_privkey;
 

lightningd/options.c

@@ -801,6 +801,10 @@ static void dev_register_opts(struct lightningd *ld)
 		     opt_set_bool,
 		     &ld->dev_fast_reconnect,
 		     "Make max default reconnect delay 3 (not 300) seconds");
+	clnopt_noarg("--dev-reject-closing-fee", OPT_DEV,
+		     opt_set_bool,
+		     &ld->dev_reject_closing_fee,
+		     "Reject every closing fee the peer offers, as if outside our limits");
 
 	clnopt_noarg("--dev-fail-on-subdaemon-fail", OPT_DEV,
 		     opt_set_bool,

tests/test_closing.py

@@ -4321,6 +4321,157 @@ def test_closing_minfee(node_factory, bitcoind):
     bitcoind.generate_block(1, wait_for_mempool=txid)
 
 
+@unittest.skipIf(TEST_NETWORK != 'regtest', 'elementsd anchors not supportd')
+def test_closing_fee_rounding_at_ceiling(node_factory, bitcoind):
+    """A close pinned at the fee ceiling stays a mutual close.
+
+    With every estimate at the floor, the opener's closing fee range is
+    a single value.  Each output is rounded down to whole satoshis, so
+    the msat remainders end up in the fee and the transaction pays one
+    satoshi more than the agreed fee.  lightningd must still accept it
+    rather than fall back to broadcasting the commitment.
+    """
+    l1, l2 = node_factory.line_graph(2, opts={'feerates': (253, 253, 253, 253)})
+    chan = only_one(l1.rpc.listpeerchannels(l2.info['id'])['channels'])
+    funding = int(Millisatoshi(chan['total_msat']).to_satoshi())
+
+    # Leave remainders which sum to exactly 1000msat: l1 keeps ...999msat,
+    # l2 gets ...001msat.  Rounding both down costs one satoshi of fee.
+    l1.pay(l2, 100000001)
+    wait_for(lambda: only_one(l1.rpc.listpeerchannels()['channels'])['htlcs'] == [])
+
+    fee = closing_fee(253, 2)
+    res = l1.rpc.close(l2.info['id'])
+    assert res['type'] == 'mutual'
+    tx = bitcoind.rpc.decoderawtransaction(only_one(res['txs']))
+
+    # A closing transaction, not the commitment.
+    assert len(tx['vout']) == 2
+    assert tx['locktime'] == 0
+
+    # The agreed fee plus the rounded-off remainders.
+    paid = funding - sum(int(round(o['value'] * 10**8)) for o in tx['vout'])
+    assert paid == fee + 1
+    billboard = only_one(l1.rpc.listpeerchannels(l2.info['id'])['channels'])['status']
+    assert billboard == ['CLOSINGD_SIGEXCHANGE:We agreed on a closing fee of {} satoshi for tx:{}'.format(fee, tx['txid'])]
+
+    bitcoind.generate_block(1, wait_for_mempool=tx['txid'])
+    wait_for(lambda: 'ONCHAIN:Tracking mutual close transaction' in only_one(l1.rpc.listpeerchannels(l2.info['id'])['channels'])['status'])
+    assert tx['txid'] in [o['txid'] for o in l1.rpc.listfunds()['outputs']]
+    assert tx['txid'] in [o['txid'] for o in l2.rpc.listfunds()['outputs']]
+
+
+@unittest.skipIf(TEST_NETWORK != 'regtest', 'elementsd anchors not supportd')
+def test_closing_fee_trims_opener_output(node_factory, bitcoind):
+    """A close whose fee leaves the opener's output below dust stays mutual.
+
+    closingd bounds the fee it agrees to at the weight of a closing
+    transaction with both outputs, and drops the opener's output once the
+    fee leaves it below the dust limit.  lightningd then sees a one-output
+    transaction paying the opener's whole balance as fee.  It must bound
+    the fee closingd agreed to, at the weight closingd used, rather than
+    reject the close and fall back to the commitment.
+    """
+    l1, l2 = node_factory.line_graph(2, opts={'feerates': (253, 253, 253, 253)})
+    chan = only_one(l1.rpc.listpeerchannels(l2.info['id'])['channels'])
+    funding = int(Millisatoshi(chan['total_msat']).to_satoshi())
+    dust = int(Millisatoshi(chan['dust_limit_msat']).to_satoshi())
+
+    # Drain the opener to what it has to keep, in whole satoshis so no
+    # msat remainder reaches the fee.
+    spendable = int(chan['spendable_msat'])
+    l1.pay(l2, spendable - spendable % 1000)
+    wait_for(lambda: only_one(l1.rpc.listpeerchannels()['channels'])['htlcs'] == [])
+    chan = only_one(l1.rpc.listpeerchannels(l2.info['id'])['channels'])
+    ours = int(Millisatoshi(chan['to_us_msat']).to_satoshi())
+
+    # A feerate whose two-output closing fee leaves l1 about half the
+    # dust limit, so its output is trimmed.
+    feerate = (ours - dust // 2) * 1000 // closing_fee(1000, 2)
+    fee = closing_fee(feerate, 2)
+    assert ours - dust < fee <= ours
+
+    res = l1.rpc.close(l2.info['id'], unilateraltimeout=10,
+                       feerange=['{}perkw'.format(feerate)] * 2)
+    assert res['type'] == 'mutual'
+    tx = bitcoind.rpc.decoderawtransaction(only_one(res['txs']))
+
+    # A closing transaction with only l2's output, not the commitment.
+    assert len(tx['vout']) == 1
+    assert tx['locktime'] == 0
+
+    # l1's whole balance is fee: the agreed fee plus the trimmed rest.
+    paid = funding - int(round(only_one(tx['vout'])['value'] * 10**8))
+    assert paid == ours
+    billboard = only_one(l1.rpc.listpeerchannels(l2.info['id'])['channels'])['status']
+    assert billboard == ['CLOSINGD_SIGEXCHANGE:We agreed on a closing fee of {} satoshi for tx:{}'.format(fee, tx['txid'])]
+
+    bitcoind.generate_block(1, wait_for_mempool=tx['txid'])
+    wait_for(lambda: 'ONCHAIN:Tracking mutual close transaction' in only_one(l1.rpc.listpeerchannels(l2.info['id'])['channels'])['status'])
+    assert tx['txid'] not in [o['txid'] for o in l1.rpc.listfunds()['outputs']]
+    assert tx['txid'] in [o['txid'] for o in l2.rpc.listfunds()['outputs']]
+
+
+@unittest.skipIf(TEST_NETWORK != 'regtest', 'elementsd anchors not supportd')
+def test_closing_feerange_below_estimates(node_factory, bitcoind):
+    """A close with a feerange below the estimate floor stays a mutual close.
+
+    closingd negotiates within the feerange given to `close`, but
+    lightningd checked the agreed fee against the floor derived from its
+    fee estimates.  With estimates above the range, the agreed fee was
+    rejected as too low and the commitment was broadcast instead.
+    """
+    l1, l2 = node_factory.line_graph(2, opts={'feerates': (253, 253, 253, 253),
+                                              'may_reconnect': True})
+    l1.pay(l2, 100000000)
+    wait_for(lambda: only_one(l1.rpc.listpeerchannels()['channels'])['htlcs'] == [])
+
+    # l1's estimate floor becomes 1000perkw, well above the range.
+    l1.force_feerates(2000)
+    l1.rpc.connect(l2.info['id'], 'localhost', l2.port)
+
+    fee = closing_fee(253, 2)
+    res = l1.rpc.close(l2.info['id'], feerange=['253perkw', '253perkw'])
+    assert res['type'] == 'mutual'
+    tx = bitcoind.rpc.decoderawtransaction(only_one(res['txs']))
+
+    # A closing transaction at the agreed fee, not the commitment.
+    assert len(tx['vout']) == 2
+    assert tx['locktime'] == 0
+    billboard = only_one(l1.rpc.listpeerchannels(l2.info['id'])['channels'])['status']
+    assert 'CLOSINGD_SIGEXCHANGE:We agreed on a closing fee of {} satoshi for tx:{}'.format(fee, tx['txid']) in billboard
+
+    bitcoind.generate_block(1, wait_for_mempool=tx['txid'])
+    wait_for(lambda: 'ONCHAIN:Tracking mutual close transaction' in only_one(l1.rpc.listpeerchannels(l2.info['id'])['channels'])['status'])
+    assert tx['txid'] in [o['txid'] for o in l1.rpc.listfunds()['outputs']]
+    assert tx['txid'] in [o['txid'] for o in l2.rpc.listfunds()['outputs']]
+
+
+def test_closing_rejected_fee_fails_negotiation(node_factory, bitcoind):
+    """A closing fee lightningd rejects ends the negotiation.
+
+    closingd only learns the txid from lightningd's reply, never the
+    verdict, so it agrees to a rejected offer and lightningd broadcasts
+    the commitment as if it were the mutual close.  The known reasons for
+    a rejection are fixed, so --dev-reject-closing-fee forces one.
+    """
+    l1, l2 = node_factory.line_graph(2, opts=[{}, {'dev-reject-closing-fee': None}])
+    l1.pay(l2, 100000000)
+    wait_for(lambda: only_one(l1.rpc.listpeerchannels()['channels'])['htlcs'] == [])
+
+    # l2 refuses l1's offer, so nobody completes the negotiation and l1
+    # closes unilaterally when its timeout expires.
+    res = l1.rpc.close(l2.info['id'], unilateraltimeout=10)
+    assert res['type'] == 'unilateral'
+    assert not l2.daemon.is_in_log('We agreed on a closing fee')
+    l2.daemon.wait_for_log('outside our fee limits')
+
+    # The only transaction on the wire is that unilateral close: with no
+    # HTLCs at stake, nothing spends its anchor to hurry it along.
+    txid = only_one(res['txids'])
+    wait_for(lambda: bitcoind.rpc.getrawmempool() == [txid])
+
+
 @unittest.skipIf(TEST_NETWORK != 'regtest', 'elementsd anchors not supportd')
 def test_peer_anchor_push(node_factory, bitcoind, executor, chainparams):
     """Test that we use anchor on peer's commit to CPFP tx"""