lightningd: always broadcast the latest close tx

Before this patch we would only update `channel->last_tx` with the newly
proposed closure tx from the peer if the fee of the new one was lower.

In negotiations where we are at the higher end and the peer starts
lower, all peer's subsequent proposals will be higher than his initial
proposal and in this case we would never update `channel->last_tx`
and would wrongly broadcast his initial proposal at the end of the
negotiation.

Fixes https://github.com/ElementsProject/lightning/issues/3549

Changelog-Fixed: Always broadcast the latest close transaction at the end of the close fee negotiation, instead of sometimes broadcasting the peer's initial closing proposal.

lightningd/closing_control.c

@@ -49,12 +49,10 @@ static struct amount_sat calc_tx_fee(struct amount_sat sat_in,
 	return fee;
 }
 
-/* Is this better than the last tx we were holding?  This can happen
- * even without closingd misbehaving, if we have multiple,
- * interrupted, rounds of negotiation. */
-static bool better_closing_fee(struct lightningd *ld,
-			       struct channel *channel,
-			       const struct bitcoin_tx *tx)
+/* 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, min_fee;
 	u64 weight;
@@ -85,15 +83,10 @@ static bool better_closing_fee(struct lightningd *ld,
 		return false;
 	}
 
-	/* In case of a tie, prefer new over old: this covers the preference
+	/* Prefer new over old: this covers the preference
 	 * for a mutual close over a unilateral one. */
 
-	/* If we don't know the feerate, prefer higher fee. */
-	if (feerate_unknown)
-		return amount_sat_greater_eq(fee, last_fee);
-
-	/* Otherwise prefer lower fee. */
-	return amount_sat_less_eq(fee, last_fee);
+	return true;
 }
 
 static void peer_received_closing_signature(struct channel *channel,
@@ -112,7 +105,7 @@ static void peer_received_closing_signature(struct channel *channel,
 	tx->chainparams = chainparams;
 
 	/* FIXME: Make sure signature is correct! */
-	if (better_closing_fee(ld, channel, tx)) {
+	if (closing_fee_is_acceptable(ld, channel, tx)) {
 		channel_set_last_tx(channel, tx, &sig, TX_CHANNEL_CLOSE);
 		wallet_channel_save(ld->wallet, channel);
 	}

tests/test_closing.py

@@ -393,6 +393,87 @@ def test_deprecated_closing_compat(node_factory, bitcoind, chainparams):
         l1.rpc.call('check', ['close', nodeid, "Given enough eyeballs, all bugs are shallow."])
 
 
+def closing_fee(node_factory, bitcoind, chainparams, opts):
+    rate = opts['funder_feerate_per_kw']
+    funder = node_factory.get_node(feerates=(rate, rate, rate))
+
+    rate = opts['fundee_feerate_per_kw']
+    fundee = node_factory.get_node(feerates=(rate, rate, rate))
+
+    funder_id = funder.info['id']
+    fundee_id = fundee.info['id']
+
+    fund_amount = 10**6
+
+    funder.rpc.connect(fundee_id, 'localhost', fundee.port)
+    funder.fund_channel(fundee, fund_amount)
+
+    assert bitcoind.rpc.getmempoolinfo()['size'] == 0
+
+    if opts['close_initiated_by'] == 'funder':
+        funder.rpc.close(peer_id=fundee_id)
+    else:
+        assert opts['close_initiated_by'] == 'fundee'
+        fundee.rpc.close(peer_id=funder_id)
+
+    # Get the closing transaction from the bitcoind mempool and get its fee
+
+    mempool = None
+    mempool_tx_ids = None
+
+    def get_mempool_when_size_1():
+        nonlocal mempool, mempool_tx_ids
+        mempool = bitcoind.rpc.getrawmempool(True)
+        mempool_tx_ids = list(mempool.keys())
+        return len(mempool_tx_ids) == 1
+
+    wait_for(get_mempool_when_size_1)
+
+    close_tx_id = mempool_tx_ids[0]
+    fee_mempool = round(mempool[close_tx_id]['fee'] * 10**8)
+
+    # Get the proclaimed closing fee from the two nodes' statuses
+
+    status_agreed_regex = re.compile("agreed on a closing fee of ([0-9]+) satoshi")
+
+    # [fee_from_funder_status, fee_from_fundee_status]
+    fees_from_status = [None, None]
+
+    def get_fee_from_status(node, peer_id, i):
+        nonlocal fees_from_status
+        status = only_one(only_one(node.rpc.listpeers(peer_id)['peers'][0]['channels'])['status'])
+        m = status_agreed_regex.search(status)
+        if not m:
+            return False
+        fees_from_status[i] = int(m.group(1))
+        return True
+
+    wait_for(lambda: get_fee_from_status(funder, fundee_id, 0))
+    wait_for(lambda: get_fee_from_status(fundee, funder_id, 1))
+
+    assert fee_mempool == fees_from_status[0]
+    assert fee_mempool == fees_from_status[1]
+    assert fee_mempool == opts['expected_close_fee']
+
+
+def test_closing_fee(node_factory, bitcoind, chainparams):
+    """Test that the closing negotiation strategy works"""
+    # feerate 27625 -> closing fee negotiation starts at 20000
+    # feerate 29006 -> closing fee negotiation starts at 21000
+
+    opts = {
+        'funder_feerate_per_kw': 29006,
+        'fundee_feerate_per_kw': 27625,
+        'close_initiated_by': 'funder',
+        'expected_close_fee': 20333
+    }
+
+    closing_fee(node_factory, bitcoind, chainparams, opts)
+
+    opts['close_initiated_by'] = 'fundee'
+    closing_fee(node_factory, bitcoind, chainparams, opts)
+
+
 @unittest.skipIf(not DEVELOPER, "needs DEVELOPER=1")
 def test_penalty_inhtlc(node_factory, bitcoind, executor, chainparams):
     """Test penalty transaction with an incoming HTLC"""