From 70fccef51337e8b1e1841bd2244b1948aec82e08 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 15 Feb 2024 17:04:46 +1030 Subject: [PATCH] WIP --- channeld/full_channel.c | 38 ++++++++++++++------------------------ common/fee_states.c | 21 +++++++++++++++++++++ common/fee_states.h | 10 ++++++++++ lightningd/peer_control.c | 10 ++++++++-- tests/test_pay.py | 31 +++++++++++++++++++++---------- 5 files changed, 74 insertions(+), 36 deletions(-) diff --git a/channeld/full_channel.c b/channeld/full_channel.c index 7b12f80d0692..8fb159ccd935 100644 --- a/channeld/full_channel.c +++ b/channeld/full_channel.c @@ -525,7 +525,7 @@ static bool htlc_dust(const struct channel *channel, * * To mostly avoid this situation, at least from our side, we apply an * additional constraint when we're opener trying to add an HTLC: make - * sure we can afford one more HTLC, even if fees increase by 100%. + * sure we can afford one more HTLC, even if fees increase. * * We could do this for the peer, as well, by rejecting their HTLC * immediately in this case. But rejecting a remote HTLC here causes @@ -560,39 +560,19 @@ static bool local_opener_has_fee_headroom(const struct channel *channel, option_anchors_zero_fee_htlc_tx, committed, adding, removing); - /* Scale the feerate up by a margin. This ensures that we have - * some leeway even in rising fees. The calculation starts - * with a 100% margin at very low fees, since they are likely - * to rise, and can do so quickly, whereas on the higher fee - * side, asking for a 100% margin is excessive, so ask for a - * 10% margin. In-between these two regions we interpolate - * linearly. Notice that minfeerate and maxfeerate are just - * the markers of the linear interpolation, they don't have - * to correspond to actual feerates seen in the network. - * - * See [CLN6974] for details and discussion. - * - * [CLN6974]: https://github.com/ElementsProject/lightning/issues/6974 - */ - u64 minfeerate = 253, maxfeerate = 45000, - min = feerate - minfeerate > maxfeerate ? maxfeerate - : feerate - minfeerate; - double marginperc = 1 - min / (maxfeerate * 1.1); - u64 marginrate = 1 + marginperc; - /* Now, how much would it cost us if feerate increases 100% and we added * another HTLC? */ - fee = commit_tx_base_fee(marginrate, untrimmed + 1, + fee = commit_tx_base_fee(marginal_feerate(feerate), untrimmed + 1, option_anchor_outputs, option_anchors_zero_fee_htlc_tx); if (amount_msat_greater_eq_sat(remainder, fee)) return true; status_debug("Adding HTLC would leave us only %s: we need %s for" - " another HTLC if fees increase by 100%% to %uperkw", + " another HTLC if fees increase from %uperkw to %uperkw", type_to_string(tmpctx, struct amount_msat, &remainder), type_to_string(tmpctx, struct amount_sat, &fee), - feerate + feerate); + feerate, marginal_feerate(feerate)); return false; } @@ -770,6 +750,11 @@ static enum channel_add_err add_htlc(struct channel *channel, if (htlc_fee) *htlc_fee = fee; + status_debug("htlc_fee for %s %zu adding %zu removing == %s", + recipient == LOCAL ? "us" : "them", + tal_count(adding), tal_count(removing), + type_to_string(tmpctx, struct amount_sat, &fee)); + /* This is a little subtle: * * The change is being applied to the receiver but it will @@ -834,6 +819,11 @@ static enum channel_add_err add_htlc(struct channel *channel, adding, removing, channel->opener); + status_debug("htlc_fee for %s %zu adding %zu removing == %s", + recipient == LOCAL ? "us" : "them", + tal_count(adding), tal_count(removing), + type_to_string(tmpctx, struct amount_sat, &fee)); + /* set fee output pointer if given */ if (htlc_fee && amount_sat_greater(fee, *htlc_fee)) *htlc_fee = fee; diff --git a/common/fee_states.c b/common/fee_states.c index adf6845d4e72..c2a24a95dc44 100644 --- a/common/fee_states.c +++ b/common/fee_states.c @@ -157,6 +157,27 @@ bool fee_states_valid(const struct fee_states *fee_states, enum side opener) return fee_states->feerate[last_fee_state(opener)] != NULL; } +/* The calculation starts with a 100% margin at very low fees, since + * they are likely to rise, and can do so quickly, whereas on the + * higher fee side, asking for a 100% margin is excessive, so ask for + * a 10% margin. In-between these two regions we interpolate + * linearly. Notice that minfeerate and maxfeerate are just the + * markers of the linear interpolation, they don't have to correspond + * to actual feerates seen in the network. */ +u32 marginal_feerate(u32 current_feerate) +{ + const u32 minfeerate = 253, maxfeerate = 45000; + + assert(current_feerate >= minfeerate); + if (current_feerate > maxfeerate) + return current_feerate * 1.1; + + /* min gives 1, max gives 0.1 */ + double proportion = 1.0 - ((double)current_feerate - minfeerate) / (maxfeerate - minfeerate) * 0.9; + + return current_feerate + proportion * current_feerate; +} + static const char *fmt_fee_states(const tal_t *ctx, const struct fee_states *fee_states) { diff --git a/common/fee_states.h b/common/fee_states.h index cf846002b90e..e5b0a0a50b85 100644 --- a/common/fee_states.h +++ b/common/fee_states.h @@ -90,4 +90,14 @@ bool fee_states_valid(const struct fee_states *fee_states, enum side opener); bool feerate_changes_done(const struct fee_states *fee_states, bool ignore_uncommitted); +/** + * https://github.com/lightningnetwork/lightning-rfc/issues/740 indicates + * we should not allow an HTLC if a fee increase of 100% would cause us to + * get stuck. + * + * This proved overzealous: if fees are already high there is less chance + * they increase by another 100%, so we scale the factor. + */ +u32 marginal_feerate(u32 current_feerate); + #endif /* LIGHTNING_COMMON_FEE_STATES_H */ diff --git a/lightningd/peer_control.c b/lightningd/peer_control.c index 0feeade9e9cd..e803c0e240b2 100644 --- a/lightningd/peer_control.c +++ b/lightningd/peer_control.c @@ -673,9 +673,13 @@ static struct amount_sat commit_txfee(const struct channel *channel, * reserve. It is recommended that this "fee spike buffer" can * handle twice the current `feerate_per_kw` to ensure * predictability between implementations. - */ - fee = commit_tx_base_fee(2 * feerate, num_untrimmed_htlcs + 1, + */ + fee = commit_tx_base_fee(marginal_feerate(feerate), num_untrimmed_htlcs + 1, option_anchor_outputs, option_anchors_zero_fee_htlc_tx); + log_debug(channel->log, "Before anchors: fee for %zu untrimmed at feerate %u (based on %u) will be %s", + num_untrimmed_htlcs + 1, + marginal_feerate(feerate), feerate, + type_to_string(tmpctx, struct amount_sat, &fee)); if (option_anchor_outputs || option_anchors_zero_fee_htlc_tx) { /* BOLT #3: @@ -686,6 +690,8 @@ static struct amount_sat commit_txfee(const struct channel *channel, */ if (!amount_sat_add(&fee, fee, AMOUNT_SAT(660))) ; /* fee is somehow astronomical already.... */ + log_debug(channel->log, "After anchors: fee is %s", + type_to_string(tmpctx, struct amount_sat, &fee)); } return fee; diff --git a/tests/test_pay.py b/tests/test_pay.py index 24bb1f88aa4b..2f4e2a910364 100644 --- a/tests/test_pay.py +++ b/tests/test_pay.py @@ -754,7 +754,8 @@ def test_wait_sendpay(node_factory, executor): @pytest.mark.parametrize("anchors", [False, True]) def test_sendpay_cant_afford(node_factory, anchors): # Set feerates the same so we don't have to wait for update. - opts = {'feerates': (15000, 15000, 15000, 15000)} + opts = {'feerates': (15000, 15000, 15000, 15000), + 'commit-feerate-offset': 0} if anchors is False: opts['dev-force-features'] = "-23" @@ -771,22 +772,28 @@ def test_sendpay_cant_afford(node_factory, anchors): # minimum = 1 # maximum = 10**9 # while maximum - minimum > 1: - # l1, l2 = node_factory.line_graph(2, fundamount=10**6, - # opts={'feerates': (15000, 15000, 15000, 15000)}) + # l1, l2 = node_factory.line_graph(2, fundamount=10**6, opts=opts) # try: # l1.pay(l2, (minimum + maximum) // 2) + # print("XXX Pay {} WORKED!".format((minimum + maximum) // 2)) # minimum = (minimum + maximum) // 2 # except RpcError: + # print("XXX Pay {} FAILED!".format((minimum + maximum) // 2)) # maximum = (minimum + maximum) // 2 # print("{} - {}".format(minimum, maximum)) - # assert False + + # # Make sure not HTLCs left to interfere + # wait_for(lambda: only_one(l1.rpc.listpeerchannels()['channels'])['htlcs'] == []) + # wait_for(lambda: only_one(l2.rpc.listpeerchannels()['channels'])['htlcs'] == []) + # assert False, "Max we can pay == {}".format(minimum) + # # Currently this gives: 976560000 for non-anchors, 969900000 for anchors + # # Add reserve to this result to derive `available` # This is the fee, which needs to be taken into account for l1. - if anchors: - # option_anchor_outputs - available = 10**9 - 44700000 + if anchors: + available = 969900000 + reserve else: - available = 10**9 - 32040000 + available = 976560000 + reserve # Can't pay past reserve. with pytest.raises(RpcError): @@ -2507,11 +2514,15 @@ def test_setchannel_startup_opts(node_factory, bitcoind): assert result[1]['htlc_maximum_msat'] == Millisatoshi(5) -def test_channel_spendable(node_factory, bitcoind): +@pytest.mark.parametrize("anchors", [False, True]) +def test_channel_spendable(node_factory, bitcoind, anchors): """Test that spendable_msat is accurate""" sats = 10**6 + opts={'plugin': os.path.join(os.getcwd(), 'tests/plugins/hold_invoice.py'), 'holdtime': '30'} + if anchors is False: + opts['dev-force-features'] = "-23" l1, l2 = node_factory.line_graph(2, fundamount=sats, wait_for_announce=True, - opts={'plugin': os.path.join(os.getcwd(), 'tests/plugins/hold_invoice.py'), 'holdtime': '30'}) + opts=opts) inv = l2.rpc.invoice('any', 'inv', 'for testing') payment_hash = inv['payment_hash']