From 9b5dd309fc80b5e1e4cedc2d1ed03cba037a555c Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 7 Apr 2023 14:08:05 +0930 Subject: [PATCH 01/16] msggen: fix incorrect assertion. Adding a new field with `added` fails: ``` AssertionError: Field Feerates.perkb.estimates[] does not have an `added` annotation ``` Looks like this assertion is wrong: we should get an added from the field itself or from the .msggen.json file. Signed-off-by: Rusty Russell --- contrib/msggen/msggen/patch.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contrib/msggen/msggen/patch.py b/contrib/msggen/msggen/patch.py index d8b12ff2b01b..f25693bee027 100644 --- a/contrib/msggen/msggen/patch.py +++ b/contrib/msggen/msggen/patch.py @@ -67,7 +67,7 @@ def visit(self, f: model.Field) -> None: added = m.get('added', None) deprecated = m.get('deprecated', None) - assert added or not f.added, f"Field {f.path} does not have an `added` annotation" + assert added or f.added, f"Field {f.path} does not have an `added` annotation" # We do not allow the added and deprecated flags to be # modified after the fact. From ba96d6ae8597d8289f1bbd65f64b5201e3b6b9a6 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 7 Apr 2023 14:09:05 +0930 Subject: [PATCH 02/16] plugins/bcli: move commit-fee (dev-max-fee-multiplier) and into core. Turns out the two bcli replacements I checked (`sauron` and `trustedcoin`) don't even implement this, and the multiplier makes more sense in lightningd, especially as we move to bcli just providing raw feerate estimates. Signed-off-by: Rusty Russell --- doc/lightning-listconfigs.7.md | 3 ++- doc/lightningd-config.5.md | 2 +- doc/schemas/listconfigs.schema.json | 5 +++++ lightningd/bitcoind.c | 9 ++++++++- lightningd/lightningd.h | 7 +++++++ lightningd/options.c | 18 ++++++++++++++++++ plugins/bcli.c | 26 ++------------------------ tests/test_misc.py | 19 ++++++++++++++----- 8 files changed, 57 insertions(+), 32 deletions(-) diff --git a/doc/lightning-listconfigs.7.md b/doc/lightning-listconfigs.7.md index fe99657469a3..f59e05719eaa 100644 --- a/doc/lightning-listconfigs.7.md +++ b/doc/lightning-listconfigs.7.md @@ -106,6 +106,7 @@ On success, an object is returned, containing: - **dev-allowdustreserve** (boolean, optional): Whether we allow setting dust reserves - **announce-addr-dns** (boolean, optional): Whether we put DNS entries into node\_announcement *(added v22.11.1)* - **require-confirmed-inputs** (boolean, optional): Request peers to only send confirmed inputs (dual-fund only) +- **commit-fee** (u64, optional): The percentage of the 6-block fee estimate to use for commitment transactions *(added v23.05)* [comment]: # (GENERATE-FROM-SCHEMA-END) @@ -224,4 +225,4 @@ RESOURCES Main web site: -[comment]: # ( SHA256STAMP:1088401b9aeae1e079dab550d3b035ef82195f0466ad471bc7373386182f37dc) +[comment]: # ( SHA256STAMP:b24158a61bb79aaf3f0f6d1c20a4b10d474613b371e80aede4aeb59ab471a989) diff --git a/doc/lightningd-config.5.md b/doc/lightningd-config.5.md index 0621ccb72e91..4ba84d8605b0 100644 --- a/doc/lightningd-config.5.md +++ b/doc/lightningd-config.5.md @@ -401,7 +401,7 @@ create a channel, and if an HTLC asks for longer, we'll refuse it. Confirmations required for the funding transaction when the other side opens a channel before the channel is usable. -* **commit-fee**=*PERCENT* [plugin `bcli`] +* **commit-fee**=*PERCENT* The percentage of *estimatesmartfee 2/CONSERVATIVE* to use for the commitment transactions: default is 100. diff --git a/doc/schemas/listconfigs.schema.json b/doc/schemas/listconfigs.schema.json index b2ebdd003c68..32e7b096d514 100644 --- a/doc/schemas/listconfigs.schema.json +++ b/doc/schemas/listconfigs.schema.json @@ -319,6 +319,11 @@ "require-confirmed-inputs": { "type": "boolean", "description": "Request peers to only send confirmed inputs (dual-fund only)" + }, + "commit-fee": { + "type": "u64", + "added": "v23.05", + "description": "The percentage of the 6-block fee estimate to use for commitment transactions" } } } diff --git a/lightningd/bitcoind.c b/lightningd/bitcoind.c index 1baec3c587a5..fb7b096491c2 100644 --- a/lightningd/bitcoind.c +++ b/lightningd/bitcoind.c @@ -215,10 +215,17 @@ static void estimatefees_callback(const char *buf, const jsmntok_t *toks, else feerates[f] = 0; #endif - } else + } else { + if (f == FEERATE_UNILATERAL_CLOSE) { + feerates[f] = feerates[f] * call->bitcoind->ld->config.commit_fee_percent / 100; + } else if (f == FEERATE_MAX) { + /* Plugins always use 10 as multiplier. */ + feerates[f] = feerates[f] * call->bitcoind->ld->config.max_fee_multiplier / 10; + } /* Rate in satoshi per kw. */ feerates[f] = feerate_from_style(feerates[f], FEERATE_PER_KBYTE); + } } call->cb(call->bitcoind, feerates, call->arg); diff --git a/lightningd/lightningd.h b/lightningd/lightningd.h index 0a57dbb9789f..e5aac115ad3c 100644 --- a/lightningd/lightningd.h +++ b/lightningd/lightningd.h @@ -85,6 +85,13 @@ struct config { /* Require peer to send confirmed inputs */ bool require_confirmed_inputs; + + /* The factor to time the urgent feerate by to get the maximum + * acceptable feerate. (10, but can be overridden by dev-max-fee-multiplier) */ + u32 max_fee_multiplier; + + /* Percent of CONSERVATIVE/2 feerate we'll use for commitment txs. */ + u64 commit_fee_percent; }; typedef STRMAP(const char *) alt_subdaemon_map; diff --git a/lightningd/options.c b/lightningd/options.c index b35b930429b2..7d44e4dd746f 100644 --- a/lightningd/options.c +++ b/lightningd/options.c @@ -799,6 +799,15 @@ static void dev_register_opts(struct lightningd *ld) opt_show_uintval, &dev_onion_reply_length, "Send onion errors of custom length"); + opt_register_arg("--dev-max-fee-multiplier", + opt_set_uintval, + opt_show_uintval, + &ld->config.max_fee_multiplier, + "Allow the fee proposed by the remote end to" + " be up to multiplier times higher than our " + "own. Small values will cause channels to be" + " closed more often due to fee fluctuations," + " large values may result in large fees."); } #endif /* DEVELOPER */ @@ -860,6 +869,9 @@ static const struct config testnet_config = { .allowdustreserve = false, .require_confirmed_inputs = false, + + .max_fee_multiplier = 10, + .commit_fee_percent = 100, }; /* aka. "Dude, where's my coins?" */ @@ -931,6 +943,9 @@ static const struct config mainnet_config = { .allowdustreserve = false, .require_confirmed_inputs = false, + + .max_fee_multiplier = 10, + .commit_fee_percent = 100, }; static void check_config(struct lightningd *ld) @@ -1290,6 +1305,9 @@ static void register_opts(struct lightningd *ld) opt_force_feerates, NULL, ld, "Set testnet/regtest feerates in sats perkw, opening/mutual_close/unlateral_close/delayed_to_us/htlc_resolution/penalty: if fewer specified, last number applies to remainder"); + opt_register_arg("--commit-fee", + opt_set_u64, opt_show_u64, &ld->config.commit_fee_percent, + "Percentage of fee to request for their commitment"); opt_register_arg("--subdaemon", opt_subdaemon, NULL, ld, "Arg specified as SUBDAEMON:PATH. " "Specifies an alternate subdaemon binary. " diff --git a/plugins/bcli.c b/plugins/bcli.c index a79b67b1907a..57ab9edf4ffe 100644 --- a/plugins/bcli.c +++ b/plugins/bcli.c @@ -60,13 +60,6 @@ struct bitcoind { /* Passthrough parameters for bitcoin-cli */ char *rpcuser, *rpcpass, *rpcconnect, *rpcport; - /* The factor to time the urgent feerate by to get the maximum - * acceptable feerate. */ - u32 max_fee_multiplier; - - /* Percent of CONSERVATIVE/2 feerate we'll use for commitment txs. */ - u64 commit_fee_percent; - /* Whether we fake fees (regtest) */ bool fake_fees; @@ -718,7 +711,7 @@ static struct command_result *estimatefees_next(struct command *cmd, json_add_feerate(response, "mutual_close", cmd, stash, stash->perkb[FEERATE_SLOW]); json_add_feerate(response, "unilateral_close", cmd, stash, - stash->perkb[FEERATE_URGENT] * bitcoind->commit_fee_percent / 100); + stash->perkb[FEERATE_URGENT]); json_add_feerate(response, "delayed_to_us", cmd, stash, stash->perkb[FEERATE_NORMAL]); json_add_feerate(response, "htlc_resolution", cmd, stash, @@ -736,8 +729,7 @@ static struct command_result *estimatefees_next(struct command *cmd, * margin (say 5x the expected fee requirement) */ json_add_feerate(response, "max_acceptable", cmd, stash, - stash->perkb[FEERATE_HIGHEST] - * bitcoind->max_fee_multiplier); + stash->perkb[FEERATE_HIGHEST] * 10); return command_finished(cmd, response); } @@ -1063,8 +1055,6 @@ static struct bitcoind *new_bitcoind(const tal_t *ctx) bitcoind->rpcpass = NULL; bitcoind->rpcconnect = NULL; bitcoind->rpcport = NULL; - bitcoind->max_fee_multiplier = 10; - bitcoind->commit_fee_percent = 100; #if DEVELOPER bitcoind->no_fake_fees = false; #endif @@ -1111,19 +1101,7 @@ int main(int argc, char *argv[]) "how long to keep retrying to contact bitcoind" " before fatally exiting", u64_option, &bitcoind->retry_timeout), - plugin_option("commit-fee", - "string", - "Percentage of fee to request for their commitment", - u64_option, &bitcoind->commit_fee_percent), #if DEVELOPER - plugin_option("dev-max-fee-multiplier", - "string", - "Allow the fee proposed by the remote end to" - " be up to multiplier times higher than our " - "own. Small values will cause channels to be" - " closed more often due to fee fluctuations," - " large values may result in large fees.", - u32_option, &bitcoind->max_fee_multiplier), plugin_option("dev-no-fake-fees", "bool", "Suppress fee faking for regtest", diff --git a/tests/test_misc.py b/tests/test_misc.py index 7eb0e87a765b..30112a92de10 100644 --- a/tests/test_misc.py +++ b/tests/test_misc.py @@ -2651,15 +2651,24 @@ def test_restorefrompeer(node_factory, bitcoind): def test_commitfee_option(node_factory): """Sanity check for the --commit-fee startup option.""" - l1, l2 = node_factory.get_nodes(2, opts=[{"commit-fee": "200"}, {}]) + l1, l2 = node_factory.get_nodes(2, opts=[{"commit-fee": "200", + "start": False}, + {"start": False}]) + # set_feerates multiplies this by 4 to get perkb; but we divide. mock_wu = 5000 for l in [l1, l2]: - l.set_feerates((0, mock_wu, 0, 0), True) - l1_commit_fees = l1.rpc.call("estimatefees")["unilateral_close"] - l2_commit_fees = l2.rpc.call("estimatefees")["unilateral_close"] + l.set_feerates((0, mock_wu, 0, 0), False) + l.start() - assert l1_commit_fees == 2 * l2_commit_fees == 2 * 4 * mock_wu # WU->VB + # plugin gives same results: + assert l1.rpc.call("estimatefees") == l2.rpc.call("estimatefees") + + # But feerates differ. + l1_commit_fees = l1.rpc.feerates("perkw")['perkw']['unilateral_close'] + l2_commit_fees = l2.rpc.feerates("perkw")['perkw']['unilateral_close'] + + assert l1_commit_fees == 2 * l2_commit_fees == 2 * mock_wu def test_listtransactions(node_factory): From 966807063f7d58389603f9c3ea0a322fbf00ad60 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 7 Apr 2023 14:09:53 +0930 Subject: [PATCH 03/16] common: add tal_arr_insert helper to utils.h We have tal_arr_remove and tal_arr_append already. Signed-off-by: Rusty Russell --- common/utils.h | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/common/utils.h b/common/utils.h index dcfa111cbc9e..2aadec5fc3cf 100644 --- a/common/utils.h +++ b/common/utils.h @@ -80,11 +80,22 @@ void clear_softref_(const tal_t *outer, size_t outersize, void **ptr); * Remove an element from an array * * This will shift the elements past the removed element, changing - * their position in memory, so only use this for arrays of pointers. + * their position in memory, so only use this for simple arrays. */ #define tal_arr_remove(p, n) tal_arr_remove_((p), sizeof(**p), (n)) void tal_arr_remove_(void *p, size_t elemsize, size_t n); +/** + * Insert an element in an array + */ +#define tal_arr_insert(p, n, v) \ + do { \ + size_t n_ = tal_count(*(p)); \ + tal_resize((p), n_+1); \ + memmove(*(p) + n + 1, *(p) + n, (n_ - n) * sizeof(**(p))); \ + (*(p))[n] = (v); \ + } while(0) + /* Check for valid UTF-8 */ bool utf8_check(const void *buf, size_t buflen); From 5506fc589e23eac338b4c3b9a895fc0c443c0fc1 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 7 Apr 2023 14:09:53 +0930 Subject: [PATCH 04/16] pytest: test parsefeerate explicitly. Since we're messing with feerates, it's good to test this directly upfront. Also, fix documentation! Signed-off-by: Rusty Russell --- doc/lightning-feerates.7.md | 4 ++-- doc/schemas/feerates.schema.json | 2 +- tests/test_misc.py | 33 ++++++++++++++++++++++++++++++++ 3 files changed, 36 insertions(+), 3 deletions(-) diff --git a/doc/lightning-feerates.7.md b/doc/lightning-feerates.7.md index db4139d929bd..e047e72fe396 100644 --- a/doc/lightning-feerates.7.md +++ b/doc/lightning-feerates.7.md @@ -48,7 +48,7 @@ RETURN VALUE On success, an object is returned, containing: - **perkb** (object, optional): If *style* parameter was perkb: - - **min\_acceptable** (u32): The smallest feerate that you can use, usually the minimum relayed feerate of the backend + - **min\_acceptable** (u32): The smallest feerate that we allow peers to specify: half the 100-block estimate - **max\_acceptable** (u32): The largest feerate we will accept from remote negotiations. If a peer attempts to set the feerate higher than this we will unilaterally close the channel (or simply forget it if it's not open yet). - **opening** (u32, optional): Default feerate for lightning-fundchannel(7) and lightning-withdraw(7) - **mutual\_close** (u32, optional): Feerate to aim for in cooperative shutdown. Note that since mutual close is a **negotiation**, the actual feerate used in mutual close will be somewhere between this and the corresponding mutual close feerate of the peer. @@ -121,4 +121,4 @@ RESOURCES Main web site: -[comment]: # ( SHA256STAMP:a34af89895413ee57defa1df715d9e50d34a49196972a81b31a4666b4fc05a10) +[comment]: # ( SHA256STAMP:773e4e66cb3654b7c3aafe54c33d433c52ff89f7a5a8be0a71a93da21a6b7eaa) diff --git a/doc/schemas/feerates.schema.json b/doc/schemas/feerates.schema.json index 980f1d20c5a6..d7019f17d502 100644 --- a/doc/schemas/feerates.schema.json +++ b/doc/schemas/feerates.schema.json @@ -19,7 +19,7 @@ "properties": { "min_acceptable": { "type": "u32", - "description": "The smallest feerate that you can use, usually the minimum relayed feerate of the backend" + "description": "The smallest feerate that we allow peers to specify: half the 100-block estimate" }, "max_acceptable": { "type": "u32", diff --git a/tests/test_misc.py b/tests/test_misc.py index 30112a92de10..a3a72077b495 100644 --- a/tests/test_misc.py +++ b/tests/test_misc.py @@ -3212,6 +3212,39 @@ def test_hsm_capabilities(node_factory): assert l1.daemon.is_in_log(r"hsmd: capability \+WIRE_HSMD_CHECK_PUBKEY") +def test_feerate_arg(node_factory): + """Make sure our variants of feerate argument work!""" + l1 = node_factory.get_node() + + # These are the get_node() defaults + by_blocks = {2: 15000, + 6: 11000, + 12: 7500, + 100: 3750} + + # Literal values: + fees = {"9999perkw": 9999, + "10000perkb": 10000 // 4, + 10000: 10000 // 4} + + fees["urgent"] = by_blocks[6] + fees["normal"] = by_blocks[12] + fees["slow"] = by_blocks[100] // 2 + + fees["opening"] = by_blocks[12] + fees["mutual_close"] = by_blocks[100] + fees["penalty"] = by_blocks[12] + fees["unilateral_close"] = by_blocks[6] + fees["delayed_to_us"] = by_blocks[12] + fees["htlc_resolution"] = by_blocks[6] + fees["min_acceptable"] = by_blocks[100] // 2 + fees["max_acceptable"] = by_blocks[2] * 10 + + for fee, expect in fees.items(): + # Put arg in assertion, so it gets printed on failure! + assert (l1.rpc.parsefeerate(fee), fee) == ({'perkw': expect}, fee) + + @pytest.mark.skip(reason="Fails by intention for creating test gossip stores") def test_create_gossip_mesh(node_factory, bitcoind): """ From e6793bdd8487fdfcf4f8e538fad2098b2ec394b6 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 7 Apr 2023 14:09:53 +0930 Subject: [PATCH 05/16] lightningd: clarify uses of dynamic (mempool) feerate floor, and static. We have the FEERATE_FLOOR constant if you don't care, but usually you want to use the current bitcoind lower limit, so call get_feerate_floor() (which is currently the same, but coming!). Signed-off-by: Rusty Russell --- bitcoin/feerate.c | 4 ++++ bitcoin/feerate.h | 2 +- channeld/watchtower.c | 4 +++- lightningd/chaintopology.c | 14 ++++++++++---- lightningd/chaintopology.h | 3 +++ lightningd/closing_control.c | 4 ++-- lightningd/onchain_control.c | 5 +++-- lightningd/opening_control.c | 5 +++-- 8 files changed, 29 insertions(+), 12 deletions(-) diff --git a/bitcoin/feerate.c b/bitcoin/feerate.c index fc73423940db..7788ebb2d3ba 100644 --- a/bitcoin/feerate.c +++ b/bitcoin/feerate.c @@ -1,8 +1,12 @@ #include "config.h" +#include #include u32 feerate_from_style(u32 feerate, enum feerate_style style) { + /* Make sure it's called somewhere! */ + assert(feerate_floor_check() == FEERATE_FLOOR); + switch (style) { case FEERATE_PER_KSIPA: return feerate; diff --git a/bitcoin/feerate.h b/bitcoin/feerate.h index 43bec21181d5..cab1e95e258c 100644 --- a/bitcoin/feerate.h +++ b/bitcoin/feerate.h @@ -39,7 +39,7 @@ enum feerate_style { FEERATE_PER_KBYTE }; -static inline u32 feerate_floor(void) +static inline u32 feerate_floor_check(void) { /* Assert that bitcoind will see this as above minRelayTxFee */ BUILD_ASSERT(FEERATE_BITCOIND_SEES(FEERATE_FLOOR, MINIMUM_TX_WEIGHT) diff --git a/channeld/watchtower.c b/channeld/watchtower.c index 269b4f405ad0..00f34d30fd66 100644 --- a/channeld/watchtower.c +++ b/channeld/watchtower.c @@ -96,7 +96,9 @@ penalty_tx_create(const tal_t *ctx, if (amount_sat_less(to_them_sats, min_out)) { /* FIXME: We should use SIGHASH_NONE so others can take it */ - fee = amount_tx_fee(feerate_floor(), weight); + /* We use the minimum possible fee here; if it doesn't + * propagate, who cares? */ + fee = amount_tx_fee(FEERATE_FLOOR, weight); } /* This can only happen if feerate_floor() is still too high; shouldn't diff --git a/lightningd/chaintopology.c b/lightningd/chaintopology.c index 9cc00d9a79b4..2c8a97fa8bd6 100644 --- a/lightningd/chaintopology.c +++ b/lightningd/chaintopology.c @@ -415,8 +415,8 @@ static void update_feerates(struct bitcoind *bitcoind, feerate, alpha); } - if (feerate < feerate_floor()) { - feerate = feerate_floor(); + if (feerate < get_feerate_floor(topo)) { + feerate = get_feerate_floor(topo); log_debug(topo->log, "... feerate estimate for %s hit floor %u", feerate_name(i), feerate); @@ -487,6 +487,12 @@ u32 penalty_feerate(struct chain_topology *topo) return try_get_feerate(topo, FEERATE_PENALTY); } +u32 get_feerate_floor(const struct chain_topology *topo) +{ + /* FIXME: Make this dynamic! */ + return FEERATE_FLOOR; +} + static struct command_result *json_feerates(struct command *cmd, const char *buffer, const jsmntok_t *obj UNNEEDED, @@ -936,8 +942,8 @@ u32 feerate_min(struct lightningd *ld, bool *unknown) } } - if (min < feerate_floor()) - return feerate_floor(); + if (min < get_feerate_floor(ld->topology)) + return get_feerate_floor(ld->topology); return min; } diff --git a/lightningd/chaintopology.h b/lightningd/chaintopology.h index 486fab0f3e10..ce8e9f98af22 100644 --- a/lightningd/chaintopology.h +++ b/lightningd/chaintopology.h @@ -143,6 +143,9 @@ struct txlocator { u32 index; }; +/* Get the minimum feerate that bitcoind will accept */ +u32 get_feerate_floor(const struct chain_topology *topo); + /* This is the number of blocks which would have to be mined to invalidate * the tx */ size_t get_tx_depth(const struct chain_topology *topo, diff --git a/lightningd/closing_control.c b/lightningd/closing_control.c index 7021825862c7..105bd6be832b 100644 --- a/lightningd/closing_control.c +++ b/lightningd/closing_control.c @@ -409,8 +409,8 @@ void peer_start_closingd(struct channel *channel, struct peer_fd *peer_fd) feerate = mutual_close_feerate(ld->topology); if (!feerate) { feerate = final_commit_feerate / 2; - if (feerate < feerate_floor()) - feerate = feerate_floor(); + if (feerate < get_feerate_floor(ld->topology)) + feerate = get_feerate_floor(ld->topology); } /* We use a feerate if anchor_outputs, otherwise max fee is set by diff --git a/lightningd/onchain_control.c b/lightningd/onchain_control.c index d43e6728060a..a0803a2094e0 100644 --- a/lightningd/onchain_control.c +++ b/lightningd/onchain_control.c @@ -703,12 +703,13 @@ static struct bitcoin_tx *onchaind_tx(const tal_t *ctx, if (amount_sat_less(out_sats, min_out)) { /* FIXME: We should use SIGHASH_NONE so others can take it? */ - fee = amount_tx_fee(feerate_floor(), weight); + /* Use lowest possible theoretical fee: who cares if it doesn't propagate */ + fee = amount_tx_fee(FEERATE_FLOOR, weight); *worthwhile = false; } else *worthwhile = true; - /* This can only happen if feerate_floor() is still too high; shouldn't + /* This can only happen if FEERATE_FLOOR is still too high; shouldn't * happen! */ if (!amount_sat_sub(&amt, out_sats, fee)) { amt = channel->our_config.dust_limit; diff --git a/lightningd/opening_control.c b/lightningd/opening_control.c index 7156d5f620f3..7b993c3dd178 100644 --- a/lightningd/opening_control.c +++ b/lightningd/opening_control.c @@ -1159,9 +1159,10 @@ static struct command_result *json_fundchannel_start(struct command *cmd, } } - if (*feerate_per_kw < feerate_floor()) { + if (*feerate_per_kw < get_feerate_floor(cmd->ld->topology)) { return command_fail(cmd, LIGHTNINGD, - "Feerate below feerate floor"); + "Feerate below feerate floor %u perkw", + get_feerate_floor(cmd->ld->topology)); } if (!topology_synced(cmd->ld->topology)) { From 6e1b64043736dded069773e71d28f3a94397cc6f Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 7 Apr 2023 14:09:53 +0930 Subject: [PATCH 06/16] lightningd: handle fees as blockcount + range. Rather than have specific-purpose levels, have an array of [blockcount, feerate], and rebuild the specific-purpose levels for now on top. We also keep a *separate* smoothed feerate, so you can ask for that explicitly. Since all the plugins used the same formula to derive the different named fee levels, we apply the reverse to return to the underlying estimates: updating the interface comes next. This is ugly for now, but various specific-purpose levels will be going away, as we shift to deadline-driven fees. This temporarily breaks the floor calculation, so that test is disabled. Signed-off-by: Rusty Russell --- contrib/pyln-testing/pyln/testing/utils.py | 2 +- lightningd/bitcoind.c | 140 +++++----- lightningd/bitcoind.h | 18 +- lightningd/chaintopology.c | 285 +++++++++++++++------ lightningd/chaintopology.h | 24 +- tests/test_misc.py | 25 +- 6 files changed, 324 insertions(+), 170 deletions(-) diff --git a/contrib/pyln-testing/pyln/testing/utils.py b/contrib/pyln-testing/pyln/testing/utils.py index 41e7707725d5..5f023b37b6dd 100644 --- a/contrib/pyln-testing/pyln/testing/utils.py +++ b/contrib/pyln-testing/pyln/testing/utils.py @@ -1191,7 +1191,7 @@ def mock_estimatesmartfee(r): self.daemon.rpcproxy.mock_rpc('estimatesmartfee', mock_estimatesmartfee) # Technically, this waits until it's called, not until it's processed. - # We wait until all three levels have been called. + # We wait until all four levels have been called. if wait_for_effect: wait_for(lambda: self.daemon.rpcproxy.mock_counts['estimatesmartfee'] >= 4) diff --git a/lightningd/bitcoind.c b/lightningd/bitcoind.c index fb7b096491c2..f154ba8a1ab5 100644 --- a/lightningd/bitcoind.c +++ b/lightningd/bitcoind.c @@ -156,20 +156,69 @@ static void bitcoin_plugin_send(struct bitcoind *bitcoind, * "max_acceptable": , * } */ - struct estimatefee_call { struct bitcoind *bitcoind; - void (*cb)(struct bitcoind *bitcoind, const u32 satoshi_per_kw[], - void *); - void *arg; + void (*cb)(struct lightningd *ld, u32 feerate_floor, + const struct feerate_est *rates); }; +/* Note: returns estimates in perkb, caller converts! */ +static struct feerate_est *parse_deprecated_feerates(const tal_t *ctx, + struct bitcoind *bitcoind, + const char *buf, + const jsmntok_t *toks) +{ + struct feerate_est *rates = tal_arr(ctx, struct feerate_est, 0); + struct oldstyle { + const char *name; + size_t blockcount; + size_t multiplier; + } oldstyles[] = { { "max_acceptable", 2, 10 }, + { "unilateral_close", 6, 1 }, + { "opening", 12, 1 }, + { "mutual_close", 100, 1 } }; + + for (size_t i = 0; i < ARRAY_SIZE(oldstyles); i++) { + const jsmntok_t *feeratetok; + struct feerate_est rate; + + feeratetok = json_get_member(buf, toks, oldstyles[i].name); + if (!feeratetok) { + bitcoin_plugin_error(bitcoind, buf, toks, + "estimatefees", + "missing '%s' field", + oldstyles[i].name); + } + if (!json_to_u32(buf, feeratetok, &rate.rate)) { + if (chainparams->testnet) + log_debug(bitcoind->log, + "Unable to estimate %s fees", + oldstyles[i].name); + else + log_unusual(bitcoind->log, + "Unable to estimate %s fees", + oldstyles[i].name); + continue; + } + + if (rate.rate == 0) + continue; + + /* Cancel out the 10x multiplier on max_acceptable */ + rate.rate /= oldstyles[i].multiplier; + rate.blockcount = oldstyles[i].blockcount; + tal_arr_expand(&rates, rate); + } + return rates; +} + static void estimatefees_callback(const char *buf, const jsmntok_t *toks, const jsmntok_t *idtok, struct estimatefee_call *call) { - const jsmntok_t *resulttok, *feeratetok; - u32 *feerates = tal_arr(call, u32, NUM_FEERATES); + const jsmntok_t *resulttok; + struct feerate_est *feerates; + u32 floor; resulttok = json_get_member(buf, toks, "result"); if (!resulttok) @@ -177,73 +226,40 @@ static void estimatefees_callback(const char *buf, const jsmntok_t *toks, "estimatefees", "bad 'result' field"); - for (int f = 0; f < NUM_FEERATES; f++) { - feeratetok = json_get_member(buf, resulttok, feerate_name(f)); - if (!feeratetok) - bitcoin_plugin_error(call->bitcoind, buf, toks, - "estimatefees", - "missing '%s' field", feerate_name(f)); - /* We still use the bcli plugin for min and max, even with - * force_feerates */ - if (f < tal_count(call->bitcoind->ld->force_feerates)) { - feerates[f] = call->bitcoind->ld->force_feerates[f]; - continue; - } - - /* FIXME: We could trawl recent blocks for median fee... */ - if (!json_to_u32(buf, feeratetok, &feerates[f])) { - if (chainparams->testnet) - log_debug(call->bitcoind->log, - "Unable to estimate %s fees", - feerate_name(f)); - else - log_unusual(call->bitcoind->log, - "Unable to estimate %s fees", - feerate_name(f)); - -#if DEVELOPER - /* This is needed to test for failed feerate estimates - * in DEVELOPER mode */ - feerates[f] = 0; -#else - /* If we are in testnet mode we want to allow payments - * with the minimal fee even if the estimate didn't - * work out. This is less disruptive than erring out - * all the time. */ - if (chainparams->testnet) - feerates[f] = FEERATE_FLOOR; - else - feerates[f] = 0; -#endif - } else { - if (f == FEERATE_UNILATERAL_CLOSE) { - feerates[f] = feerates[f] * call->bitcoind->ld->config.commit_fee_percent / 100; - } else if (f == FEERATE_MAX) { - /* Plugins always use 10 as multiplier. */ - feerates[f] = feerates[f] * call->bitcoind->ld->config.max_fee_multiplier / 10; - } - /* Rate in satoshi per kw. */ - feerates[f] = feerate_from_style(feerates[f], - FEERATE_PER_KBYTE); - } + feerates = parse_deprecated_feerates(call, call->bitcoind, + buf, resulttok); + /* FIXME: get from plugin! */ + floor = feerate_from_style(FEERATE_FLOOR, FEERATE_PER_KSIPA); + + /* Convert to perkw */ + floor = feerate_from_style(floor, FEERATE_PER_KBYTE); + if (floor < FEERATE_FLOOR) + floor = FEERATE_FLOOR; + + /* 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 < floor) + feerates[i].rate = floor; } - call->cb(call->bitcoind, feerates, call->arg); + call->cb(call->bitcoind->ld, floor, feerates); tal_free(call); } -void bitcoind_estimate_fees_(struct bitcoind *bitcoind, - size_t num_estimates, - void (*cb)(struct bitcoind *bitcoind, - const u32 satoshi_per_kw[], void *), - void *arg) +void bitcoind_estimate_fees(struct bitcoind *bitcoind, + void (*cb)(struct lightningd *ld, + u32 feerate_floor, + const struct feerate_est *feerates)) { struct jsonrpc_request *req; struct estimatefee_call *call = tal(bitcoind, struct estimatefee_call); call->bitcoind = bitcoind; call->cb = cb; - call->arg = arg; req = jsonrpc_request_start(bitcoind, "estimatefees", NULL, true, bitcoind->log, diff --git a/lightningd/bitcoind.h b/lightningd/bitcoind.h index f17217d78da0..0986d438abfb 100644 --- a/lightningd/bitcoind.h +++ b/lightningd/bitcoind.h @@ -9,6 +9,7 @@ struct bitcoin_blkid; struct bitcoin_tx_output; struct block; +struct feerate_est; struct lightningd; struct ripemd160; struct bitcoin_tx; @@ -57,19 +58,10 @@ struct bitcoind *new_bitcoind(const tal_t *ctx, struct lightningd *ld, struct log *log); -void bitcoind_estimate_fees_(struct bitcoind *bitcoind, - size_t num_estimates, - void (*cb)(struct bitcoind *bitcoind, - const u32 satoshi_per_kw[], void *), - void *arg); - -#define bitcoind_estimate_fees(bitcoind_, num, cb, arg) \ - bitcoind_estimate_fees_((bitcoind_), (num), \ - typesafe_cb_preargs(void, void *, \ - (cb), (arg), \ - struct bitcoind *, \ - const u32 *), \ - (arg)) +void bitcoind_estimate_fees(struct bitcoind *bitcoind, + void (*cb)(struct lightningd *ld, + u32 feerate_floor, + const struct feerate_est *feerates)); void bitcoind_sendrawtx_(struct bitcoind *bitcoind, const char *id_prefix TAKES, diff --git a/lightningd/chaintopology.c b/lightningd/chaintopology.c index 2c8a97fa8bd6..041a205dd77c 100644 --- a/lightningd/chaintopology.c +++ b/lightningd/chaintopology.c @@ -352,88 +352,180 @@ static void watch_for_utxo_reconfirmation(struct chain_topology *topo, /* Mutual recursion via timer. */ static void next_updatefee_timer(struct chain_topology *topo); -static void init_feerate_history(struct chain_topology *topo, - enum feerate feerate, u32 val) +static u32 interp_feerate(const struct feerate_est *rates, u32 blockcount) { - for (size_t i = 0; i < FEE_HISTORY_NUM; i++) - topo->feehistory[feerate][i] = val; + const struct feerate_est *before = NULL, *after = NULL; + + /* Find before and after. */ + for (size_t i = 0; i < tal_count(rates); i++) { + if (rates[i].blockcount <= blockcount) { + before = &rates[i]; + } else if (rates[i].blockcount > blockcount && !after) { + after = &rates[i]; + } + } + /* No estimates at all? */ + if (!before && !after) + return 0; + /* We don't extrapolate. */ + if (!before && after) + return after->rate; + if (before && !after) + return before->rate; + + /* Interpolate, eg. blockcount 10, rate 15000, blockcount 20, rate 5000. + * At 15, rate should be 10000. + * 15000 + (15 - 10) / (20 - 10) * (15000 - 5000) + * 15000 + 5 / 10 * 10000 + * => 10000 + */ + /* Don't go backwards though! */ + if (before->rate < after->rate) + return before->rate; + + return before->rate + - ((u64)(blockcount - before->blockcount) + * (before->rate - after->rate) + / (after->blockcount - before->blockcount)); + +} + +u32 feerate_for_deadline(const struct chain_topology *topo, u32 blockcount) +{ + u32 rate = interp_feerate(topo->feerates[0], blockcount); + + /* 0 is a special value, meaning "don't know" */ + if (rate && rate < topo->feerate_floor) + rate = topo->feerate_floor; + return rate; } -static void add_feerate_history(struct chain_topology *topo, - enum feerate feerate, u32 val) +u32 smoothed_feerate_for_deadline(const struct chain_topology *topo, + u32 blockcount) { - memmove(&topo->feehistory[feerate][1], &topo->feehistory[feerate][0], - (FEE_HISTORY_NUM - 1) * sizeof(u32)); - topo->feehistory[feerate][0] = val; + /* Note: we cap it at feerate_floor when we smooth */ + return interp_feerate(topo->smoothed_feerates, blockcount); } -/* We sanitize feerates if necessary to put them in descending order. */ -static void update_feerates(struct bitcoind *bitcoind, - const u32 *satoshi_per_kw, - struct chain_topology *topo) +/* Mixes in fresh feerate rate into old smoothed values, modifies rate */ +static void smooth_one_feerate(const struct chain_topology *topo, + struct feerate_est *rate) { - u32 old_feerates[NUM_FEERATES]; /* Smoothing factor alpha for simple exponential smoothing. The goal is to * have the feerate account for 90 percent of the values polled in the last * 2 minutes. The following will do that in a polling interval * independent manner. */ double alpha = 1 - pow(0.1,(double)topo->poll_seconds / 120); - bool notify_feerate_changed = false; + u32 old_feerate, feerate_smooth; - for (size_t i = 0; i < NUM_FEERATES; i++) { - u32 feerate = satoshi_per_kw[i]; + /* We don't call this unless we had a previous feerate */ + old_feerate = smoothed_feerate_for_deadline(topo, rate->blockcount); + assert(old_feerate); - /* Takes into account override_fee_rate */ - old_feerates[i] = try_get_feerate(topo, i); + feerate_smooth = rate->rate * alpha + old_feerate * (1 - alpha); - /* If estimatefee failed, don't do anything. */ - if (!feerate) - continue; + /* But to avoid updating forever, only apply smoothing when its + * effect is more then 10 percent */ + if (abs((int)rate->rate - (int)feerate_smooth) > (0.1 * rate->rate)) { + rate->rate = feerate_smooth; + log_debug(topo->log, + "... polled feerate estimate for %u blocks smoothed to %u (alpha=%.2f)", + rate->blockcount, rate->rate, alpha); + } - /* Initial smoothed feerate is the polled feerate */ - if (!old_feerates[i]) { - notify_feerate_changed = true; - old_feerates[i] = feerate; - init_feerate_history(topo, i, feerate); - - log_debug(topo->log, - "Smoothed feerate estimate for %s initialized to polled estimate %u", - feerate_name(i), feerate); - } else { - add_feerate_history(topo, i, feerate); - } + if (rate->rate < get_feerate_floor(topo)) { + rate->rate = get_feerate_floor(topo); + log_debug(topo->log, + "... feerate estimate for %u blocks hit floor %u", + rate->blockcount, rate->rate); + } - /* Smooth the feerate to avoid spikes. */ - u32 feerate_smooth = feerate * alpha + old_feerates[i] * (1 - alpha); - /* But to avoid updating forever, only apply smoothing when its - * effect is more then 10 percent */ - if (abs((int)feerate - (int)feerate_smooth) > (0.1 * feerate)) { - feerate = feerate_smooth; - log_debug(topo->log, - "... polled feerate estimate for %s (%u) smoothed to %u (alpha=%.2f)", - feerate_name(i), satoshi_per_kw[i], - feerate, alpha); - } + if (rate->rate != feerate_smooth) + log_debug(topo->log, + "Feerate estimate for %u blocks set to %u (was %u)", + rate->blockcount, rate->rate, feerate_smooth); +} - if (feerate < get_feerate_floor(topo)) { - feerate = get_feerate_floor(topo); - log_debug(topo->log, - "... feerate estimate for %s hit floor %u", - feerate_name(i), feerate); - } +static bool feerates_differ(const struct feerate_est *a, + const struct feerate_est *b) +{ + if (tal_count(a) != tal_count(b)) + return true; + for (size_t i = 0; i < tal_count(a); i++) { + if (a[i].blockcount != b[i].blockcount) + return true; + if (a[i].rate != b[i].rate) + return true; + } + return false; +} - if (feerate != topo->feerate[i]) { - log_debug(topo->log, "Feerate estimate for %s set to %u (was %u)", - feerate_name(i), - feerate, topo->feerate[i]); +/* In case the plugin does weird stuff! */ +static bool different_blockcounts(struct chain_topology *topo, + const struct feerate_est *old, + const struct feerate_est *new) +{ + if (tal_count(old) != tal_count(new)) { + log_unusual(topo->log, "Presented with %zu feerates this time (was %zu!)", + tal_count(new), tal_count(old)); + return true; + } + for (size_t i = 0; i < tal_count(old); i++) { + if (old[i].blockcount != new[i].blockcount) { + log_unusual(topo->log, "Presented with feerates" + " for blockcount %u, previously %u", + new[i].blockcount, old[i].blockcount); + return true; } - topo->feerate[i] = feerate; + } + return false; +} + +static void update_feerates(struct lightningd *ld, + u32 feerate_floor, + const struct feerate_est *rates TAKES) +{ + struct feerate_est *new_smoothed; + bool changed; + struct chain_topology *topo = ld->topology; + + topo->feerate_floor = feerate_floor; + + /* Don't bother updating if we got no feerates; we'd rather have + * historical ones, if any. */ + if (tal_count(rates) == 0) + goto rearm; + + /* If the feerate blockcounts differ, don't average, just override */ + if (topo->feerates[0] && different_blockcounts(topo, topo->feerates[0], rates)) { + for (size_t i = 0; i < ARRAY_SIZE(topo->feerates); i++) + topo->feerates[i] = tal_free(topo->feerates[i]); + topo->smoothed_feerates = tal_free(topo->smoothed_feerates); + } + + /* Move down historical rates, insert these */ + tal_free(topo->feerates[FEE_HISTORY_NUM-1]); + memmove(topo->feerates + 1, topo->feerates, + sizeof(topo->feerates[0]) * (FEE_HISTORY_NUM-1)); + topo->feerates[0] = tal_dup_talarr(topo, struct feerate_est, rates); + changed = feerates_differ(topo->feerates[0], topo->feerates[1]); - /* After adjustment, If any entry doesn't match prior reported, report all */ - if (feerate != old_feerates[i]) - notify_feerate_changed = true; + /* Use this as basis of new smoothed ones. */ + new_smoothed = tal_dup_talarr(topo, struct feerate_est, topo->feerates[0]); + + /* If there were old smoothed feerates, incorporate those */ + if (tal_count(topo->smoothed_feerates) != 0) { + for (size_t i = 0; i < tal_count(new_smoothed); i++) + smooth_one_feerate(topo, &new_smoothed[i]); } + changed |= feerates_differ(topo->smoothed_feerates, new_smoothed); + tal_free(topo->smoothed_feerates); + topo->smoothed_feerates = new_smoothed; + + if (changed) + notify_feerate_change(topo->ld); +rearm: if (topo->feerate_uninitialized) { /* This doesn't mean we *have* a fee estimate, but it does * mean we tried. */ @@ -441,9 +533,6 @@ static void update_feerates(struct bitcoind *bitcoind, maybe_completed_init(topo); } - if (notify_feerate_changed) - notify_feerate_change(bitcoind->ld); - next_updatefee_timer(topo); } @@ -453,8 +542,7 @@ static void start_fee_estimate(struct chain_topology *topo) if (topo->stopping) return; /* Once per new block head, update fee estimates. */ - bitcoind_estimate_fees(topo->bitcoind, NUM_FEERATES, update_feerates, - topo); + bitcoind_estimate_fees(topo->bitcoind, update_feerates); } u32 opening_feerate(struct chain_topology *topo) @@ -910,10 +998,58 @@ u32 get_network_blockheight(const struct chain_topology *topo) return topo->headercount; } +struct rate_conversion { + u32 blockcount; +}; + +static struct rate_conversion conversions[] = { + [FEERATE_OPENING] = { 12 }, + [FEERATE_MUTUAL_CLOSE] = { 100 }, + [FEERATE_UNILATERAL_CLOSE] = { 6 }, + [FEERATE_DELAYED_TO_US] = { 12 }, + [FEERATE_HTLC_RESOLUTION] = { 6 }, + [FEERATE_PENALTY] = { 12 }, +}; u32 try_get_feerate(const struct chain_topology *topo, enum feerate feerate) { - return topo->feerate[feerate]; + u32 val; + + /* Max and min look over history as well. */ + if (feerate == FEERATE_MAX) { + u32 max = 0; + for (size_t i = 0; i < ARRAY_SIZE(topo->feerates); i++) { + for (size_t j = 0; j < tal_count(topo->feerates[i]); j++) { + if (topo->feerates[i][j].rate > max) + max = topo->feerates[i][j].rate; + } + } + return max * topo->ld->config.max_fee_multiplier; + } + + if (feerate == FEERATE_MIN) { + u32 min = 0xFFFFFFFF; + for (size_t i = 0; i < ARRAY_SIZE(topo->feerates); i++) { + for (size_t j = 0; j < tal_count(topo->feerates[i]); j++) { + if (topo->feerates[i][j].rate < min) + min = topo->feerates[i][j].rate; + } + } + if (min == 0xFFFFFFFF) + return 0; + /* FIXME: This is what bcli used to do: halve the slow feerate! */ + min /= 2; + return min; + } + + if (topo->ld->force_feerates) + val = topo->ld->force_feerates[feerate]; + else + val = smoothed_feerate_for_deadline(topo, conversions[feerate].blockcount); + if (feerate == FEERATE_UNILATERAL_CLOSE) + val = val * topo->ld->config.commit_fee_percent / 100; + + return val; } u32 feerate_min(struct lightningd *ld, bool *unknown) @@ -931,14 +1067,6 @@ u32 feerate_min(struct lightningd *ld, bool *unknown) if (!min) { if (unknown) *unknown = true; - } else { - const u32 *hist = ld->topology->feehistory[FEERATE_MIN]; - - /* If one of last three was an outlier, use that. */ - for (size_t i = 0; i < FEE_HISTORY_NUM; i++) { - if (hist[i] < min) - min = hist[i]; - } } } @@ -950,7 +1078,6 @@ u32 feerate_min(struct lightningd *ld, bool *unknown) u32 feerate_max(struct lightningd *ld, bool *unknown) { u32 feerate; - const u32 *feehistory = ld->topology->feehistory[FEERATE_MAX]; if (unknown) *unknown = false; @@ -965,12 +1092,6 @@ u32 feerate_max(struct lightningd *ld, bool *unknown) *unknown = true; return UINT_MAX; } - - /* If one of last three was an outlier, use that. */ - for (size_t i = 0; i < FEE_HISTORY_NUM; i++) { - if (feehistory[i] > feerate) - feerate = feehistory[i]; - } return feerate; } @@ -1001,10 +1122,11 @@ struct chain_topology *new_topology(struct lightningd *ld, struct log *log) topo->txowatches = tal(topo, struct txowatch_hash); txowatch_hash_init(topo->txowatches); topo->log = log; - memset(topo->feerate, 0, sizeof(topo->feerate)); topo->bitcoind = new_bitcoind(topo, ld, log); topo->poll_seconds = 30; topo->feerate_uninitialized = true; + memset(topo->feerates, 0, sizeof(topo->feerates)); + topo->smoothed_feerates = NULL; topo->root = NULL; topo->sync_waiters = tal(topo, struct list_head); topo->extend_timer = NULL; @@ -1110,7 +1232,6 @@ void setup_topology(struct chain_topology *topo, u32 min_blockheight, u32 max_blockheight) { void *ret; - memset(&topo->feerate, 0, sizeof(topo->feerate)); topo->min_blockheight = min_blockheight; topo->max_blockheight = max_blockheight; diff --git a/lightningd/chaintopology.h b/lightningd/chaintopology.h index ce8e9f98af22..b123885e85f1 100644 --- a/lightningd/chaintopology.h +++ b/lightningd/chaintopology.h @@ -88,15 +88,31 @@ static inline bool outgoing_tx_eq(const struct outgoing_tx *b, const struct bitc HTABLE_DEFINE_TYPE(struct outgoing_tx, keyof_outgoing_tx_map, outgoing_tx_hash_sha, outgoing_tx_eq, outgoing_tx_map); +/* Our plugins give us a series of blockcount, feerate pairs. */ +struct feerate_est { + u32 blockcount; + u32 rate; +}; + struct chain_topology { struct lightningd *ld; struct block *root; struct block *tip; struct bitcoin_blkid prev_tip; struct block_map *block_map; - u32 feerate[NUM_FEERATES]; + + /* Set during startup */ bool feerate_uninitialized; - u32 feehistory[NUM_FEERATES][FEE_HISTORY_NUM]; + + /* This is the lowest feerate that bitcoind is saying will broadcast. */ + u32 feerate_floor; + + /* We keep last three feerates we got: this is useful for min/max. */ + struct feerate_est *feerates[FEE_HISTORY_NUM]; + + /* We keep a smoothed feerate: this is useful when we're going to + * suggest feerates / check feerates from our peers. */ + struct feerate_est *smoothed_feerates; /* Where to log things. */ struct log *log; @@ -161,6 +177,10 @@ u32 get_block_height(const struct chain_topology *topo); * likely to lag behind the rest of the network.*/ u32 get_network_blockheight(const struct chain_topology *topo); +/* Get feerate estimate for getting a tx in this many blocks */ +u32 feerate_for_deadline(const struct chain_topology *topo, u32 blockcount); +u32 smoothed_feerate_for_deadline(const struct chain_topology *topo, u32 blockcount); + /* Get fee rate in satoshi per kiloweight, or 0 if unavailable! */ u32 try_get_feerate(const struct chain_topology *topo, enum feerate feerate); diff --git a/tests/test_misc.py b/tests/test_misc.py index a3a72077b495..92e5ba43362c 100644 --- a/tests/test_misc.py +++ b/tests/test_misc.py @@ -1556,35 +1556,39 @@ def test_feerates(node_factory): l1.set_feerates((15000, 0, 0, 0), True) wait_for(lambda: l1.rpc.feerates('perkw')['perkw']['max_acceptable'] == 15000 * 10) feerates = l1.rpc.feerates('perkw') - assert feerates['warning_missing_feerates'] == 'Some fee estimates unavailable: bitcoind startup?' + # We only get the warning if *no* feerates are avail. + assert 'warning_missing_feerates' not in feerates assert 'perkb' not in feerates - assert feerates['perkw']['min_acceptable'] == 253 + # With only one data point, this is a terrible guess! + assert feerates['perkw']['min_acceptable'] == 15000 // 2 + # assert feerates['perkw']['min_acceptable'] == 253 # Set ECONOMICAL/6 feerate, for unilateral_close and htlc_resolution l1.set_feerates((15000, 11000, 0, 0), True) - wait_for(lambda: len(l1.rpc.feerates('perkw')['perkw']) == 4) feerates = l1.rpc.feerates('perkw') assert feerates['perkw']['unilateral_close'] == 11000 assert feerates['perkw']['htlc_resolution'] == 11000 - assert feerates['warning_missing_feerates'] == 'Some fee estimates unavailable: bitcoind startup?' + assert 'warning_missing_feerates' not in feerates assert 'perkb' not in feerates assert feerates['perkw']['max_acceptable'] == 15000 * 10 - assert feerates['perkw']['min_acceptable'] == 253 + # With only two data points, this is a terrible guess! + assert feerates['perkw']['min_acceptable'] == 11000 // 2 # Set ECONOMICAL/12 feerate, for all but min (so, no mutual_close feerate) l1.set_feerates((15000, 11000, 6250, 0), True) - wait_for(lambda: len(l1.rpc.feerates('perkb')['perkb']) == len(types) - 1 + 2) feerates = l1.rpc.feerates('perkb') assert feerates['perkb']['unilateral_close'] == 11000 * 4 assert feerates['perkb']['htlc_resolution'] == 11000 * 4 - assert 'mutual_close' not in feerates['perkb'] + # We dont' extrapolate, so it uses the same for mutual_close + assert feerates['perkb']['mutual_close'] == 6250 * 4 for t in types: if t not in ("unilateral_close", "htlc_resolution", "mutual_close"): assert feerates['perkb'][t] == 25000 - assert feerates['warning_missing_feerates'] == 'Some fee estimates unavailable: bitcoind startup?' + assert 'warning_missing_feerates' not in feerates assert 'perkw' not in feerates assert feerates['perkb']['max_acceptable'] == 15000 * 4 * 10 - assert feerates['perkb']['min_acceptable'] == 253 * 4 + # With only three data points, this is a terrible guess! + assert feerates['perkb']['min_acceptable'] == 6250 // 2 * 4 # Set ECONOMICAL/100 feerate for min and mutual_close l1.set_feerates((15000, 11000, 6250, 5000), True) @@ -1596,7 +1600,7 @@ def test_feerates(node_factory): for t in types: if t not in ("unilateral_close", "htlc_resolution", "mutual_close"): assert feerates['perkw'][t] == 25000 // 4 - assert 'warning' not in feerates + assert 'warning_missing_feerates' not in feerates assert 'perkb' not in feerates assert feerates['perkw']['max_acceptable'] == 15000 * 10 assert feerates['perkw']['min_acceptable'] == 5000 // 2 @@ -1910,6 +1914,7 @@ def mock_fail(*args): @unittest.skipIf(TEST_NETWORK == 'liquid-regtest', "Fees on elements are different") +@unittest.skip("FIXME: temporarily broken") def test_bitcoind_feerate_floor(node_factory, bitcoind): """Don't return a feerate less than minrelaytxfee/mempoolnifee.""" l1 = node_factory.get_node() From 1f9f1ca9dab30247304b5b3d23783c35f9eff523 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 7 Apr 2023 14:13:39 +0930 Subject: [PATCH 07/16] lightningd: clean up feerate handling, deprecate old terms. Drop try_get_feerate() in favor of explicit feerate_for_deadline() and smoothed_feerate_for_deadline(). This shows us everywhere we deal with old-style feerates by names. `delayed_to_us` and `htlc_resolution` will be moving to dynamic fees, so deprecate those. Note that "penalty" is still used for generating penalty txs for watchtowers, and "unilateral_close" still used until we get zero-fee anchors. Changelog-Added: JSON-RPC: `feerates` `estimates` array shows fee estimates by blockcount from underlying plugin (usually *bcli*). Changelog-Changed: JSON-RPC: `close`, `fundchannel`, `fundpsbt`, `multifundchannel`, `multiwithdraw`, `txprepare`, `upgradewallet`, `withdraw` `feerate` (`feerange` for `close`) value *slow* is now 100 block-estimate, not half of 100-block estimate. Changelog-Deprecated: JSON-RPC: `close`, `fundchannel`, `fundpsbt`, `multifundchannel`, `multiwithdraw`, `txprepare`, `upgradewallet`, `withdraw` `feerate` (`feerange` for `close`) expressed as, "delayed_to_us", "htlc_resolution", "max_acceptable" or "min_acceptable". Use explicit block counts or *slow*/*normal*/*urgent*/*minimum*. Signed-off-by: Rusty Russell --- .msggen.json | 48 ++++- cln-grpc/proto/node.proto | 14 ++ cln-grpc/src/convert.rs | 52 +++++ cln-rpc/src/model.rs | 28 +++ common/jsonrpc_errors.h | 1 + contrib/pyln-testing/pyln/testing/grpc2py.py | 18 ++ doc/lightning-feerates.7.md | 24 ++- doc/schemas/feerates.schema.json | 78 +++++++- lightningd/chaintopology.c | 188 +++++++++++-------- lightningd/chaintopology.h | 4 +- lightningd/channel_control.c | 9 +- lightningd/feerate.c | 101 ++++++++-- lightningd/feerate.h | 5 - lightningd/test/run-jsonrpc.c | 30 ++- tests/test_closing.py | 2 +- tests/test_misc.py | 81 +++++--- tests/test_wallet.py | 4 +- 17 files changed, 530 insertions(+), 157 deletions(-) diff --git a/.msggen.json b/.msggen.json index 5b58cc3bfdc5..79601bd08a72 100644 --- a/.msggen.json +++ b/.msggen.json @@ -357,6 +357,7 @@ }, "FeeratesPerkb": { "Feerates.perkb.delayed_to_us": 6, + "Feerates.perkb.estimates[]": 9, "Feerates.perkb.htlc_resolution": 7, "Feerates.perkb.max_acceptable": 2, "Feerates.perkb.min_acceptable": 1, @@ -365,8 +366,14 @@ "Feerates.perkb.penalty": 8, "Feerates.perkb.unilateral_close": 5 }, + "FeeratesPerkbEstimates": { + "Feerates.perkb.estimates[].blockcount": 1, + "Feerates.perkb.estimates[].feerate": 2, + "Feerates.perkb.estimates[].smoothed_feerate": 3 + }, "FeeratesPerkw": { "Feerates.perkw.delayed_to_us": 6, + "Feerates.perkw.estimates[]": 9, "Feerates.perkw.htlc_resolution": 7, "Feerates.perkw.max_acceptable": 2, "Feerates.perkw.min_acceptable": 1, @@ -375,6 +382,11 @@ "Feerates.perkw.penalty": 8, "Feerates.perkw.unilateral_close": 5 }, + "FeeratesPerkwEstimates": { + "Feerates.perkw.estimates[].blockcount": 1, + "Feerates.perkw.estimates[].feerate": 2, + "Feerates.perkw.estimates[].smoothed_feerate": 3 + }, "FeeratesRequest": { "Feerates.style": 1 }, @@ -1542,11 +1554,27 @@ }, "Feerates.perkb.delayed_to_us": { "added": "pre-v0.10.1", + "deprecated": "v23.05" + }, + "Feerates.perkb.estimates[]": { + "added": "v23.05", + "deprecated": false + }, + "Feerates.perkb.estimates[].blockcount": { + "added": "v23.05", + "deprecated": false + }, + "Feerates.perkb.estimates[].feerate": { + "added": "v23.05", + "deprecated": false + }, + "Feerates.perkb.estimates[].smoothed_feerate": { + "added": "v23.05", "deprecated": false }, "Feerates.perkb.htlc_resolution": { "added": "pre-v0.10.1", - "deprecated": false + "deprecated": "v23.05" }, "Feerates.perkb.max_acceptable": { "added": "pre-v0.10.1", @@ -1578,11 +1606,27 @@ }, "Feerates.perkw.delayed_to_us": { "added": "pre-v0.10.1", + "deprecated": "v23.05" + }, + "Feerates.perkw.estimates[]": { + "added": "v23.05", + "deprecated": false + }, + "Feerates.perkw.estimates[].blockcount": { + "added": "v23.05", + "deprecated": false + }, + "Feerates.perkw.estimates[].feerate": { + "added": "v23.05", + "deprecated": false + }, + "Feerates.perkw.estimates[].smoothed_feerate": { + "added": "v23.05", "deprecated": false }, "Feerates.perkw.htlc_resolution": { "added": "pre-v0.10.1", - "deprecated": false + "deprecated": "v23.05" }, "Feerates.perkw.max_acceptable": { "added": "pre-v0.10.1", diff --git a/cln-grpc/proto/node.proto b/cln-grpc/proto/node.proto index 4f48ee7ec0a5..982414fa0741 100644 --- a/cln-grpc/proto/node.proto +++ b/cln-grpc/proto/node.proto @@ -1130,6 +1130,7 @@ message FeeratesResponse { message FeeratesPerkb { uint32 min_acceptable = 1; uint32 max_acceptable = 2; + repeated FeeratesPerkbEstimates estimates = 9; optional uint32 opening = 3; optional uint32 mutual_close = 4; optional uint32 unilateral_close = 5; @@ -1138,9 +1139,16 @@ message FeeratesPerkb { optional uint32 penalty = 8; } +message FeeratesPerkbEstimates { + optional uint32 blockcount = 1; + optional uint32 feerate = 2; + optional uint32 smoothed_feerate = 3; +} + message FeeratesPerkw { uint32 min_acceptable = 1; uint32 max_acceptable = 2; + repeated FeeratesPerkwEstimates estimates = 9; optional uint32 opening = 3; optional uint32 mutual_close = 4; optional uint32 unilateral_close = 5; @@ -1149,6 +1157,12 @@ message FeeratesPerkw { optional uint32 penalty = 8; } +message FeeratesPerkwEstimates { + optional uint32 blockcount = 1; + optional uint32 feerate = 2; + optional uint32 smoothed_feerate = 3; +} + message FeeratesOnchain_fee_estimates { uint64 opening_channel_satoshis = 1; uint64 mutual_close_satoshis = 2; diff --git a/cln-grpc/src/convert.rs b/cln-grpc/src/convert.rs index 5e2039db79f2..17a456438a88 100644 --- a/cln-grpc/src/convert.rs +++ b/cln-grpc/src/convert.rs @@ -915,32 +915,60 @@ impl From for pb::DisconnectResponse { } } +#[allow(unused_variables,deprecated)] +impl From for pb::FeeratesPerkbEstimates { + fn from(c: responses::FeeratesPerkbEstimates) -> Self { + Self { + blockcount: c.blockcount, // Rule #2 for type u32? + feerate: c.feerate, // Rule #2 for type u32? + smoothed_feerate: c.smoothed_feerate, // Rule #2 for type u32? + } + } +} + #[allow(unused_variables,deprecated)] impl From for pb::FeeratesPerkb { fn from(c: responses::FeeratesPerkb) -> Self { Self { min_acceptable: c.min_acceptable, // Rule #2 for type u32 max_acceptable: c.max_acceptable, // Rule #2 for type u32 + estimates: c.estimates.map(|arr| arr.into_iter().map(|i| i.into()).collect()).unwrap_or(vec![]), // Rule #3 opening: c.opening, // Rule #2 for type u32? mutual_close: c.mutual_close, // Rule #2 for type u32? unilateral_close: c.unilateral_close, // Rule #2 for type u32? + #[allow(deprecated)] delayed_to_us: c.delayed_to_us, // Rule #2 for type u32? + #[allow(deprecated)] htlc_resolution: c.htlc_resolution, // Rule #2 for type u32? penalty: c.penalty, // Rule #2 for type u32? } } } +#[allow(unused_variables,deprecated)] +impl From for pb::FeeratesPerkwEstimates { + fn from(c: responses::FeeratesPerkwEstimates) -> Self { + Self { + blockcount: c.blockcount, // Rule #2 for type u32? + feerate: c.feerate, // Rule #2 for type u32? + smoothed_feerate: c.smoothed_feerate, // Rule #2 for type u32? + } + } +} + #[allow(unused_variables,deprecated)] impl From for pb::FeeratesPerkw { fn from(c: responses::FeeratesPerkw) -> Self { Self { min_acceptable: c.min_acceptable, // Rule #2 for type u32 max_acceptable: c.max_acceptable, // Rule #2 for type u32 + estimates: c.estimates.map(|arr| arr.into_iter().map(|i| i.into()).collect()).unwrap_or(vec![]), // Rule #3 opening: c.opening, // Rule #2 for type u32? mutual_close: c.mutual_close, // Rule #2 for type u32? unilateral_close: c.unilateral_close, // Rule #2 for type u32? + #[allow(deprecated)] delayed_to_us: c.delayed_to_us, // Rule #2 for type u32? + #[allow(deprecated)] htlc_resolution: c.htlc_resolution, // Rule #2 for type u32? penalty: c.penalty, // Rule #2 for type u32? } @@ -3252,12 +3280,24 @@ impl From for responses::DisconnectResponse { } } +#[allow(unused_variables,deprecated)] +impl From for responses::FeeratesPerkbEstimates { + fn from(c: pb::FeeratesPerkbEstimates) -> Self { + Self { + blockcount: c.blockcount, // Rule #1 for type u32? + feerate: c.feerate, // Rule #1 for type u32? + smoothed_feerate: c.smoothed_feerate, // Rule #1 for type u32? + } + } +} + #[allow(unused_variables,deprecated)] impl From for responses::FeeratesPerkb { fn from(c: pb::FeeratesPerkb) -> Self { Self { min_acceptable: c.min_acceptable, // Rule #1 for type u32 max_acceptable: c.max_acceptable, // Rule #1 for type u32 + estimates: Some(c.estimates.into_iter().map(|s| s.into()).collect()), // Rule #4 opening: c.opening, // Rule #1 for type u32? mutual_close: c.mutual_close, // Rule #1 for type u32? unilateral_close: c.unilateral_close, // Rule #1 for type u32? @@ -3268,12 +3308,24 @@ impl From for responses::FeeratesPerkb { } } +#[allow(unused_variables,deprecated)] +impl From for responses::FeeratesPerkwEstimates { + fn from(c: pb::FeeratesPerkwEstimates) -> Self { + Self { + blockcount: c.blockcount, // Rule #1 for type u32? + feerate: c.feerate, // Rule #1 for type u32? + smoothed_feerate: c.smoothed_feerate, // Rule #1 for type u32? + } + } +} + #[allow(unused_variables,deprecated)] impl From for responses::FeeratesPerkw { fn from(c: pb::FeeratesPerkw) -> Self { Self { min_acceptable: c.min_acceptable, // Rule #1 for type u32 max_acceptable: c.max_acceptable, // Rule #1 for type u32 + estimates: Some(c.estimates.into_iter().map(|s| s.into()).collect()), // Rule #4 opening: c.opening, // Rule #1 for type u32? mutual_close: c.mutual_close, // Rule #1 for type u32? unilateral_close: c.unilateral_close, // Rule #1 for type u32? diff --git a/cln-rpc/src/model.rs b/cln-rpc/src/model.rs index a79780216782..3b7855863b48 100644 --- a/cln-rpc/src/model.rs +++ b/cln-rpc/src/model.rs @@ -3217,36 +3217,64 @@ pub mod responses { } } + #[derive(Clone, Debug, Deserialize, Serialize)] + pub struct FeeratesPerkbEstimates { + #[serde(skip_serializing_if = "Option::is_none")] + pub blockcount: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub feerate: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub smoothed_feerate: Option, + } + #[derive(Clone, Debug, Deserialize, Serialize)] pub struct FeeratesPerkb { pub min_acceptable: u32, pub max_acceptable: u32, + #[serde(skip_serializing_if = "crate::is_none_or_empty")] + pub estimates: Option>, #[serde(skip_serializing_if = "Option::is_none")] pub opening: Option, #[serde(skip_serializing_if = "Option::is_none")] pub mutual_close: Option, #[serde(skip_serializing_if = "Option::is_none")] pub unilateral_close: Option, + #[deprecated] #[serde(skip_serializing_if = "Option::is_none")] pub delayed_to_us: Option, + #[deprecated] #[serde(skip_serializing_if = "Option::is_none")] pub htlc_resolution: Option, #[serde(skip_serializing_if = "Option::is_none")] pub penalty: Option, } + #[derive(Clone, Debug, Deserialize, Serialize)] + pub struct FeeratesPerkwEstimates { + #[serde(skip_serializing_if = "Option::is_none")] + pub blockcount: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub feerate: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub smoothed_feerate: Option, + } + #[derive(Clone, Debug, Deserialize, Serialize)] pub struct FeeratesPerkw { pub min_acceptable: u32, pub max_acceptable: u32, + #[serde(skip_serializing_if = "crate::is_none_or_empty")] + pub estimates: Option>, #[serde(skip_serializing_if = "Option::is_none")] pub opening: Option, #[serde(skip_serializing_if = "Option::is_none")] pub mutual_close: Option, #[serde(skip_serializing_if = "Option::is_none")] pub unilateral_close: Option, + #[deprecated] #[serde(skip_serializing_if = "Option::is_none")] pub delayed_to_us: Option, + #[deprecated] #[serde(skip_serializing_if = "Option::is_none")] pub htlc_resolution: Option, #[serde(skip_serializing_if = "Option::is_none")] diff --git a/common/jsonrpc_errors.h b/common/jsonrpc_errors.h index 899081765ed6..6744e87d30c6 100644 --- a/common/jsonrpc_errors.h +++ b/common/jsonrpc_errors.h @@ -71,6 +71,7 @@ enum jsonrpc_errcode { /* bitcoin-cli plugin errors */ BCLI_ERROR = 500, + BCLI_NO_FEE_ESTIMATES = 501, /* Errors from `invoice` or `delinvoice` commands */ INVOICE_LABEL_ALREADY_EXISTS = 900, diff --git a/contrib/pyln-testing/pyln/testing/grpc2py.py b/contrib/pyln-testing/pyln/testing/grpc2py.py index b82591ec6376..5d2f1d8dddf7 100644 --- a/contrib/pyln-testing/pyln/testing/grpc2py.py +++ b/contrib/pyln-testing/pyln/testing/grpc2py.py @@ -729,10 +729,19 @@ def disconnect2py(m): }) +def feerates_perkb_estimates2py(m): + return remove_default({ + "blockcount": m.blockcount, # PrimitiveField in generate_composite + "feerate": m.feerate, # PrimitiveField in generate_composite + "smoothed_feerate": m.smoothed_feerate, # PrimitiveField in generate_composite + }) + + def feerates_perkb2py(m): return remove_default({ "min_acceptable": m.min_acceptable, # PrimitiveField in generate_composite "max_acceptable": m.max_acceptable, # PrimitiveField in generate_composite + "estimates": [feerates_perkb_estimates2py(i) for i in m.estimates], # ArrayField[composite] in generate_composite "opening": m.opening, # PrimitiveField in generate_composite "mutual_close": m.mutual_close, # PrimitiveField in generate_composite "unilateral_close": m.unilateral_close, # PrimitiveField in generate_composite @@ -742,10 +751,19 @@ def feerates_perkb2py(m): }) +def feerates_perkw_estimates2py(m): + return remove_default({ + "blockcount": m.blockcount, # PrimitiveField in generate_composite + "feerate": m.feerate, # PrimitiveField in generate_composite + "smoothed_feerate": m.smoothed_feerate, # PrimitiveField in generate_composite + }) + + def feerates_perkw2py(m): return remove_default({ "min_acceptable": m.min_acceptable, # PrimitiveField in generate_composite "max_acceptable": m.max_acceptable, # PrimitiveField in generate_composite + "estimates": [feerates_perkw_estimates2py(i) for i in m.estimates], # ArrayField[composite] in generate_composite "opening": m.opening, # PrimitiveField in generate_composite "mutual_close": m.mutual_close, # PrimitiveField in generate_composite "unilateral_close": m.unilateral_close, # PrimitiveField in generate_composite diff --git a/doc/lightning-feerates.7.md b/doc/lightning-feerates.7.md index e047e72fe396..3344f4b92da2 100644 --- a/doc/lightning-feerates.7.md +++ b/doc/lightning-feerates.7.md @@ -50,25 +50,33 @@ On success, an object is returned, containing: - **perkb** (object, optional): If *style* parameter was perkb: - **min\_acceptable** (u32): The smallest feerate that we allow peers to specify: half the 100-block estimate - **max\_acceptable** (u32): The largest feerate we will accept from remote negotiations. If a peer attempts to set the feerate higher than this we will unilaterally close the channel (or simply forget it if it's not open yet). + - **estimates** (array of objects): Feerate estimates from plugin which we are using (usuallly bcli) *(added v23.05)*: + - **blockcount** (u32): The number of blocks the feerate is expected to get a transaction in *(added v23.05)* + - **feerate** (u32): The feerate for this estimate, in given *style* *(added v23.05)* + - **smoothed\_feerate** (u32): The feerate, smoothed over time (useful for coordinating with other nodes) *(added v23.05)* - **opening** (u32, optional): Default feerate for lightning-fundchannel(7) and lightning-withdraw(7) - **mutual\_close** (u32, optional): Feerate to aim for in cooperative shutdown. Note that since mutual close is a **negotiation**, the actual feerate used in mutual close will be somewhere between this and the corresponding mutual close feerate of the peer. - **unilateral\_close** (u32, optional): Feerate for commitment\_transaction in a live channel which we originally funded - - **delayed\_to\_us** (u32, optional): Feerate for returning unilateral close funds to our wallet - - **htlc\_resolution** (u32, optional): Feerate for returning unilateral close HTLC outputs to our wallet - - **penalty** (u32, optional): Feerate to start at when penalizing a cheat attempt + - **delayed\_to\_us** (u32, optional): Feerate for returning unilateral close funds to our wallet **deprecated, removal in v24.02** + - **htlc\_resolution** (u32, optional): Feerate for returning unilateral close HTLC outputs to our wallet **deprecated, removal in v24.02** + - **penalty** (u32, optional): Feerate to use when creating penalty tx for watchtowers - **perkw** (object, optional): If *style* parameter was perkw: - **min\_acceptable** (u32): The smallest feerate that you can use, usually the minimum relayed feerate of the backend - **max\_acceptable** (u32): The largest feerate we will accept from remote negotiations. If a peer attempts to set the feerate higher than this we will unilaterally close the channel (or simply forget it if it's not open yet). + - **estimates** (array of objects): Feerate estimates from plugin which we are using (usuallly bcli) *(added v23.05)*: + - **blockcount** (u32): The number of blocks the feerate is expected to get a transaction in *(added v23.05)* + - **feerate** (u32): The feerate for this estimate, in given *style* *(added v23.05)* + - **smoothed\_feerate** (u32): The feerate, smoothed over time (useful for coordinating with other nodes) *(added v23.05)* - **opening** (u32, optional): Default feerate for lightning-fundchannel(7) and lightning-withdraw(7) - **mutual\_close** (u32, optional): Feerate to aim for in cooperative shutdown. Note that since mutual close is a **negotiation**, the actual feerate used in mutual close will be somewhere between this and the corresponding mutual close feerate of the peer. - **unilateral\_close** (u32, optional): Feerate for commitment\_transaction in a live channel which we originally funded - - **delayed\_to\_us** (u32, optional): Feerate for returning unilateral close funds to our wallet - - **htlc\_resolution** (u32, optional): Feerate for returning unilateral close HTLC outputs to our wallet - - **penalty** (u32, optional): Feerate to start at when penalizing a cheat attempt + - **delayed\_to\_us** (u32, optional): Feerate for returning unilateral close funds to our wallet **deprecated, removal in v24.02** + - **htlc\_resolution** (u32, optional): Feerate for returning unilateral close HTLC outputs to our wallet **deprecated, removal in v24.02** + - **penalty** (u32, optional): Feerate to use when creating penalty tx for watchtowers - **onchain\_fee\_estimates** (object, optional): - **opening\_channel\_satoshis** (u64): Estimated cost of typical channel open - **mutual\_close\_satoshis** (u64): Estimated cost of typical channel close - - **unilateral\_close\_satoshis** (u64): Estimated cost of typical unilateral close (without HTLCs) + - **unilateral\_close\_satoshis** (u64): Estimated cost of typical (non-anchor) unilateral close (without HTLCs) - **htlc\_timeout\_satoshis** (u64): Estimated cost of typical HTLC timeout transaction - **htlc\_success\_satoshis** (u64): Estimated cost of typical HTLC fulfillment transaction @@ -121,4 +129,4 @@ RESOURCES Main web site: -[comment]: # ( SHA256STAMP:773e4e66cb3654b7c3aafe54c33d433c52ff89f7a5a8be0a71a93da21a6b7eaa) +[comment]: # ( SHA256STAMP:c21d903c29fd6195d5890962eaa3265a26a57885b95714696916bd32168b66bc) diff --git a/doc/schemas/feerates.schema.json b/doc/schemas/feerates.schema.json index d7019f17d502..29c45a69ce13 100644 --- a/doc/schemas/feerates.schema.json +++ b/doc/schemas/feerates.schema.json @@ -14,7 +14,8 @@ "additionalProperties": false, "required": [ "min_acceptable", - "max_acceptable" + "max_acceptable", + "estimates" ], "properties": { "min_acceptable": { @@ -25,6 +26,37 @@ "type": "u32", "description": "The largest feerate we will accept from remote negotiations. If a peer attempts to set the feerate higher than this we will unilaterally close the channel (or simply forget it if it's not open yet)." }, + "estimates": { + "type": "array", + "added": "v23.05", + "description": "Feerate estimates from plugin which we are using (usuallly bcli)", + "items": { + "type": "object", + "additionalProperties": false, + "required": [ + "blockcount", + "feerate", + "smoothed_feerate" + ], + "properties": { + "blockcount": { + "type": "u32", + "added": "v23.05", + "description": "The number of blocks the feerate is expected to get a transaction in" + }, + "feerate": { + "type": "u32", + "added": "v23.05", + "description": "The feerate for this estimate, in given *style*" + }, + "smoothed_feerate": { + "type": "u32", + "added": "v23.05", + "description": "The feerate, smoothed over time (useful for coordinating with other nodes)" + } + } + } + }, "opening": { "type": "u32", "description": "Default feerate for lightning-fundchannel(7) and lightning-withdraw(7)" @@ -39,15 +71,17 @@ }, "delayed_to_us": { "type": "u32", + "deprecated": "v23.05", "description": "Feerate for returning unilateral close funds to our wallet" }, "htlc_resolution": { "type": "u32", + "deprecated": "v23.05", "description": "Feerate for returning unilateral close HTLC outputs to our wallet" }, "penalty": { "type": "u32", - "description": "Feerate to start at when penalizing a cheat attempt" + "description": "Feerate to use when creating penalty tx for watchtowers" } } }, @@ -57,7 +91,8 @@ "additionalProperties": false, "required": [ "min_acceptable", - "max_acceptable" + "max_acceptable", + "estimates" ], "properties": { "min_acceptable": { @@ -68,6 +103,37 @@ "type": "u32", "description": "The largest feerate we will accept from remote negotiations. If a peer attempts to set the feerate higher than this we will unilaterally close the channel (or simply forget it if it's not open yet)." }, + "estimates": { + "type": "array", + "added": "v23.05", + "description": "Feerate estimates from plugin which we are using (usuallly bcli)", + "items": { + "type": "object", + "additionalProperties": false, + "required": [ + "blockcount", + "feerate", + "smoothed_feerate" + ], + "properties": { + "blockcount": { + "type": "u32", + "added": "v23.05", + "description": "The number of blocks the feerate is expected to get a transaction in" + }, + "feerate": { + "type": "u32", + "added": "v23.05", + "description": "The feerate for this estimate, in given *style*" + }, + "smoothed_feerate": { + "type": "u32", + "added": "v23.05", + "description": "The feerate, smoothed over time (useful for coordinating with other nodes)" + } + } + } + }, "opening": { "type": "u32", "description": "Default feerate for lightning-fundchannel(7) and lightning-withdraw(7)" @@ -82,15 +148,17 @@ }, "delayed_to_us": { "type": "u32", + "deprecated": "v23.05", "description": "Feerate for returning unilateral close funds to our wallet" }, "htlc_resolution": { "type": "u32", + "deprecated": "v23.05", "description": "Feerate for returning unilateral close HTLC outputs to our wallet" }, "penalty": { "type": "u32", - "description": "Feerate to start at when penalizing a cheat attempt" + "description": "Feerate to use when creating penalty tx for watchtowers" } } }, @@ -115,7 +183,7 @@ }, "unilateral_close_satoshis": { "type": "u64", - "description": "Estimated cost of typical unilateral close (without HTLCs)" + "description": "Estimated cost of typical (non-anchor) unilateral close (without HTLCs)" }, "htlc_timeout_satoshis": { "type": "u64", diff --git a/lightningd/chaintopology.c b/lightningd/chaintopology.c index 041a205dd77c..3b56c465c87d 100644 --- a/lightningd/chaintopology.c +++ b/lightningd/chaintopology.c @@ -5,6 +5,7 @@ #include #include #include +#include #include #include #include @@ -545,34 +546,66 @@ static void start_fee_estimate(struct chain_topology *topo) bitcoind_estimate_fees(topo->bitcoind, update_feerates); } +struct rate_conversion { + u32 blockcount; +}; + +static struct rate_conversion conversions[] = { + [FEERATE_OPENING] = { 12 }, + [FEERATE_MUTUAL_CLOSE] = { 100 }, + [FEERATE_UNILATERAL_CLOSE] = { 6 }, + [FEERATE_DELAYED_TO_US] = { 12 }, + [FEERATE_HTLC_RESOLUTION] = { 6 }, + [FEERATE_PENALTY] = { 12 }, +}; + u32 opening_feerate(struct chain_topology *topo) { - return try_get_feerate(topo, FEERATE_OPENING); + if (topo->ld->force_feerates) + return topo->ld->force_feerates[FEERATE_OPENING]; + return feerate_for_deadline(topo, + conversions[FEERATE_OPENING].blockcount); } u32 mutual_close_feerate(struct chain_topology *topo) { - return try_get_feerate(topo, FEERATE_MUTUAL_CLOSE); + if (topo->ld->force_feerates) + return topo->ld->force_feerates[FEERATE_MUTUAL_CLOSE]; + return smoothed_feerate_for_deadline(topo, + conversions[FEERATE_MUTUAL_CLOSE].blockcount); } u32 unilateral_feerate(struct chain_topology *topo) { - return try_get_feerate(topo, FEERATE_UNILATERAL_CLOSE); + if (topo->ld->force_feerates) + return topo->ld->force_feerates[FEERATE_UNILATERAL_CLOSE]; + return smoothed_feerate_for_deadline(topo, + conversions[FEERATE_UNILATERAL_CLOSE].blockcount) + * topo->ld->config.commit_fee_percent / 100; } u32 delayed_to_us_feerate(struct chain_topology *topo) { - return try_get_feerate(topo, FEERATE_DELAYED_TO_US); + if (topo->ld->force_feerates) + return topo->ld->force_feerates[FEERATE_DELAYED_TO_US]; + return smoothed_feerate_for_deadline(topo, + conversions[FEERATE_DELAYED_TO_US].blockcount); } u32 htlc_resolution_feerate(struct chain_topology *topo) { - return try_get_feerate(topo, FEERATE_HTLC_RESOLUTION); + if (topo->ld->force_feerates) + return topo->ld->force_feerates[FEERATE_HTLC_RESOLUTION]; + return smoothed_feerate_for_deadline(topo, + conversions[FEERATE_HTLC_RESOLUTION].blockcount); } u32 penalty_feerate(struct chain_topology *topo) { - return try_get_feerate(topo, FEERATE_PENALTY); + if (topo->ld->force_feerates) + return topo->ld->force_feerates[FEERATE_PENALTY]; + return smoothed_feerate_for_deadline(topo, + conversions[FEERATE_PENALTY].blockcount); } u32 get_feerate_floor(const struct chain_topology *topo) @@ -588,39 +621,68 @@ static struct command_result *json_feerates(struct command *cmd, { struct chain_topology *topo = cmd->ld->topology; struct json_stream *response; - u32 feerates[NUM_FEERATES]; bool missing; enum feerate_style *style; + u32 rate; if (!param(cmd, buffer, params, p_req("style", param_feerate_style, &style), NULL)) return command_param_failed(); - missing = false; - for (size_t i = 0; i < ARRAY_SIZE(feerates); i++) { - feerates[i] = try_get_feerate(topo, i); - if (!feerates[i]) - missing = true; - } + missing = (tal_count(topo->feerates[0]) == 0); response = json_stream_success(cmd); - if (missing) json_add_string(response, "warning_missing_feerates", "Some fee estimates unavailable: bitcoind startup?"); json_object_start(response, feerate_style_name(*style)); - for (size_t i = 0; i < ARRAY_SIZE(feerates); i++) { - if (!feerates[i] || i == FEERATE_MIN || i == FEERATE_MAX) - continue; - json_add_num(response, feerate_name(i), - feerate_to_style(feerates[i], *style)); + rate = opening_feerate(topo); + if (rate) + json_add_num(response, "opening", feerate_to_style(rate, *style)); + rate = mutual_close_feerate(topo); + if (rate) + json_add_num(response, "mutual_close", + feerate_to_style(rate, *style)); + rate = unilateral_feerate(topo); + if (rate) + json_add_num(response, "unilateral_close", + feerate_to_style(rate, *style)); + rate = penalty_feerate(topo); + if (rate) + json_add_num(response, "penalty", + feerate_to_style(rate, *style)); + if (deprecated_apis) { + rate = delayed_to_us_feerate(topo); + if (rate) + json_add_num(response, "delayed_to_us", + feerate_to_style(rate, *style)); + rate = htlc_resolution_feerate(topo); + if (rate) + json_add_num(response, "htlc_resolution", + feerate_to_style(rate, *style)); } + json_add_u64(response, "min_acceptable", feerate_to_style(feerate_min(cmd->ld, NULL), *style)); json_add_u64(response, "max_acceptable", feerate_to_style(feerate_max(cmd->ld, NULL), *style)); + + json_array_start(response, "estimates"); + assert(tal_count(topo->smoothed_feerates) == tal_count(topo->feerates[0])); + for (size_t i = 0; i < tal_count(topo->feerates[0]); i++) { + json_object_start(response, NULL); + json_add_num(response, "blockcount", + topo->feerates[0][i].blockcount); + json_add_u64(response, "feerate", + feerate_to_style(topo->feerates[0][i].rate, *style)); + json_add_u64(response, "smoothed_feerate", + feerate_to_style(topo->smoothed_feerates[i].rate, + *style)); + json_object_end(response); + } + json_array_end(response); json_object_end(response); if (!missing) { @@ -998,62 +1060,9 @@ u32 get_network_blockheight(const struct chain_topology *topo) return topo->headercount; } -struct rate_conversion { - u32 blockcount; -}; - -static struct rate_conversion conversions[] = { - [FEERATE_OPENING] = { 12 }, - [FEERATE_MUTUAL_CLOSE] = { 100 }, - [FEERATE_UNILATERAL_CLOSE] = { 6 }, - [FEERATE_DELAYED_TO_US] = { 12 }, - [FEERATE_HTLC_RESOLUTION] = { 6 }, - [FEERATE_PENALTY] = { 12 }, -}; - -u32 try_get_feerate(const struct chain_topology *topo, enum feerate feerate) -{ - u32 val; - - /* Max and min look over history as well. */ - if (feerate == FEERATE_MAX) { - u32 max = 0; - for (size_t i = 0; i < ARRAY_SIZE(topo->feerates); i++) { - for (size_t j = 0; j < tal_count(topo->feerates[i]); j++) { - if (topo->feerates[i][j].rate > max) - max = topo->feerates[i][j].rate; - } - } - return max * topo->ld->config.max_fee_multiplier; - } - - if (feerate == FEERATE_MIN) { - u32 min = 0xFFFFFFFF; - for (size_t i = 0; i < ARRAY_SIZE(topo->feerates); i++) { - for (size_t j = 0; j < tal_count(topo->feerates[i]); j++) { - if (topo->feerates[i][j].rate < min) - min = topo->feerates[i][j].rate; - } - } - if (min == 0xFFFFFFFF) - return 0; - /* FIXME: This is what bcli used to do: halve the slow feerate! */ - min /= 2; - return min; - } - - if (topo->ld->force_feerates) - val = topo->ld->force_feerates[feerate]; - else - val = smoothed_feerate_for_deadline(topo, conversions[feerate].blockcount); - if (feerate == FEERATE_UNILATERAL_CLOSE) - val = val * topo->ld->config.commit_fee_percent / 100; - - return val; -} - u32 feerate_min(struct lightningd *ld, bool *unknown) { + const struct chain_topology *topo = ld->topology; u32 min; if (unknown) @@ -1063,21 +1072,32 @@ u32 feerate_min(struct lightningd *ld, bool *unknown) if (ld->config.ignore_fee_limits) min = 1; else { - min = try_get_feerate(ld->topology, FEERATE_MIN); - if (!min) { + min = 0xFFFFFFFF; + for (size_t i = 0; i < ARRAY_SIZE(topo->feerates); i++) { + for (size_t j = 0; j < tal_count(topo->feerates[i]); j++) { + if (topo->feerates[i][j].rate < min) + min = topo->feerates[i][j].rate; + } + } + if (min == 0xFFFFFFFF) { if (unknown) *unknown = true; + min = 0; } + + /* FIXME: This is what bcli used to do: halve the slow feerate! */ + min /= 2; } - if (min < get_feerate_floor(ld->topology)) - return get_feerate_floor(ld->topology); + if (min < get_feerate_floor(topo)) + return get_feerate_floor(topo); return min; } u32 feerate_max(struct lightningd *ld, bool *unknown) { - u32 feerate; + const struct chain_topology *topo = ld->topology; + u32 max = 0; if (unknown) *unknown = false; @@ -1085,14 +1105,18 @@ u32 feerate_max(struct lightningd *ld, bool *unknown) if (ld->config.ignore_fee_limits) return UINT_MAX; - /* If we don't know feerate, don't limit other side. */ - feerate = try_get_feerate(ld->topology, FEERATE_MAX); - if (!feerate) { + for (size_t i = 0; i < ARRAY_SIZE(topo->feerates); i++) { + for (size_t j = 0; j < tal_count(topo->feerates[i]); j++) { + if (topo->feerates[i][j].rate > max) + max = topo->feerates[i][j].rate; + } + } + if (!max) { if (unknown) *unknown = true; return UINT_MAX; } - return feerate; + return max * topo->ld->config.max_fee_multiplier; } /* On shutdown, channels get deleted last. That frees from our list, so diff --git a/lightningd/chaintopology.h b/lightningd/chaintopology.h index b123885e85f1..a90b50c23100 100644 --- a/lightningd/chaintopology.h +++ b/lightningd/chaintopology.h @@ -181,14 +181,12 @@ u32 get_network_blockheight(const struct chain_topology *topo); u32 feerate_for_deadline(const struct chain_topology *topo, u32 blockcount); u32 smoothed_feerate_for_deadline(const struct chain_topology *topo, u32 blockcount); -/* Get fee rate in satoshi per kiloweight, or 0 if unavailable! */ -u32 try_get_feerate(const struct chain_topology *topo, enum feerate feerate); - /* Get range of feerates to insist other side abide by for normal channels. * If we have to guess, sets *unknown to true, otherwise false. */ u32 feerate_min(struct lightningd *ld, bool *unknown); u32 feerate_max(struct lightningd *ld, bool *unknown); +/* These return 0 if unknown */ u32 opening_feerate(struct chain_topology *topo); u32 mutual_close_feerate(struct chain_topology *topo); u32 unilateral_feerate(struct chain_topology *topo); diff --git a/lightningd/channel_control.c b/lightningd/channel_control.c index adee64850949..2bbcef4aa19a 100644 --- a/lightningd/channel_control.c +++ b/lightningd/channel_control.c @@ -38,12 +38,12 @@ static void update_feerates(struct lightningd *ld, struct channel *channel) feerate, feerate_min(ld, NULL), feerate_max(ld, NULL), - try_get_feerate(ld->topology, FEERATE_PENALTY)); + penalty_feerate(ld->topology)); msg = towire_channeld_feerates(NULL, feerate, feerate_min(ld, NULL), feerate_max(ld, NULL), - try_get_feerate(ld->topology, FEERATE_PENALTY)); + penalty_feerate(ld->topology)); subd_send_msg(channel->owner, take(msg)); } @@ -736,7 +736,7 @@ bool peer_start_channeld(struct channel *channel, channel->fee_states, feerate_min(ld, NULL), feerate_max(ld, NULL), - try_get_feerate(ld->topology, FEERATE_PENALTY), + penalty_feerate(ld->topology), &channel->last_sig, &channel->channel_info.remote_fundingkey, &channel->channel_info.theirbase, @@ -1146,8 +1146,7 @@ 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), - try_get_feerate(cmd->ld->topology, - FEERATE_PENALTY)); + penalty_feerate(cmd->ld->topology)); subd_send_msg(channel->owner, take(msg)); response = json_stream_success(cmd); diff --git a/lightningd/feerate.c b/lightningd/feerate.c index dd060032187b..d0e0a277bdef 100644 --- a/lightningd/feerate.c +++ b/lightningd/feerate.c @@ -1,4 +1,5 @@ #include "config.h" +#include #include #include #include @@ -45,36 +46,100 @@ struct command_result *param_feerate_style(struct command *cmd, json_tok_full_len(tok), json_tok_full(buffer, tok)); } -struct command_result *param_feerate(struct command *cmd, const char *name, - const char *buffer, const jsmntok_t *tok, - u32 **feerate) +/* This can set **feerate to 0, if it's unknown. */ +static struct command_result *param_feerate_unchecked(struct command *cmd, + const char *name, + const char *buffer, + const jsmntok_t *tok, + u32 **feerate) { + *feerate = tal(cmd, u32); + + if (json_tok_streq(buffer, tok, "opening")) { + **feerate = opening_feerate(cmd->ld->topology); + return NULL; + } + if (json_tok_streq(buffer, tok, "mutual_close")) { + **feerate = mutual_close_feerate(cmd->ld->topology); + return NULL; + } + if (json_tok_streq(buffer, tok, "penalty")) { + **feerate = penalty_feerate(cmd->ld->topology); + return NULL; + } + if (json_tok_streq(buffer, tok, "unilateral_close")) { + **feerate = unilateral_feerate(cmd->ld->topology); + return NULL; + } + + /* Other names are deprecated */ for (size_t i = 0; i < NUM_FEERATES; i++) { - if (json_tok_streq(buffer, tok, feerate_name(i))) - return param_feerate_estimate(cmd, feerate, i); + bool unknown; + + if (!json_tok_streq(buffer, tok, feerate_name(i))) + continue; + if (!deprecated_apis) + return command_fail_badparam(cmd, name, buffer, tok, + "removed feerate by names"); + switch (i) { + case FEERATE_OPENING: + case FEERATE_MUTUAL_CLOSE: + case FEERATE_PENALTY: + case FEERATE_UNILATERAL_CLOSE: + /* Handled above */ + abort(); + case FEERATE_DELAYED_TO_US: + **feerate = delayed_to_us_feerate(cmd->ld->topology); + return NULL; + case FEERATE_HTLC_RESOLUTION: + **feerate = htlc_resolution_feerate(cmd->ld->topology); + return NULL; + case FEERATE_MAX: + **feerate = feerate_max(cmd->ld, &unknown); + if (unknown) + **feerate = 0; + return NULL; + case FEERATE_MIN: + **feerate = feerate_min(cmd->ld, &unknown); + if (unknown) + **feerate = 0; + return NULL; + } + abort(); } + /* We used SLOW, NORMAL, and URGENT as feerate targets previously, * and many commands rely on this syntax now. * It's also really more natural for an user interface. */ - if (json_tok_streq(buffer, tok, "slow")) - return param_feerate_estimate(cmd, feerate, FEERATE_MIN); - else if (json_tok_streq(buffer, tok, "normal")) - return param_feerate_estimate(cmd, feerate, FEERATE_OPENING); - else if (json_tok_streq(buffer, tok, "urgent")) - return param_feerate_estimate(cmd, feerate, FEERATE_UNILATERAL_CLOSE); + if (json_tok_streq(buffer, tok, "slow")) { + **feerate = feerate_for_deadline(cmd->ld->topology, 100); + return NULL; + } else if (json_tok_streq(buffer, tok, "normal")) { + **feerate = feerate_for_deadline(cmd->ld->topology, 12); + return NULL; + } else if (json_tok_streq(buffer, tok, "urgent")) { + **feerate = feerate_for_deadline(cmd->ld->topology, 6); + return NULL; + } /* It's a number... */ + tal_free(*feerate); return param_feerate_val(cmd, name, buffer, tok, feerate); } -struct command_result *param_feerate_estimate(struct command *cmd, - u32 **feerate_per_kw, - enum feerate feerate) +struct command_result *param_feerate(struct command *cmd, const char *name, + const char *buffer, const jsmntok_t *tok, + u32 **feerate) { - *feerate_per_kw = tal(cmd, u32); - **feerate_per_kw = try_get_feerate(cmd->ld->topology, feerate); - if (!**feerate_per_kw) - return command_fail(cmd, LIGHTNINGD, "Cannot estimate fees"); + struct command_result *ret; + + ret = param_feerate_unchecked(cmd, name, buffer, tok, feerate); + if (ret) + return ret; + + if (**feerate == 0) + return command_fail(cmd, BCLI_NO_FEE_ESTIMATES, + "Cannot estimate fees (yet)"); return NULL; } diff --git a/lightningd/feerate.h b/lightningd/feerate.h index 80ab365f8d06..ca0793d5b963 100644 --- a/lightningd/feerate.h +++ b/lightningd/feerate.h @@ -28,11 +28,6 @@ struct command_result *param_feerate_style(struct command *cmd, const jsmntok_t *tok, enum feerate_style **style); -/* Set feerate_per_kw to this estimate & return NULL, or fail cmd */ -struct command_result *param_feerate_estimate(struct command *cmd, - u32 **feerate_per_kw, - enum feerate feerate); - /* Extract a feerate with optional style suffix. */ struct command_result *param_feerate_val(struct command *cmd, const char *name, const char *buffer, diff --git a/lightningd/test/run-jsonrpc.c b/lightningd/test/run-jsonrpc.c index 0919a3a93599..87475b204f6d 100644 --- a/lightningd/test/run-jsonrpc.c +++ b/lightningd/test/run-jsonrpc.c @@ -13,11 +13,23 @@ void db_begin_transaction_(struct db *db UNNEEDED, const char *location UNNEEDED /* Generated stub for db_commit_transaction */ void db_commit_transaction(struct db *db UNNEEDED) { fprintf(stderr, "db_commit_transaction called!\n"); abort(); } +/* Generated stub for delayed_to_us_feerate */ +u32 delayed_to_us_feerate(struct chain_topology *topo UNNEEDED) +{ fprintf(stderr, "delayed_to_us_feerate called!\n"); abort(); } /* Generated stub for deprecated_apis */ bool deprecated_apis; /* Generated stub for fatal */ void fatal(const char *fmt UNNEEDED, ...) { fprintf(stderr, "fatal called!\n"); abort(); } +/* Generated stub for feerate_for_deadline */ +u32 feerate_for_deadline(const struct chain_topology *topo UNNEEDED, u32 blockcount UNNEEDED) +{ fprintf(stderr, "feerate_for_deadline called!\n"); abort(); } +/* Generated stub for feerate_max */ +u32 feerate_max(struct lightningd *ld UNNEEDED, bool *unknown UNNEEDED) +{ fprintf(stderr, "feerate_max called!\n"); abort(); } +/* Generated stub for feerate_min */ +u32 feerate_min(struct lightningd *ld UNNEEDED, bool *unknown UNNEEDED) +{ fprintf(stderr, "feerate_min called!\n"); abort(); } /* Generated stub for fromwire_bigsize */ bigsize_t fromwire_bigsize(const u8 **cursor UNNEEDED, size_t *max UNNEEDED) { fprintf(stderr, "fromwire_bigsize called!\n"); abort(); } @@ -28,6 +40,9 @@ bool fromwire_channel_id(const u8 **cursor UNNEEDED, size_t *max UNNEEDED, /* Generated stub for fromwire_node_id */ void fromwire_node_id(const u8 **cursor UNNEEDED, size_t *max UNNEEDED, struct node_id *id UNNEEDED) { fprintf(stderr, "fromwire_node_id called!\n"); abort(); } +/* Generated stub for htlc_resolution_feerate */ +u32 htlc_resolution_feerate(struct chain_topology *topo UNNEEDED) +{ fprintf(stderr, "htlc_resolution_feerate called!\n"); abort(); } /* Generated stub for json_to_jsonrpc_errcode */ bool json_to_jsonrpc_errcode(const char *buffer UNNEEDED, const jsmntok_t *tok UNNEEDED, enum jsonrpc_errcode *errcode UNNEEDED) @@ -52,6 +67,9 @@ void log_io(struct log *log UNNEEDED, enum log_level dir UNNEEDED, /* Generated stub for log_level_name */ const char *log_level_name(enum log_level level UNNEEDED) { fprintf(stderr, "log_level_name called!\n"); abort(); } +/* Generated stub for mutual_close_feerate */ +u32 mutual_close_feerate(struct chain_topology *topo UNNEEDED) +{ fprintf(stderr, "mutual_close_feerate called!\n"); abort(); } /* Generated stub for new_log */ struct log *new_log(const tal_t *ctx UNNEEDED, struct log_book *record UNNEEDED, const struct node_id *default_node_id UNNEEDED, @@ -63,6 +81,9 @@ struct oneshot *new_reltimer_(struct timers *timers UNNEEDED, struct timerel expire UNNEEDED, void (*cb)(void *) UNNEEDED, void *arg UNNEEDED) { fprintf(stderr, "new_reltimer_ called!\n"); abort(); } +/* Generated stub for opening_feerate */ +u32 opening_feerate(struct chain_topology *topo UNNEEDED) +{ fprintf(stderr, "opening_feerate called!\n"); abort(); } /* Generated stub for param */ bool param(struct command *cmd UNNEEDED, const char *buffer UNNEEDED, const jsmntok_t params[] UNNEEDED, ...) @@ -97,6 +118,9 @@ const char *param_subcommand(struct command *cmd UNNEEDED, const char *buffer UN const jsmntok_t tokens[] UNNEEDED, const char *name UNNEEDED, ...) { fprintf(stderr, "param_subcommand called!\n"); abort(); } +/* Generated stub for penalty_feerate */ +u32 penalty_feerate(struct chain_topology *topo UNNEEDED) +{ fprintf(stderr, "penalty_feerate called!\n"); abort(); } /* Generated stub for plugin_hook_call_ */ bool plugin_hook_call_(struct lightningd *ld UNNEEDED, const struct plugin_hook *hook UNNEEDED, @@ -112,9 +136,9 @@ void towire_channel_id(u8 **pptr UNNEEDED, const struct channel_id *channel_id U /* Generated stub for towire_node_id */ void towire_node_id(u8 **pptr UNNEEDED, const struct node_id *id UNNEEDED) { fprintf(stderr, "towire_node_id called!\n"); abort(); } -/* Generated stub for try_get_feerate */ -u32 try_get_feerate(const struct chain_topology *topo UNNEEDED, enum feerate feerate UNNEEDED) -{ fprintf(stderr, "try_get_feerate called!\n"); abort(); } +/* Generated stub for unilateral_feerate */ +u32 unilateral_feerate(struct chain_topology *topo UNNEEDED) +{ fprintf(stderr, "unilateral_feerate called!\n"); abort(); } /* AUTOGENERATED MOCKS END */ static int test_json_filter(void) diff --git a/tests/test_closing.py b/tests/test_closing.py index 4c1e664cbb90..736dbf610770 100644 --- a/tests/test_closing.py +++ b/tests/test_closing.py @@ -2688,7 +2688,7 @@ def test_onchain_all_dust(node_factory, bitcoind, executor): # Make l1's fees really high (and wait for it to exceed 50000) l1.set_feerates((1000000, 1000000, 1000000, 1000000)) - l1.daemon.wait_for_log('Feerate estimate for unilateral_close set to [56789][0-9]{4}') + l1.daemon.wait_for_log('feerate estimate for 6 blocks smoothed to [56789][0-9]{4}') bitcoind.generate_block(1) l1.daemon.wait_for_log(' to ONCHAIN') diff --git a/tests/test_misc.py b/tests/test_misc.py index 92e5ba43362c..e53d18b6709f 100644 --- a/tests/test_misc.py +++ b/tests/test_misc.py @@ -1524,8 +1524,7 @@ def test_feerates(node_factory): l1.start() # All estimation types - types = ["opening", "mutual_close", "unilateral_close", "delayed_to_us", - "htlc_resolution", "penalty"] + types = ["opening", "mutual_close", "unilateral_close", "penalty"] # Try parsing the feerates, won't work because can't estimate for t in types: @@ -1533,21 +1532,23 @@ def test_feerates(node_factory): feerate = l1.rpc.parsefeerate(t) # Query feerates (shouldn't give any!) - wait_for(lambda: len(l1.rpc.feerates('perkw')['perkw']) == 2) + wait_for(lambda: len(l1.rpc.feerates('perkw')['perkw']) == 3) 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 assert feerates['perkw']['min_acceptable'] == 253 + assert feerates['perkw']['min_acceptable'] == 253 + assert feerates['perkw']['estimates'] == [] for t in types: assert t not in feerates['perkw'] - wait_for(lambda: len(l1.rpc.feerates('perkb')['perkb']) == 2) 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']['min_acceptable'] == 253 * 4 + assert feerates['perkb']['estimates'] == [] for t in types: assert t not in feerates['perkb'] @@ -1561,24 +1562,30 @@ def test_feerates(node_factory): assert 'perkb' not in feerates # With only one data point, this is a terrible guess! assert feerates['perkw']['min_acceptable'] == 15000 // 2 - # assert feerates['perkw']['min_acceptable'] == 253 + assert feerates['perkw']['estimates'] == [{'blockcount': 2, + 'feerate': 15000, + 'smoothed_feerate': 15000}] # Set ECONOMICAL/6 feerate, for unilateral_close and htlc_resolution l1.set_feerates((15000, 11000, 0, 0), True) feerates = l1.rpc.feerates('perkw') assert feerates['perkw']['unilateral_close'] == 11000 - assert feerates['perkw']['htlc_resolution'] == 11000 assert 'warning_missing_feerates' not in feerates assert 'perkb' not in feerates assert feerates['perkw']['max_acceptable'] == 15000 * 10 # With only two data points, this is a terrible guess! assert feerates['perkw']['min_acceptable'] == 11000 // 2 + assert feerates['perkw']['estimates'] == [{'blockcount': 2, + 'feerate': 15000, + 'smoothed_feerate': 15000}, + {'blockcount': 6, + 'feerate': 11000, + 'smoothed_feerate': 11000}] # Set ECONOMICAL/12 feerate, for all but min (so, no mutual_close feerate) l1.set_feerates((15000, 11000, 6250, 0), True) feerates = l1.rpc.feerates('perkb') assert feerates['perkb']['unilateral_close'] == 11000 * 4 - assert feerates['perkb']['htlc_resolution'] == 11000 * 4 # We dont' extrapolate, so it uses the same for mutual_close assert feerates['perkb']['mutual_close'] == 6250 * 4 for t in types: @@ -1589,13 +1596,21 @@ def test_feerates(node_factory): assert feerates['perkb']['max_acceptable'] == 15000 * 4 * 10 # With only three data points, this is a terrible guess! assert feerates['perkb']['min_acceptable'] == 6250 // 2 * 4 + assert feerates['perkb']['estimates'] == [{'blockcount': 2, + 'feerate': 15000 * 4, + 'smoothed_feerate': 15000 * 4}, + {'blockcount': 6, + 'feerate': 11000 * 4, + 'smoothed_feerate': 11000 * 4}, + {'blockcount': 12, + 'feerate': 6250 * 4, + 'smoothed_feerate': 6250 * 4}] # Set ECONOMICAL/100 feerate for min and mutual_close l1.set_feerates((15000, 11000, 6250, 5000), True) wait_for(lambda: len(l1.rpc.feerates('perkw')['perkw']) >= len(types) + 2) feerates = l1.rpc.feerates('perkw') assert feerates['perkw']['unilateral_close'] == 11000 - assert feerates['perkw']['htlc_resolution'] == 11000 assert feerates['perkw']['mutual_close'] == 5000 for t in types: if t not in ("unilateral_close", "htlc_resolution", "mutual_close"): @@ -1604,12 +1619,25 @@ def test_feerates(node_factory): assert 'perkb' not in feerates assert feerates['perkw']['max_acceptable'] == 15000 * 10 assert feerates['perkw']['min_acceptable'] == 5000 // 2 + assert feerates['perkw']['estimates'] == [{'blockcount': 2, + 'feerate': 15000, + 'smoothed_feerate': 15000}, + {'blockcount': 6, + 'feerate': 11000, + 'smoothed_feerate': 11000}, + {'blockcount': 12, + 'feerate': 6250, + 'smoothed_feerate': 6250}, + {'blockcount': 100, + 'feerate': 5000, + 'smoothed_feerate': 5000}] assert len(feerates['onchain_fee_estimates']) == 5 assert feerates['onchain_fee_estimates']['opening_channel_satoshis'] == feerates['perkw']['opening'] * 702 // 1000 assert feerates['onchain_fee_estimates']['mutual_close_satoshis'] == feerates['perkw']['mutual_close'] * 673 // 1000 assert feerates['onchain_fee_estimates']['unilateral_close_satoshis'] == feerates['perkw']['unilateral_close'] * 598 // 1000 - htlc_feerate = feerates["perkw"]["htlc_resolution"] + # htlc resolution currently uses 6 block estimate + htlc_feerate = [f['feerate'] for f in feerates['perkw']['estimates'] if f['blockcount'] == 6][0] htlc_timeout_cost = feerates["onchain_fee_estimates"]["htlc_timeout_satoshis"] htlc_success_cost = feerates["onchain_fee_estimates"]["htlc_success_satoshis"] @@ -2906,15 +2934,28 @@ def test_force_feerates(node_factory): l1 = node_factory.get_node(options={'force-feerates': 1111}) assert l1.rpc.listconfigs()['force-feerates'] == '1111' + # Note that estimates are still valid here, despite "force-feerates" + estimates = [{"blockcount": 2, + "feerate": 15000, + "smoothed_feerate": 15000}, + {"blockcount": 6, + "feerate": 11000, + "smoothed_feerate": 11000}, + {"blockcount": 12, + "feerate": 7500, + "smoothed_feerate": 7500}, + {"blockcount": 100, + "feerate": 3750, + "smoothed_feerate": 3750}] + assert l1.rpc.feerates('perkw')['perkw'] == { "opening": 1111, "mutual_close": 1111, "unilateral_close": 1111, - "delayed_to_us": 1111, - "htlc_resolution": 1111, "penalty": 1111, "min_acceptable": 1875, - "max_acceptable": 150000} + "max_acceptable": 150000, + "estimates": estimates} l1.stop() l1.daemon.opts['force-feerates'] = '1111/2222' @@ -2925,11 +2966,10 @@ def test_force_feerates(node_factory): "opening": 1111, "mutual_close": 2222, "unilateral_close": 2222, - "delayed_to_us": 2222, - "htlc_resolution": 2222, "penalty": 2222, "min_acceptable": 1875, - "max_acceptable": 150000} + "max_acceptable": 150000, + "estimates": estimates} l1.stop() l1.daemon.opts['force-feerates'] = '1111/2222/3333/4444/5555/6666' @@ -2940,11 +2980,10 @@ def test_force_feerates(node_factory): "opening": 1111, "mutual_close": 2222, "unilateral_close": 3333, - "delayed_to_us": 4444, - "htlc_resolution": 5555, "penalty": 6666, "min_acceptable": 1875, - "max_acceptable": 150000} + "max_acceptable": 150000, + "estimates": estimates} def test_datastore_escapeing(node_factory): @@ -3234,16 +3273,12 @@ def test_feerate_arg(node_factory): fees["urgent"] = by_blocks[6] fees["normal"] = by_blocks[12] - fees["slow"] = by_blocks[100] // 2 + fees["slow"] = by_blocks[100] fees["opening"] = by_blocks[12] fees["mutual_close"] = by_blocks[100] fees["penalty"] = by_blocks[12] fees["unilateral_close"] = by_blocks[6] - fees["delayed_to_us"] = by_blocks[12] - fees["htlc_resolution"] = by_blocks[6] - fees["min_acceptable"] = by_blocks[100] // 2 - fees["max_acceptable"] = by_blocks[2] * 10 for fee, expect in fees.items(): # Put arg in assertion, so it gets printed on failure! diff --git a/tests/test_wallet.py b/tests/test_wallet.py index 883d4aef74d5..61d5f2075dfd 100644 --- a/tests/test_wallet.py +++ b/tests/test_wallet.py @@ -1658,12 +1658,12 @@ def test_upgradewallet(node_factory, bitcoind): # Doing it with 'reserved ok' should have 1 # We use a big feerate so we can get over the RBF hump - upgrade = l1.rpc.upgradewallet(feerate="max_acceptable", reservedok=True) + upgrade = l1.rpc.upgradewallet(feerate="urgent", reservedok=True) assert upgrade['upgraded_outs'] == 1 assert bitcoind.rpc.getmempoolinfo()['size'] == 1 # Mine it, nothing to upgrade l1.bitcoin.generate_block(1) sync_blockheight(l1.bitcoin, [l1]) - upgrade = l1.rpc.upgradewallet(feerate="max_acceptable", reservedok=True) + upgrade = l1.rpc.upgradewallet(feerate="urgent", reservedok=True) assert upgrade['upgraded_outs'] == 0 From cba5e3e52a0b61ce1c0c7e7ca13d077be97ff0ab Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 7 Apr 2023 14:13:45 +0930 Subject: [PATCH 08/16] lightningd: allow "NNblocks" and "minimum" as feerates. And consolidate descriptions into lightning-feerates(). Signed-off-by: Rusty Russell Changelog-Added: JSON-RPC: `close`, `fundchannel`, `fundpsbt`, `multifundchannel`, `multiwithdraw`, `txprepare`, `upgradewallet`, `withdraw` now allow "minimum" and NN"blocks" as `feerate` (`feerange` for `close`). --- contrib/pyln-testing/pyln/testing/fixtures.py | 2 +- doc/lightning-close.7.md | 11 +--------- doc/lightning-feerates.7.md | 21 ++++++++++++------- doc/lightning-fundchannel.7.md | 13 +++--------- doc/lightning-fundpsbt.7.md | 10 ++------- doc/lightning-multifundchannel.7.md | 15 ++++--------- doc/lightning-multiwithdraw.7.md | 11 ++-------- doc/lightning-txprepare.7.md | 11 ++-------- doc/lightning-upgradewallet.7.md | 10 ++------- doc/lightning-withdraw.7.md | 11 ++-------- lightningd/feerate.c | 19 +++++++++++++++++ lightningd/test/run-jsonrpc.c | 3 +++ tests/test_misc.py | 15 +++++++++++++ 13 files changed, 70 insertions(+), 82 deletions(-) diff --git a/contrib/pyln-testing/pyln/testing/fixtures.py b/contrib/pyln-testing/pyln/testing/fixtures.py index c70bee6e4691..bf3293d24452 100644 --- a/contrib/pyln-testing/pyln/testing/fixtures.py +++ b/contrib/pyln-testing/pyln/testing/fixtures.py @@ -295,7 +295,7 @@ def is_feerate(checker, instance): return True if not checker.is_type(instance, "string"): return False - if instance in ("urgent", "normal", "slow"): + if instance in ("urgent", "normal", "slow", "minimum"): return True if instance in ("opening", "mutual_close", "unilateral_close", "delayed_to_us", "htlc_resolution", "penalty", "min_acceptable", "max_acceptable"): return True diff --git a/doc/lightning-close.7.md b/doc/lightning-close.7.md index 8107124fbbd5..853df15c6fe1 100644 --- a/doc/lightning-close.7.md +++ b/doc/lightning-close.7.md @@ -66,16 +66,7 @@ unless this flag is passed in. Defaults to false. *feerange* is an optional array [ *min*, *max* ], indicating the minimum and maximum feerates to offer: the peer will obey these if it supports the quick-close protocol. *slow* and *unilateral\_close* are -the defaults. - -Rates are one of the strings *urgent* (aim for next block), *normal* -(next 4 blocks or so) or *slow* (next 100 blocks or so) to use -lightningd's internal estimates, or one of the names from -lightning-feerates(7). Otherwise, they can be numbers with -an optional suffix: *perkw* means the number is interpreted as -satoshi-per-kilosipa (weight), and *perkb* means it is interpreted -bitcoind-style as satoshi-per-kilobyte. Omitting the suffix is -equivalent to *perkb*. +the defaults. See NOTES in lightning-feerates(7) for possible values. Note that the maximum fee will be capped at the final commitment transaction fee (unless the experimental anchor-outputs option is diff --git a/doc/lightning-feerates.7.md b/doc/lightning-feerates.7.md index 3344f4b92da2..bcda750e5299 100644 --- a/doc/lightning-feerates.7.md +++ b/doc/lightning-feerates.7.md @@ -96,13 +96,20 @@ if feerate estimates for that kind of transaction are unavailable. NOTES ----- -Many other commands have a *feerate* parameter, which can be the strings -*urgent*, *normal*, or *slow*. -These are mapped to the **feerates** outputs as: - -* *urgent* - equal to *unilateral\_close* -* *normal* - equal to *opening* -* *slow* - equal to *min\_acceptable*. +Many other commands have a *feerate* parameter. This can be: + +* One of the strings to use lightningd's internal estimates: + * *urgent* (aim for next block), + * *normal* (next 6 blocks or so) + * *slow* (next 100 blocks or so) + * *minimum* for the lowest value bitcoind will currently accept (added in v23.05) + +* A number, with an optional suffix: + * *blocks* means aim for confirmation in that many blocks (added in v23.05) + * *perkw* means the number is interpreted as satoshi-per-kilosipa (weight) + * *perkb* means it is interpreted bitcoind-style as satoshi-per-kilobyte. + +Omitting the suffix is equivalent to *perkb*. TRIVIA ------ diff --git a/doc/lightning-fundchannel.7.md b/doc/lightning-fundchannel.7.md index 5ee6c46a2031..b8c15e8425d1 100644 --- a/doc/lightning-fundchannel.7.md +++ b/doc/lightning-fundchannel.7.md @@ -35,16 +35,9 @@ decimal places ending in *btc*. The value cannot be less than the dust limit, currently set to 546, nor more than 16777215 satoshi (unless large channels were negotiated with the peer). -*feerate* is an optional feerate used for the opening transaction and as -initial feerate for commitment and HTLC transactions. It can be one of -the strings *urgent* (aim for next block), *normal* (next 4 blocks or -so) or *slow* (next 100 blocks or so) to use lightningd's internal -estimates: *normal* is the default. - -Otherwise, *feerate* is a number, with an optional suffix: *perkw* means -the number is interpreted as satoshi-per-kilosipa (weight), and *perkb* -means it is interpreted bitcoind-style as satoshi-per-kilobyte. Omitting -the suffix is equivalent to *perkb*. +*feerate* is an optional feerate used for the opening transaction and +as initial feerate for commitment and HTLC transactions (see NOTES in +lightning-feerates(7)). The default is *normal*. *announce* is an optional flag that triggers whether to announce this channel or not. Defaults to `true`. An unannounced channel is considered diff --git a/doc/lightning-fundpsbt.7.md b/doc/lightning-fundpsbt.7.md index 5e25e4861b5c..eaac0fb05e3c 100644 --- a/doc/lightning-fundpsbt.7.md +++ b/doc/lightning-fundpsbt.7.md @@ -18,14 +18,8 @@ be a whole number, a whole number ending in *sat*, a whole number ending in *000msat*, or a number with 1 to 8 decimal places ending in *btc*. -*feerate* can be one of the feerates listed in lightning-feerates(7), -or one of the strings *urgent* (aim for next block), *normal* (next 4 -blocks or so) or *slow* (next 100 blocks or so) to use lightningd's -internal estimates. It can also be a *feerate* is a number, with an -optional suffix: *perkw* means the number is interpreted as -satoshi-per-kilosipa (weight), and *perkb* means it is interpreted -bitcoind-style as satoshi-per-kilobyte. Omitting the suffix is -equivalent to *perkb*. +*feerate* is an optional feerate: see NOTES in lightning-feerates(7) +for possible values. The default is *normal*. *startweight* is the weight of the transaction before *fundpsbt* has added any inputs. diff --git a/doc/lightning-multifundchannel.7.md b/doc/lightning-multifundchannel.7.md index 2c6b63a02376..0203be78e939 100644 --- a/doc/lightning-multifundchannel.7.md +++ b/doc/lightning-multifundchannel.7.md @@ -65,17 +65,10 @@ Readiness is indicated by **listpeers** reporting a *state* of There must be at least one entry in *destinations*; it cannot be an empty array. -*feerate* is an optional feerate used for the opening transaction and, if -*commitment\_feerate* is not set, as the initial feerate for -commitment and HTLC transactions. It can be one of -the strings *urgent* (aim for next block), *normal* (next 4 blocks or -so) or *slow* (next 100 blocks or so) to use lightningd's internal -estimates: *normal* is the default. - -Otherwise, *feerate* is a number, with an optional suffix: *perkw* means -the number is interpreted as satoshi-per-kilosipa (weight), and *perkb* -means it is interpreted bitcoind-style as satoshi-per-kilobyte. Omitting -the suffix is equivalent to *perkb*. +*feerate* is an optional feerate used for the opening transaction, and +if *commitment\_feerate* is not set, as initial feerate for commitment +and HTLC transactions. See NOTES in lightning-feerates(7) for possible +values. The default is *normal*. *minconf* specifies the minimum number of confirmations that used outputs should have. Default is 1. diff --git a/doc/lightning-multiwithdraw.7.md b/doc/lightning-multiwithdraw.7.md index ae09c2bdf251..a1807c7e8218 100644 --- a/doc/lightning-multiwithdraw.7.md +++ b/doc/lightning-multiwithdraw.7.md @@ -21,15 +21,8 @@ a whole number ending in *sat*, a whole number ending in *000msat*, or a number with 1 to 8 decimal places ending in *btc*. -*feerate* is an optional feerate to use. It can be one of the strings -*urgent* (aim for next block), *normal* (next 4 blocks or so) or *slow* -(next 100 blocks or so) to use lightningd's internal estimates: *normal* -is the default. - -Otherwise, *feerate* is a number, with an optional suffix: *perkw* means -the number is interpreted as satoshi-per-kilosipa (weight), and *perkb* -means it is interpreted bitcoind-style as satoshi-per-kilobyte. Omitting -the suffix is equivalent to *perkb*. +*feerate* is an optional feerate: see NOTES in lightning-feerates(7) +for possible values. The default is *normal*. *minconf* specifies the minimum number of confirmations that used outputs should have. Default is 1. diff --git a/doc/lightning-txprepare.7.md b/doc/lightning-txprepare.7.md index 37655098c771..62681d142d43 100644 --- a/doc/lightning-txprepare.7.md +++ b/doc/lightning-txprepare.7.md @@ -29,15 +29,8 @@ all available funds. Otherwise, it is in amount precision; it can be a whole number, a whole number ending in *sat*, a whole number ending in *000msat*, or a number with 1 to 8 decimal places ending in *btc*. -*feerate* is an optional feerate to use. It can be one of the strings -*urgent* (aim for next block), *normal* (next 4 blocks or so) or *slow* -(next 100 blocks or so) to use lightningd's internal estimates: *normal* -is the default. - -Otherwise, *feerate* is a number, with an optional suffix: *perkw* means -the number is interpreted as satoshi-per-kilosipa (weight), and *perkb* -means it is interpreted bitcoind-style as satoshi-per-kilobyte. Omitting -the suffix is equivalent to *perkb*. +*feerate* is an optional feerate to use: see NOTES in lightning-feerates(7) +for possible values. The default is *normal*. *minconf* specifies the minimum number of confirmations that used outputs should have. Default is 1. diff --git a/doc/lightning-upgradewallet.7.md b/doc/lightning-upgradewallet.7.md index 7df504509d88..a92cf7d04fe3 100644 --- a/doc/lightning-upgradewallet.7.md +++ b/doc/lightning-upgradewallet.7.md @@ -12,14 +12,8 @@ DESCRIPTION `upgradewallet` is a convenience RPC which will spend all p2sh-wrapped Segwit deposits in a wallet into a single Native Segwit P2WPKH address. -*feerate* can be one of the feerates listed in lightning-feerates(7), -or one of the strings *urgent* (aim for next block), *normal* (next 4 -blocks or so) or *slow* (next 100 blocks or so) to use lightningd's -internal estimates. It can also be a *feerate* is a number, with an -optional suffix: *perkw* means the number is interpreted as -satoshi-per-kilosipa (weight), and *perkb* means it is interpreted -bitcoind-style as satoshi-per-kilobyte. Omitting the suffix is -equivalent to *perkb*. +*feerate* is an optional feerate: see NOTES in lightning-feerates(7) +for possible values. The default is *opening*. *reservedok* tells the wallet to include all P2SH-wrapped inputs, including reserved ones. diff --git a/doc/lightning-withdraw.7.md b/doc/lightning-withdraw.7.md index d5e188d09657..23ae414ffda1 100644 --- a/doc/lightning-withdraw.7.md +++ b/doc/lightning-withdraw.7.md @@ -21,15 +21,8 @@ satoshi precision; it can be a whole number, a whole number ending in *sat*, a whole number ending in *000msat*, or a number with 1 to 8 decimal places ending in *btc*. -*feerate* is an optional feerate to use. It can be one of the strings -*urgent* (aim for next block), *normal* (next 4 blocks or so) or *slow* -(next 100 blocks or so) to use lightningd's internal estimates: *normal* -is the default. - -Otherwise, *feerate* is a number, with an optional suffix: *perkw* means -the number is interpreted as satoshi-per-kilosipa (weight), and *perkb* -means it is interpreted bitcoind-style as satoshi-per-kilobyte. Omitting -the suffix is equivalent to *perkb*. +*feerate* is an optional feerate: see NOTES in lightning-feerates(7) +for possible values. The default is *normal*. *minconf* specifies the minimum number of confirmations that used outputs should have. Default is 1. diff --git a/lightningd/feerate.c b/lightningd/feerate.c index d0e0a277bdef..07a5f5f78a84 100644 --- a/lightningd/feerate.c +++ b/lightningd/feerate.c @@ -120,6 +120,25 @@ static struct command_result *param_feerate_unchecked(struct command *cmd, } else if (json_tok_streq(buffer, tok, "urgent")) { **feerate = feerate_for_deadline(cmd->ld->topology, 6); return NULL; + } else if (json_tok_streq(buffer, tok, "minimum")) { + **feerate = get_feerate_floor(cmd->ld->topology); + return NULL; + } + + /* Can specify number of blocks as a target */ + if (json_tok_endswith(buffer, tok, "blocks")) { + jsmntok_t base = *tok; + base.end -= strlen("blocks"); + u32 numblocks; + + if (!json_to_number(buffer, &base, &numblocks)) { + return command_fail(cmd, JSONRPC2_INVALID_PARAMS, + "'%s' should be an integer not '%.*s'", + name, base.end - base.start, + buffer + base.start); + } + **feerate = feerate_for_deadline(cmd->ld->topology, numblocks); + return NULL; } /* It's a number... */ diff --git a/lightningd/test/run-jsonrpc.c b/lightningd/test/run-jsonrpc.c index 87475b204f6d..215130fe11a2 100644 --- a/lightningd/test/run-jsonrpc.c +++ b/lightningd/test/run-jsonrpc.c @@ -40,6 +40,9 @@ bool fromwire_channel_id(const u8 **cursor UNNEEDED, size_t *max UNNEEDED, /* Generated stub for fromwire_node_id */ void fromwire_node_id(const u8 **cursor UNNEEDED, size_t *max UNNEEDED, struct node_id *id UNNEEDED) { fprintf(stderr, "fromwire_node_id called!\n"); abort(); } +/* Generated stub for get_feerate_floor */ +u32 get_feerate_floor(const struct chain_topology *topo UNNEEDED) +{ fprintf(stderr, "get_feerate_floor called!\n"); abort(); } /* Generated stub for htlc_resolution_feerate */ u32 htlc_resolution_feerate(struct chain_topology *topo UNNEEDED) { fprintf(stderr, "htlc_resolution_feerate called!\n"); abort(); } diff --git a/tests/test_misc.py b/tests/test_misc.py index e53d18b6709f..05273ff49b05 100644 --- a/tests/test_misc.py +++ b/tests/test_misc.py @@ -3280,10 +3280,25 @@ def test_feerate_arg(node_factory): fees["penalty"] = by_blocks[12] fees["unilateral_close"] = by_blocks[6] + fees["2blocks"] = by_blocks[2] + fees["6blocks"] = by_blocks[6] + fees["12blocks"] = by_blocks[12] + fees["100blocks"] = by_blocks[100] + + # Simple interpolation + fees["9blocks"] = (by_blocks[6] + by_blocks[12]) // 2 + for fee, expect in fees.items(): # Put arg in assertion, so it gets printed on failure! assert (l1.rpc.parsefeerate(fee), fee) == ({'perkw': expect}, fee) + # More thorough interpolation + for block in range(12, 100): + # y = y1 + (x-x1)(y2-y1)/(x2-x1) + fee = by_blocks[12] + (block - 12) * (by_blocks[100] - by_blocks[12]) // (100 - 12) + # Rounding error is a thing! + assert abs(l1.rpc.parsefeerate(f"{block}blocks")['perkw'] - fee) <= 1 + @pytest.mark.skip(reason="Fails by intention for creating test gossip stores") def test_create_gossip_mesh(node_factory, bitcoind): From 19f953153a5e0769d815a56bd4af5a00c7ef31ed Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 7 Apr 2023 14:14:12 +0930 Subject: [PATCH 09/16] lightningd: handle bcli plugins returning fee_floor and feerates parameters. Changelog-Added: Plugins: `estimatefees` can return explicit `fee_floor` and `feerates` by block number. Signed-off-by: Rusty Russell --- doc/PLUGINS.md | 31 ++++++++----- lightningd/bitcoind.c | 95 +++++++++++++++++++++++++++++++++++--- lightningd/chaintopology.c | 3 +- tests/test_misc.py | 62 ++++++++++++++++++------- 4 files changed, 156 insertions(+), 35 deletions(-) diff --git a/doc/PLUGINS.md b/doc/PLUGINS.md index e8f7c21be5e1..bc31025010f2 100644 --- a/doc/PLUGINS.md +++ b/doc/PLUGINS.md @@ -1729,17 +1729,26 @@ The plugin must respond to `getchaininfo` with the following fields: Polled by `lightningd` to get the current feerate, all values must be passed in sat/kVB. -If fee estimation fails, the plugin must set all the fields to `null`. - -The plugin, if fee estimation succeeds, must respond with the following fields: - - `opening` (number), used for funding and also misc transactions - - `mutual_close` (number), used for the mutual close transaction - - `unilateral_close` (number), used for unilateral close (/commitment) transactions - - `delayed_to_us` (number), used for resolving our output from our unilateral close - - `htlc_resolution` (number), used for resolving HTLCs after an unilateral close - - `penalty` (number), used for resolving revoked transactions - - `min_acceptable` (number), used as the minimum acceptable feerate - - `max_acceptable` (number), used as the maximum acceptable feerate +The plugin must return `feerate_floor` (e.g. 1000 if mempool is +empty), and an array of 0 or more `feerates`. Each element of +`feerates` is an object with `blocks` and `feerate`, in +ascending-blocks order, for example: + +``` +{ + "feerate_floor": , + "feerates": { + { "blocks": 2, "feerate": }, + { "blocks": 6, "feerate": }, + { "blocks": 12, "feerate": } + { "blocks": 100, "feerate": } + } +} +``` + +lightningd will currently linearly interpolate to estimate between +given blocks (it will not extrapolate, but use the min/max blocks +values). ### `getrawblockbyheight` diff --git a/lightningd/bitcoind.c b/lightningd/bitcoind.c index f154ba8a1ab5..3db25e2ce9c2 100644 --- a/lightningd/bitcoind.c +++ b/lightningd/bitcoind.c @@ -12,6 +12,7 @@ #include #include #include +#include #include #include #include @@ -144,7 +145,7 @@ static void bitcoin_plugin_send(struct bitcoind *bitcoind, * - `min` is the minimum acceptable feerate * - `max` is the maximum acceptable feerate * - * Plugin response: + * Plugin response (deprecated): * { * "opening": , * "mutual_close": , @@ -155,6 +156,19 @@ static void bitcoin_plugin_send(struct bitcoind *bitcoind, * "min_acceptable": , * "max_acceptable": , * } + * + * Plugin response (modern): + * { + * "feerate_floor": , + * "feerates": { + * { "blocks": 2, "feerate": }, + * { "blocks": 6, "feerate": }, + * { "blocks": 12, "feerate": } + * { "blocks": 100, "feerate": } + * } + * } + * + * If rates are missing, we linearly interpolate (we don't extrapolate tho!). */ struct estimatefee_call { struct bitcoind *bitcoind; @@ -163,6 +177,66 @@ struct estimatefee_call { }; /* Note: returns estimates in perkb, caller converts! */ +static struct feerate_est *parse_feerate_ranges(const tal_t *ctx, + struct bitcoind *bitcoind, + const char *buf, + const jsmntok_t *floortok, + const jsmntok_t *feerates, + u32 *floor) +{ + size_t i; + const jsmntok_t *t; + struct feerate_est *rates = tal_arr(ctx, struct feerate_est, 0); + + if (!json_to_u32(buf, floortok, floor)) + bitcoin_plugin_error(bitcoind, buf, floortok, + "estimatefees.feerate_floor", "Not a u32?"); + + json_for_each_arr(i, t, feerates) { + struct feerate_est rate; + const char *err; + + err = json_scan(tmpctx, buf, t, "{blocks:%,feerate:%}", + JSON_SCAN(json_to_u32, &rate.blockcount), + JSON_SCAN(json_to_u32, &rate.rate)); + if (err) + bitcoin_plugin_error(bitcoind, buf, t, + "estimatefees.feerates", err); + + /* Block count must be in order. If rates go up somehow, we + * reduce to prev. */ + if (tal_count(rates) != 0) { + const struct feerate_est *prev = &rates[tal_count(rates)-1]; + if (rate.blockcount <= prev->blockcount) + bitcoin_plugin_error(bitcoind, buf, feerates, + "estimatefees.feerates", + "Blocks must be ascending" + " order: %u <= %u!", + rate.blockcount, + prev->blockcount); + if (rate.rate > prev->rate) { + log_unusual(bitcoind->log, + "Feerate for %u blocks (%u) is > rate" + " for %u blocks (%u)!", + rate.blockcount, rate.rate, + prev->blockcount, prev->rate); + rate.rate = prev->rate; + } + } + + tal_arr_expand(&rates, rate); + } + + if (tal_count(rates) == 0) { + if (chainparams->testnet) + log_debug(bitcoind->log, "Unable to estimate any fees"); + else + log_unusual(bitcoind->log, "Unable to estimate any fees"); + } + + return rates; +} + static struct feerate_est *parse_deprecated_feerates(const tal_t *ctx, struct bitcoind *bitcoind, const char *buf, @@ -216,7 +290,7 @@ static void estimatefees_callback(const char *buf, const jsmntok_t *toks, const jsmntok_t *idtok, struct estimatefee_call *call) { - const jsmntok_t *resulttok; + const jsmntok_t *resulttok, *floortok; struct feerate_est *feerates; u32 floor; @@ -226,10 +300,19 @@ static void estimatefees_callback(const char *buf, const jsmntok_t *toks, "estimatefees", "bad 'result' field"); - feerates = parse_deprecated_feerates(call, call->bitcoind, - buf, resulttok); - /* FIXME: get from plugin! */ - floor = feerate_from_style(FEERATE_FLOOR, FEERATE_PER_KSIPA); + /* Modern style has floor. */ + floortok = json_get_member(buf, resulttok, "feerate_floor"); + if (floortok) { + feerates = parse_feerate_ranges(call, call->bitcoind, + buf, floortok, + json_get_member(buf, resulttok, + "feerates"), + &floor); + } else { + feerates = parse_deprecated_feerates(call, call->bitcoind, + buf, resulttok); + floor = feerate_from_style(FEERATE_FLOOR, FEERATE_PER_KSIPA); + } /* Convert to perkw */ floor = feerate_from_style(floor, FEERATE_PER_KBYTE); diff --git a/lightningd/chaintopology.c b/lightningd/chaintopology.c index 3b56c465c87d..78c3344eac11 100644 --- a/lightningd/chaintopology.c +++ b/lightningd/chaintopology.c @@ -610,8 +610,7 @@ u32 penalty_feerate(struct chain_topology *topo) u32 get_feerate_floor(const struct chain_topology *topo) { - /* FIXME: Make this dynamic! */ - return FEERATE_FLOOR; + return topo->feerate_floor; } static struct command_result *json_feerates(struct command *cmd, diff --git a/tests/test_misc.py b/tests/test_misc.py index 05273ff49b05..2acea0323ff3 100644 --- a/tests/test_misc.py +++ b/tests/test_misc.py @@ -1942,9 +1942,8 @@ def mock_fail(*args): @unittest.skipIf(TEST_NETWORK == 'liquid-regtest', "Fees on elements are different") -@unittest.skip("FIXME: temporarily broken") def test_bitcoind_feerate_floor(node_factory, bitcoind): - """Don't return a feerate less than minrelaytxfee/mempoolnifee.""" + """Don't return a feerate less than minrelaytxfee/mempoolminfee.""" l1 = node_factory.get_node() anchors = EXPERIMENTAL_FEATURES @@ -1953,11 +1952,21 @@ def test_bitcoind_feerate_floor(node_factory, bitcoind): "opening": 30000, "mutual_close": 15000, "unilateral_close": 44000, - "delayed_to_us": 30000, - "htlc_resolution": 44000, "penalty": 30000, "min_acceptable": 7500, - "max_acceptable": 600000 + "max_acceptable": 600000, + "estimates": [{"blockcount": 2, + "feerate": 60000, + "smoothed_feerate": 60000}, + {"blockcount": 6, + "feerate": 44000, + "smoothed_feerate": 44000}, + {"blockcount": 12, + "feerate": 30000, + "smoothed_feerate": 30000}, + {"blockcount": 100, + "feerate": 15000, + "smoothed_feerate": 15000}], }, "onchain_fee_estimates": { "opening_channel_satoshis": 5265, @@ -1980,12 +1989,22 @@ def test_bitcoind_feerate_floor(node_factory, bitcoind): # This has increased (rounded up) "mutual_close": 20004, "unilateral_close": 44000, - "delayed_to_us": 30000, - "htlc_resolution": 44000, "penalty": 30000, - # This has increased (rounded up!) - "min_acceptable": 20004, - "max_acceptable": 600000 + # FIXME: this should increase: + "min_acceptable": 10000, + "max_acceptable": 600000, + "estimates": [{"blockcount": 2, + "feerate": 60000, + "smoothed_feerate": 60000}, + {"blockcount": 6, + "feerate": 44000, + "smoothed_feerate": 44000}, + {"blockcount": 12, + "feerate": 30000, + "smoothed_feerate": 30000}, + {"blockcount": 100, + "feerate": 20004, + "smoothed_feerate": 20004}], }, "onchain_fee_estimates": { "opening_channel_satoshis": 5265, @@ -2011,13 +2030,24 @@ def test_bitcoind_feerate_floor(node_factory, bitcoind): "mutual_close": 30004, "unilateral_close": 44000, # This has increased (rounded up!) - "delayed_to_us": 30004, - "htlc_resolution": 44000, - # This has increased (rounded up!) "penalty": 30004, - # This has increased (rounded up!) - "min_acceptable": 30004, - "max_acceptable": 600000 + # FIXME: this should increase to 30004! + "min_acceptable": 15000, + "max_acceptable": 600000, + "estimates": [{"blockcount": 2, + "feerate": 60000, + "smoothed_feerate": 60000}, + {"blockcount": 6, + "feerate": 44000, + "smoothed_feerate": 44000}, + # This has increased (rounded up!) + {"blockcount": 12, + "feerate": 30004, + "smoothed_feerate": 30004}, + # This has increased (rounded up!) + {"blockcount": 100, + "feerate": 30004, + "smoothed_feerate": 30004}], }, "onchain_fee_estimates": { "opening_channel_satoshis": 5265, From 8c0847014dfd0c8ed88fb79d6f5da76b0f86859b Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 7 Apr 2023 14:14:18 +0930 Subject: [PATCH 10/16] plugins/bcli: use the new feerate levels, and the floor. Fixes: #4473 Changelog-Deprecated: Plugins: `estimatefees` returning feerates by name (e.g. "opening"); use `fee_floor` and `feerates`. Changelog-Fixed: Plugins: `bcli` now tells us the minimal possible feerate, such as with mempool congestion, rather than assuming 1 sat/vbyte. --- lightningd/bitcoind.c | 5 ++ plugins/bcli.c | 126 +++++++++++++++++++++++++----------------- tests/test_misc.py | 14 ++--- tests/test_plugin.py | 6 +- 4 files changed, 89 insertions(+), 62 deletions(-) diff --git a/lightningd/bitcoind.c b/lightningd/bitcoind.c index 3db25e2ce9c2..e4bbb1eba25a 100644 --- a/lightningd/bitcoind.c +++ b/lightningd/bitcoind.c @@ -309,6 +309,11 @@ static void estimatefees_callback(const char *buf, const jsmntok_t *toks, "feerates"), &floor); } else { + if (!deprecated_apis) + bitcoin_plugin_error(call->bitcoind, buf, resulttok, + "estimatefees", + "missing fee_floor field"); + feerates = parse_deprecated_feerates(call, call->bitcoind, buf, resulttok); floor = feerate_from_style(FEERATE_FLOOR, FEERATE_PER_KSIPA); diff --git a/plugins/bcli.c b/plugins/bcli.c index 57ab9edf4ffe..a13a939bd041 100644 --- a/plugins/bcli.c +++ b/plugins/bcli.c @@ -456,20 +456,24 @@ static struct command_result *process_getblockchaininfo(struct bitcoin_cli *bcli return command_finished(bcli->cmd, response); } -enum feerate_levels { - FEERATE_HIGHEST, - FEERATE_URGENT, - FEERATE_NORMAL, - FEERATE_SLOW, +struct estimatefee_params { + u32 blocks; + const char *style; +}; + +static const struct estimatefee_params estimatefee_params[] = { + { 2, "CONSERVATIVE" }, + { 6, "ECONOMICAL" }, + { 12, "ECONOMICAL" }, + { 100, "ECONOMICAL" }, }; -#define FEERATE_LEVEL_MAX (FEERATE_SLOW) struct estimatefees_stash { /* This is max(mempoolminfee,minrelaytxfee) */ u64 perkb_floor; u32 cursor; /* FIXME: We use u64 but lightningd will store them as u32. */ - u64 perkb[FEERATE_LEVEL_MAX+1]; + u64 perkb[ARRAY_SIZE(estimatefee_params)]; }; static struct command_result * @@ -477,14 +481,21 @@ estimatefees_null_response(struct bitcoin_cli *bcli) { struct json_stream *response = jsonrpc_stream_success(bcli->cmd); - json_add_null(response, "opening"); - json_add_null(response, "mutual_close"); - json_add_null(response, "unilateral_close"); - json_add_null(response, "delayed_to_us"); - json_add_null(response, "htlc_resolution"); - json_add_null(response, "penalty"); - json_add_null(response, "min_acceptable"); - json_add_null(response, "max_acceptable"); + /* We give a floor, which is the standard minimum */ + json_array_start(response, "feerates"); + json_array_end(response); + json_add_u32(response, "feerate_floor", 1000); + + if (deprecated_apis) { + json_add_null(response, "opening"); + json_add_null(response, "mutual_close"); + json_add_null(response, "unilateral_close"); + json_add_null(response, "delayed_to_us"); + json_add_null(response, "htlc_resolution"); + json_add_null(response, "penalty"); + json_add_null(response, "min_acceptable"); + json_add_null(response, "max_acceptable"); + } return command_finished(bcli->cmd, response); } @@ -658,18 +669,6 @@ static struct command_result *getchaininfo(struct command *cmd, /* Mutual recursion. */ static struct command_result *estimatefees_done(struct bitcoin_cli *bcli); -struct estimatefee_params { - u32 blocks; - const char *style; -}; - -static const struct estimatefee_params estimatefee_params[] = { - [FEERATE_HIGHEST] = { 2, "CONSERVATIVE" }, - [FEERATE_URGENT] = { 6, "ECONOMICAL" }, - [FEERATE_NORMAL] = { 12, "ECONOMICAL" }, - [FEERATE_SLOW] = { 100, "ECONOMICAL" }, -}; - /* Add a feerate, but don't publish one that bitcoind won't accept. */ static void json_add_feerate(struct json_stream *result, const char *fieldname, struct command *cmd, @@ -688,6 +687,16 @@ static void json_add_feerate(struct json_stream *result, const char *fieldname, } } +static u32 feerate_for_block(const struct estimatefees_stash *stash, u32 blocks) +{ + for (size_t i = 0; i < ARRAY_SIZE(stash->perkb); i++) { + if (estimatefee_params[i].blocks != blocks) + continue; + return stash->perkb[i]; + } + abort(); +} + static struct command_result *estimatefees_next(struct command *cmd, struct estimatefees_stash *stash) { @@ -706,30 +715,45 @@ static struct command_result *estimatefees_next(struct command *cmd, } response = jsonrpc_stream_success(cmd); - json_add_feerate(response, "opening", cmd, stash, - stash->perkb[FEERATE_NORMAL]); - json_add_feerate(response, "mutual_close", cmd, stash, - stash->perkb[FEERATE_SLOW]); - json_add_feerate(response, "unilateral_close", cmd, stash, - stash->perkb[FEERATE_URGENT]); - json_add_feerate(response, "delayed_to_us", cmd, stash, - stash->perkb[FEERATE_NORMAL]); - json_add_feerate(response, "htlc_resolution", cmd, stash, - stash->perkb[FEERATE_URGENT]); - json_add_feerate(response, "penalty", cmd, stash, - stash->perkb[FEERATE_NORMAL]); - /* We divide the slow feerate for the minimum acceptable, lightningd - * will use floor if it's hit, though. */ - json_add_feerate(response, "min_acceptable", cmd, stash, - stash->perkb[FEERATE_SLOW] / 2); - /* BOLT #2: - * - * Given the variance in fees, and the fact that the transaction may be - * spent in the future, it's a good idea for the fee payer to keep a good - * margin (say 5x the expected fee requirement) - */ - json_add_feerate(response, "max_acceptable", cmd, stash, - stash->perkb[FEERATE_HIGHEST] * 10); + if (deprecated_apis) { + json_add_feerate(response, "opening", cmd, stash, + feerate_for_block(stash, 12)); + json_add_feerate(response, "mutual_close", cmd, stash, + feerate_for_block(stash, 100)); + json_add_feerate(response, "unilateral_close", cmd, stash, + feerate_for_block(stash, 6)); + json_add_feerate(response, "delayed_to_us", cmd, stash, + feerate_for_block(stash, 12)); + json_add_feerate(response, "htlc_resolution", cmd, stash, + feerate_for_block(stash, 6)); + json_add_feerate(response, "penalty", cmd, stash, + feerate_for_block(stash, 12)); + /* We divide the slow feerate for the minimum acceptable, lightningd + * will use floor if it's hit, though. */ + json_add_feerate(response, "min_acceptable", cmd, stash, + feerate_for_block(stash, 100) / 2); + /* BOLT #2: + * + * Given the variance in fees, and the fact that the transaction may be + * spent in the future, it's a good idea for the fee payer to keep a good + * margin (say 5x the expected fee requirement) + */ + json_add_feerate(response, "max_acceptable", cmd, stash, + feerate_for_block(stash, 2) * 10); + } + + /* Modern style: present an ordered array of block deadlines, and a floor. */ + json_array_start(response, "feerates"); + for (size_t i = 0; i < ARRAY_SIZE(stash->perkb); i++) { + if (!stash->perkb[i]) + continue; + json_object_start(response, NULL); + json_add_u32(response, "blocks", estimatefee_params[i].blocks); + json_add_feerate(response, "feerate", cmd, stash, stash->perkb[i]); + json_object_end(response); + } + json_array_end(response); + json_add_u64(response, "feerate_floor", stash->perkb_floor); return command_finished(cmd, response); } diff --git a/tests/test_misc.py b/tests/test_misc.py index 2acea0323ff3..f06b21c82ec7 100644 --- a/tests/test_misc.py +++ b/tests/test_misc.py @@ -86,11 +86,11 @@ def crash_bitcoincli(r): l1.daemon.rpcproxy.mock_rpc('getblockhash', crash_bitcoincli) # This should cause both estimatefee and getblockhash fail - l1.daemon.wait_for_logs(['Unable to estimate .* fee', + l1.daemon.wait_for_logs(['Unable to estimate any fees', 'getblockhash .* exited with status 1']) # And they should retry! - l1.daemon.wait_for_logs(['Unable to estimate .* fee', + l1.daemon.wait_for_logs(['Unable to estimate any fees', 'getblockhash .* exited with status 1']) # Restore, then it should recover and get blockheight. @@ -1931,7 +1931,7 @@ def mock_fail(*args): l1.daemon.start(wait_for_initialized=False, stderr_redir=True) l1.daemon.wait_for_logs([r'getblockhash [a-z0-9]* exited with status 1', - r'Unable to estimate opening fees', + r'Unable to estimate any fees', r'BROKEN.*we have been retrying command for --bitcoin-retry-timeout={} seconds'.format(timeout)]) # Will exit with failure code. assert l1.daemon.wait() == 1 @@ -1990,8 +1990,8 @@ def test_bitcoind_feerate_floor(node_factory, bitcoind): "mutual_close": 20004, "unilateral_close": 44000, "penalty": 30000, - # FIXME: this should increase: - "min_acceptable": 10000, + # This has increased (rounded up) + "min_acceptable": 20004, "max_acceptable": 600000, "estimates": [{"blockcount": 2, "feerate": 60000, @@ -2031,8 +2031,8 @@ def test_bitcoind_feerate_floor(node_factory, bitcoind): "unilateral_close": 44000, # This has increased (rounded up!) "penalty": 30004, - # FIXME: this should increase to 30004! - "min_acceptable": 15000, + # This has increased (rounded up) + "min_acceptable": 30004, "max_acceptable": 600000, "estimates": [{"blockcount": 2, "feerate": 60000, diff --git a/tests/test_plugin.py b/tests/test_plugin.py index c6e4f1999062..981abb178fc2 100644 --- a/tests/test_plugin.py +++ b/tests/test_plugin.py @@ -1743,10 +1743,8 @@ def test_bcli(node_factory, bitcoind, chainparams): # Failure case of feerate is tested in test_misc.py estimates = l1.rpc.call("estimatefees") - for est in ["opening", "mutual_close", "unilateral_close", "delayed_to_us", - "htlc_resolution", "penalty", "min_acceptable", - "max_acceptable"]: - assert est in estimates + assert 'feerate_floor' in estimates + assert [f['blocks'] for f in estimates['feerates']] == [2, 6, 12, 100] resp = l1.rpc.call("getchaininfo") assert resp["chain"] == chainparams['name'] From d48c97690f1959c1e6d79fd50e2b2bd9fc2025b4 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 7 Apr 2023 14:23:49 +0930 Subject: [PATCH 11/16] feerates: add `floor` field for the current minimum feerate bitcoind will accept Changelog-Added: JSON-RPC: `feerates`: added `floor` field for current minimum feerate bitcoind will accept Signed-off-by: Rusty Russell --- .msggen.json | 10 ++++++++++ cln-grpc/proto/node.proto | 2 ++ cln-grpc/src/convert.rs | 4 ++++ cln-rpc/src/model.rs | 4 ++++ contrib/pyln-testing/pyln/testing/grpc2py.py | 2 ++ doc/lightning-feerates.7.md | 4 +++- doc/schemas/feerates.schema.json | 12 ++++++++++++ lightningd/chaintopology.c | 3 +++ tests/test_misc.py | 17 +++++++++++++---- 9 files changed, 53 insertions(+), 5 deletions(-) diff --git a/.msggen.json b/.msggen.json index 79601bd08a72..e795167369d4 100644 --- a/.msggen.json +++ b/.msggen.json @@ -358,6 +358,7 @@ "FeeratesPerkb": { "Feerates.perkb.delayed_to_us": 6, "Feerates.perkb.estimates[]": 9, + "Feerates.perkb.floor": 10, "Feerates.perkb.htlc_resolution": 7, "Feerates.perkb.max_acceptable": 2, "Feerates.perkb.min_acceptable": 1, @@ -374,6 +375,7 @@ "FeeratesPerkw": { "Feerates.perkw.delayed_to_us": 6, "Feerates.perkw.estimates[]": 9, + "Feerates.perkw.floor": 10, "Feerates.perkw.htlc_resolution": 7, "Feerates.perkw.max_acceptable": 2, "Feerates.perkw.min_acceptable": 1, @@ -1572,6 +1574,10 @@ "added": "v23.05", "deprecated": false }, + "Feerates.perkb.floor": { + "added": "v23.05", + "deprecated": false + }, "Feerates.perkb.htlc_resolution": { "added": "pre-v0.10.1", "deprecated": "v23.05" @@ -1624,6 +1630,10 @@ "added": "v23.05", "deprecated": false }, + "Feerates.perkw.floor": { + "added": "v23.05", + "deprecated": false + }, "Feerates.perkw.htlc_resolution": { "added": "pre-v0.10.1", "deprecated": "v23.05" diff --git a/cln-grpc/proto/node.proto b/cln-grpc/proto/node.proto index 982414fa0741..2ff3e7a37272 100644 --- a/cln-grpc/proto/node.proto +++ b/cln-grpc/proto/node.proto @@ -1130,6 +1130,7 @@ message FeeratesResponse { message FeeratesPerkb { uint32 min_acceptable = 1; uint32 max_acceptable = 2; + optional uint32 floor = 10; repeated FeeratesPerkbEstimates estimates = 9; optional uint32 opening = 3; optional uint32 mutual_close = 4; @@ -1148,6 +1149,7 @@ message FeeratesPerkbEstimates { message FeeratesPerkw { uint32 min_acceptable = 1; uint32 max_acceptable = 2; + optional uint32 floor = 10; repeated FeeratesPerkwEstimates estimates = 9; optional uint32 opening = 3; optional uint32 mutual_close = 4; diff --git a/cln-grpc/src/convert.rs b/cln-grpc/src/convert.rs index 17a456438a88..c5d1eff4564f 100644 --- a/cln-grpc/src/convert.rs +++ b/cln-grpc/src/convert.rs @@ -932,6 +932,7 @@ impl From for pb::FeeratesPerkb { Self { min_acceptable: c.min_acceptable, // Rule #2 for type u32 max_acceptable: c.max_acceptable, // Rule #2 for type u32 + floor: c.floor, // Rule #2 for type u32? estimates: c.estimates.map(|arr| arr.into_iter().map(|i| i.into()).collect()).unwrap_or(vec![]), // Rule #3 opening: c.opening, // Rule #2 for type u32? mutual_close: c.mutual_close, // Rule #2 for type u32? @@ -962,6 +963,7 @@ impl From for pb::FeeratesPerkw { Self { min_acceptable: c.min_acceptable, // Rule #2 for type u32 max_acceptable: c.max_acceptable, // Rule #2 for type u32 + floor: c.floor, // Rule #2 for type u32? estimates: c.estimates.map(|arr| arr.into_iter().map(|i| i.into()).collect()).unwrap_or(vec![]), // Rule #3 opening: c.opening, // Rule #2 for type u32? mutual_close: c.mutual_close, // Rule #2 for type u32? @@ -3297,6 +3299,7 @@ impl From for responses::FeeratesPerkb { Self { min_acceptable: c.min_acceptable, // Rule #1 for type u32 max_acceptable: c.max_acceptable, // Rule #1 for type u32 + floor: c.floor, // Rule #1 for type u32? estimates: Some(c.estimates.into_iter().map(|s| s.into()).collect()), // Rule #4 opening: c.opening, // Rule #1 for type u32? mutual_close: c.mutual_close, // Rule #1 for type u32? @@ -3325,6 +3328,7 @@ impl From for responses::FeeratesPerkw { Self { min_acceptable: c.min_acceptable, // Rule #1 for type u32 max_acceptable: c.max_acceptable, // Rule #1 for type u32 + floor: c.floor, // Rule #1 for type u32? estimates: Some(c.estimates.into_iter().map(|s| s.into()).collect()), // Rule #4 opening: c.opening, // Rule #1 for type u32? mutual_close: c.mutual_close, // Rule #1 for type u32? diff --git a/cln-rpc/src/model.rs b/cln-rpc/src/model.rs index 3b7855863b48..fc065cd5732d 100644 --- a/cln-rpc/src/model.rs +++ b/cln-rpc/src/model.rs @@ -3231,6 +3231,8 @@ pub mod responses { pub struct FeeratesPerkb { pub min_acceptable: u32, pub max_acceptable: u32, + #[serde(skip_serializing_if = "Option::is_none")] + pub floor: Option, #[serde(skip_serializing_if = "crate::is_none_or_empty")] pub estimates: Option>, #[serde(skip_serializing_if = "Option::is_none")] @@ -3263,6 +3265,8 @@ pub mod responses { pub struct FeeratesPerkw { pub min_acceptable: u32, pub max_acceptable: u32, + #[serde(skip_serializing_if = "Option::is_none")] + pub floor: Option, #[serde(skip_serializing_if = "crate::is_none_or_empty")] pub estimates: Option>, #[serde(skip_serializing_if = "Option::is_none")] diff --git a/contrib/pyln-testing/pyln/testing/grpc2py.py b/contrib/pyln-testing/pyln/testing/grpc2py.py index 5d2f1d8dddf7..00229aad6c77 100644 --- a/contrib/pyln-testing/pyln/testing/grpc2py.py +++ b/contrib/pyln-testing/pyln/testing/grpc2py.py @@ -741,6 +741,7 @@ def feerates_perkb2py(m): return remove_default({ "min_acceptable": m.min_acceptable, # PrimitiveField in generate_composite "max_acceptable": m.max_acceptable, # PrimitiveField in generate_composite + "floor": m.floor, # PrimitiveField in generate_composite "estimates": [feerates_perkb_estimates2py(i) for i in m.estimates], # ArrayField[composite] in generate_composite "opening": m.opening, # PrimitiveField in generate_composite "mutual_close": m.mutual_close, # PrimitiveField in generate_composite @@ -763,6 +764,7 @@ def feerates_perkw2py(m): return remove_default({ "min_acceptable": m.min_acceptable, # PrimitiveField in generate_composite "max_acceptable": m.max_acceptable, # PrimitiveField in generate_composite + "floor": m.floor, # PrimitiveField in generate_composite "estimates": [feerates_perkw_estimates2py(i) for i in m.estimates], # ArrayField[composite] in generate_composite "opening": m.opening, # PrimitiveField in generate_composite "mutual_close": m.mutual_close, # PrimitiveField in generate_composite diff --git a/doc/lightning-feerates.7.md b/doc/lightning-feerates.7.md index bcda750e5299..76dcaece31d8 100644 --- a/doc/lightning-feerates.7.md +++ b/doc/lightning-feerates.7.md @@ -50,6 +50,7 @@ On success, an object is returned, containing: - **perkb** (object, optional): If *style* parameter was perkb: - **min\_acceptable** (u32): The smallest feerate that we allow peers to specify: half the 100-block estimate - **max\_acceptable** (u32): The largest feerate we will accept from remote negotiations. If a peer attempts to set the feerate higher than this we will unilaterally close the channel (or simply forget it if it's not open yet). + - **floor** (u32): The smallest feerate that our backend tells us it will accept (i.e. minrelayfee or mempoolminfee) *(added v23.05)* - **estimates** (array of objects): Feerate estimates from plugin which we are using (usuallly bcli) *(added v23.05)*: - **blockcount** (u32): The number of blocks the feerate is expected to get a transaction in *(added v23.05)* - **feerate** (u32): The feerate for this estimate, in given *style* *(added v23.05)* @@ -63,6 +64,7 @@ On success, an object is returned, containing: - **perkw** (object, optional): If *style* parameter was perkw: - **min\_acceptable** (u32): The smallest feerate that you can use, usually the minimum relayed feerate of the backend - **max\_acceptable** (u32): The largest feerate we will accept from remote negotiations. If a peer attempts to set the feerate higher than this we will unilaterally close the channel (or simply forget it if it's not open yet). + - **floor** (u32): The smallest feerate that our backend tells us it will accept (i.e. minrelayfee or mempoolminfee) *(added v23.05)* - **estimates** (array of objects): Feerate estimates from plugin which we are using (usuallly bcli) *(added v23.05)*: - **blockcount** (u32): The number of blocks the feerate is expected to get a transaction in *(added v23.05)* - **feerate** (u32): The feerate for this estimate, in given *style* *(added v23.05)* @@ -136,4 +138,4 @@ RESOURCES Main web site: -[comment]: # ( SHA256STAMP:c21d903c29fd6195d5890962eaa3265a26a57885b95714696916bd32168b66bc) +[comment]: # ( SHA256STAMP:4921275aec48da8b9ddcba5d4237efa72f06b6e005008f2c3aa7029d3bd187fd) diff --git a/doc/schemas/feerates.schema.json b/doc/schemas/feerates.schema.json index 29c45a69ce13..9cff8a8bb53e 100644 --- a/doc/schemas/feerates.schema.json +++ b/doc/schemas/feerates.schema.json @@ -15,6 +15,7 @@ "required": [ "min_acceptable", "max_acceptable", + "floor", "estimates" ], "properties": { @@ -26,6 +27,11 @@ "type": "u32", "description": "The largest feerate we will accept from remote negotiations. If a peer attempts to set the feerate higher than this we will unilaterally close the channel (or simply forget it if it's not open yet)." }, + "floor": { + "type": "u32", + "added": "v23.05", + "description": "The smallest feerate that our backend tells us it will accept (i.e. minrelayfee or mempoolminfee)" + }, "estimates": { "type": "array", "added": "v23.05", @@ -92,6 +98,7 @@ "required": [ "min_acceptable", "max_acceptable", + "floor", "estimates" ], "properties": { @@ -103,6 +110,11 @@ "type": "u32", "description": "The largest feerate we will accept from remote negotiations. If a peer attempts to set the feerate higher than this we will unilaterally close the channel (or simply forget it if it's not open yet)." }, + "floor": { + "type": "u32", + "added": "v23.05", + "description": "The smallest feerate that our backend tells us it will accept (i.e. minrelayfee or mempoolminfee)" + }, "estimates": { "type": "array", "added": "v23.05", diff --git a/lightningd/chaintopology.c b/lightningd/chaintopology.c index 78c3344eac11..d9c76e477a54 100644 --- a/lightningd/chaintopology.c +++ b/lightningd/chaintopology.c @@ -667,6 +667,9 @@ static struct command_result *json_feerates(struct command *cmd, feerate_to_style(feerate_min(cmd->ld, NULL), *style)); json_add_u64(response, "max_acceptable", feerate_to_style(feerate_max(cmd->ld, NULL), *style)); + json_add_u64(response, "floor", + feerate_to_style(get_feerate_floor(cmd->ld->topology), + *style)); json_array_start(response, "estimates"); assert(tal_count(topo->smoothed_feerates) == tal_count(topo->feerates[0])); diff --git a/tests/test_misc.py b/tests/test_misc.py index f06b21c82ec7..6b5dc5d8f5fa 100644 --- a/tests/test_misc.py +++ b/tests/test_misc.py @@ -1532,13 +1532,14 @@ def test_feerates(node_factory): feerate = l1.rpc.parsefeerate(t) # Query feerates (shouldn't give any!) - wait_for(lambda: len(l1.rpc.feerates('perkw')['perkw']) == 3) + wait_for(lambda: len(l1.rpc.feerates('perkw')['perkw']) == 4) 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 assert feerates['perkw']['min_acceptable'] == 253 assert feerates['perkw']['min_acceptable'] == 253 + assert feerates['perkw']['floor'] == 253 assert feerates['perkw']['estimates'] == [] for t in types: assert t not in feerates['perkw'] @@ -1548,6 +1549,8 @@ def test_feerates(node_factory): assert 'perkw' not in feerates assert feerates['perkb']['max_acceptable'] == (2**32 - 1) assert feerates['perkb']['min_acceptable'] == 253 * 4 + # Note: This is floored at the FEERATE_FLOOR constant (253) + assert feerates['perkb']['floor'] == 1012 assert feerates['perkb']['estimates'] == [] for t in types: assert t not in feerates['perkb'] @@ -1955,6 +1958,7 @@ def test_bitcoind_feerate_floor(node_factory, bitcoind): "penalty": 30000, "min_acceptable": 7500, "max_acceptable": 600000, + "floor": 1012, "estimates": [{"blockcount": 2, "feerate": 60000, "smoothed_feerate": 60000}, @@ -1993,6 +1997,7 @@ def test_bitcoind_feerate_floor(node_factory, bitcoind): # This has increased (rounded up) "min_acceptable": 20004, "max_acceptable": 600000, + "floor": 20004, "estimates": [{"blockcount": 2, "feerate": 60000, "smoothed_feerate": 60000}, @@ -2034,6 +2039,7 @@ def test_bitcoind_feerate_floor(node_factory, bitcoind): # This has increased (rounded up) "min_acceptable": 30004, "max_acceptable": 600000, + "floor": 30004, "estimates": [{"blockcount": 2, "feerate": 60000, "smoothed_feerate": 60000}, @@ -2985,7 +2991,8 @@ def test_force_feerates(node_factory): "penalty": 1111, "min_acceptable": 1875, "max_acceptable": 150000, - "estimates": estimates} + "estimates": estimates, + "floor": 253} l1.stop() l1.daemon.opts['force-feerates'] = '1111/2222' @@ -2999,7 +3006,8 @@ def test_force_feerates(node_factory): "penalty": 2222, "min_acceptable": 1875, "max_acceptable": 150000, - "estimates": estimates} + "estimates": estimates, + "floor": 253} l1.stop() l1.daemon.opts['force-feerates'] = '1111/2222/3333/4444/5555/6666' @@ -3013,7 +3021,8 @@ def test_force_feerates(node_factory): "penalty": 6666, "min_acceptable": 1875, "max_acceptable": 150000, - "estimates": estimates} + "estimates": estimates, + "floor": 253} def test_datastore_escapeing(node_factory): From dfcb1e8c70bba3e113cbb27ad17273e19c72a403 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 7 Apr 2023 14:28:17 +0930 Subject: [PATCH 12/16] lightningd: base feerate for onchain txs on deadlines, not fixed fees. --- lightningd/onchain_control.c | 152 ++++++++++++++++++++--------------- 1 file changed, 88 insertions(+), 64 deletions(-) diff --git a/lightningd/onchain_control.c b/lightningd/onchain_control.c index a0803a2094e0..869fe9a8e7d8 100644 --- a/lightningd/onchain_control.c +++ b/lightningd/onchain_control.c @@ -642,6 +642,29 @@ onchain_witness_htlc_tx(const tal_t *ctx, u8 **witness) return cast_const2(const struct onchain_witness_element **, welements); } +/* feerate_for_deadline, but really lowball for distant targets */ +static u32 feerate_for_target(const struct chain_topology *topo, u64 deadline) +{ + u64 blocks, blockheight; + + blockheight = get_block_height(topo); + + /* Past deadline? Want it now. */ + if (blockheight > deadline) + return feerate_for_deadline(topo, 1); + + blocks = deadline - blockheight; + + /* Over 200 blocks, we *always* use min fee! */ + if (blocks > 200) + return FEERATE_FLOOR; + /* Over 100 blocks, use min fee bitcoind will accept */ + if (blocks > 100) + return get_feerate_floor(topo); + + return feerate_for_deadline(topo, blocks); +} + /* Always sets *welements, returns tx. Sets *worthwhile to false if * it wasn't worthwhile at the given feerate (and it had to drop feerate). * Returns NULL iff it called channel_internal_error(). @@ -652,7 +675,6 @@ static struct bitcoin_tx *onchaind_tx(const tal_t *ctx, struct amount_sat out_sats, u32 to_self_delay, u32 locktime, - u32 feerate, u8 *(*sign)(const tal_t *ctx, const struct bitcoin_tx *tx, const struct onchain_signing_info *info), @@ -661,13 +683,14 @@ static struct bitcoin_tx *onchaind_tx(const tal_t *ctx, const struct onchain_witness_element ***welements) { struct bitcoin_tx *tx; - struct amount_sat fee, min_out, amt; + struct amount_sat amt; struct bitcoin_signature sig; size_t weight; u8 *msg; u8 **witness; struct pubkey final_key; struct ext_key final_wallet_ext_key; + u64 block_target; struct lightningd *ld = channel->peer->ld; bip32_pubkey(ld, &final_key, channel->final_key_idx); @@ -692,33 +715,61 @@ static struct bitcoin_tx *onchaind_tx(const tal_t *ctx, /* Worst-case sig is 73 bytes */ weight = bitcoin_tx_weight(tx) + 1 + 3 + 73 + 0 + tal_count(info->wscript); weight += elements_tx_overhead(chainparams, 1, 1); - fee = amount_tx_fee(feerate, weight); - - /* Result is trivial? Spend with small feerate, but don't wait - * around for it as it might not confirm. */ - if (!amount_sat_add(&min_out, channel->our_config.dust_limit, fee)) - fatal("Cannot add dust_limit %s and fee %s", - type_to_string(tmpctx, struct amount_sat, &channel->our_config.dust_limit), - type_to_string(tmpctx, struct amount_sat, &fee)); - - if (amount_sat_less(out_sats, min_out)) { - /* FIXME: We should use SIGHASH_NONE so others can take it? */ - /* Use lowest possible theoretical fee: who cares if it doesn't propagate */ - fee = amount_tx_fee(FEERATE_FLOOR, weight); - *worthwhile = false; - } else - *worthwhile = true; - - /* This can only happen if FEERATE_FLOOR is still too high; shouldn't - * happen! */ - if (!amount_sat_sub(&amt, out_sats, fee)) { - amt = channel->our_config.dust_limit; - log_broken(channel->log, "TX can't afford minimal feerate" - "; setting output to %s", - type_to_string(tmpctx, struct amount_sat, - &amt)); - *worthwhile = false; + + block_target = info->deadline_block; + for (;;) { + struct amount_sat fee; + u32 feerate; + + feerate = feerate_for_target(ld->topology, block_target); + fee = amount_tx_fee(feerate, weight); + + log_debug(channel->log, + "Feerate for target %"PRIu64" (%+"PRId64" blocks) is %u, fee %s of %s", + block_target, + block_target - get_block_height(ld->topology), + feerate, + type_to_string(tmpctx, struct amount_sat, &fee), + type_to_string(tmpctx, struct amount_sat, &out_sats)); + + /* If we can afford fee and it's not dust, we're done */ + if (amount_sat_sub(&amt, out_sats, fee) + && amount_sat_greater_eq(amt, channel->our_config.dust_limit)) + break; + + /* Hmm, can't afford with recommended fee. Try increasing deadline! */ + block_target++; + + /* If we can't even afford at FEERATE_FLOOR, something is wrong! */ + if (feerate == FEERATE_FLOOR) { + amt = channel->our_config.dust_limit; + log_broken(channel->log, "TX can't afford minimal feerate" + "; setting output to %s", + type_to_string(tmpctx, struct amount_sat, &amt)); + break; + } + } + + /* If we anticipate waiting a long time (say, 20 blocks past + * the deadline), tell onchaind not to wait */ + *worthwhile = (block_target < info->deadline_block + (u64)20); + + /* If we came close to target, it's worthwhile to wait for. */ + if (block_target != info->deadline_block) + log_debug(channel->log, "Had to adjust deadline from %u to %"PRIu64" for %s", + info->deadline_block, block_target, + type_to_string(tmpctx, struct amount_sat, &out_sats)); + + if (!*worthwhile) { + log_unusual(channel->log, + "Lowballing feerate for %s sats from %u to %u (deadline %u->%"PRIu64"):" + " won't count on it being spent!", + type_to_string(tmpctx, struct amount_sat, &out_sats), + feerate_for_target(ld->topology, info->deadline_block), + feerate_for_target(ld->topology, block_target), + info->deadline_block, block_target); } + bitcoin_tx_output_set_amount(tx, 0, amt); bitcoin_tx_finalize(tx); @@ -798,7 +849,6 @@ static void create_onchain_tx(struct channel *channel, struct amount_sat out_sats, u32 to_self_delay, u32 locktime, - u32 initial_feerate, u8 *(*sign)(const tal_t *ctx, const struct bitcoin_tx *tx, const struct onchain_signing_info *info), @@ -808,9 +858,10 @@ static void create_onchain_tx(struct channel *channel, struct bitcoin_tx *tx; const struct onchain_witness_element **welements; bool worthwhile; + struct lightningd *ld = channel->peer->ld; tx = onchaind_tx(tmpctx, channel, - out, out_sats, to_self_delay, locktime, initial_feerate, + out, out_sats, to_self_delay, locktime, sign, info, &worthwhile, &welements); if (!tx) return; @@ -819,7 +870,7 @@ static void create_onchain_tx(struct channel *channel, type_to_string(tmpctx, struct bitcoin_tx, tx), worthwhile ? "" : "(NOT WORTHWHILE, LOWBALL FEE!)"); - broadcast_tx(channel->peer->ld->topology, + broadcast_tx(ld->topology, channel, take(tx), NULL, false, info->minblock, NULL, consider_onchain_rebroadcast, take(info)); @@ -832,11 +883,9 @@ static void create_onchain_tx(struct channel *channel, static void handle_onchaind_spend_to_us(struct channel *channel, const u8 *msg) { - struct lightningd *ld = channel->peer->ld; struct onchain_signing_info *info; struct bitcoin_outpoint out; struct amount_sat out_sats; - u32 initial_feerate; info = new_signing_info(msg, channel, WIRE_ONCHAIND_SPEND_TO_US); @@ -870,27 +919,20 @@ static void handle_onchaind_spend_to_us(struct channel *channel, return; } - /* FIXME: Be more sophisticated! */ - initial_feerate = delayed_to_us_feerate(ld->topology); - if (!initial_feerate) - initial_feerate = tx_feerate(channel->last_tx); - /* No real deadline on this, it's just returning to our wallet. */ - info->deadline_block = infinite_block_deadline(ld->topology); + info->deadline_block = infinite_block_deadline(channel->peer->ld->topology); create_onchain_tx(channel, &out, out_sats, channel->channel_info.their_config.to_self_delay, 0, - initial_feerate, sign_tx_to_us, info, + sign_tx_to_us, info, __func__); } static void handle_onchaind_spend_penalty(struct channel *channel, const u8 *msg) { - struct lightningd *ld = channel->peer->ld; struct onchain_signing_info *info; struct bitcoin_outpoint out; struct amount_sat out_sats; - u32 initial_feerate; u8 *stack_elem; info = new_signing_info(msg, channel, WIRE_ONCHAIND_SPEND_PENALTY); @@ -908,11 +950,6 @@ static void handle_onchaind_spend_penalty(struct channel *channel, /* info->stack_elem is const void * */ info->stack_elem = stack_elem; - /* FIXME: Be more sophisticated! */ - initial_feerate = penalty_feerate(ld->topology); - if (!initial_feerate) - initial_feerate = tx_feerate(channel->last_tx); - /* FIXME: deadline for HTLCs is actually a bit longer, but for * their output it's channel->our_config.to_self_delay after * the commitment tx is mined. */ @@ -920,19 +957,17 @@ static void handle_onchaind_spend_penalty(struct channel *channel, + channel->our_config.to_self_delay; create_onchain_tx(channel, &out, out_sats, 0, 0, - initial_feerate, sign_penalty, info, + sign_penalty, info, __func__); } static void handle_onchaind_spend_fulfill(struct channel *channel, const u8 *msg) { - struct lightningd *ld = channel->peer->ld; struct onchain_signing_info *info; struct bitcoin_outpoint out; struct amount_sat out_sats; struct preimage preimage; - u32 initial_feerate; u64 htlc_id; const bool anchor_outputs = channel_has(channel, OPT_ANCHOR_OUTPUTS); @@ -951,11 +986,6 @@ static void handle_onchaind_spend_fulfill(struct channel *channel, } info->stack_elem = tal_dup(info, struct preimage, &preimage); - /* FIXME: Be more sophisticated! */ - initial_feerate = htlc_resolution_feerate(ld->topology); - if (!initial_feerate) - initial_feerate = tx_feerate(channel->last_tx); - info->deadline_block = htlc_incoming_deadline(channel, htlc_id); /* BOLT #3: * @@ -965,7 +995,7 @@ static void handle_onchaind_spend_fulfill(struct channel *channel, create_onchain_tx(channel, &out, out_sats, anchor_outputs ? 1 : 0, 0, - initial_feerate, sign_fulfill, info, + sign_fulfill, info, __func__); } @@ -1122,12 +1152,11 @@ static void handle_onchaind_spend_htlc_timeout(struct channel *channel, static void handle_onchaind_spend_htlc_expired(struct channel *channel, const u8 *msg) { - struct lightningd *ld = channel->peer->ld; struct onchain_signing_info *info; struct bitcoin_outpoint out; struct amount_sat out_sats; u64 htlc_id; - u32 cltv_expiry, initial_feerate; + u32 cltv_expiry; const bool anchor_outputs = channel_has(channel, OPT_ANCHOR_OUTPUTS); info = new_signing_info(msg, channel, WIRE_ONCHAIND_SPEND_HTLC_EXPIRED); @@ -1158,17 +1187,12 @@ static void handle_onchaind_spend_htlc_expired(struct channel *channel, /* nLocktime: we have to be *after* that block! */ info->minblock = cltv_expiry + 1; - /* FIXME: Be more sophisticated! */ - initial_feerate = htlc_resolution_feerate(ld->topology); - if (!initial_feerate) - initial_feerate = tx_feerate(channel->last_tx); - /* We have to spend it before we can close incoming */ info->deadline_block = htlc_outgoing_incoming_deadline(channel, htlc_id); create_onchain_tx(channel, &out, out_sats, anchor_outputs ? 1 : 0, cltv_expiry, - initial_feerate, sign_htlc_expired, info, + sign_htlc_expired, info, __func__); } From 17d84bb06c45ab7599fbf32971105df6b8643a26 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 7 Apr 2023 14:29:58 +0930 Subject: [PATCH 13/16] lightningd: split the simple onchain tx signing code. Splitting into onchaind_tx() into onchaind_tx_unsigned() and sign_and_get_witness() makes it easier to reuse for RBF. Adding more information in onchain_signing_info is required too. Signed-off-by: Rusty Russell --- lightningd/onchain_control.c | 145 +++++++++++++++++++++++------------ 1 file changed, 95 insertions(+), 50 deletions(-) diff --git a/lightningd/onchain_control.c b/lightningd/onchain_control.c index 869fe9a8e7d8..a90c9212407a 100644 --- a/lightningd/onchain_control.c +++ b/lightningd/onchain_control.c @@ -466,6 +466,16 @@ struct onchain_signing_info { /* Trailing element for witness stack */ const tal_t *stack_elem; + /* Information for consider_onchain_rebroadcast */ + struct amount_sat fee; + struct bitcoin_outpoint out; + struct amount_sat out_sats; + u32 to_self_delay; + u32 locktime; + u8 *(*sign)(const tal_t *ctx, + const struct bitcoin_tx *tx, + const struct onchain_signing_info *info); + /* Tagged union (for sanity checking!) */ enum onchaind_wire msgtype; union { @@ -665,29 +675,21 @@ static u32 feerate_for_target(const struct chain_topology *topo, u64 deadline) return feerate_for_deadline(topo, blocks); } -/* Always sets *welements, returns tx. Sets *worthwhile to false if - * it wasn't worthwhile at the given feerate (and it had to drop feerate). - * Returns NULL iff it called channel_internal_error(). - */ -static struct bitcoin_tx *onchaind_tx(const tal_t *ctx, - struct channel *channel, - const struct bitcoin_outpoint *out, - struct amount_sat out_sats, - u32 to_self_delay, - u32 locktime, - u8 *(*sign)(const tal_t *ctx, - const struct bitcoin_tx *tx, - const struct onchain_signing_info *info), - const struct onchain_signing_info *info, - bool *worthwhile, - const struct onchain_witness_element ***welements) +/* Make normal 1-input-1-output tx to us, but don't sign it yet. + * + * If worthwhile is not NULL, we set it to true normally, or false if + * we had to lower fees so much it's unlikely to get mined + * (i.e. "don't wait up!"). +*/ +static struct bitcoin_tx *onchaind_tx_unsigned(const tal_t *ctx, + struct channel *channel, + const struct onchain_signing_info *info, + struct amount_sat *fee, + bool *worthwhile) { struct bitcoin_tx *tx; struct amount_sat amt; - struct bitcoin_signature sig; size_t weight; - u8 *msg; - u8 **witness; struct pubkey final_key; struct ext_key final_wallet_ext_key; u64 block_target; @@ -704,12 +706,12 @@ static struct bitcoin_tx *onchaind_tx(const tal_t *ctx, return NULL; } - tx = bitcoin_tx(ctx, chainparams, 1, 1, locktime); - bitcoin_tx_add_input(tx, out, to_self_delay, - NULL, out_sats, NULL, info->wscript); + tx = bitcoin_tx(ctx, chainparams, 1, 1, info->locktime); + bitcoin_tx_add_input(tx, &info->out, info->to_self_delay, + NULL, info->out_sats, NULL, info->wscript); bitcoin_tx_add_output( - tx, scriptpubkey_p2wpkh(tmpctx, &final_key), NULL, out_sats); + tx, scriptpubkey_p2wpkh(tmpctx, &final_key), NULL, info->out_sats); psbt_add_keypath_to_last_output(tx, channel->final_key_idx, &final_wallet_ext_key); /* Worst-case sig is 73 bytes */ @@ -718,22 +720,22 @@ static struct bitcoin_tx *onchaind_tx(const tal_t *ctx, block_target = info->deadline_block; for (;;) { - struct amount_sat fee; u32 feerate; feerate = feerate_for_target(ld->topology, block_target); - fee = amount_tx_fee(feerate, weight); + *fee = amount_tx_fee(feerate, weight); log_debug(channel->log, "Feerate for target %"PRIu64" (%+"PRId64" blocks) is %u, fee %s of %s", block_target, block_target - get_block_height(ld->topology), feerate, - type_to_string(tmpctx, struct amount_sat, &fee), - type_to_string(tmpctx, struct amount_sat, &out_sats)); + type_to_string(tmpctx, struct amount_sat, fee), + type_to_string(tmpctx, struct amount_sat, + &info->out_sats)); /* If we can afford fee and it's not dust, we're done */ - if (amount_sat_sub(&amt, out_sats, fee) + if (amount_sat_sub(&amt, info->out_sats, *fee) && amount_sat_greater_eq(amt, channel->our_config.dust_limit)) break; @@ -743,6 +745,8 @@ static struct bitcoin_tx *onchaind_tx(const tal_t *ctx, /* If we can't even afford at FEERATE_FLOOR, something is wrong! */ if (feerate == FEERATE_FLOOR) { amt = channel->our_config.dust_limit; + /* Not quite true, but Never Happens */ + *fee = AMOUNT_SAT(0); log_broken(channel->log, "TX can't afford minimal feerate" "; setting output to %s", type_to_string(tmpctx, struct amount_sat, &amt)); @@ -752,38 +756,71 @@ static struct bitcoin_tx *onchaind_tx(const tal_t *ctx, /* If we anticipate waiting a long time (say, 20 blocks past * the deadline), tell onchaind not to wait */ - *worthwhile = (block_target < info->deadline_block + (u64)20); + if (worthwhile) { + *worthwhile = (block_target < info->deadline_block + (u64)20); + if (!*worthwhile) { + log_unusual(channel->log, + "Lowballing feerate for %s sats from %u to %u (deadline %u->%"PRIu64"):" + " won't count on it being spent!", + type_to_string(tmpctx, struct amount_sat, &info->out_sats), + feerate_for_target(ld->topology, info->deadline_block), + feerate_for_target(ld->topology, block_target), + info->deadline_block, block_target); + } + } /* If we came close to target, it's worthwhile to wait for. */ if (block_target != info->deadline_block) log_debug(channel->log, "Had to adjust deadline from %u to %"PRIu64" for %s", info->deadline_block, block_target, - type_to_string(tmpctx, struct amount_sat, &out_sats)); - - if (!*worthwhile) { - log_unusual(channel->log, - "Lowballing feerate for %s sats from %u to %u (deadline %u->%"PRIu64"):" - " won't count on it being spent!", - type_to_string(tmpctx, struct amount_sat, &out_sats), - feerate_for_target(ld->topology, info->deadline_block), - feerate_for_target(ld->topology, block_target), - info->deadline_block, block_target); - } - + type_to_string(tmpctx, struct amount_sat, &info->out_sats)); bitcoin_tx_output_set_amount(tx, 0, amt); bitcoin_tx_finalize(tx); - /* Now sign, and set witness */ - msg = sign(NULL, tx, info); + return tx; +} + +static u8 **sign_and_get_witness(const tal_t *ctx, + const struct channel *channel, + struct bitcoin_tx *tx, + const struct onchain_signing_info *info) +{ + u8 *msg; + struct bitcoin_signature sig; + struct lightningd *ld = channel->peer->ld; + + msg = info->sign(NULL, tx, info); if (!wire_sync_write(ld->hsm_fd, take(msg))) fatal("Writing sign request to hsm"); msg = wire_sync_read(tmpctx, ld->hsm_fd); if (!msg || !fromwire_hsmd_sign_tx_reply(msg, &sig)) fatal("Reading sign_tx_reply: %s", tal_hex(tmpctx, msg)); - witness = bitcoin_witness_sig_and_element(NULL, &sig, info->stack_elem, - tal_bytelen(info->stack_elem), - info->wscript); + return bitcoin_witness_sig_and_element(ctx, &sig, info->stack_elem, + tal_bytelen(info->stack_elem), + info->wscript); +} + +/* Always sets *welements, returns tx. Sets *worthwhile to false if + * it wasn't worthwhile at the given feerate (and it had to drop feerate). + * Returns NULL iff it called channel_internal_error(). + */ +static struct bitcoin_tx *onchaind_tx(const tal_t *ctx, + struct channel *channel, + const struct onchain_signing_info *info, + struct amount_sat *fee, + bool *worthwhile, + const struct onchain_witness_element ***welements) +{ + struct bitcoin_tx *tx; + u8 **witness; + + tx = onchaind_tx_unsigned(ctx, channel, info, fee, worthwhile); + if (!tx) + return NULL; + + /* Now sign, and set witness */ + witness = sign_and_get_witness(NULL, channel, tx, info); *welements = onchain_witness_sig_and_element(ctx, witness); bitcoin_tx_input_set_witness(tx, 0, take(witness)); @@ -860,11 +897,19 @@ static void create_onchain_tx(struct channel *channel, bool worthwhile; struct lightningd *ld = channel->peer->ld; - tx = onchaind_tx(tmpctx, channel, - out, out_sats, to_self_delay, locktime, - sign, info, &worthwhile, &welements); - if (!tx) + /* Save these in case we need to RBF. We could extract from + * tx, but this is clearer and simpler. */ + info->out = *out; + info->out_sats = out_sats; + info->to_self_delay = to_self_delay; + info->locktime = locktime; + info->sign = sign; + + tx = onchaind_tx(tmpctx, channel, info, &info->fee, &worthwhile, &welements); + if (!tx) { + tal_free(info); return; + } log_debug(channel->log, "Broadcast for onchaind tx %s%s", type_to_string(tmpctx, struct bitcoin_tx, tx), From 0c4a63db46c1fe80be4cefef7dc4ca430595bd5a Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 7 Apr 2023 14:30:01 +0930 Subject: [PATCH 14/16] lightningd: remember if they set "allowhighfees" when we rebroadcast. We would only set it the first time, which was OK for how we were using it before. Now we want to also set it for rebroadcast. Signed-off-by: Rusty Russell --- lightningd/chaintopology.c | 8 +++++++- lightningd/chaintopology.h | 1 + 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/lightningd/chaintopology.c b/lightningd/chaintopology.c index d9c76e477a54..916ee4550f9f 100644 --- a/lightningd/chaintopology.c +++ b/lightningd/chaintopology.c @@ -122,6 +122,9 @@ struct txs_to_broadcast { /* IDs to attach to each tx (could be NULL!) */ const char **cmd_id; + + /* allowhighfees flags for each tx */ + bool *allowhighfees; }; /* We just sent the last entry in txs[]. Shrink and send the next last. */ @@ -143,7 +146,7 @@ static void broadcast_remainder(struct bitcoind *bitcoind, /* Broadcast next one. */ bitcoind_sendrawtx(bitcoind, txs->cmd_id[txs->cursor], txs->txs[txs->cursor], - false, + txs->allowhighfees[txs->cursor], broadcast_remainder, txs); } @@ -162,6 +165,7 @@ static void rebroadcast_txs(struct chain_topology *topo) /* Put any txs we want to broadcast in ->txs. */ txs->txs = tal_arr(txs, const char *, 0); + txs->allowhighfees = tal_arr(txs, bool, 0); for (otx = outgoing_tx_map_first(topo->outgoing_txs, &it); otx; otx = outgoing_tx_map_next(topo->outgoing_txs, &it)) { @@ -181,6 +185,7 @@ static void rebroadcast_txs(struct chain_topology *topo) } tal_arr_expand(&txs->txs, fmt_bitcoin_tx(txs->txs, otx->tx)); + tal_arr_expand(&txs->allowhighfees, otx->allowhighfees); tal_arr_expand(&txs->cmd_id, otx->cmd_id ? tal_strdup(txs, otx->cmd_id) : NULL); } @@ -252,6 +257,7 @@ void broadcast_tx_(struct chain_topology *topo, bitcoin_txid(tx, &otx->txid); otx->tx = clone_bitcoin_tx(otx, tx); otx->minblock = minblock; + otx->allowhighfees = allowhighfees; otx->finished = finished; otx->refresh = refresh; otx->refresh_arg = refresh_arg; diff --git a/lightningd/chaintopology.h b/lightningd/chaintopology.h index a90b50c23100..faddc491677f 100644 --- a/lightningd/chaintopology.h +++ b/lightningd/chaintopology.h @@ -22,6 +22,7 @@ struct outgoing_tx { const struct bitcoin_tx *tx; struct bitcoin_txid txid; u32 minblock; + bool allowhighfees; const char *cmd_id; void (*finished)(struct channel *channel, bool success, const char *err); bool (*refresh)(struct channel *, const struct bitcoin_tx **, void *arg); From 64fb4f09b15ab1457274f609013105ea221cc5bb Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 7 Apr 2023 14:30:01 +0930 Subject: [PATCH 15/16] lightningd: rebroadcast all pending txs each 30-60 seconds. We also do it on every block, but since bitcoind can't always be counted to rebroadcast for us, we might as well be aggressive! Signed-off-by: Rusty Russell --- lightningd/chaintopology.c | 8 ++++++++ lightningd/chaintopology.h | 2 +- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/lightningd/chaintopology.c b/lightningd/chaintopology.c index 916ee4550f9f..b956508c6d8c 100644 --- a/lightningd/chaintopology.c +++ b/lightningd/chaintopology.c @@ -191,6 +191,13 @@ static void rebroadcast_txs(struct chain_topology *topo) } tal_free(cleanup_ctx); + /* Free explicitly in case we were called because a block came in. + * Then set a new timer 30-60 seconds away */ + tal_free(topo->rebroadcast_timer); + topo->rebroadcast_timer = new_reltimer(topo->ld->timers, topo, + time_from_sec(30 + pseudorand(30)), + rebroadcast_txs, topo); + /* Let this do the dirty work. */ txs->cursor = (size_t)-1; broadcast_remainder(topo->bitcoind, true, "", txs); @@ -1162,6 +1169,7 @@ struct chain_topology *new_topology(struct lightningd *ld, struct log *log) topo->root = NULL; topo->sync_waiters = tal(topo, struct list_head); topo->extend_timer = NULL; + topo->rebroadcast_timer = NULL; topo->stopping = false; list_head_init(topo->sync_waiters); diff --git a/lightningd/chaintopology.h b/lightningd/chaintopology.h index faddc491677f..4c7ce628a9fe 100644 --- a/lightningd/chaintopology.h +++ b/lightningd/chaintopology.h @@ -133,7 +133,7 @@ struct chain_topology { struct bitcoind *bitcoind; /* Timers we're running. */ - struct oneshot *extend_timer, *updatefee_timer; + struct oneshot *extend_timer, *updatefee_timer, *rebroadcast_timer; /* Bitcoin transactions we're broadcasting */ struct outgoing_tx_map *outgoing_txs; From c31153cc5c1c367501e8b29dc8eb10155adfa631 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Sun, 9 Apr 2023 14:30:28 +0930 Subject: [PATCH 16/16] lightningd: do RBF again for all the txs. Now we've set everything up, the replacement code is quite simple. Some tests now have to deal with RBF though, and our rbf tests need work since they look for the old onchaind messages. In particular, when we can't afford the fee we want, we back off to the next blockcount estimate, rather than spending all on fees (necessarily). So test_penalty_rbf_burn no longer applies. Changelog-Changed: Protocol: spending unilateral close transactions now use dynamic fees based on deadlines (and RBF), instead of fixed fees. Signed-off-by: Rusty Russell --- contrib/pyln-testing/pyln/testing/utils.py | 24 +++ lightningd/onchain_control.c | 38 +++- tests/test_closing.py | 229 ++++----------------- tests/test_pay.py | 3 +- tests/test_plugin.py | 3 +- 5 files changed, 100 insertions(+), 197 deletions(-) diff --git a/contrib/pyln-testing/pyln/testing/utils.py b/contrib/pyln-testing/pyln/testing/utils.py index 5f023b37b6dd..1ed1b35bc022 100644 --- a/contrib/pyln-testing/pyln/testing/utils.py +++ b/contrib/pyln-testing/pyln/testing/utils.py @@ -1228,6 +1228,30 @@ def wait_for_onchaind_txs(self, *args): def wait_for_onchaind_tx(self, name, resolve): return self.wait_for_onchaind_txs((name, resolve))[0] + def mine_txid_or_rbf(self, txid, numblocks=1): + """Wait for a txid to be broadcast, or an rbf. Return the one actually mined""" + # Hack so we can mutate the txid: pass it in a list + def rbf_or_txid_broadcast(txids): + # RBF onchain txid d4b597505b543a4b8b42ab4d481fd7a533febb7e7df150ca70689e6d046612f7 (fee 6564sat) with txid 979878b8f855d3895d1cd29bd75a60b21492c4842e38099186a8e649bee02c7c (fee 8205sat) + line = self.daemon.is_in_log("RBF onchain txid {}".format(txids[-1])) + if line is not None: + newtxid = re.search(r'with txid ([0-9a-fA-F]*)', line).group(1) + txids.append(newtxid) + mempool = self.bitcoin.rpc.getrawmempool() + return any([t in mempool for t in txids]) + + txids = [txid] + wait_for(lambda: rbf_or_txid_broadcast(txids)) + blocks = self.bitcoin.generate_block(numblocks) + + # It might have snuck an RBF in at the last minute! + rbf_or_txid_broadcast(txids) + + for tx in self.bitcoin.rpc.getblock(blocks[0])['tx']: + if tx in txids: + return tx + raise ValueError("None of the rbf txs were mined?") + def wait_for_onchaind_broadcast(self, name, resolve=None): """Wait for onchaind to drop tx name to resolve (if any)""" if resolve: diff --git a/lightningd/onchain_control.c b/lightningd/onchain_control.c index a90c9212407a..71062245bd5e 100644 --- a/lightningd/onchain_control.c +++ b/lightningd/onchain_control.c @@ -831,7 +831,39 @@ static bool consider_onchain_rebroadcast(struct channel *channel, const struct bitcoin_tx **tx, struct onchain_signing_info *info) { - /* FIXME: Implement rbf! */ + struct bitcoin_tx *newtx; + struct amount_sat newfee; + struct bitcoin_txid oldtxid, newtxid; + u8 **witness; + + newtx = onchaind_tx_unsigned(tmpctx, channel, info, &newfee, NULL); + if (!newtx) + return true; + + /* FIXME: Don't RBF if fee is not sufficiently increased? */ + + /* OK! RBF time! */ + witness = sign_and_get_witness(NULL, channel, newtx, info); + bitcoin_tx_input_set_witness(newtx, 0, take(witness)); + + bitcoin_txid(newtx, &newtxid); + bitcoin_txid(*tx, &oldtxid); + log_info(channel->log, + "RBF onchain txid %s (fee %s) with txid %s (fee %s)", + type_to_string(tmpctx, struct bitcoin_txid, &oldtxid), + fmt_amount_sat(tmpctx, info->fee), + type_to_string(tmpctx, struct bitcoin_txid, &newtxid), + fmt_amount_sat(tmpctx, newfee)); + log_debug(channel->log, + "RBF %s->%s", + type_to_string(tmpctx, struct bitcoin_tx, *tx), + type_to_string(tmpctx, struct bitcoin_tx, newtx)); + + /* FIXME: This is ugly, but we want the same parent as old tx. */ + tal_steal(tal_parent(*tx), newtx); + tal_free(*tx); + *tx = newtx; + info->fee = newfee; return true; } @@ -915,8 +947,10 @@ static void create_onchain_tx(struct channel *channel, type_to_string(tmpctx, struct bitcoin_tx, tx), worthwhile ? "" : "(NOT WORTHWHILE, LOWBALL FEE!)"); + /* We allow "excessive" fees, as we may be fighting with censors and + * we'd rather spend fees than have our adversary win. */ broadcast_tx(ld->topology, - channel, take(tx), NULL, false, info->minblock, + channel, take(tx), NULL, true, info->minblock, NULL, consider_onchain_rebroadcast, take(info)); subd_send_msg(channel->owner, diff --git a/tests/test_closing.py b/tests/test_closing.py index 736dbf610770..a58a5189aa79 100644 --- a/tests/test_closing.py +++ b/tests/test_closing.py @@ -95,50 +95,14 @@ def test_closing_simple(node_factory, bitcoind, chainparams): 'ONCHAIN:All outputs resolved: waiting 90 more blocks before forgetting channel' ]) - # Capture both side's image of channel before it's dead. - l1channel = only_one(l1.rpc.listpeerchannels(l2.info['id'])['channels']) - l2channel = only_one(l2.rpc.listpeerchannels(l1.info['id'])['channels']) - # Make sure both have forgotten about it bitcoind.generate_block(90) - wait_for(lambda: len(l1.rpc.listpeerchannels()['channels']) == 0) - wait_for(lambda: len(l2.rpc.listpeerchannels()['channels']) == 0) + wait_for(lambda: len(l1.rpc.listchannels()['channels']) == 0) + wait_for(lambda: len(l2.rpc.listchannels()['channels']) == 0) # The entry in the channels table should still be there assert l1.db_query("SELECT count(*) as c FROM channels;")[0]['c'] == 1 assert l2.db_query("SELECT count(*) as c FROM channels;")[0]['c'] == 1 - assert l1.db_query("SELECT count(*) as p FROM peers;")[0]['p'] == 1 - assert l2.db_query("SELECT count(*) as p FROM peers;")[0]['p'] == 1 - - # Test listclosedchannels is correct. - l1closedchannel = only_one(l1.rpc.listclosedchannels()['closedchannels']) - l2closedchannel = only_one(l2.rpc.listclosedchannels(l1.info['id'])['closedchannels']) - - # These fields do not appear in listpeerchannels! - l1_only_closed = {'total_local_commitments': 2, - 'total_remote_commitments': 2, - 'total_htlcs_sent': 1, - 'leased': False, - 'close_cause': 'user'} - l2_only_closed = {'total_local_commitments': 2, - 'total_remote_commitments': 2, - 'total_htlcs_sent': 0, - 'leased': False, - 'close_cause': 'remote'} - - # These fields have different names - renamed = {'last_commitment_txid': 'scratch_txid', - 'last_commitment_fee_msat': 'last_tx_fee_msat', - 'final_to_us_msat': 'to_us_msat'} - - for chan, closedchan, onlyclosed in (l1channel, l1closedchannel, l1_only_closed), (l2channel, l2closedchannel, l2_only_closed): - for k, v in closedchan.items(): - if k in renamed: - assert chan[renamed[k]] == v - elif k in onlyclosed: - assert closedchan[k] == onlyclosed[k] - else: - assert chan[k] == v assert account_balance(l1, channel_id) == 0 assert account_balance(l2, channel_id) == 0 @@ -1328,8 +1292,8 @@ def test_penalty_htlc_tx_fulfill(node_factory, bitcoind, chainparams): assert blocks == 0 txids.append(txid) - # First one is already spent by their fulfill attempt - bitcoind.generate_block(1, wait_for_mempool=txids[1:]) + # First one is already spent by their fulfill attempt. Others may be RBF! + bitcoind.generate_block(1, len(txids[1:])) l3.daemon.wait_for_log('Resolved OUR_HTLC_FULFILL_TO_THEM/DELAYED_CHEAT_OUTPUT_TO_THEM ' 'by our proposal OUR_PENALTY_TX') l2.daemon.wait_for_log('Unknown spend of OUR_HTLC_SUCCESS_TX/DELAYED_OUTPUT_TO_US') @@ -1598,7 +1562,6 @@ def test_penalty_htlc_tx_timeout(node_factory, bitcoind, chainparams): assert acc['resolved_at_block'] > 0 -@pytest.mark.xfail(strict=True) @pytest.mark.developer("uses dev_sign_last_tx") def test_penalty_rbf_normal(node_factory, bitcoind, executor, chainparams): ''' @@ -1664,40 +1627,46 @@ def censoring_sendrawtx(r): # l2 notices. l2.daemon.wait_for_log(' to ONCHAIN') - def get_rbf_tx(self, depth, name, resolve): - r = self.daemon.wait_for_log('Broadcasting RBF {} .* to resolve {} depth={}' - .format(name, resolve, depth)) - return re.search(r'.* \(([0-9a-fA-F]*)\)', r).group(1) + ((_, txid1, blocks1), (_, txid2, blocks2)) = \ + l2.wait_for_onchaind_txs(('OUR_PENALTY_TX', + 'THEIR_REVOKED_UNILATERAL/THEIR_HTLC'), + ('OUR_PENALTY_TX', + 'THEIR_REVOKED_UNILATERAL/DELAYED_CHEAT_OUTPUT_TO_THEM')) + assert blocks1 == 0 + assert blocks2 == 0 + + def get_rbf_txid(node, txid): + line = node.daemon.wait_for_log("RBF onchain .*{}".format(txid)) + newtxid = re.search(r'with txid ([0-9a-fA-F]*)', line).group(1) + return newtxid - rbf_txes = [] # Now the censoring miners generate some blocks. - for depth in range(2, 8): + for depth in range(2, 10): bitcoind.generate_block(1) - sync_blockheight(bitcoind, [l2]) # l2 should RBF, twice even, one for the l1 main output, # one for the l1 HTLC output. - rbf_txes.append(get_rbf_tx(l2, depth, - 'OUR_PENALTY_TX', - 'THEIR_REVOKED_UNILATERAL/THEIR_HTLC')) - rbf_txes.append(get_rbf_tx(l2, depth, - 'OUR_PENALTY_TX', - 'THEIR_REVOKED_UNILATERAL/DELAYED_CHEAT_OUTPUT_TO_THEM')) + # Don't assume a specific order! + start = l2.daemon.logsearch_start + txid1 = get_rbf_txid(l2, txid1) + l2.daemon.logsearch_start = start + txid2 = get_rbf_txid(l2, txid2) # Now that the transactions have high fees, independent miners # realize they can earn potentially more money by grabbing the # high-fee censored transactions, and fresh, non-censoring # hashpower arises, evicting the censor. l2.daemon.rpcproxy.mock_rpc('sendrawtransaction', None) + bitcoind.generate_block(1) - # Check that the order in which l2 generated RBF transactions - # would be acceptable to Bitcoin. - for tx in rbf_txes: - # Use the bcli interface as well, so that we also check the - # bcli interface. - l2.rpc.call('sendrawtransaction', [tx, True]) + # This triggers the final RBF attempt + start = l2.daemon.logsearch_start + txid1 = get_rbf_txid(l2, txid1) + l2.daemon.logsearch_start = start + txid2 = get_rbf_txid(l2, txid2) # Now the non-censoring miners overpower the censoring miners. - bitcoind.generate_block(1) + # FIXME: Some of those RBFs may not be accepted by bitcoind, so just check number in mempool. + bitcoind.generate_block(1, wait_for_mempool=len([txid1, txid2])) sync_blockheight(bitcoind, [l2]) # And l2 should consider it resolved now. @@ -1723,134 +1692,6 @@ def get_rbf_tx(self, depth, name, resolve): check_utxos_channel(l2, [channel_id], expected_2) -@pytest.mark.xfail(strict=True) -@pytest.mark.developer("uses dev_sign_last_tx") -def test_penalty_rbf_burn(node_factory, bitcoind, executor, chainparams): - ''' - Test that penalty transactions are RBFed and we are willing to burn - it all up to spite the thief. - ''' - # We track channel balances, to verify that accounting is ok. - coin_mvt_plugin = os.path.join(os.getcwd(), 'tests/plugins/coin_movements.py') - to_self_delay = 10 - # l1 is the thief, which causes our honest upstanding lightningd - # code to break, so l1 can fail. - # Initially, disconnect before the HTLC can be resolved. - l1 = node_factory.get_node(options={'dev-disable-commit-after': 1}, - may_fail=True, allow_broken_log=True) - l2 = node_factory.get_node(options={'dev-disable-commit-after': 1, - 'watchtime-blocks': to_self_delay, - 'plugin': coin_mvt_plugin}, - # Exporbitant feerates mean we don't have cap on RBF! - feerates=(15000000, 11000, 7500, 3750)) - - l1.rpc.connect(l2.info['id'], 'localhost', l2.port) - l1.fundchannel(l2, 10**7) - channel_id = first_channel_id(l1, l2) - - # Trigger an HTLC being added. - t = executor.submit(l1.pay, l2, 1000000 * 1000) - - # Make sure the channel is still alive. - assert len(l1.getactivechannels()) == 2 - assert len(l2.getactivechannels()) == 2 - - # Wait for the disconnection. - l1.daemon.wait_for_log('dev-disable-commit-after: disabling') - l2.daemon.wait_for_log('dev-disable-commit-after: disabling') - # Make sure l1 gets the new HTLC. - l1.daemon.wait_for_log('got commitsig') - - # l1 prepares a theft commitment transaction - theft_tx = l1.rpc.dev_sign_last_tx(l2.info['id'])['tx'] - - # Now continue processing until fulfilment. - l1.rpc.dev_reenable_commit(l2.info['id']) - l2.rpc.dev_reenable_commit(l1.info['id']) - - # Wait for the fulfilment. - l1.daemon.wait_for_log('peer_in WIRE_UPDATE_FULFILL_HTLC') - l1.daemon.wait_for_log('peer_out WIRE_REVOKE_AND_ACK') - l2.daemon.wait_for_log('peer_out WIRE_UPDATE_FULFILL_HTLC') - l1.daemon.wait_for_log('peer_in WIRE_REVOKE_AND_ACK') - - # Now payment should complete. - t.result(timeout=10) - - # l1 goes offline and bribes the miners to censor transactions from l2. - l1.rpc.stop() - - def censoring_sendrawtx(r): - return {'id': r['id'], 'result': {}} - - l2.daemon.rpcproxy.mock_rpc('sendrawtransaction', censoring_sendrawtx) - - # l1 now performs the theft attack! - bitcoind.rpc.sendrawtransaction(theft_tx) - bitcoind.generate_block(1) - - # l2 notices. - l2.daemon.wait_for_log(' to ONCHAIN') - - def get_rbf_tx(self, depth, name, resolve): - r = self.daemon.wait_for_log('Broadcasting RBF {} .* to resolve {} depth={}' - .format(name, resolve, depth)) - return re.search(r'.* \(([0-9a-fA-F]*)\)', r).group(1) - - rbf_txes = [] - # Now the censoring miners generate some blocks. - for depth in range(2, 10): - bitcoind.generate_block(1) - sync_blockheight(bitcoind, [l2]) - # l2 should RBF, twice even, one for the l1 main output, - # one for the l1 HTLC output. - rbf_txes.append(get_rbf_tx(l2, depth, - 'OUR_PENALTY_TX', - 'THEIR_REVOKED_UNILATERAL/THEIR_HTLC')) - rbf_txes.append(get_rbf_tx(l2, depth, - 'OUR_PENALTY_TX', - 'THEIR_REVOKED_UNILATERAL/DELAYED_CHEAT_OUTPUT_TO_THEM')) - - # Now that the transactions have high fees, independent miners - # realize they can earn potentially more money by grabbing the - # high-fee censored transactions, and fresh, non-censoring - # hashpower arises, evicting the censor. - l2.daemon.rpcproxy.mock_rpc('sendrawtransaction', None) - - # Check that the last two txes can be broadcast. - # These should donate the total amount to miners. - rbf_txes = rbf_txes[-2:] - for tx in rbf_txes: - l2.rpc.call('sendrawtransaction', [tx, True]) - - # Now the non-censoring miners overpower the censoring miners. - bitcoind.generate_block(1) - sync_blockheight(bitcoind, [l2]) - - # And l2 should consider it resolved now. - l2.daemon.wait_for_log('Resolved THEIR_REVOKED_UNILATERAL/DELAYED_CHEAT_OUTPUT_TO_THEM by our proposal OUR_PENALTY_TX') - l2.daemon.wait_for_log('Resolved THEIR_REVOKED_UNILATERAL/THEIR_HTLC by our proposal OUR_PENALTY_TX') - - # l2 donated it to the miners, so it owns nothing - assert(len(l2.rpc.listfunds()['outputs']) == 0) - assert account_balance(l2, channel_id) == 0 - - expected_2 = { - 'A': [('cid1', ['channel_open'], ['channel_close'], 'B')], - 'B': [('cid1', ['penalty'], ['to_miner'], 'C'), ('cid1', ['penalty'], ['to_miner'], 'D')], - } - - if anchor_expected(): - expected_2['B'].append(('external', ['anchor'], None, None)) - expected_2['B'].append(('wallet', ['anchor', 'ignored'], None, None)) - - check_utxos_channel(l2, [channel_id], expected_2) - - # Make sure that l2's account is considered closed (has a fee output) - fees = [e for e in l2.rpc.bkpr_listincome()['income_events'] if e['tag'] == 'onchain_fee'] - assert len(fees) == 1 - - @pytest.mark.developer("needs DEVELOPER=1") def test_onchain_first_commit(node_factory, bitcoind): """Onchain handling where opener immediately drops to chain""" @@ -2000,7 +1841,8 @@ def test_onchaind_replay(node_factory, bitcoind): 'OUR_UNILATERAL/DELAYED_OUTPUT_TO_US') assert blocks == 200 bitcoind.generate_block(200) - bitcoind.generate_block(1, wait_for_mempool=txid) + # Could be RBF! + l1.mine_txid_or_rbf(txid) @pytest.mark.developer("needs DEVELOPER=1") @@ -2493,7 +2335,8 @@ def try_pay(): assert not t.is_alive() # 100 blocks after last spend, l1+l2 should be done. - l2.bitcoin.generate_block(100, wait_for_mempool=txid) + # Could be RBF! + l1.mine_txid_or_rbf(txid, numblocks=100) l1.daemon.wait_for_log('onchaind complete, forgetting peer') l2.daemon.wait_for_log('onchaind complete, forgetting peer') @@ -2613,8 +2456,8 @@ def test_onchain_feechange(node_factory, bitcoind, executor): assert blocks == 5 bitcoind.generate_block(5) - # Make sure that gets included. - bitcoind.generate_block(1, wait_for_mempool=txid) + # Could be RBF! + l1.mine_txid_or_rbf(txid) # Now we restart with different feerates. l1.stop() diff --git a/tests/test_pay.py b/tests/test_pay.py index 5d9fe8f588da..83d7dc848535 100644 --- a/tests/test_pay.py +++ b/tests/test_pay.py @@ -1626,7 +1626,8 @@ def test_forward_local_failed_stats(node_factory, bitcoind, executor): assert blocks == 5 bitcoind.generate_block(5) - bitcoind.generate_block(1, wait_for_mempool=txid) + # Could be RBF! + l2.mine_txid_or_rbf(txid) l2.daemon.wait_for_log('Resolved THEIR_UNILATERAL/OUR_HTLC by our proposal OUR_HTLC_TIMEOUT_TO_US') l4.daemon.wait_for_log('Ignoring output.*: OUR_UNILATERAL/THEIR_HTLC') diff --git a/tests/test_plugin.py b/tests/test_plugin.py index 981abb178fc2..d53382b6d1c5 100644 --- a/tests/test_plugin.py +++ b/tests/test_plugin.py @@ -1337,7 +1337,8 @@ def test_forward_event_notification(node_factory, bitcoind, executor): assert blocks == 5 bitcoind.generate_block(5) - bitcoind.generate_block(1, wait_for_mempool=txid) + # Could be RBF! + l2.mine_txid_or_rbf(txid) l2.daemon.wait_for_log('Resolved THEIR_UNILATERAL/OUR_HTLC by our proposal OUR_HTLC_TIMEOUT_TO_US') l5.daemon.wait_for_log('Ignoring output.*: OUR_UNILATERAL/THEIR_HTLC')