diff --git a/closingd/closingd.c b/closingd/closingd.c index 3648a914cede..bd4e575b90ed 100644 --- a/closingd/closingd.c +++ b/closingd/closingd.c @@ -215,10 +215,12 @@ static void send_offer(struct per_peer_state *pps, peer_write(pps, take(msg)); } -static void tell_master_their_offer(const struct bitcoin_signature *their_sig, +/* Returns false if master says we must not agree to this offer. */ +static bool tell_master_their_offer(const struct bitcoin_signature *their_sig, const struct bitcoin_tx *tx, struct bitcoin_txid *tx_id) { + bool acceptable; u8 *msg = towire_closingd_received_signature(NULL, their_sig, tx); if (!wire_sync_write(REQ_FD, take(msg))) status_failed(STATUS_FAIL_MASTER_IO, @@ -227,9 +229,11 @@ static void tell_master_their_offer(const struct bitcoin_signature *their_sig, /* Wait for master to ack, to make sure it's in db. */ msg = wire_sync_read(NULL, REQ_FD); - if (!fromwire_closingd_received_signature_reply(msg, tx_id)) + if (!fromwire_closingd_received_signature_reply(msg, tx_id, + &acceptable)) master_badmsg(WIRE_CLOSINGD_RECEIVED_SIGNATURE_REPLY, msg); tal_free(msg); + return acceptable; } /* Returns fee they offered. */ @@ -384,7 +388,17 @@ receive_offer(struct per_peer_state *pps, /* Master sorts out what is best offer, we just tell it any above min */ if (amount_sat_greater_eq(received_fee, min_fee_to_accept)) { status_debug("...offer is reasonable"); - tell_master_their_offer(&their_sig, tx, closing_txid); + /* Our own closing_signed for this round has usually gone + * out by now (the opener sends first), so if their fee + * matched ours they hold both signatures and can broadcast + * the close whatever we do here. Refusing only keeps us + * from recording the close as agreed. lightningd checks + * the fee against the same bounds we negotiate within, so + * this is not expected to fire. */ + if (!tell_master_their_offer(&their_sig, tx, closing_txid)) + peer_failed_warn(pps, channel_id, + "Closing fee %s is outside our fee limits", + fmt_amount_sat(tmpctx, received_fee)); } return received_fee; diff --git a/closingd/closingd_wire.csv b/closingd/closingd_wire.csv index 55ca558979d0..a0f3ddeee7e0 100644 --- a/closingd/closingd_wire.csv +++ b/closingd/closingd_wire.csv @@ -44,6 +44,8 @@ msgdata,closingd_received_signature,tx,bitcoin_tx, msgtype,closingd_received_signature_reply,2102 msgdata,closingd_received_signature_reply,closing_txid,bitcoin_txid, +# Whether we may agree to this offer at all. +msgdata,closingd_received_signature_reply,acceptable,bool, # Negotiations complete, we're exiting. msgtype,closingd_complete,2004 diff --git a/lightningd/closing_control.c b/lightningd/closing_control.c index 47806d21ec44..e7acf0179383 100644 --- a/lightningd/closing_control.c +++ b/lightningd/closing_control.c @@ -211,13 +211,62 @@ static u32 calc_max_close_feerate(struct lightningd *ld, return max_feerate; } +/* The fee closingd negotiated is what it took off our output (we only + * bound the fee when we are the opener, and the opener pays it), and + * closingd bounded it at the weight of the closing transaction before + * any fee came off, with our output still present. The transaction + * itself pays more than that whenever the outputs' msat remainders were + * rounded away or the fee trimmed our output as dust: neither is a fee + * we chose, so neither counts against our maximum. *weight is the + * transaction's weight on entry and the weight closingd bounded the fee + * at on return. */ +static bool negotiated_close_fee(const struct channel *channel, + const struct bitcoin_tx *tx, + struct amount_sat *fee, + u64 *weight) +{ + struct amount_sat ours = amount_msat_to_sat_round_down(channel->our_msat); + const u8 *our_script = channel->shutdown_scriptpubkey[LOCAL]; + struct amount_sat out_amt; + + for (size_t i = 0; i < tx->wtx->num_outputs; i++) { + const struct wally_tx_output *out = &tx->wtx->outputs[i]; + const u8 *script = tal_dup_arr(tmpctx, u8, + out->script, out->script_len, 0); + if (!scripteq(script, our_script)) + continue; + out_amt = bitcoin_tx_output_get_amount_sat(tx, i); + if (!amount_sat_sub(fee, ours, out_amt)) { + /* closingd built the tx from this same balance, so + * this cannot underflow; count the whole fee if it + * does. */ + log_broken(channel->log, + "Closing tx output %zu pays us %s," + " more than our balance %s", + i, fmt_amount_sat(tmpctx, out_amt), + fmt_amount_sat(tmpctx, ours)); + return false; + } + return true; + } + + /* Our output was trimmed. closingd drops it once the fee leaves + * it below the dust limit, so the fee is at least our balance less + * that, and the weight closingd bounded it at included the + * output. */ + if (!amount_sat_sub(fee, ours, channel->our_config.dust_limit)) + *fee = AMOUNT_SAT(0); + *weight += bitcoin_tx_output_weight(tal_bytelen(our_script)); + return true; +} + /* 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; - u64 weight; + struct amount_sat fee, last_fee, negotiated; + u64 weight, negotiated_weight; /* Calculate actual fee (adds in eliminated outputs) */ fee = calc_tx_fee(channel->funding_sats, tx); @@ -234,12 +283,21 @@ static bool closing_fee_is_acceptable(struct lightningd *ld, fmt_amount_sat(tmpctx, last_fee), weight); + if (ld->dev_reject_closing_fee) { + log_debug(channel->log, "... dev-reject-closing-fee"); + return false; + } + if (!channel->ignore_fee_limits && !ld->config.ignore_fee_limits) { struct amount_sat min_fee, max_fee; u32 min_feerate, max_feerate; /* If we don't have a feerate estimate, this gives feerate_floor */ min_feerate = feerate_min(ld, NULL); + /* A feerange given to `close` is what closingd negotiated + * within; its minimum is our floor too. */ + if (channel->closing_feerate_range) + min_feerate = channel->closing_feerate_range[0]; max_feerate = calc_max_close_feerate(ld, channel); min_fee = amount_tx_fee(min_feerate, weight); @@ -250,12 +308,19 @@ static bool closing_fee_is_acceptable(struct lightningd *ld, weight, min_feerate); return false; } - max_fee = amount_tx_fee(max_feerate, weight); - if (channel->opener == LOCAL && amount_sat_less(max_fee, fee)) { - log_debug(channel->log, "... That's above our max %s" - " for weight %"PRIu64" at feerate %u", + negotiated_weight = weight; + if (!negotiated_close_fee(channel, tx, &negotiated, + &negotiated_weight)) + negotiated = fee; + max_fee = amount_tx_fee(max_feerate, negotiated_weight); + if (channel->opener == LOCAL + && amount_sat_less(max_fee, negotiated)) { + log_debug(channel->log, "... Negotiated fee %s is above" + " our max %s for weight %"PRIu64 + " at feerate %u", + fmt_amount_sat(tmpctx, negotiated), fmt_amount_sat(tmpctx, max_fee), - weight, max_feerate); + negotiated_weight, max_feerate); return false; } } @@ -274,6 +339,7 @@ static void peer_received_closing_signature(struct channel *channel, struct bitcoin_txid tx_id; struct lightningd *ld = channel->peer->ld; u8 *funding_wscript; + bool acceptable; if (!fromwire_closingd_received_signature(msg, msg, &sig, &tx)) { channel_internal_error(channel, @@ -302,17 +368,23 @@ static void peer_received_closing_signature(struct channel *channel, return; } - if (closing_fee_is_acceptable(ld, channel, tx)) { + acceptable = closing_fee_is_acceptable(ld, channel, tx); + if (acceptable) { channel_set_last_tx(channel, tx, &sig); wallet_channel_save(ld->wallet, channel); - } - - - // Send back the txid so we can update the billboard on selection. + } else + log_unusual(channel->log, + "Rejecting peer's closing fee offer:" + " closingd must not agree to it"); + + /* Send back the txid so closingd can update the billboard, and + * whether it may agree to this offer at all. Without the verdict + * a rejected offer would still complete the close, and last_tx, + * still the commitment, would be broadcast as the mutual close. */ bitcoin_txid(channel->last_tx, &tx_id); - /* OK, you can continue now. */ subd_send_msg(channel->owner, - take(towire_closingd_received_signature_reply(channel, &tx_id))); + take(towire_closingd_received_signature_reply(channel, &tx_id, + acceptable))); } static void peer_closing_complete(struct channel *channel, const u8 *msg) diff --git a/lightningd/lightningd.c b/lightningd/lightningd.c index 44ed3fa66425..6dbc2a0473b1 100644 --- a/lightningd/lightningd.c +++ b/lightningd/lightningd.c @@ -131,6 +131,7 @@ static struct lightningd *new_lightningd(const tal_t *ctx) ld->dev_throttle_gossip = false; ld->dev_suppress_gossip = false; ld->dev_fast_reconnect = false; + ld->dev_reject_closing_fee = false; ld->dev_force_privkey = NULL; ld->dev_force_bip32_seed = NULL; ld->dev_force_channel_secrets = NULL; diff --git a/lightningd/lightningd.h b/lightningd/lightningd.h index 6d778c929e95..87a2851ceff4 100644 --- a/lightningd/lightningd.h +++ b/lightningd/lightningd.h @@ -322,6 +322,9 @@ struct lightningd { /* Speedup reconnect delay, for testing. */ bool dev_fast_reconnect; + /* Reject every closing fee the peer offers. */ + bool dev_reject_closing_fee; + /* This is the forced private key for the node. */ struct privkey *dev_force_privkey; diff --git a/lightningd/options.c b/lightningd/options.c index 42ee3a6e43f3..6e3979b46850 100644 --- a/lightningd/options.c +++ b/lightningd/options.c @@ -801,6 +801,10 @@ static void dev_register_opts(struct lightningd *ld) opt_set_bool, &ld->dev_fast_reconnect, "Make max default reconnect delay 3 (not 300) seconds"); + clnopt_noarg("--dev-reject-closing-fee", OPT_DEV, + opt_set_bool, + &ld->dev_reject_closing_fee, + "Reject every closing fee the peer offers, as if outside our limits"); clnopt_noarg("--dev-fail-on-subdaemon-fail", OPT_DEV, opt_set_bool, diff --git a/tests/test_closing.py b/tests/test_closing.py index c9d0b4512a0c..9e2cc41176cf 100644 --- a/tests/test_closing.py +++ b/tests/test_closing.py @@ -4321,6 +4321,157 @@ def test_closing_minfee(node_factory, bitcoind): bitcoind.generate_block(1, wait_for_mempool=txid) +@unittest.skipIf(TEST_NETWORK != 'regtest', 'elementsd anchors not supportd') +def test_closing_fee_rounding_at_ceiling(node_factory, bitcoind): + """A close pinned at the fee ceiling stays a mutual close. + + With every estimate at the floor, the opener's closing fee range is + a single value. Each output is rounded down to whole satoshis, so + the msat remainders end up in the fee and the transaction pays one + satoshi more than the agreed fee. lightningd must still accept it + rather than fall back to broadcasting the commitment. + """ + l1, l2 = node_factory.line_graph(2, opts={'feerates': (253, 253, 253, 253)}) + chan = only_one(l1.rpc.listpeerchannels(l2.info['id'])['channels']) + funding = int(Millisatoshi(chan['total_msat']).to_satoshi()) + + # Leave remainders which sum to exactly 1000msat: l1 keeps ...999msat, + # l2 gets ...001msat. Rounding both down costs one satoshi of fee. + l1.pay(l2, 100000001) + wait_for(lambda: only_one(l1.rpc.listpeerchannels()['channels'])['htlcs'] == []) + + fee = closing_fee(253, 2) + res = l1.rpc.close(l2.info['id']) + assert res['type'] == 'mutual' + tx = bitcoind.rpc.decoderawtransaction(only_one(res['txs'])) + + # A closing transaction, not the commitment. + assert len(tx['vout']) == 2 + assert tx['locktime'] == 0 + + # The agreed fee plus the rounded-off remainders. + paid = funding - sum(int(round(o['value'] * 10**8)) for o in tx['vout']) + assert paid == fee + 1 + billboard = only_one(l1.rpc.listpeerchannels(l2.info['id'])['channels'])['status'] + assert billboard == ['CLOSINGD_SIGEXCHANGE:We agreed on a closing fee of {} satoshi for tx:{}'.format(fee, tx['txid'])] + + bitcoind.generate_block(1, wait_for_mempool=tx['txid']) + wait_for(lambda: 'ONCHAIN:Tracking mutual close transaction' in only_one(l1.rpc.listpeerchannels(l2.info['id'])['channels'])['status']) + assert tx['txid'] in [o['txid'] for o in l1.rpc.listfunds()['outputs']] + assert tx['txid'] in [o['txid'] for o in l2.rpc.listfunds()['outputs']] + + +@unittest.skipIf(TEST_NETWORK != 'regtest', 'elementsd anchors not supportd') +def test_closing_fee_trims_opener_output(node_factory, bitcoind): + """A close whose fee leaves the opener's output below dust stays mutual. + + closingd bounds the fee it agrees to at the weight of a closing + transaction with both outputs, and drops the opener's output once the + fee leaves it below the dust limit. lightningd then sees a one-output + transaction paying the opener's whole balance as fee. It must bound + the fee closingd agreed to, at the weight closingd used, rather than + reject the close and fall back to the commitment. + """ + l1, l2 = node_factory.line_graph(2, opts={'feerates': (253, 253, 253, 253)}) + chan = only_one(l1.rpc.listpeerchannels(l2.info['id'])['channels']) + funding = int(Millisatoshi(chan['total_msat']).to_satoshi()) + dust = int(Millisatoshi(chan['dust_limit_msat']).to_satoshi()) + + # Drain the opener to what it has to keep, in whole satoshis so no + # msat remainder reaches the fee. + spendable = int(chan['spendable_msat']) + l1.pay(l2, spendable - spendable % 1000) + wait_for(lambda: only_one(l1.rpc.listpeerchannels()['channels'])['htlcs'] == []) + chan = only_one(l1.rpc.listpeerchannels(l2.info['id'])['channels']) + ours = int(Millisatoshi(chan['to_us_msat']).to_satoshi()) + + # A feerate whose two-output closing fee leaves l1 about half the + # dust limit, so its output is trimmed. + feerate = (ours - dust // 2) * 1000 // closing_fee(1000, 2) + fee = closing_fee(feerate, 2) + assert ours - dust < fee <= ours + + res = l1.rpc.close(l2.info['id'], unilateraltimeout=10, + feerange=['{}perkw'.format(feerate)] * 2) + assert res['type'] == 'mutual' + tx = bitcoind.rpc.decoderawtransaction(only_one(res['txs'])) + + # A closing transaction with only l2's output, not the commitment. + assert len(tx['vout']) == 1 + assert tx['locktime'] == 0 + + # l1's whole balance is fee: the agreed fee plus the trimmed rest. + paid = funding - int(round(only_one(tx['vout'])['value'] * 10**8)) + assert paid == ours + billboard = only_one(l1.rpc.listpeerchannels(l2.info['id'])['channels'])['status'] + assert billboard == ['CLOSINGD_SIGEXCHANGE:We agreed on a closing fee of {} satoshi for tx:{}'.format(fee, tx['txid'])] + + bitcoind.generate_block(1, wait_for_mempool=tx['txid']) + wait_for(lambda: 'ONCHAIN:Tracking mutual close transaction' in only_one(l1.rpc.listpeerchannels(l2.info['id'])['channels'])['status']) + assert tx['txid'] not in [o['txid'] for o in l1.rpc.listfunds()['outputs']] + assert tx['txid'] in [o['txid'] for o in l2.rpc.listfunds()['outputs']] + + +@unittest.skipIf(TEST_NETWORK != 'regtest', 'elementsd anchors not supportd') +def test_closing_feerange_below_estimates(node_factory, bitcoind): + """A close with a feerange below the estimate floor stays a mutual close. + + closingd negotiates within the feerange given to `close`, but + lightningd checked the agreed fee against the floor derived from its + fee estimates. With estimates above the range, the agreed fee was + rejected as too low and the commitment was broadcast instead. + """ + l1, l2 = node_factory.line_graph(2, opts={'feerates': (253, 253, 253, 253), + 'may_reconnect': True}) + l1.pay(l2, 100000000) + wait_for(lambda: only_one(l1.rpc.listpeerchannels()['channels'])['htlcs'] == []) + + # l1's estimate floor becomes 1000perkw, well above the range. + l1.force_feerates(2000) + l1.rpc.connect(l2.info['id'], 'localhost', l2.port) + + fee = closing_fee(253, 2) + res = l1.rpc.close(l2.info['id'], feerange=['253perkw', '253perkw']) + assert res['type'] == 'mutual' + tx = bitcoind.rpc.decoderawtransaction(only_one(res['txs'])) + + # A closing transaction at the agreed fee, not the commitment. + assert len(tx['vout']) == 2 + assert tx['locktime'] == 0 + billboard = only_one(l1.rpc.listpeerchannels(l2.info['id'])['channels'])['status'] + assert 'CLOSINGD_SIGEXCHANGE:We agreed on a closing fee of {} satoshi for tx:{}'.format(fee, tx['txid']) in billboard + + bitcoind.generate_block(1, wait_for_mempool=tx['txid']) + wait_for(lambda: 'ONCHAIN:Tracking mutual close transaction' in only_one(l1.rpc.listpeerchannels(l2.info['id'])['channels'])['status']) + assert tx['txid'] in [o['txid'] for o in l1.rpc.listfunds()['outputs']] + assert tx['txid'] in [o['txid'] for o in l2.rpc.listfunds()['outputs']] + + +def test_closing_rejected_fee_fails_negotiation(node_factory, bitcoind): + """A closing fee lightningd rejects ends the negotiation. + + closingd only learns the txid from lightningd's reply, never the + verdict, so it agrees to a rejected offer and lightningd broadcasts + the commitment as if it were the mutual close. The known reasons for + a rejection are fixed, so --dev-reject-closing-fee forces one. + """ + l1, l2 = node_factory.line_graph(2, opts=[{}, {'dev-reject-closing-fee': None}]) + l1.pay(l2, 100000000) + wait_for(lambda: only_one(l1.rpc.listpeerchannels()['channels'])['htlcs'] == []) + + # l2 refuses l1's offer, so nobody completes the negotiation and l1 + # closes unilaterally when its timeout expires. + res = l1.rpc.close(l2.info['id'], unilateraltimeout=10) + assert res['type'] == 'unilateral' + assert not l2.daemon.is_in_log('We agreed on a closing fee') + l2.daemon.wait_for_log('outside our fee limits') + + # The only transaction on the wire is that unilateral close: with no + # HTLCs at stake, nothing spends its anchor to hurry it along. + txid = only_one(res['txids']) + wait_for(lambda: bitcoind.rpc.getrawmempool() == [txid]) + + @unittest.skipIf(TEST_NETWORK != 'regtest', 'elementsd anchors not supportd') def test_peer_anchor_push(node_factory, bitcoind, executor, chainparams): """Test that we use anchor on peer's commit to CPFP tx"""