From 66eca2ff3fc01dcb09b0e5d7129b1919232f7e01 Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Fri, 28 Feb 2020 16:11:22 +0100 Subject: [PATCH] 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 | 21 ++++------ tests/test_closing.py | 81 ++++++++++++++++++++++++++++++++++++ 2 files changed, 88 insertions(+), 14 deletions(-) diff --git a/lightningd/closing_control.c b/lightningd/closing_control.c index 374d4f72d..349959fe7 100644 --- a/lightningd/closing_control.c +++ b/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); } diff --git a/tests/test_closing.py b/tests/test_closing.py index 88c7b9738..4547225a5 100644 --- a/tests/test_closing.py +++ b/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"""