During close sometimes a node would broadcast the wrong transaction

[vasild]

Lets have two nodes l1 and l2 that go into channel closing negotiation:

• l1 proposes a closing fee of 2000
• l2 proposes a closing fee of 1000
• they start negotiating on a fee and eventually agree on some number in between, lets say 1500
• both nodes should broadcast the closing transaction with the agreed fee of 1500
• however l1 wrongly broadcasts the transaction it initially got from l2 with a fee of 1000

This is a test case, run as pytest tests/ -v -k test_closing_fee_negotiation_strategy -r P:

diff --git i/tests/test_closing.py w/tests/test_closing.py
index 88c7b973..04279bfc 100644
--- i/tests/test_closing.py
+++ w/tests/test_closing.py
@@ -5,12 +5,13 @@ from utils import (
     only_one, sync_blockheight, wait_for, DEVELOPER, TIMEOUT, VALGRIND,
     SLOW_MACHINE, COMPAT
 )
 
 import os
 import queue
+import pprint
 import pytest
 import re
 import threading
 import unittest
 
 
@@ -390,12 +391,63 @@ def test_deprecated_closing_compat(node_factory, bitcoind, chainparams):
     l1.rpc.call('check', ['close', nodeid, None, 10])
     # Not new-style nor old-style
     with pytest.raises(RpcError, match=r'Expected unilerataltimeout to be a number'):
         l1.rpc.call('check', ['close', nodeid, "Given enough eyeballs, all bugs are shallow."])
 
 
+def test_closing_fee_negotiation_strategy(node_factory, bitcoind, chainparams):
+    """ Test that the closing negotiation strategy works"""
+    p = pprint.PrettyPrinter(indent=4)
+
+    l1_feerate_per_kw = 5000
+    l2_feerate_per_kw = 4000
+
+    l1 = node_factory.get_node(feerates=(l1_feerate_per_kw, l1_feerate_per_kw, l1_feerate_per_kw))
+    l2 = node_factory.get_node(feerates=(l2_feerate_per_kw, l2_feerate_per_kw, l2_feerate_per_kw))
+
+    l1_id = l1.info['id']
+    l2_id = l2.info['id']
+
+    fund_amount = 10**6
+
+    l1.rpc.connect(l2_id, 'localhost', l2.port)
+    l1.fund_channel(l2, fund_amount)
+
+    assert bitcoind.rpc.getmempoolinfo()['size'] == 0
+
+    #l1.rpc.close(peer_id=l2_id)
+    l2.rpc.close(peer_id=l1_id)
+
+    mempool = None
+    mempool_txids = None
+
+    def getpool():
+        nonlocal mempool, mempool_txids
+        mempool = bitcoind.rpc.getrawmempool(True)
+        mempool_txids = list(mempool.keys())
+        return len(mempool_txids) == 1
+
+    wait_for(getpool)
+
+    closetxid = mempool_txids[0]
+
+    print('{} mempool tx fee, id=${}'.format(round(mempool[closetxid]['fee'] * 10**8), closetxid))
+
+    r = re.compile('.*agreed on a closing fee of ([0-9]+) satoshi.*')
+    status = only_one(only_one(l1.rpc.listpeers(l2_id)['peers'][0]['channels'])['status'])
+    print('{} fee from l1 status'.format(r.sub('\\1', status)))
+    status = only_one(only_one(l2.rpc.listpeers(l1_id)['peers'][0]['channels'])['status'])
+    print('{} fee from l2 status'.format(r.sub('\\1', status)))
+
+    print('============== l1 logs ==============')
+    p.pprint(l1.daemon.logs)
+
+    print('============== l2 logs ==============')
+    p.pprint(l2.daemon.logs)
+
+
 @unittest.skipIf(not DEVELOPER, "needs DEVELOPER=1")
 def test_penalty_inhtlc(node_factory, bitcoind, executor, chainparams):
     """Test penalty transaction with an incoming HTLC"""
     # We suppress each one after first commit; HTLC gets added not fulfilled.
     # Feerates identical so we don't get gratuitous commit to update them
     l1 = node_factory.get_node(disconnect=['=WIRE_COMMITMENT_SIGNED-nocommit'], may_fail=True, feerates=(7500, 7500, 7500), allow_broken_log=True)

Sometimes it would print:

2896 mempool tx fee, id=...
3135 fee from l1 status
3135 fee from l2 status

and sometimes:

3135 mempool tx fee, id=...
3135 fee from l1 status
3135 fee from l2 status

This is because both l1 and l2 race in getting their tx broadcasted and included in bitcoind's mempool. If l1 succeeds then we get the first print, if l2 succeeds, then we get the second print. The too-late-to-broadcast node gets txn-mempool-conflict (code 18) from bitcoind.

From the dumped l1 log (I added the trailing (previous txid=%s) to ensure it is the same tx):

Their actual closing tx fee is 3077sat vs previous 2896sat (previous txid=b57e65...)
Their actual closing tx fee is 3122sat vs previous 2896sat (previous txid=b57e65...)
Their actual closing tx fee is 3133sat vs previous 2896sat (previous txid=b57e65...)
...

The previous transaction (channel->last_tx) that has fee of 2896 is never updated because all newer ones have higher fees: https://github.com/ElementsProject/lightning/blob/master/lightningd/closing_control.c#L115

[ZmnSCPxj]

This better_closing_fee was introduced here in 2017: 2b7c091

[vasild]

I confirm that the following patch fixes this issue:

--- i/lightningd/closing_control.c
+++ w/lightningd/closing_control.c
@@ -82,21 +89,16 @@ static bool better_closing_fee(struct lightningd *ld,
                          " for weight %"PRIu64" at feerate %u",
                          type_to_string(tmpctx, struct amount_sat, &fee),
                          weight, min_feerate);
                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,
                                            const u8 *msg)
 {
        struct bitcoin_signature sig;

Now both l1 and l2 successfully broadcast the same, identical, tx.

I am not sure (yet) whether this would introduce other problems though.

[ZmnSCPxj]

The patch looks good but the function now deserves a rename.

[vasild]

Is closing_fee_is_acceptable() an acceptable name? ;-)

Submitted the above patch + a polished test at #3556

[rustyrussell]

Ideally we'd use whatever is closer to our ideal fee. That helps if we restart negotiation. However, it's all an approximation anyway, so this is better than the prior behavior.

[vasild]

I think it now works "ideally". We have two cases:

1. If we start the negotiation at a higher fee, say 2000 and the peer starts lower, at 1000.
   During the negotiation the peer's proposals are (in chronological order): 1000, 1200, 1300, 1350 and we agree on 1350. So, every subsequent proposal is closer to our ideal of 2000.

2. We start lower, at 1000 and the peer starts higher at 2000. Then the peer's proposals are lowering the fee down from 2000 and the previous and the new code work correctly.