Skip to content

Commit

Permalink
closingd: fix case where we we can pass under min-relay-fee for mutua…
Browse files Browse the repository at this point in the history 
…l 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 <rusty@rustcorp.com.au>
Fixes: ElementsProject#6545
  • Loading branch information
@rustyrussell
rustyrussell committed Aug 12, 2023
1 parent c78b126 commit c7d6dd3
Show file tree
Hide file tree
Showing 4 changed files with 39 additions and 72 deletions.
57 changes: 20 additions & 37 deletions closingd/closingd.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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))
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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],
Expand All @@ -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 */
Expand Down Expand Up @@ -1099,7 +1083,6 @@ int main(int argc, char *argv[])
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);
Expand Down
3 changes: 1 addition & 2 deletions closingd/closingd_wire.csv
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
40 changes: 12 additions & 28 deletions lightningd/closing_control.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand All @@ -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:
Expand Down Expand Up @@ -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],
Expand Down
11 changes: 6 additions & 5 deletions tests/test_closing.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3354,13 +3354,15 @@ def test_segwit_shutdown_script(node_factory, bitcoind, executor):


@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.
Expand All @@ -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.'):
Expand Down Expand Up @@ -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):
Expand Down

0 comments on commit c7d6dd3

Please sign in to comment.