From 7ae9a65e359ee2fe86918ca0d8e737368aaf17ac 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 +++ common/test/run-marginal_feerate.c | 127 +++++++++++++++++++++++++++++ lightningd/peer_control.c | 10 ++- tests/test_pay.py | 31 ++++--- 6 files changed, 201 insertions(+), 36 deletions(-) create mode 100644 common/test/run-marginal_feerate.c 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/common/test/run-marginal_feerate.c b/common/test/run-marginal_feerate.c new file mode 100644 index 000000000000..e0514cfdd83f --- /dev/null +++ b/common/test/run-marginal_feerate.c @@ -0,0 +1,127 @@ +#include "config.h" +#include +#include +#include +#include +#include +#include "../fee_states.c" + +/* AUTOGENERATED MOCKS START */ +/* Generated stub for amount_asset_is_main */ +bool amount_asset_is_main(struct amount_asset *asset UNNEEDED) +{ fprintf(stderr, "amount_asset_is_main called!\n"); abort(); } +/* Generated stub for amount_asset_to_sat */ +struct amount_sat amount_asset_to_sat(struct amount_asset *asset UNNEEDED) +{ fprintf(stderr, "amount_asset_to_sat called!\n"); abort(); } +/* Generated stub for amount_feerate */ + bool amount_feerate(u32 *feerate UNNEEDED, struct amount_sat fee UNNEEDED, size_t weight UNNEEDED) +{ fprintf(stderr, "amount_feerate called!\n"); abort(); } +/* Generated stub for amount_sat */ +struct amount_sat amount_sat(u64 satoshis UNNEEDED) +{ fprintf(stderr, "amount_sat called!\n"); abort(); } +/* Generated stub for amount_sat_add */ + bool amount_sat_add(struct amount_sat *val UNNEEDED, + struct amount_sat a UNNEEDED, + struct amount_sat b UNNEEDED) +{ fprintf(stderr, "amount_sat_add called!\n"); abort(); } +/* Generated stub for amount_sat_eq */ +bool amount_sat_eq(struct amount_sat a UNNEEDED, struct amount_sat b UNNEEDED) +{ fprintf(stderr, "amount_sat_eq called!\n"); abort(); } +/* Generated stub for amount_sat_greater_eq */ +bool amount_sat_greater_eq(struct amount_sat a UNNEEDED, struct amount_sat b UNNEEDED) +{ fprintf(stderr, "amount_sat_greater_eq called!\n"); abort(); } +/* Generated stub for amount_sat_sub */ + bool amount_sat_sub(struct amount_sat *val UNNEEDED, + struct amount_sat a UNNEEDED, + struct amount_sat b UNNEEDED) +{ fprintf(stderr, "amount_sat_sub called!\n"); abort(); } +/* Generated stub for amount_sat_to_asset */ +struct amount_asset amount_sat_to_asset(struct amount_sat *sat UNNEEDED, const u8 *asset UNNEEDED) +{ fprintf(stderr, "amount_sat_to_asset called!\n"); abort(); } +/* Generated stub for amount_tx_fee */ +struct amount_sat amount_tx_fee(u32 fee_per_kw UNNEEDED, size_t weight UNNEEDED) +{ fprintf(stderr, "amount_tx_fee called!\n"); abort(); } +/* Generated stub for fromwire */ +const u8 *fromwire(const u8 **cursor UNNEEDED, size_t *max UNNEEDED, void *copy UNNEEDED, size_t n UNNEEDED) +{ fprintf(stderr, "fromwire called!\n"); abort(); } +/* Generated stub for fromwire_bool */ +bool fromwire_bool(const u8 **cursor UNNEEDED, size_t *max UNNEEDED) +{ fprintf(stderr, "fromwire_bool called!\n"); abort(); } +/* Generated stub for fromwire_fail */ +void *fromwire_fail(const u8 **cursor UNNEEDED, size_t *max UNNEEDED) +{ fprintf(stderr, "fromwire_fail called!\n"); abort(); } +/* Generated stub for fromwire_secp256k1_ecdsa_signature */ +void fromwire_secp256k1_ecdsa_signature(const u8 **cursor UNNEEDED, size_t *max UNNEEDED, + secp256k1_ecdsa_signature *signature UNNEEDED) +{ fprintf(stderr, "fromwire_secp256k1_ecdsa_signature called!\n"); abort(); } +/* Generated stub for fromwire_sha256 */ +void fromwire_sha256(const u8 **cursor UNNEEDED, size_t *max UNNEEDED, struct sha256 *sha256 UNNEEDED) +{ fprintf(stderr, "fromwire_sha256 called!\n"); abort(); } +/* Generated stub for fromwire_tal_arrn */ +u8 *fromwire_tal_arrn(const tal_t *ctx UNNEEDED, + const u8 **cursor UNNEEDED, size_t *max UNNEEDED, size_t num UNNEEDED) +{ fprintf(stderr, "fromwire_tal_arrn called!\n"); abort(); } +/* Generated stub for fromwire_u32 */ +u32 fromwire_u32(const u8 **cursor UNNEEDED, size_t *max UNNEEDED) +{ fprintf(stderr, "fromwire_u32 called!\n"); abort(); } +/* Generated stub for fromwire_u64 */ +u64 fromwire_u64(const u8 **cursor UNNEEDED, size_t *max UNNEEDED) +{ fprintf(stderr, "fromwire_u64 called!\n"); abort(); } +/* Generated stub for fromwire_u8 */ +u8 fromwire_u8(const u8 **cursor UNNEEDED, size_t *max UNNEEDED) +{ fprintf(stderr, "fromwire_u8 called!\n"); abort(); } +/* Generated stub for fromwire_u8_array */ +void fromwire_u8_array(const u8 **cursor UNNEEDED, size_t *max UNNEEDED, u8 *arr UNNEEDED, size_t num UNNEEDED) +{ fprintf(stderr, "fromwire_u8_array called!\n"); abort(); } +/* Generated stub for htlc_state_flags */ +int htlc_state_flags(enum htlc_state state UNNEEDED) +{ fprintf(stderr, "htlc_state_flags called!\n"); abort(); } +/* Generated stub for htlc_state_name */ +const char *htlc_state_name(enum htlc_state s UNNEEDED) +{ fprintf(stderr, "htlc_state_name called!\n"); abort(); } +/* Generated stub for towire */ +void towire(u8 **pptr UNNEEDED, const void *data UNNEEDED, size_t len UNNEEDED) +{ fprintf(stderr, "towire called!\n"); abort(); } +/* Generated stub for towire_bool */ +void towire_bool(u8 **pptr UNNEEDED, bool v UNNEEDED) +{ fprintf(stderr, "towire_bool called!\n"); abort(); } +/* Generated stub for towire_secp256k1_ecdsa_signature */ +void towire_secp256k1_ecdsa_signature(u8 **pptr UNNEEDED, + const secp256k1_ecdsa_signature *signature UNNEEDED) +{ fprintf(stderr, "towire_secp256k1_ecdsa_signature called!\n"); abort(); } +/* Generated stub for towire_sha256 */ +void towire_sha256(u8 **pptr UNNEEDED, const struct sha256 *sha256 UNNEEDED) +{ fprintf(stderr, "towire_sha256 called!\n"); abort(); } +/* Generated stub for towire_u32 */ +void towire_u32(u8 **pptr UNNEEDED, u32 v UNNEEDED) +{ fprintf(stderr, "towire_u32 called!\n"); abort(); } +/* Generated stub for towire_u64 */ +void towire_u64(u8 **pptr UNNEEDED, u64 v UNNEEDED) +{ fprintf(stderr, "towire_u64 called!\n"); abort(); } +/* Generated stub for towire_u8 */ +void towire_u8(u8 **pptr UNNEEDED, u8 v UNNEEDED) +{ fprintf(stderr, "towire_u8 called!\n"); abort(); } +/* Generated stub for towire_u8_array */ +void towire_u8_array(u8 **pptr UNNEEDED, const u8 *arr UNNEEDED, size_t num UNNEEDED) +{ fprintf(stderr, "towire_u8_array called!\n"); abort(); } +/* AUTOGENERATED MOCKS END */ + +int main(int argc, char *argv[]) +{ + common_setup(argv[0]); + + /* At minimum, it's 100% */ + assert(marginal_feerate(253) == 253 * 2); + + /* At maximum, it's 10% */ + assert(marginal_feerate(45000) == (u32)(45000 * 1.1)); + + /* Above maximum, still 10% */ + assert(marginal_feerate(500000) == (u32)(500000 * 1.1)); + + /* Half way, it's 55% */ + u32 half = (45000 + 253)/2; + assert(marginal_feerate(half) == (u32)(half * 1.55)); + + common_shutdown(); +} 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']