diff --git a/bitcoin/feerate.c b/bitcoin/feerate.c index 7788ebb2d3ba..86a4e805f91f 100644 --- a/bitcoin/feerate.c +++ b/bitcoin/feerate.c @@ -12,8 +12,15 @@ u32 feerate_from_style(u32 feerate, enum feerate_style style) return feerate; case FEERATE_PER_KBYTE: /* Everyone uses satoshi per kbyte, but we use satoshi per ksipa - * (don't round down to zero though)! */ - return (feerate + 3) / 4; + * (don't round down to zero though)! + * + * Widen before rounding up: on a u32 the +3 wraps for the top + * three values, turning an absurd feerate into 0 or 1 perkw. + * That is the dangerous direction (it underpays our unilateral + * close), and it slips under every bound we check afterwards, + * since those are applied to the converted value. The result + * always fits a u32: (UINT_MAX + 3) / 4 < UINT_MAX. */ + return ((u64)feerate + 3) / 4; } abort(); } @@ -31,6 +38,30 @@ u32 feerate_to_style(u32 feerate_perkw, enum feerate_style style) abort(); } +bool next_funding_feerate(u32 last_feerate, u32 *next_feerate) +{ + u64 next; + + /* Not a feerate we could ever have proposed, and 25/24 of it is + * still 0. */ + if (last_feerate == 0) + return false; + + /* Widen: anything above UINT_MAX/25 overflows a u32 here, and that + * is exactly the range a broken fee estimator can leave in the db. */ + next = (u64)last_feerate * 25 / 24; + if (next > UINT_MAX) + return false; + + /* Rounding down means feerates below 24 map back onto themselves, + * and the rule requires strictly more to be a valid bump. */ + if (next <= last_feerate) + return false; + + *next_feerate = next; + return true; +} + const char *feerate_style_name(enum feerate_style style) { switch (style) { diff --git a/bitcoin/feerate.h b/bitcoin/feerate.h index f4dbdbbd672c..0cd8c37290aa 100644 --- a/bitcoin/feerate.h +++ b/bitcoin/feerate.h @@ -33,6 +33,29 @@ */ #define FEERATE_FLOOR 253 +/* + * Sanity ceiling on any feerate estimate entering lightningd (sat/kw). + * This is a sanity bound rather than a policy limit: an estimate above it + * means a broken fee source rather than an expensive mempool, so it sits + * far above anything the real chain has ever seen (4000 sat/vB, several + * times the historical peak). Clamping on the way in keeps every + * downstream feerate calculation working on a plausible number. + */ +#define FEERATE_CEILING 1000000 + +/* + * The most we are ever willing to pay ourselves (sat/kw). + * + * Unlike FEERATE_CEILING this *is* a policy limit, and the two are + * deliberately an order of magnitude apart because they answer different + * questions. FEERATE_CEILING bounds what we let a peer drive us to: their + * estimator being broken is not by itself worth dropping a channel over, so + * it only has to exclude the absurd. This one bounds what we propose with + * our own money, where we can simply decline: 400 sat/vB is around 0.011 BTC + * for a bare anchor commitment, which we would rather not spend by accident. + */ +#define MAX_OUR_FEERATE_PER_KW 100000 + enum feerate_style { FEERATE_PER_KSIPA, FEERATE_PER_KBYTE @@ -57,4 +80,16 @@ u32 feerate_from_style(u32 feerate, enum feerate_style style); u32 feerate_to_style(u32 feerate_perkw, enum feerate_style style); const char *feerate_style_name(enum feerate_style style); +/* Sets *next_feerate to the smallest feerate which satisfies the BOLT #2 + * rule that the next funding transaction pays 25/24 times the feerate of + * the previously constructed one, rounded down, and returns true. Returns + * false, leaving *next_feerate untouched, if last_feerate admits no such + * value: it is 0, or 25/24 of it does not fit a u32, or rounding down lands + * back on last_feerate. + * + * last_feerate is generally read back out of the database, where a broken + * fee estimator (ours or a peer's) may have left something absurd, so + * callers must handle false rather than assume it away. */ +bool next_funding_feerate(u32 last_feerate, u32 *next_feerate); + #endif /* LIGHTNING_BITCOIN_FEERATE_H */ diff --git a/channeld/channeld.c b/channeld/channeld.c index f069c546d617..2672f56b5e35 100644 --- a/channeld/channeld.c +++ b/channeld/channeld.c @@ -11,6 +11,7 @@ * limits, unlikely as that is. */ #include "config.h" +#include #include #include #include @@ -82,6 +83,14 @@ struct peer { /* Tolerable amounts for feerate (only relevant for fundee). */ u32 feerate_min, feerate_max; + /* The most we're prepared to pay ourselves: stricter than + * feerate_max, which is what we'll tolerate from them. */ + u32 our_feerate_max; + + /* Set by --ignore-fee-limits or dev-ignore-fee-limits: drop the + * policy bounds above (but never the sanity ceiling). */ + bool ignore_fee_limits; + /* Feerate to be used when creating penalty transactions. */ u32 feerate_penalty; @@ -690,6 +699,32 @@ static void handle_peer_add_htlc(struct peer *peer, const u8 *msg) channel_add_err_name(add_err)); } +/* Ignoring the fee limits drops the policy bounds, but never the sanity + * ceiling: a feerate above that means a broken fee source, and whatever we + * accept here we go on to store. */ +static u32 accepted_feerate_min(const struct peer *peer) +{ + if (peer->ignore_fee_limits) + return 1; + return peer->feerate_min; +} + +static u32 accepted_feerate_max(const struct peer *peer) +{ + if (peer->ignore_fee_limits) + return FEERATE_CEILING; + return peer->feerate_max; +} + +/* The most we'll pay ourselves, as opposed to what we'll put up with + * from them. */ +static u32 proposed_feerate_max(const struct peer *peer) +{ + if (peer->ignore_fee_limits) + return FEERATE_CEILING; + return peer->our_feerate_max; +} + /* We don't get upset if they're outside the range, as long as they're * improving (or at least, not getting worse!). */ static bool feerate_same_or_better(const struct channel *channel, @@ -728,7 +763,8 @@ static void handle_peer_feechange(struct peer *peer, const u8 *msg) "update_fee from non-opener?"); status_debug("update_fee %u, range %u-%u", - feerate, peer->feerate_min, peer->feerate_max); + feerate, accepted_feerate_min(peer), + accepted_feerate_max(peer)); /* BOLT #2: * @@ -739,12 +775,14 @@ static void handle_peer_feechange(struct peer *peer, const u8 *msg) * `error` and fail the channel. */ if (!feerate_same_or_better(peer->channel, feerate, - peer->feerate_min, peer->feerate_max)) + accepted_feerate_min(peer), + accepted_feerate_max(peer))) peer_failed_warn(peer->pps, &peer->channel_id, "update_fee %u outside range %u-%u" " (currently %u)", feerate, - peer->feerate_min, peer->feerate_max, + accepted_feerate_min(peer), + accepted_feerate_max(peer), channel_feerate(peer->channel, LOCAL)); /* BOLT #2: @@ -1928,8 +1966,10 @@ static void check_tx_abort(struct peer *peer, const u8 *msg, struct bitcoin_txid exit(0); } -static void splice_abort(struct peer *peer, struct inflight *inflight, - const char *fmt, ...) +/* Sends tx_abort, waits for their ack, tells master, and exits: callers rely + * on this not returning (check_balances falls through to further checks). */ +static NORETURN void splice_abort(struct peer *peer, struct inflight *inflight, + const char *fmt, ...) { struct bitcoin_outpoint *outpoint; u8 *msg; @@ -3599,10 +3639,18 @@ static struct amount_sat check_balances(struct peer *peer, /* As a safeguard max feerate is checked (only) locally, if it's * particularly high we fail and tell the user but allow them to - * override with `splice_force_feerate` */ - max_accepter_fee = amount_tx_fee(peer->feerate_max, + * override with `splice_force_feerate`. + * + * Whichever side is ours is held to what we're prepared to pay; the + * other side is their money, so it only has to clear the looser + * bound we apply to anything they propose. */ + max_accepter_fee = amount_tx_fee(opener + ? accepted_feerate_max(peer) + : proposed_feerate_max(peer), calc_weight(TX_ACCEPTER, psbt, false)); - max_initiator_fee = amount_tx_fee(peer->feerate_max, + max_initiator_fee = amount_tx_fee(opener + ? proposed_feerate_max(peer) + : accepted_feerate_max(peer), calc_weight(TX_INITIATOR, psbt, opener)); if (opener) { @@ -4277,9 +4325,27 @@ static void splice_accepter(struct peer *peer, const u8 *inmsg) &peer->channel->funding_pubkey[REMOTE])) status_info("Splice peer is rotating funding pubkey"); - if (funding_feerate_perkw < peer->feerate_min) + /* They initiated, so it's their fee: the looser bound applies. + * + * We disconnect rather than tx_abort here. A tx_abort has to be + * acked, and splice_abort() blocks reading until it is: a peer that + * proposes a nonsense feerate and then goes silent would leave us + * parked in that read with the channel quiesced in STFU. Since the + * bound is FEERATE_CEILING, a peer reaching it is not disagreeing + * with us about the mempool, they are broken. */ + if (funding_feerate_perkw < accepted_feerate_min(peer)) + peer_failed_warn(peer->pps, &peer->channel_id, + "Splice feerate_perkw %u is below our" + " minimum %u", + funding_feerate_perkw, + accepted_feerate_min(peer)); + + if (funding_feerate_perkw > accepted_feerate_max(peer)) peer_failed_warn(peer->pps, &peer->channel_id, - "Splice feerate_perkw is too low"); + "Splice feerate_perkw %u is above our" + " maximum %u", + funding_feerate_perkw, + accepted_feerate_max(peer)); /* TODO: Add plugin hook for user to adjust accepter amount */ peer->splicing->accepter_relative = 0; @@ -5014,14 +5080,30 @@ static void handle_splice_init(struct peer *peer, const u8 *inmsg) wire_sync_write(MASTER_FD, take(msg)); return; } - if (peer->splicing->feerate_per_kw < peer->feerate_min) { + if (peer->splicing->feerate_per_kw < accepted_feerate_min(peer)) { msg = towire_channeld_splice_state_error(NULL, tal_fmt(tmpctx, "Feerate %u is too" " low. Lower than" " channel feerate_min" " %u", peer->splicing->feerate_per_kw, - peer->feerate_min)); + accepted_feerate_min(peer))); + wire_sync_write(MASTER_FD, take(msg)); + return; + } + /* We initiated, so this is our money: hold it to what we're + * prepared to pay, not to what we'd tolerate from them. Like the + * fee check in check_balances, `force_feerate` is the user saying + * they meant it: this is a policy limit, not a safety one. */ + if (!peer->splicing->force_feerate + && peer->splicing->feerate_per_kw > proposed_feerate_max(peer)) { + msg = towire_channeld_splice_state_error(NULL, tal_fmt(tmpctx, + "Feerate %u is too" + " high. Higher than the most" + " we'll pay ourselves" + " %u", + peer->splicing->feerate_per_kw, + proposed_feerate_max(peer))); wire_sync_write(MASTER_FD, take(msg)); return; } @@ -6492,6 +6574,8 @@ static void handle_feerates(struct peer *peer, const u8 *inmsg) &feerate, &peer->feerate_min, &peer->feerate_max, + &peer->our_feerate_max, + &peer->ignore_fee_limits, &peer->feerate_penalty, &peer->feerate_opening, &peer->feerate_splice)) @@ -6865,6 +6949,8 @@ static void init_channel(struct peer *peer) &peer->feerate_splice, &peer->feerate_min, &peer->feerate_max, + &peer->our_feerate_max, + &peer->ignore_fee_limits, &peer->feerate_penalty, &peer->feerate_opening, &peer->their_commit_sig, @@ -6956,7 +7042,7 @@ static void init_channel(struct peer *peer) peer->next_index[LOCAL], peer->next_index[REMOTE], peer->revocations_received, fmt_fee_states(tmpctx, fee_states), - peer->feerate_min, peer->feerate_max, + accepted_feerate_min(peer), accepted_feerate_max(peer), fmt_height_states(tmpctx, blockheight_states), peer->our_blockheight); diff --git a/channeld/channeld_wire.csv b/channeld/channeld_wire.csv index 208da3038edb..f04442532e2f 100644 --- a/channeld/channeld_wire.csv +++ b/channeld/channeld_wire.csv @@ -31,6 +31,8 @@ msgdata,channeld_init,fee_states,fee_states, msgdata,channeld_init,feerate_splice,u32, msgdata,channeld_init,feerate_min,u32, msgdata,channeld_init,feerate_max,u32, +msgdata,channeld_init,our_feerate_max,u32, +msgdata,channeld_init,ignore_fee_limits,bool, msgdata,channeld_init,feerate_penalty,u32, msgdata,channeld_init,feerate_opening,u32, msgdata,channeld_init,first_commit_sig,bitcoin_signature, @@ -331,6 +333,8 @@ msgtype,channeld_feerates,1027 msgdata,channeld_feerates,feerate,u32, msgdata,channeld_feerates,min_feerate,u32, msgdata,channeld_feerates,max_feerate,u32, +msgdata,channeld_feerates,our_max_feerate,u32, +msgdata,channeld_feerates,ignore_fee_limits,bool, msgdata,channeld_feerates,penalty_feerate,u32, msgdata,channeld_feerates,opening_feerate,u32, msgdata,channeld_feerates,feerate_splice,u32, diff --git a/closingd/closingd.c b/closingd/closingd.c index fdf2c26341a4..3648a914cede 100644 --- a/closingd/closingd.c +++ b/closingd/closingd.c @@ -142,13 +142,22 @@ static void send_offer(struct per_peer_state *pps, struct amount_sat our_dust_limit, struct amount_sat fee_to_offer, const struct bitcoin_outpoint *wrong_funding, - const struct tlv_closing_signed_tlvs_fee_range *tlv_fees) + const struct tlv_closing_signed_tlvs_fee_range *tlv_fees, + struct amount_sat max_fee_to_accept) { struct bitcoin_tx *tx; struct bitcoin_signature our_sig; struct tlv_closing_signed_tlvs *close_tlvs; u8 *msg; + /* We can arrive here in multiple ways, so add a final sanity check + * that we did not go over our max fee */ + if (amount_sat_greater(fee_to_offer, max_fee_to_accept)) + peer_failed_warn(pps, channel_id, "Fee %s became larger than our" + " max fee %s", + fmt_amount_sat(tmpctx, fee_to_offer), + fmt_amount_sat(tmpctx, max_fee_to_accept)); + /* BOLT #2: * * - MUST set `signature` to the Bitcoin signature of the close @@ -731,7 +740,8 @@ static void do_quickclose(struct amount_sat offer[NUM_SIDES], our_dust_limit, offer[LOCAL], wrong_funding, - our_feerange); + our_feerange, + our_feerange->max_fee_satoshis); } } else { /* BOLT #2: @@ -767,7 +777,8 @@ static void do_quickclose(struct amount_sat offer[NUM_SIDES], our_dust_limit, offer[LOCAL], wrong_funding, - our_feerange); + our_feerange, + our_feerange->max_fee_satoshis); /* They will reply unless we completely agreed. */ if (!amount_sat_eq(offer[LOCAL], offer[REMOTE])) { @@ -941,7 +952,8 @@ int main(int argc, char *argv[]) our_dust_limit, offer[LOCAL], wrong_funding, - our_feerange); + our_feerange, + max_fee_to_accept); } else { if (i == 0) peer_billboard(false, "Waiting for their initial" @@ -1011,7 +1023,8 @@ int main(int argc, char *argv[]) our_dust_limit, offer[LOCAL], wrong_funding, - our_feerange); + our_feerange, + max_fee_to_accept); } else { peer_billboard(false, "Waiting for another" " closing fee offer:" diff --git a/contrib/msggen/msggen/schema.json b/contrib/msggen/msggen/schema.json index 6219f03f96e2..07b8a990ae8f 100644 --- a/contrib/msggen/msggen/schema.json +++ b/contrib/msggen/msggen/schema.json @@ -27005,7 +27005,7 @@ "next_feerate": { "type": "string", "description": [ - "For inflight opens, the next feerate we'll use for the channel open." + "For inflight opens, the next feerate we'll use for the channel open. Omitted if *last_feerate* admits no valid next feerate, which only a channel written by an older, unbounded version can do." ] }, "next_fee_step": { @@ -27971,8 +27971,7 @@ "additionalProperties": false, "required": [ "initial_feerate", - "last_feerate", - "next_feerate" + "last_feerate" ], "properties": { "state": {}, @@ -28066,7 +28065,7 @@ "next_feerate": { "type": "string", "description": [ - "The minimum feerate for the next funding transaction in per-1000-weight, with `kpw` appended." + "The minimum feerate for the next funding transaction in per-1000-weight, with `kpw` appended. Omitted if *last_feerate* admits no valid next feerate." ] } } @@ -35956,7 +35955,7 @@ "added": "v23.08", "type": "boolean", "description": [ - "If set to True means to allow the peer to set the commitment transaction fees (or closing transaction fees) to any value they want. This is dangerous: they could set an exorbitant fee (so HTLCs are unenforcable), or a tiny fee (so that commitment transactions cannot be relayed), but avoids channel breakage in case of feerate disagreements. (Note: the global `ignore_fee_limits` setting overrides this)." + "If set to True means to allow the peer to set the commitment transaction fees (or closing transaction fees) to any value they want, short of a sanity ceiling of 1000000perkw (4000 sat/vB) which we refuse regardless: a feerate above that means a broken fee estimator rather than a busy mempool. This is dangerous: they could set an exorbitant fee (so HTLCs are unenforcable), or a tiny fee (so that commitment transactions cannot be relayed), but avoids channel breakage in case of feerate disagreements. (Note: the global `ignore_fee_limits` setting overrides this)." ] } } diff --git a/doc/getting-started/getting-started/configuration.md b/doc/getting-started/getting-started/configuration.md index 4a9ab4d88741..c7c1557e41fb 100644 --- a/doc/getting-started/getting-started/configuration.md +++ b/doc/getting-started/getting-started/configuration.md @@ -268,7 +268,7 @@ The [`listconfigs`](ref:listconfigs) command will output a valid configuration f - **ignore-fee-limits**=_BOOL_ - Allow nodes which establish channels to us to set any fee they want. This may result in a channel which cannot be closed, should fees increase, but make channels far more reliable since we never close it due to unreasonable fees. + Allow nodes which establish channels to us to set any fee they want, short of a sanity ceiling of 1000000perkw (4000 sat/vB) which we refuse regardless: a feerate above that means a broken fee estimator rather than a busy mempool. This may result in a channel which cannot be closed, should fees increase, but make channels far more reliable since we never close it due to unreasonable fees. - **commit-time**=_MILLISECONDS_ diff --git a/doc/lightningd-config.5.md b/doc/lightningd-config.5.md index aaa3e8fb8bc7..55f6f59d177a 100644 --- a/doc/lightningd-config.5.md +++ b/doc/lightningd-config.5.md @@ -381,7 +381,10 @@ falls below this. * **ignore-fee-limits**=*BOOL* - Allow nodes which establish channels to us to set any fee they want. + Allow nodes which establish channels to us to set any fee they want, +short of a sanity ceiling of 1000000perkw (4000 sat/vB) which we refuse +regardless: a feerate above that means a broken fee estimator rather +than a busy mempool. This may result in a channel which cannot be closed, should fees increase, but make channels far more reliable since we never close it due to unreasonable fees. Note that this can be set on a per-channel diff --git a/doc/schemas/listpeerchannels.json b/doc/schemas/listpeerchannels.json index 40b5252758b9..3ed953be56b1 100644 --- a/doc/schemas/listpeerchannels.json +++ b/doc/schemas/listpeerchannels.json @@ -360,7 +360,7 @@ "next_feerate": { "type": "string", "description": [ - "For inflight opens, the next feerate we'll use for the channel open." + "For inflight opens, the next feerate we'll use for the channel open. Omitted if *last_feerate* admits no valid next feerate, which only a channel written by an older, unbounded version can do." ] }, "next_fee_step": { @@ -1326,8 +1326,7 @@ "additionalProperties": false, "required": [ "initial_feerate", - "last_feerate", - "next_feerate" + "last_feerate" ], "properties": { "state": {}, @@ -1421,7 +1420,7 @@ "next_feerate": { "type": "string", "description": [ - "The minimum feerate for the next funding transaction in per-1000-weight, with `kpw` appended." + "The minimum feerate for the next funding transaction in per-1000-weight, with `kpw` appended. Omitted if *last_feerate* admits no valid next feerate." ] } } diff --git a/doc/schemas/setchannel.json b/doc/schemas/setchannel.json index e8f51f626bf5..fbacdde55e67 100644 --- a/doc/schemas/setchannel.json +++ b/doc/schemas/setchannel.json @@ -57,7 +57,7 @@ "added": "v23.08", "type": "boolean", "description": [ - "If set to True means to allow the peer to set the commitment transaction fees (or closing transaction fees) to any value they want. This is dangerous: they could set an exorbitant fee (so HTLCs are unenforcable), or a tiny fee (so that commitment transactions cannot be relayed), but avoids channel breakage in case of feerate disagreements. (Note: the global `ignore_fee_limits` setting overrides this)." + "If set to True means to allow the peer to set the commitment transaction fees (or closing transaction fees) to any value they want, short of a sanity ceiling of 1000000perkw (4000 sat/vB) which we refuse regardless: a feerate above that means a broken fee estimator rather than a busy mempool. This is dangerous: they could set an exorbitant fee (so HTLCs are unenforcable), or a tiny fee (so that commitment transactions cannot be relayed), but avoids channel breakage in case of feerate disagreements. (Note: the global `ignore_fee_limits` setting overrides this)." ] } } diff --git a/lightningd/bitcoind.c b/lightningd/bitcoind.c index c1f461f7f25e..bc336c700aa2 100644 --- a/lightningd/bitcoind.c +++ b/lightningd/bitcoind.c @@ -277,12 +277,30 @@ static void estimatefees_callback(const char *buf, const jsmntok_t *toks, if (floor < FEERATE_FLOOR) floor = FEERATE_FLOOR; + /* Anything this high is a broken fee source, not a busy mempool: + * clamp it rather than let it loose on the rest of lightningd. */ + if (floor > FEERATE_CEILING) { + log_unusual(call->bitcoind->log, + "Feerate floor (%u) is above sanity ceiling (%u):" + " clamping!", + floor, (u32)FEERATE_CEILING); + floor = FEERATE_CEILING; + } + /* FIXME: We could let this go below the dynamic floor, but we'd * need to know if the floor is because of their node's policy * (minrelaytxfee) or mempool conditions (mempoolminfee). */ for (size_t i = 0; i < tal_count(feerates); i++) { feerates[i].rate = feerate_from_style(feerates[i].rate, FEERATE_PER_KBYTE); + if (feerates[i].rate > FEERATE_CEILING) { + log_unusual(call->bitcoind->log, + "Feerate for %u blocks (%u) is above sanity" + " ceiling (%u): clamping!", + feerates[i].blockcount, feerates[i].rate, + (u32)FEERATE_CEILING); + feerates[i].rate = FEERATE_CEILING; + } if (feerates[i].rate < floor) feerates[i].rate = floor; } diff --git a/lightningd/chaintopology.c b/lightningd/chaintopology.c index b77b51dfbfb4..3b2dd33e95c8 100644 --- a/lightningd/chaintopology.c +++ b/lightningd/chaintopology.c @@ -616,10 +616,19 @@ static struct rate_conversion conversions[] = { u32 opening_feerate(struct chain_topology *topo) { + u32 rate; + + /* An explicitly forced feerate is the operator saying they meant + * it, so we don't second-guess it. */ if (topo->ld->force_feerates) return topo->ld->force_feerates[FEERATE_OPENING]; - return feerate_for_deadline(topo, + + rate = feerate_for_deadline(topo, conversions[FEERATE_OPENING].blockcount); + /* We fund the opening tx, so this is our money. */ + if (rate > our_feerate_max(topo->ld, NULL)) + rate = our_feerate_max(topo->ld, NULL); + return rate; } u32 splice_feerate(struct chain_topology *topo, struct lightningd *ld) @@ -628,8 +637,9 @@ u32 splice_feerate(struct chain_topology *topo, struct lightningd *ld) if (!rate) return 0; rate += ld->config.feerate_offset; - if (rate > feerate_max(ld, NULL)) - rate = feerate_max(ld, NULL); + /* We pay for the splice we initiate. */ + if (rate > our_feerate_max(ld, NULL)) + rate = our_feerate_max(ld, NULL); return rate; } @@ -1184,6 +1194,15 @@ u32 feerate_min(struct lightningd *ld, bool *unknown) /* FIXME: This is what bcli used to do: halve the slow feerate! */ min /= 2; + /* Never demand more than we would ever propose ourselves. We cap what + * we offer at MAX_OUR_FEERATE_PER_KW (see our_feerate_max), so anything + * above that would have us refuse a peer the very feerate we would have + * sent them, which costs us the channel for nothing. A broken fee + * source clamped to FEERATE_CEILING puts this at FEERATE_CEILING/2, + * five times that cap. */ + if (min > MAX_OUR_FEERATE_PER_KW) + min = MAX_OUR_FEERATE_PER_KW; + /* We can't allow less than feerate_floor, since that won't relay */ if (min < get_feerate_floor(topo)) return get_feerate_floor(topo); @@ -1194,6 +1213,7 @@ u32 feerate_max(struct lightningd *ld, bool *unknown) { const struct chain_topology *topo = ld->topology; u32 max = 0; + u64 scaled; if (unknown) *unknown = false; @@ -1208,9 +1228,35 @@ u32 feerate_max(struct lightningd *ld, bool *unknown) if (!max) { if (unknown) *unknown = true; - return UINT_MAX; + /* No estimates: fall back to the ceiling, exactly as + * feerate_min falls back to the floor. Returning UINT_MAX + * here would be a sentinel meaning "no bound at all", and + * that bound is what a peer's proposal gets measured + * against and what we then store: a value that large + * overflows the 25/24 RBF calculation. */ + return FEERATE_CEILING; } - return max * topo->ld->config.max_fee_multiplier; + /* Estimates are clamped to FEERATE_CEILING on the way in, but the + * multiplier is settable, so widen before multiplying. */ + scaled = (u64)max * topo->ld->config.max_fee_multiplier; + + /* Don't let the multiplier carry us past the sanity ceiling: above + * that a peer is not congested, their fee source is broken. */ + if (scaled > FEERATE_CEILING) + return FEERATE_CEILING; + return scaled; +} + +u32 our_feerate_max(struct lightningd *ld, bool *unknown) +{ + u32 max = feerate_max(ld, unknown); + + /* We are stricter with ourselves than with a peer: this is our + * money, and declining to propose a feerate costs us nothing, where + * refusing a peer's costs us the channel. */ + if (max > MAX_OUR_FEERATE_PER_KW) + return MAX_OUR_FEERATE_PER_KW; + return max; } u32 default_locktime(const struct chain_topology *topo) diff --git a/lightningd/chaintopology.h b/lightningd/chaintopology.h index 11baec043756..8657546eb4a2 100644 --- a/lightningd/chaintopology.h +++ b/lightningd/chaintopology.h @@ -194,6 +194,11 @@ bool unknown_feerates(const struct chain_topology *topo); u32 feerate_min(struct lightningd *ld, bool *unknown); u32 feerate_max(struct lightningd *ld, bool *unknown); +/* Same, but the most we're willing to *pay*, which is stricter than what + * we'll tolerate from a peer (see MAX_OUR_FEERATE_PER_KW). Use this + * wherever the feerate comes out of our own funds. */ +u32 our_feerate_max(struct lightningd *ld, bool *unknown); + /* These return 0 if unknown */ u32 opening_feerate(struct chain_topology *topo); u32 splice_feerate(struct chain_topology *topo, struct lightningd *ld); diff --git a/lightningd/channel_control.c b/lightningd/channel_control.c index 202f0103990e..5a98c8216167 100644 --- a/lightningd/channel_control.c +++ b/lightningd/channel_control.c @@ -65,7 +65,8 @@ static u32 default_feerate(struct lightningd *ld, const struct channel *channel, if (!feerate) return 0; - max_feerate = feerate_max(ld, NULL); + /* We only clamp the feerate we propose here, and the opener pays it. */ + max_feerate = our_feerate_max(ld, NULL); /* The channel opener should use a slightly higher than minimal feerate * in order to avoid excessive feerate disagreements */ @@ -81,7 +82,8 @@ static u32 default_feerate(struct lightningd *ld, const struct channel *channel, void channel_update_feerates(struct lightningd *ld, const struct channel *channel) { u8 *msg; - u32 min_feerate, max_feerate; + u32 min_feerate, max_feerate, our_max_feerate; + bool ignore_fee_limits; bool anchors = channel_type_has_anchors(channel->type); u32 feerate = default_feerate(ld, channel, (channel->opener == LOCAL)); u32 feerate_splice = splice_feerate(ld->topology, ld); @@ -95,26 +97,30 @@ void channel_update_feerates(struct lightningd *ld, const struct channel *channe min_feerate = get_feerate_floor(ld->topology); else min_feerate = feerate_min(ld, NULL); + /* max_feerate is what we'll tolerate from them, our_max_feerate what + * we're prepared to pay ourselves. */ max_feerate = feerate_max(ld, NULL); - - if (channel->ignore_fee_limits || ld->config.ignore_fee_limits) { - min_feerate = 1; - max_feerate = 0xFFFFFFFF; - } + our_max_feerate = our_feerate_max(ld, NULL); + ignore_fee_limits = channel->ignore_fee_limits + || ld->config.ignore_fee_limits; log_debug(ld->log, - "update_feerates: feerate = %u, min=%u, max=%u, penalty=%u," - " opening=%u, splicing: %u", + "update_feerates: feerate = %u, min=%u, max=%u, our_max=%u," + " penalty=%u, opening=%u, splicing: %u%s", feerate, min_feerate, - feerate_max(ld, NULL), + max_feerate, + our_max_feerate, penalty_feerate(ld->topology), opening_feerate(ld->topology), - feerate_splice); + feerate_splice, + ignore_fee_limits ? " (limits ignored)" : ""); msg = towire_channeld_feerates(NULL, feerate, min_feerate, max_feerate, + our_max_feerate, + ignore_fee_limits, penalty_feerate(ld->topology), opening_feerate(ld->topology), feerate_splice); @@ -1759,7 +1765,9 @@ bool peer_start_channeld(struct channel *channel, const struct config *cfg = &ld->config; struct secret last_remote_per_commit_secret; struct penalty_base *pbases; - u32 feerate_splice, min_feerate, max_feerate, curr_blockheight; + u32 feerate_splice, min_feerate, max_feerate, our_max_feerate; + u32 curr_blockheight; + bool ignore_fee_limits; struct channel_inflight *inflight; struct inflight **inflights; struct bitcoin_txid txid; @@ -1861,12 +1869,14 @@ bool peer_start_channeld(struct channel *channel, min_feerate = get_feerate_floor(ld->topology); else min_feerate = feerate_min(ld, NULL); + /* max_feerate is what we'll tolerate from them, our_max_feerate what + * we're prepared to pay ourselves. */ max_feerate = feerate_max(ld, NULL); + our_max_feerate = our_feerate_max(ld, NULL); - if (channel->ignore_fee_limits || ld->config.ignore_fee_limits) { - min_feerate = 1; - max_feerate = 0xFFFFFFFF; - } + /* channeld applies this: the bounds above stay honest on the wire. */ + ignore_fee_limits = channel->ignore_fee_limits + || ld->config.ignore_fee_limits; /* Make sure we don't go backsards on blockheights */ curr_blockheight = get_block_height(ld->topology); @@ -1939,6 +1949,8 @@ bool peer_start_channeld(struct channel *channel, feerate_splice, min_feerate, max_feerate, + our_max_feerate, + ignore_fee_limits, penalty_feerate(ld->topology), opening_feerate(ld->topology), &channel->last_sig, @@ -2737,6 +2749,9 @@ static struct command_result *json_dev_feerate(struct command *cmd, msg = towire_channeld_feerates(NULL, *feerate, feerate_min(cmd->ld, NULL), feerate_max(cmd->ld, NULL), + our_feerate_max(cmd->ld, NULL), + channel->ignore_fee_limits + || cmd->ld->config.ignore_fee_limits, penalty_feerate(cmd->ld->topology), opening_feerate(cmd->ld->topology), splice_feerate(cmd->ld->topology, cmd->ld)); diff --git a/lightningd/closing_control.c b/lightningd/closing_control.c index 9d1859c7197e..6cdaf1956d0b 100644 --- a/lightningd/closing_control.c +++ b/lightningd/closing_control.c @@ -193,6 +193,24 @@ static struct amount_sat calc_tx_fee(struct amount_sat sat_in, return fee; } +static u32 calc_max_close_feerate(struct lightningd *ld, + struct channel *channel) +{ + u32 max_feerate; + + /* Aim for reasonable max, but use final if we don't know. */ + max_feerate = unilateral_feerate(ld->topology, false); + if (!max_feerate) + max_feerate = get_feerate(channel->fee_states, + channel->opener, LOCAL); + + /* If they specified feerates in `close`, they apply now! */ + if (channel->closing_feerate_range) + max_feerate = channel->closing_feerate_range[1]; + + return max_feerate; +} + /* Assess whether a proposed closing fee is acceptable. */ static bool closing_fee_is_acceptable(struct lightningd *ld, struct channel *channel, @@ -217,11 +235,12 @@ static bool closing_fee_is_acceptable(struct lightningd *ld, weight); if (!channel->ignore_fee_limits && !ld->config.ignore_fee_limits) { - struct amount_sat min_fee; - u32 min_feerate; + 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); + max_feerate = calc_max_close_feerate(ld, channel); min_fee = amount_tx_fee(min_feerate, weight); if (amount_sat_less(fee, min_fee)) { @@ -231,6 +250,14 @@ 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", + fmt_amount_sat(tmpctx, max_fee), + weight, max_feerate); + return false; + } } /* Prefer new over old: this covers the preference @@ -419,17 +446,13 @@ void peer_start_closingd(struct channel *channel, struct peer_fd *peer_fd) feerate = get_feerate_floor(ld->topology); } - /* 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; + max_feerate = calc_max_close_feerate(ld, channel); 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]; } /* BOLT #3: diff --git a/lightningd/dual_open_control.c b/lightningd/dual_open_control.c index 451cf1f94246..b33b6eb3e4f4 100644 --- a/lightningd/dual_open_control.c +++ b/lightningd/dual_open_control.c @@ -3,6 +3,7 @@ * saves and funding tx watching for a channel open */ #include "config.h" +#include #include #include #include @@ -2595,8 +2596,14 @@ json_openchannel_bump(struct command *cmd, * down. */ last_feerate_perkw = channel_last_funding_feerate(channel); - next_feerate_min = last_feerate_perkw * 25 / 24; - assert(next_feerate_min > last_feerate_perkw); + /* Whatever is stored could be absurd, in which case there is no next + * feerate to bump to. Fail the command rather than the daemon. */ + if (!next_funding_feerate(last_feerate_perkw, &next_feerate_min)) + return command_fail(cmd, JSONRPC2_INVALID_PARAMS, + "Can't calculate the next feerate: the" + " last funding feerate recorded for this" + " channel (%u) is out of range", + last_feerate_perkw); if (!info->feerate_per_kw_funding) { info->feerate_per_kw_funding = tal(info, u32); *info->feerate_per_kw_funding = next_feerate_min; @@ -2608,6 +2615,16 @@ json_openchannel_bump(struct command *cmd, next_feerate_min, *info->feerate_per_kw_funding); + /* We fund this, so it's our money: don't let the 25/24 escalation + * (or an ambitious caller) carry us past the most we'll pay. */ + if (*info->feerate_per_kw_funding > our_feerate_max(cmd->ld, NULL)) + return command_fail(cmd, JSONRPC2_INVALID_PARAMS, + "Feerate %u is above the most we'll pay" + " (%u); the last attempt was at %u", + *info->feerate_per_kw_funding, + our_feerate_max(cmd->ld, NULL), + last_feerate_perkw); + /* BOLT #2: * - if both nodes advertised `option_support_large_channel`: * - MAY set `funding_satoshis` greater than or equal to 2^24 satoshi. @@ -4216,6 +4233,7 @@ bool peer_start_dualopend(struct peer *peer, /* FIXME: We should override this to 0 in the openchannel2 hook of we want zeroconf*/ channel->minimum_depth = peer->ld->config.funding_confirms; + /* dualopend applies ignore_fee_limits itself, so these stay honest. */ msg = towire_dualopend_init(NULL, chainparams, peer->ld->our_features, peer->their_features, @@ -4225,6 +4243,10 @@ bool peer_start_dualopend(struct peer *peer, &channel->local_basepoints, &channel->local_funding_pubkey, channel->minimum_depth, + feerate_min(peer->ld, NULL), + feerate_max(peer->ld, NULL), + channel->ignore_fee_limits + || peer->ld->config.ignore_fee_limits, peer->ld->config.require_confirmed_inputs, *channel->alias[LOCAL], peer->ld->dev_any_channel_type); @@ -4332,6 +4354,10 @@ bool peer_restart_dualopend(struct peer *peer, &channel->local_funding_pubkey, &channel->channel_info.remote_fundingkey, channel->minimum_depth, + feerate_min(peer->ld, NULL), + feerate_max(peer->ld, NULL), + channel->ignore_fee_limits + || peer->ld->config.ignore_fee_limits, &inflight->funding->outpoint, inflight->funding->feerate, channel->funding_sats, diff --git a/lightningd/opening_control.c b/lightningd/opening_control.c index 6cc3d82f1e61..046f67d0d0a5 100644 --- a/lightningd/opening_control.c +++ b/lightningd/opening_control.c @@ -1001,13 +1001,9 @@ bool peer_start_openingd(struct peer *peer, struct peer_fd *peer_fd) &max_to_self_delay, &min_effective_htlc_capacity); - if (peer->ld->config.ignore_fee_limits) { - minrate = 1; - maxrate = 0xFFFFFFFF; - } else { - minrate = feerate_min(peer->ld, NULL); - maxrate = feerate_max(peer->ld, NULL); - } + /* openingd applies ignore_fee_limits itself, so these stay honest. */ + minrate = feerate_min(peer->ld, NULL); + maxrate = feerate_max(peer->ld, NULL); msg = towire_openingd_init(NULL, chainparams, @@ -1020,6 +1016,7 @@ bool peer_start_openingd(struct peer *peer, struct peer_fd *peer_fd) &uc->local_funding_pubkey, uc->minimum_depth, minrate, maxrate, + peer->ld->config.ignore_fee_limits, peer->ld->dev_force_tmp_channel_id, peer->ld->config.allowdustreserve, peer->ld->dev_any_channel_type); diff --git a/lightningd/peer_control.c b/lightningd/peer_control.c index 1134bca48d70..d7a9f8e60257 100644 --- a/lightningd/peer_control.c +++ b/lightningd/peer_control.c @@ -1052,14 +1052,13 @@ static void NON_NULL_ARGS(1, 2, 4, 5) json_add_channel(struct command *cmd, initial = list_top(&channel->inflights, struct channel_inflight, list); json_add_string(response, "initial_feerate", - tal_fmt(tmpctx, "%d%s", + tal_fmt(tmpctx, "%u%s", initial->funding->feerate, feerate_style_name(FEERATE_PER_KSIPA))); last_feerate = channel_last_funding_feerate(channel); - assert(last_feerate > 0); json_add_string(response, "last_feerate", - tal_fmt(tmpctx, "%d%s", last_feerate, + tal_fmt(tmpctx, "%u%s", last_feerate, feerate_style_name(FEERATE_PER_KSIPA))); /* BOLT #2: @@ -1067,11 +1066,21 @@ static void NON_NULL_ARGS(1, 2, 4, 5) json_add_channel(struct command *cmd, * times the `feerate` of the previously constructed * transaction, rounded down. */ - next_feerate = last_feerate * 25 / 24; - assert(next_feerate > last_feerate); - json_add_string(response, "next_feerate", - tal_fmt(tmpctx, "%d%s", next_feerate, - feerate_style_name(FEERATE_PER_KSIPA))); + /* The bounds and the migration together keep last_feerate in + * range, so this holds; we used to assert it. But this is + * read-only introspection that plugins call at startup, and + * aborting here would turn one bad row into a crash-loop with + * no RPC left to repair it with. */ + if (next_funding_feerate(last_feerate, &next_feerate)) { + json_add_string(response, "next_feerate", + tal_fmt(tmpctx, "%u%s", next_feerate, + feerate_style_name(FEERATE_PER_KSIPA))); + } else { + log_broken(channel->log, + "Funding feerate %u leaves no valid next" + " feerate: omitting next_feerate", + last_feerate); + } /* List the inflights */ json_array_start(response, "inflight"); @@ -1086,7 +1095,7 @@ static void NON_NULL_ARGS(1, 2, 4, 5) json_add_channel(struct command *cmd, json_add_num(response, "funding_outnum", inflight->funding->outpoint.n); json_add_string(response, "feerate", - tal_fmt(tmpctx, "%d%s", + tal_fmt(tmpctx, "%u%s", inflight->funding->feerate, feerate_style_name( FEERATE_PER_KSIPA))); diff --git a/openingd/dualopend.c b/openingd/dualopend.c index 2f1849773496..9e42a84988f1 100644 --- a/openingd/dualopend.c +++ b/openingd/dualopend.c @@ -12,6 +12,7 @@ * contribute inputs to the transaction */ #include "config.h" +#include #include #include #include @@ -162,6 +163,9 @@ struct state { /* Constraints on a channel they open. */ u32 minimum_depth; + u32 min_feerate, max_feerate; + /* Drop the policy bounds above (never the sanity ceiling). */ + bool ignore_fee_limits; struct amount_msat min_effective_htlc_capacity; /* Limits on what remote config we accept. */ @@ -424,6 +428,56 @@ static void negotiation_failed(struct state *state, open_abort(state, "You gave bad parameters: %s", errmsg); } +/* Ignoring the fee limits drops the policy bounds, but never the sanity + * ceiling: a feerate above that means a broken fee source, and whatever we + * accept here is what we go on to store. + * + * For anchor channels the commitment only has to relay: its fee gets topped + * up by the anchor spend when we actually need it onchain, so the relay floor + * is the real bound there. Holding the opener to our *policy* minimum would + * refuse the very feerate we would propose ourselves, since lightningd also + * uses the floor for anchors (see update_feerates()). */ +static u32 accepted_commitment_feerate_min(const struct state *state) +{ + if (state->ignore_fee_limits) + return 1; + if (channel_type_has_anchors(state->channel_type)) + return FEERATE_FLOOR; + return state->min_feerate; +} + +static u32 accepted_feerate_max(const struct state *state) +{ + if (state->ignore_fee_limits) + return FEERATE_CEILING; + return state->max_feerate; +} + +/* Both feerates in an open (or an RBF of one) come straight off the wire from + * the opener, and nothing downstream bounds them: the openchannel2 hook only + * *reports* our limits, so with no plugin hooked nothing enforces them, and + * check_funding_feerate() governs only the lower 25/24 RBF step. Returns + * false having already failed the negotiation. + * + * The two feerates want different floors, hence min_feerate: see the callers. */ +static bool feerate_in_range(struct state *state, const char *name, + u32 feerate, u32 min_feerate) +{ + if (feerate < min_feerate) { + negotiation_failed(state, "%s %u below minimum %u", + name, feerate, min_feerate); + return false; + } + + if (feerate > accepted_feerate_max(state)) { + negotiation_failed(state, "%s %u above maximum %u", + name, feerate, accepted_feerate_max(state)); + return false; + } + + return true; +} + static void billboard_update(struct state *state) { const char *update = billboard_message(tmpctx, state->channel_ready, @@ -2427,6 +2481,20 @@ static void accepter_start(struct state *state, const u8 *oc2_msg) fmt_channel_id(tmpctx, &cid)); } + /* Now state->channel_id is set, so an abort is one the opener can + * match up, check the feerates: do it before anything else we might + * commit to, as these are what we would go on to sign for and store. + * + * The funding feerate only has to be relayable. If the opener picks a + * slow one that is their problem, and RBF is the remedy, so holding it + * to our *policy* minimum would refuse perfectly good opens. But 0 is + * not a feerate, and it is precisely the value that leaves no valid + * next RBF step downstream, so the relay floor is the right bound. */ + if (!feerate_in_range(state, "funding_feerate_perkw", + tx_state->feerate_per_kw_funding, + FEERATE_FLOOR)) + return; + /* BOLT #2: * The receiving node MUST fail the channel if: *... @@ -2458,6 +2526,16 @@ static void accepter_start(struct state *state, const u8 *oc2_msg) } } + /* The commitment feerate is a different matter: too low and the + * commitment we are signing cannot be relayed when we need it. This + * has to wait for channel_type above, since what counts as too low + * depends on whether we negotiated anchors. Nothing between the two + * commits us to anything. */ + if (!feerate_in_range(state, "commitment_feerate_perkw", + state->feerate_per_kw_commitment, + accepted_commitment_feerate_min(state))) + return; + /* Since anchor outputs are optional, we * only support liquidity ads if those are enabled. */ if (open_tlv->request_funds && @@ -3350,10 +3428,10 @@ static bool check_funding_feerate(u32 proposed_next_feerate, * - the `feerate` is not greater than or equal to 25/24 times `feerate` * of the last successfully constructed transaction */ - u32 next_min = last_feerate * 25 / 24; + u32 next_min; - if (next_min < last_feerate) { - status_broken("Overflow calculating next feerate. last %u", + if (!next_funding_feerate(last_feerate, &next_min)) { + status_broken("Can't calculate next feerate. last %u", last_feerate); return false; } @@ -3707,6 +3785,13 @@ static void rbf_remote_start(struct state *state, const u8 *rbf_msg) goto free_rbf_ctx; } + /* check_funding_feerate() only enforces the 25/24 step upwards, so + * without this an RBF can walk the feerate up without limit. */ + if (!feerate_in_range(state, "funding_feerate_perkw", + tx_state->feerate_per_kw_funding, + FEERATE_FLOOR)) + goto free_rbf_ctx; + /* We ask master if this is ok */ msg = towire_dualopend_got_rbf_offer(NULL, &state->channel_id, @@ -4340,6 +4425,9 @@ int main(int argc, char *argv[]) &state->our_points, &state->our_funding_pubkey, &state->minimum_depth, + &state->min_feerate, + &state->max_feerate, + &state->ignore_fee_limits, &state->require_confirmed_inputs[LOCAL], &state->local_alias, &state->dev_accept_any_channel_type)) { @@ -4379,6 +4467,9 @@ int main(int argc, char *argv[]) &state->our_funding_pubkey, &state->their_funding_pubkey, &state->minimum_depth, + &state->min_feerate, + &state->max_feerate, + &state->ignore_fee_limits, &state->tx_state->funding, &state->tx_state->feerate_per_kw_funding, &total_funding, diff --git a/openingd/dualopend_wire.csv b/openingd/dualopend_wire.csv index d6b157495437..0e967ac04353 100644 --- a/openingd/dualopend_wire.csv +++ b/openingd/dualopend_wire.csv @@ -29,6 +29,9 @@ msgdata,dualopend_init,our_basepoints,basepoints, msgdata,dualopend_init,our_funding_pubkey,pubkey, # Constraints in case the other end tries to open a channel. msgdata,dualopend_init,minimum_depth,u32, +msgdata,dualopend_init,min_feerate,u32, +msgdata,dualopend_init,max_feerate,u32, +msgdata,dualopend_init,ignore_fee_limits,bool, msgdata,dualopend_init,require_confirmed_inputs,bool, msgdata,dualopend_init,local_alias,short_channel_id, msgdata,dualopend_init,dev_accept_any_channel_type,bool, @@ -49,6 +52,9 @@ msgdata,dualopend_reinit,our_basepoints,basepoints, msgdata,dualopend_reinit,our_funding_pubkey,pubkey, msgdata,dualopend_reinit,their_funding_pubkey,pubkey, msgdata,dualopend_reinit,minimum_depth,u32, +msgdata,dualopend_reinit,min_feerate,u32, +msgdata,dualopend_reinit,max_feerate,u32, +msgdata,dualopend_reinit,ignore_fee_limits,bool, msgdata,dualopend_reinit,funding,bitcoin_outpoint, msgdata,dualopend_reinit,most_recent_feerate_per_kw_funding,u32, msgdata,dualopend_reinit,funding_satoshi,amount_sat, diff --git a/openingd/openingd.c b/openingd/openingd.c index a0585c6cfd9d..d4be40e0af02 100644 --- a/openingd/openingd.c +++ b/openingd/openingd.c @@ -8,6 +8,7 @@ * commit to the database once openingd succeeds. */ #include "config.h" +#include #include #include #include @@ -49,6 +50,8 @@ struct state { /* Constraints on a channel they open. */ u32 minimum_depth; u32 min_feerate, max_feerate; + /* Drop the policy bounds above (never the sanity ceiling). */ + bool ignore_fee_limits; struct amount_msat min_effective_htlc_capacity; /* Limits on what remote config we accept. */ @@ -845,6 +848,7 @@ static u8 *fundee_channel(struct state *state, const u8 *open_channel_msg) struct tlv_accept_channel_tlvs *accept_tlvs; struct tlv_open_channel_tlvs *open_tlvs; struct amount_sat *reserve; + u32 min_feerate, max_feerate; /* BOLT #2: * @@ -955,17 +959,24 @@ static u8 *fundee_channel(struct state *state, const u8 *open_channel_msg) * - it considers `feerate_per_kw` too small for timely processing or * unreasonably large. */ - if (state->feerate_per_kw < state->min_feerate) { + /* Even ignoring the limits we refuse 0: that is not a feerate any + * commitment could be relayed at, and it is what we would store. */ + min_feerate = state->ignore_fee_limits ? 1 : state->min_feerate; + if (state->feerate_per_kw < min_feerate) { negotiation_failed(state, "feerate_per_kw %u below minimum %u", - state->feerate_per_kw, state->min_feerate); + state->feerate_per_kw, min_feerate); return NULL; } - if (state->feerate_per_kw > state->max_feerate) { + /* Even ignoring the limits we refuse the absurd: this is the feerate + * we would go on to store. */ + max_feerate = state->ignore_fee_limits + ? FEERATE_CEILING : state->max_feerate; + if (state->feerate_per_kw > max_feerate) { negotiation_failed(state, "feerate_per_kw %u above maximum %u", - state->feerate_per_kw, state->max_feerate); + state->feerate_per_kw, max_feerate); return NULL; } @@ -1452,6 +1463,7 @@ int main(int argc, char *argv[]) &state->our_funding_pubkey, &state->minimum_depth, &state->min_feerate, &state->max_feerate, + &state->ignore_fee_limits, &state->dev_force_tmp_channel_id, &state->allowdustreserve, &state->dev_accept_any_channel_type)) diff --git a/openingd/openingd_wire.csv b/openingd/openingd_wire.csv index f96c50b0037c..7ea719bbd740 100644 --- a/openingd/openingd_wire.csv +++ b/openingd/openingd_wire.csv @@ -24,6 +24,7 @@ msgdata,openingd_init,our_funding_pubkey,pubkey, msgdata,openingd_init,minimum_depth,u32, msgdata,openingd_init,min_feerate,u32, msgdata,openingd_init,max_feerate,u32, +msgdata,openingd_init,ignore_fee_limits,bool, msgdata,openingd_init,dev_temporary_channel_id,?byte,32 # Do we allow `fundchannel` or the `openchannel` hook to set sub-dust # reserves? This is explicitly required by the spec for safety diff --git a/tests/fuzz/fuzz-open_channel.c b/tests/fuzz/fuzz-open_channel.c index 999685c65060..070a638bf89d 100644 --- a/tests/fuzz/fuzz-open_channel.c +++ b/tests/fuzz/fuzz-open_channel.c @@ -347,6 +347,9 @@ static struct state *fromwire_new_state(const tal_t *ctx) state->minimum_depth = fromwire_u32(cursor, max); state->min_feerate = fromwire_u32(cursor, max); state->max_feerate = fromwire_u32(cursor, max); + /* Take this from the input too, so we explore both the bounded and the + * ignore-fee-limits path through fundee_channel(). */ + state->ignore_fee_limits = fromwire_bool(cursor, max); state->our_funding_pubkey = dummy_pubkey; /* Set developer options to false. */ diff --git a/tests/plugins/channeld_fakenet.c b/tests/plugins/channeld_fakenet.c index f43384fa371c..a175e1b04cea 100644 --- a/tests/plugins/channeld_fakenet.c +++ b/tests/plugins/channeld_fakenet.c @@ -959,10 +959,12 @@ static void handle_offer_htlc(struct info *info, const u8 *inmsg) static void handle_feerates(struct info *info, const u8 *inmsg) { - u32 feerate, min, max, penalty, opening, splicing; + u32 feerate, min, max, our_max, penalty, opening, splicing; + bool ignore_fee_limits; if (!fromwire_channeld_feerates(inmsg, &feerate, - &min, &max, &penalty, &opening, + &min, &max, &our_max, + &ignore_fee_limits, &penalty, &opening, &splicing)) master_badmsg(WIRE_CHANNELD_FEERATES, inmsg); @@ -1056,6 +1058,8 @@ static struct channel *handle_init(struct info *info, const u8 *init_msg) struct penalty_base *pbases; struct channel_type *channel_type; u32 feerate_splice, feerate_min, feerate_max, feerate_penalty, feerate_opening; + u32 our_feerate_max; + bool ignore_fee_limits; struct pubkey remote_per_commit; struct pubkey old_remote_per_commit; u32 commit_msec; @@ -1098,6 +1102,8 @@ static struct channel *handle_init(struct info *info, const u8 *init_msg) &feerate_splice, &feerate_min, &feerate_max, + &our_feerate_max, + &ignore_fee_limits, &feerate_penalty, &feerate_opening, &their_commit_sig, diff --git a/tests/test_misc.py b/tests/test_misc.py index a87f956cdbb5..1337349b3102 100644 --- a/tests/test_misc.py +++ b/tests/test_misc.py @@ -1884,7 +1884,8 @@ def test_feerates(node_factory, anchors): feerates = l1.rpc.feerates('perkw') assert feerates['warning_missing_feerates'] == 'Some fee estimates unavailable: bitcoind startup?' assert 'perkb' not in feerates - assert feerates['perkw']['max_acceptable'] == 2**32 - 1 + # No estimates: falls back to the ceiling, as min falls back to the floor. + assert feerates['perkw']['max_acceptable'] == 1000000 assert feerates['perkw']['min_acceptable'] == 253 assert feerates['perkw']['min_acceptable'] == 253 assert feerates['perkw']['floor'] == 253 @@ -1895,7 +1896,7 @@ def test_feerates(node_factory, anchors): feerates = l1.rpc.feerates('perkb') assert feerates['warning_missing_feerates'] == 'Some fee estimates unavailable: bitcoind startup?' assert 'perkw' not in feerates - assert feerates['perkb']['max_acceptable'] == (2**32 - 1) + assert feerates['perkb']['max_acceptable'] == 1000000 * 4 assert feerates['perkb']['min_acceptable'] == 253 * 4 # Note: This is floored at the FEERATE_FLOOR constant (253) assert feerates['perkb']['floor'] == 1012 @@ -2015,6 +2016,35 @@ def test_feerates(node_factory, anchors): assert htlc_success_cost == htlc_feerate * 703 // 1000 +@unittest.skipIf(TEST_NETWORK == 'liquid-regtest', "Fees on elements are different") +def test_feerate_ceiling(node_factory): + """A broken fee source can't feed absurd feerates into the daemon.""" + l1 = node_factory.get_node() + + # bcli trims anything wider than a u32 of perkb down to exactly + # 0xFFFFFFFF. That is also the interesting value for the conversion: + # (0xFFFFFFFF + 3) / 4 wraps to 0 on a u32, so before the conversion was + # widened this arrived as 0perkw and was quietly raised to the floor, + # i.e. an absurd fee source produced an absurdly *low* feerate and the + # ceiling never saw it. + def absurd_feerate(r): + return {'id': r['id'], 'error': None, + 'result': {'feerate': Decimal(900000)}} + + l1.daemon.rpcproxy.mock_rpc('estimatesmartfee', absurd_feerate) + l1.restart() + + l1.daemon.wait_for_log(r'is above sanity ceiling \(1000000\): clamping!') + + feerates = l1.rpc.feerates('perkw')['perkw'] + assert [e['feerate'] for e in feerates['estimates']] == [1000000] * 4 + # max_fee_multiplier can't carry max_acceptable past the ceiling either. + assert feerates['max_acceptable'] == 1000000 + # And what we're prepared to pay ourselves stays well under it. + assert feerates['opening'] <= 100000 + assert feerates['splice'] <= 100000 + + def test_logging(node_factory): # Since we redirect, node.start() will fail: do manually. l1 = node_factory.get_node(options={'log-file': 'logfile'}, start=False) @@ -4986,8 +5016,11 @@ def test_set_feerate_offset(node_factory, bitcoind): else: feerate = 11100 min_feerate = 1875 + # our_max is what we're willing to pay ourselves (MAX_OUR_FEERATE_PER_KW), + # as opposed to max, which is what we'll tolerate from the peer. l1.daemon.wait_for_log(f'lightningd: update_feerates: feerate = {feerate}, ' - f'min={min_feerate}, max=150000, penalty=7500') + f'min={min_feerate}, max=150000, our_max=100000, ' + f'penalty=7500') l2.daemon.wait_for_log(f'peer updated fee to {feerate}') l2.pay(l1, 100000000) diff --git a/tests/test_opening.py b/tests/test_opening.py index e23283f7251c..ace1a4c0c57b 100644 --- a/tests/test_opening.py +++ b/tests/test_opening.py @@ -20,6 +20,40 @@ def find_next_feerate(node, peer): return chan['next_feerate'] +@unittest.skipIf(TEST_NETWORK != 'regtest', 'elementsd doesnt yet support PSBT features we need') +@pytest.mark.openchannel('v2') +def test_v2_open_feerate_out_of_range(node_factory, bitcoind): + """We refuse an open_channel2 whose feerates are outside our bounds. + + dualopend was never given min_feerate/max_feerate at all, and the + openchannel2 hook only *reports* our limits, so with no plugin hooked + nothing enforced them: the opener could name any feerate in either + direction and we would sign for it and store it. + """ + # l1 has expensive estimates, l2 has cheap ones, so what l1 proposes is + # well above what l2 will put up with. + l1 = node_factory.get_node(feerates=(50000, 50000, 50000, 50000)) + l2 = node_factory.get_node(feerates=(3000, 3000, 3000, 3000), + allow_warning=True) + + assert l2.rpc.feerates('perkw')['perkw']['max_acceptable'] == 30000 + + l1.fundwallet(10**7) + l1.rpc.connect(l2.info['id'], 'localhost', l2.port) + + # The abort has to name the channel we're opening, or the opener can't + # match it up and answers "Unknown channel" instead of failing the open. + with pytest.raises(RpcError, match=r'funding_feerate_perkw 50000 above maximum 30000'): + l1.rpc.fundchannel(l2.info['id'], 500000) + + l2.daemon.wait_for_log(r'funding_feerate_perkw 50000 above maximum 30000') + assert not l1.daemon.is_in_log(r'Unknown channel for WIRE_TX_ABORT') + + # No channel, and nothing stored to trip over later. + assert l2.rpc.listpeerchannels()['channels'] == [] + assert l2.db_query("SELECT count(*) AS c FROM channel_funding_inflights;")[0]['c'] == 0 + + @unittest.skipIf(TEST_NETWORK != 'regtest', 'elementsd doesnt yet support PSBT features we need') @pytest.mark.openchannel('v2') def test_queryrates(node_factory, bitcoind): diff --git a/tests/test_splicing.py b/tests/test_splicing.py index 8e9ba0e40b4d..ee03b1439e1c 100644 --- a/tests/test_splicing.py +++ b/tests/test_splicing.py @@ -1,6 +1,8 @@ from fixtures import * # noqa: F401,F403 from pyln.client import RpcError +import os import pytest +import threading import unittest import time from utils import ( @@ -47,6 +49,112 @@ def test_splice(node_factory, bitcoind): assert l1.db_query("SELECT count(*) as c FROM channeltxs;")[0]['c'] == 0 +def _splice_to_inflight(l1, chan_id, amount=100000): + """Drive a splice as far as an inflight in the db, and return its txid.""" + funds_result = l1.rpc.fundpsbt("111722sat", 0, 0, excess_as_change=True) + result = l1.rpc.splice_init(chan_id, amount, funds_result['psbt']) + result = l1.rpc.splice_update(chan_id, result['psbt']) + result = l1.rpc.splice_update(chan_id, result['psbt']) + assert result['commitments_secured'] is True + result = l1.rpc.signpsbt(result['psbt']) + result = l1.rpc.splice_signed(chan_id, result['signed_psbt']) + l1.daemon.wait_for_log(r'CHANNELD_NORMAL to CHANNELD_AWAITING_SPLICE') + return result['txid'] + + +@pytest.mark.openchannel('v1') +@unittest.skipIf(TEST_NETWORK != 'regtest', 'elementsd doesnt yet support PSBT features we need') +@unittest.skipIf(os.getenv('TEST_DB_PROVIDER', 'sqlite3') != 'sqlite3', + "modifies database, which is assumed sqlite3") +# -1 is how lightningd itself stored a u32 above INT_MAX (db_bind_int); the +# positive form is what you get writing the same value by hand. +@pytest.mark.parametrize("poison,repaired", [(4294967295, 1000000), + (-1, 1000000), + (2000000, 1000000), + (0, 253)]) +def test_splice_stored_feerate_repaired_on_upgrade(node_factory, bitcoind, + poison, repaired): + """An out-of-range stored funding feerate is repaired when we upgrade. + + Nothing used to bound what got written to + channel_funding_inflights.funding_feerate, and json_add_channel then + asserted on it: the BOLT #2 25/24 RBF bump overflows a u32 above + UINT_MAX/25, and 0 tripped the assert right above it. Since plugins call + listpeerchannels at startup, a single bad row crash-looped the node with + no RPC left to repair it with, which is what the migration is for. + """ + l1, l2 = node_factory.line_graph(2, fundamount=1000000, + wait_for_announce=True) + chan_id = l1.get_channel_id(l2) + _splice_to_inflight(l1, chan_id) + + l1.stop() + l1.db_manip("UPDATE channel_funding_inflights" + " SET funding_feerate = {}".format(poison)) + + # Rewind past the two clamping migrations so they run again over the row + # we just planted, which is the upgrade an attacked node goes through. + # They are plain idempotent UPDATEs, so re-running them is safe. + l1.db_manip("UPDATE version SET version = version - 2") + l1.daemon.opts['database-upgrade'] = 'true' + l1.start() + + assert l1.daemon.is_in_log(r'Updating database from version') + + row = l1.db_query("SELECT funding_feerate AS f" + " FROM channel_funding_inflights;")[0] + assert row['f'] == repaired + + # And the read path, which used to abort here, agrees. + chan = only_one(l1.rpc.listpeerchannels()['channels']) + assert chan['last_feerate'] == '{}perkw'.format(repaired) + assert chan['next_feerate'] == '{}perkw'.format(repaired * 25 // 24) + + +@pytest.mark.openchannel('v1') +@unittest.skipIf(TEST_NETWORK != 'regtest', 'elementsd doesnt yet support PSBT features we need') +def test_splice_feerate_too_high(node_factory, bitcoind): + """We refuse a splice a peer proposes at an absurd feerate. + + The fee comes out of the initiator's balance, so we gain nothing by + signing it; they lose the difference to a broken fee estimator of theirs. + Before this there was no upper bound on the accepter side at all. + """ + l1, l2 = node_factory.line_graph(2, fundamount=1000000, + wait_for_announce=True, + opts={'allow_warning': True, + 'may_reconnect': True}) + chan_id = l1.get_channel_id(l2) + + # Both sides agree feerate_max is 15000 * max_fee_multiplier. + assert l2.rpc.feerates('perkw')['perkw']['max_acceptable'] == 150000 + + # force_feerate gets us past *our* check on what we're willing to pay, + # which leaves l2's bound as the thing under test. + funds_result = l1.rpc.fundpsbt("111722sat", 0, 0, excess_as_change=True) + + # l2 refuses on receipt of splice_init, so the splice_ack l1 is waiting + # for never arrives: run it in a daemon thread so the test can proceed. + def _splice(): + try: + # force_feerate isn't in the pyln-client wrapper, so call directly. + l1.rpc.call('splice_init', + {'channel_id': chan_id, + 'relative_amount': 100000, + 'initialpsbt': funds_result['psbt'], + # param_feerate reads a bare number as perkb, and + # the schema only allows a bare number here: + # 800000perkb == 200000perkw. + 'feerate_per_kw': 800000, + 'force_feerate': True}) + except Exception: + pass + + threading.Thread(target=_splice, daemon=True).start() + + l2.daemon.wait_for_log(r'Splice feerate_perkw 200000 is above our maximum 150000') + + @pytest.mark.openchannel('v1') @pytest.mark.openchannel('v2') @unittest.skipIf(TEST_NETWORK != 'regtest', 'elementsd doesnt yet support PSBT features we need') diff --git a/wallet/migrations.c b/wallet/migrations.c index 66dd26ca50dc..bcbbcef6c0cd 100644 --- a/wallet/migrations.c +++ b/wallet/migrations.c @@ -1186,6 +1186,40 @@ static const struct db_migration dbmigrations[] = { * after the failure was recorded (issue #9341). */ {SQL("ALTER TABLE payments ADD failmsg BLOB;"), NULL, SQL("ALTER TABLE payments DROP COLUMN failmsg"), NULL}, + /* Nothing used to bound the feerate we record for an inflight funding + * transaction, so a broken fee estimator (ours or a peer's) could get an + * absurd value in here. That is not merely cosmetic: the BOLT #2 rule + * that the next RBF attempt pays 25/24 times the last feerate is computed + * on a u32, so anything above UINT_MAX/25 overflows and trips the + * assert(next_feerate > last_feerate) in listpeerchannels, which plugins + * call at startup: the node crash-loops with no way out but to rewrite the + * stored value. A stored 0 trips the assert immediately above it. + * + * The preceding commits close every path such a value could arrive on. + * This repairs what is already there, so that from here the bounds hold + * for stored feerates too and the read path can rely on them. + * + * We write the feerate with db_bind_int(), so a u32 above INT_MAX reads + * back negative; those are exactly the values that overflow, hence the + * second clause below. + * + * The bounds are spelled out rather than written as FEERATE_CEILING and + * FEERATE_FLOOR on purpose: a migration has to keep doing exactly what it + * did on the day it shipped, so it must not move when those constants do. + * + * Rewriting is safe: this feerate only sanity checks the fee the funding + * transaction already pays, and tells the user what the next RBF must + * beat. It never feeds anything we have signed. */ + {SQL("UPDATE channel_funding_inflights" + " SET funding_feerate = 1000000" + " WHERE funding_feerate > 1000000 OR funding_feerate < 0;"), NULL, + /* Clamping is idempotent, so no revert needed */ + NULL, NULL}, + {SQL("UPDATE channel_funding_inflights" + " SET funding_feerate = 253" + " WHERE funding_feerate = 0 OR funding_feerate IS NULL;"), NULL, + /* Clamping is idempotent, so no revert needed */ + NULL, NULL}, /* ^v26.09 */ };