From 6a16a6fe25bac9ea76d614c0e861425334e0ad12 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Sat, 12 Aug 2023 09:44:11 +0930 Subject: [PATCH] closingd: fix case where we we can pass under min-relay-fee for mutual close. In spec commit 498f104fd399488c77f449d05cb21c0b604636a2 (August 2021), Bastien Teinturier removed the requirement that the mutual close fee be less than or equal the final commitment tx. We adopted that change in v0.10.2, but we made sure to never offer a fee under the final commitment tx's fee, so we didn't break older nodes. However, the closing tx can actually be larger than the final commitment tx! The final commit tx has a 22-byte P2WKH output and a 34-byte P2WSH output; the closing can have two 34-byte outputs, making it 4*8 = 32 Sipa heavier. Previously this would only happen if both sides asked for P2WSH outputs, but now it happens with P2TR, which we now do. The result is that we create a tx which is below the finally commitment tx fee, and may be below minrelayfee (as it was in regtest). So it's time to remove that backwards-compatibility hack. Changelog-Fixed: Protocol: We may propose mutual close transaction which has a slightly higher fee than the final commitment tx (depending on the outputs, e.g. two taproot outputs). Signed-off-by: Rusty Russell Fixes: #6545 --- closingd/closingd.c | 57 +++++++++++++----------------------- closingd/closingd_wire.csv | 3 +- lightningd/closing_control.c | 40 ++++++++----------------- tests/test_closing.py | 11 +++---- 4 files changed, 39 insertions(+), 72 deletions(-) diff --git a/closingd/closingd.c b/closingd/closingd.c index 3528f5d3c..ef13af7b8 100644 --- a/closingd/closingd.c +++ b/closingd/closingd.c @@ -598,8 +598,7 @@ static size_t closing_tx_weight_estimate(u8 *scriptpubkey[NUM_SIDES], static void calc_fee_bounds(size_t expected_weight, u32 min_feerate, u32 desired_feerate, - u32 *max_feerate, - struct amount_sat commitment_fee, + u32 max_feerate, struct amount_sat funding, enum side opener, struct amount_sat *minfee, @@ -620,36 +619,22 @@ static void calc_fee_bounds(size_t expected_weight, if (opener == REMOTE) { *maxfee = funding; - /* This used to appear in BOLT #2: we still set it for non-anchor - * peers who may still enforce it: - * - If the channel does not use `option_anchor_outputs`: - * - MUST set `fee_satoshis` less than or equal to the base fee of - * the final commitment transaction, as calculated in - * [BOLT #3](03-transactions.md#fee-calculation). - */ - } else if (max_feerate) { - *maxfee = amount_tx_fee(*max_feerate, expected_weight); - - status_debug("deriving max fee from rate %u -> %s (not %s)", - *max_feerate, - type_to_string(tmpctx, struct amount_sat, maxfee), - type_to_string(tmpctx, struct amount_sat, &commitment_fee)); - - /* option_anchor_outputs sets commitment_fee to max, so this - * doesn't do anything */ - if (amount_sat_greater(*maxfee, commitment_fee)) { - /* FIXME: would be nice to notify close cmd here! */ - status_unusual("Maximum feerate %u would give fee %s:" - " we must limit it to the final commitment fee %s", - *max_feerate, - type_to_string(tmpctx, struct amount_sat, - maxfee), - type_to_string(tmpctx, struct amount_sat, - &commitment_fee)); - *maxfee = commitment_fee; - } - } else - *maxfee = commitment_fee; + } else { + /* BOLT #2: + * The sending node: + * + * - SHOULD set the initial `fee_satoshis` according to its + * estimate of cost of inclusion in a block. + * + * - SHOULD set `fee_range` according to the minimum and + * maximum fees it is prepared to pay for a close + * transaction. + */ + *maxfee = amount_tx_fee(max_feerate, expected_weight); + status_debug("deriving max fee from rate %u -> %s", + max_feerate, + type_to_string(tmpctx, struct amount_sat, maxfee)); + } /* Can't exceed maxfee. */ if (amount_sat_greater(*minfee, *maxfee)) @@ -868,9 +853,9 @@ int main(int argc, char *argv[]) struct bitcoin_outpoint funding; struct amount_sat funding_sats, out[NUM_SIDES]; struct amount_sat our_dust_limit; - struct amount_sat min_fee_to_accept, commitment_fee, offer[NUM_SIDES], + struct amount_sat min_fee_to_accept, offer[NUM_SIDES], max_fee_to_accept; - u32 min_feerate, initial_feerate, *max_feerate; + u32 min_feerate, initial_feerate, max_feerate; struct feerange feerange; enum side opener; u32 *local_wallet_index; @@ -902,7 +887,6 @@ int main(int argc, char *argv[]) &out[REMOTE], &our_dust_limit, &min_feerate, &initial_feerate, &max_feerate, - &commitment_fee, &local_wallet_index, &local_wallet_ext_key, &scriptpubkey[LOCAL], @@ -929,7 +913,7 @@ int main(int argc, char *argv[]) local_wallet_index, local_wallet_ext_key), min_feerate, initial_feerate, max_feerate, - commitment_fee, funding_sats, opener, + funding_sats, opener, &min_fee_to_accept, &offer[LOCAL], &max_fee_to_accept); /* Write values into tlv for updated closing fee neg */ @@ -1099,7 +1083,6 @@ exit_thru_the_giftshop: tal_free(wrong_funding); tal_free(our_feerange); tal_free(their_feerange); - tal_free(max_feerate); tal_free(local_wallet_index); tal_free(local_wallet_ext_key); closing_dev_memleak(ctx, scriptpubkey, funding_wscript); diff --git a/closingd/closingd_wire.csv b/closingd/closingd_wire.csv index 29f6eccab..10a5d8cbb 100644 --- a/closingd/closingd_wire.csv +++ b/closingd/closingd_wire.csv @@ -19,8 +19,7 @@ msgdata,closingd_init,remote_sat,amount_sat, msgdata,closingd_init,our_dust_limit,amount_sat, msgdata,closingd_init,min_feerate_perksipa,u32, msgdata,closingd_init,preferred_feerate_perksipa,u32, -msgdata,closingd_init,max_feerate_perksipa,?u32, -msgdata,closingd_init,fee_limit_satoshi,amount_sat, +msgdata,closingd_init,max_feerate_perksipa,u32, msgdata,closingd_init,local_wallet_index,?u32, msgdata,closingd_init,local_wallet_ext_key,?ext_key, msgdata,closingd_init,local_scriptpubkey_len,u16, diff --git a/lightningd/closing_control.c b/lightningd/closing_control.c index b806c15c0..797a2e48b 100644 --- a/lightningd/closing_control.c +++ b/lightningd/closing_control.c @@ -363,9 +363,8 @@ static unsigned closing_msg(struct subd *sd, const u8 *msg, const int *fds UNUSE void peer_start_closingd(struct channel *channel, struct peer_fd *peer_fd) { u8 *initmsg; - u32 min_feerate, feerate, *max_feerate; + u32 min_feerate, feerate, max_feerate; struct amount_msat their_msat; - struct amount_sat feelimit; int hsmfd; struct lightningd *ld = channel->peer->ld; u32 final_commit_feerate; @@ -404,19 +403,15 @@ void peer_start_closingd(struct channel *channel, struct peer_fd *peer_fd) return; } - /* FIXME: This is the old BOLT 2 text, which restricted the closing - * fee to cap at the final commitment fee. We still do this for now. - * + /* BOLT #2: * The sending node: - * - MUST set `fee_satoshis` less than or equal to the base - * fee of the final commitment transaction, as calculated in - * [BOLT #3](03-transactions.md#fee-calculation). + * - SHOULD set the initial `fee_satoshis` according to its estimate of cost of + * inclusion in a block. + * - SHOULD set `fee_range` according to the minimum and maximum fees it is + * prepared to pay for a close transaction. */ final_commit_feerate = get_feerate(channel->fee_states, channel->opener, LOCAL); - feelimit = commit_tx_base_fee(final_commit_feerate, 0, - option_anchor_outputs, - option_anchors_zero_fee_htlc_tx); /* If we can't determine feerate, start at half unilateral feerate. */ feerate = mutual_close_feerate(ld->topology); @@ -426,30 +421,20 @@ void peer_start_closingd(struct channel *channel, struct peer_fd *peer_fd) feerate = get_feerate_floor(ld->topology); } - /* We use a feerate if anchor_outputs, otherwise max fee is set by - * the final unilateral. */ - if (option_anchor_outputs || option_anchors_zero_fee_htlc_tx) { - max_feerate = tal(tmpctx, u32); - /* Aim for reasonable max, but use final if we don't know. */ - *max_feerate = unilateral_feerate(ld->topology, false); - if (!*max_feerate) - *max_feerate = final_commit_feerate; - /* No other limit on fees */ - feelimit = channel->funding_sats; - } else - max_feerate = NULL; + /* Aim for reasonable max, but use final if we don't know. */ + max_feerate = unilateral_feerate(ld->topology, false); + if (!max_feerate) + max_feerate = final_commit_feerate; min_feerate = feerate_min(ld, NULL); /* If they specified feerates in `close`, they apply now! */ if (channel->closing_feerate_range) { min_feerate = channel->closing_feerate_range[0]; - max_feerate = &channel->closing_feerate_range[1]; + max_feerate = channel->closing_feerate_range[1]; } else if (channel->ignore_fee_limits || ld->config.ignore_fee_limits) { min_feerate = 253; - tal_free(max_feerate); - max_feerate = tal(tmpctx, u32); - *max_feerate = 0xFFFFFFFF; + max_feerate = 0xFFFFFFFF; } /* BOLT #3: @@ -506,7 +491,6 @@ void peer_start_closingd(struct channel *channel, struct peer_fd *peer_fd) amount_msat_to_sat_round_down(their_msat), channel->our_config.dust_limit, min_feerate, feerate, max_feerate, - feelimit, local_wallet_index, local_wallet_ext_key, channel->shutdown_scriptpubkey[LOCAL], diff --git a/tests/test_closing.py b/tests/test_closing.py index 50b1bf24a..b7a46becf 100644 --- a/tests/test_closing.py +++ b/tests/test_closing.py @@ -3354,13 +3354,15 @@ Try a range of future segwit versions as shutdown scripts. We create many nodes @pytest.mark.developer("needs to set dev-disconnect") -def test_closing_higherfee(node_factory, bitcoind, executor): - """With anchor outputs we can ask for a *higher* fee than the last commit tx""" +@pytest.mark.parametrize("anchors", [False, True]) +def test_closing_higherfee(node_factory, bitcoind, executor, anchors): + """We can ask for a *higher* fee than the last commit tx""" opts = {'may_reconnect': True, 'dev-no-reconnect': None, - 'experimental-anchors': None, 'feerates': (7500, 7500, 7500, 7500)} + if anchors: + opts['experimental-anchors'] = None # We change the feerate before it starts negotiating close, so it aims # for *higher* than last commit tx. @@ -3379,7 +3381,7 @@ def test_closing_higherfee(node_factory, bitcoind, executor): l1.rpc.connect(l2.info['id'], 'localhost', l2.port) # This causes us to *exceed* previous requirements! - l1.daemon.wait_for_log(r'deriving max fee from rate 30000 -> .*sat \(not 1000000sat\)') + l1.daemon.wait_for_log(r'deriving max fee from rate 30000 -> .*sat') # This will fail because l1 restarted! with pytest.raises(RpcError, match=r'Connection to RPC server lost.'): @@ -3904,7 +3906,6 @@ def test_closing_tx_valid(node_factory, bitcoind): assert bitcoind.rpc.getrawtransaction(close['txid']) == close['tx'] -@pytest.mark.xfail(strict=True) @pytest.mark.developer("needs dev-no-reconnect") @unittest.skipIf(TEST_NETWORK != 'regtest', 'elementsd does not provide feerates on regtest') def test_closing_minfee(node_factory, bitcoind):