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):