From dce2e8792880cda4ea81c713d08e3477a71de497 Mon Sep 17 00:00:00 2001 From: darosior Date: Tue, 10 Mar 2020 17:52:13 +0100 Subject: [PATCH] chaintopology: better feerate targets differentiation We kept track of an URGENT, a NORMAL, and a SLOW feerate. They were used for opening (NORMAL), mutual (NORMAL), UNILATERAL (URGENT) transactions as well as minimum and maximum estimations, and onchain resolution. We now keep track of more fine-grained feerates: - `opening` used for funding and also misc transactions - `mutual_close` used for the mutual close transaction - `unilateral_close` used for unilateral close (commitment transactions) - `delayed_to_us` used for resolving our output from our unilateral close - `htlc_resolution` used for resolving onchain HTLCs - `penalty` used for resolving revoked transactions We don't modify our requests to our Bitcoin backend, as the next commit will batch them ! Changelog-deprecated: The "urgent", "slow", and "normal" field of the `feerates` command are now deprecated. Changelog-added: The fields "opening", "mutual_close", "unilateral_close", "delayed_to_us", "htlc_resolution" and "penalty" have been added to the `feerates` command. --- contrib/pyln-testing/pyln/testing/utils.py | 2 +- lightningd/chaintopology.c | 93 ++++++++++++---------- lightningd/chaintopology.h | 13 ++- lightningd/json.c | 9 +++ tests/test_closing.py | 2 +- tests/test_misc.py | 36 ++++++--- wallet/walletrpc.c | 4 +- 7 files changed, 100 insertions(+), 59 deletions(-) diff --git a/contrib/pyln-testing/pyln/testing/utils.py b/contrib/pyln-testing/pyln/testing/utils.py index c24f87432..c9d4a30a7 100644 --- a/contrib/pyln-testing/pyln/testing/utils.py +++ b/contrib/pyln-testing/pyln/testing/utils.py @@ -887,7 +887,7 @@ class LightningNode(object): self.set_feerates([rate] * 3, False) self.restart() self.daemon.wait_for_log('peer_out WIRE_UPDATE_FEE') - assert(self.rpc.feerates('perkw')['perkw']['normal'] == rate) + assert(self.rpc.feerates('perkw')['perkw']['opening'] == rate) def wait_for_onchaind_broadcast(self, name, resolve=None): """Wait for onchaind to drop tx name to resolve (if any)""" diff --git a/lightningd/chaintopology.c b/lightningd/chaintopology.c index 399b775f9..bad9d657a 100644 --- a/lightningd/chaintopology.c +++ b/lightningd/chaintopology.c @@ -14,6 +14,7 @@ #include #include #include +#include #include #include #include @@ -300,9 +301,14 @@ static void watch_for_utxo_reconfirmation(struct chain_topology *topo, const char *feerate_name(enum feerate feerate) { switch (feerate) { - case FEERATE_URGENT: return "urgent"; - case FEERATE_NORMAL: return "normal"; - case FEERATE_SLOW: return "slow"; + case FEERATE_OPENING: return "opening"; + case FEERATE_MUTUAL_CLOSE: return "mutual_close"; + case FEERATE_UNILATERAL_CLOSE: return "unilateral_close"; + case FEERATE_DELAYED_TO_US: return "delayed_to_us"; + case FEERATE_HTLC_RESOLUTION: return "htlc_resolution"; + case FEERATE_PENALTY: return "penalty"; + case FEERATE_MIN: return "min_acceptable"; + case FEERATE_MAX: return "max_acceptable"; } abort(); } @@ -337,13 +343,23 @@ static void add_feerate_history(struct chain_topology *topo, topo->feehistory[feerate][0] = val; } +/* Did the the feerate change since we last estimated it ? */ +static bool feerate_changed(struct chain_topology *topo, u32 old_feerates[]) +{ + for (enum feerate f = 0; f < NUM_FEERATES; f++) { + if (try_get_feerate(topo, f) != old_feerates[f]) + return true; + } + + return false; +} + /* 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) { u32 old_feerates[NUM_FEERATES]; - bool changed = false; /* 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 @@ -405,26 +421,7 @@ static void update_feerates(struct bitcoind *bitcoind, maybe_completed_init(topo); } - /* Make sure (known) fee rates are in order. */ - for (size_t i = 0; i < NUM_FEERATES; i++) { - if (!topo->feerate[i]) - continue; - for (size_t j = 0; j < i; j++) { - if (!topo->feerate[j]) - continue; - if (topo->feerate[j] < topo->feerate[i]) { - log_unusual(topo->log, - "Feerate estimate for %s (%u) above %s (%u)", - feerate_name(i), topo->feerate[i], - feerate_name(j), topo->feerate[j]); - topo->feerate[j] = topo->feerate[i]; - } - } - if (try_get_feerate(topo, i) != old_feerates[i]) - changed = true; - } - - if (changed) + if (feerate_changed(topo, old_feerates)) notify_feerate_change(bitcoind->ld); next_updatefee_timer(topo); @@ -432,45 +429,43 @@ static void update_feerates(struct bitcoind *bitcoind, static void start_fee_estimate(struct chain_topology *topo) { - /* FEERATE_IMMEDIATE, FEERATE_NORMAL, FEERATE_SLOW */ const char *estmodes[] = { "CONSERVATIVE", "ECONOMICAL", "ECONOMICAL" }; const u32 blocks[] = { 2, 4, 100 }; - BUILD_ASSERT(ARRAY_SIZE(blocks) == NUM_FEERATES); /* Once per new block head, update fee estimates. */ bitcoind_estimate_fees(topo->bitcoind, blocks, estmodes, NUM_FEERATES, update_feerates, topo); } -u32 mutual_close_feerate(struct chain_topology *topo) +u32 opening_feerate(struct chain_topology *topo) { - return try_get_feerate(topo, FEERATE_NORMAL); + return try_get_feerate(topo, FEERATE_OPENING); } -u32 opening_feerate(struct chain_topology *topo) +u32 mutual_close_feerate(struct chain_topology *topo) { - return try_get_feerate(topo, FEERATE_NORMAL); + return try_get_feerate(topo, FEERATE_MUTUAL_CLOSE); } u32 unilateral_feerate(struct chain_topology *topo) { - return try_get_feerate(topo, FEERATE_URGENT); + return try_get_feerate(topo, FEERATE_UNILATERAL_CLOSE); } u32 delayed_to_us_feerate(struct chain_topology *topo) { - return try_get_feerate(topo, FEERATE_NORMAL); + return try_get_feerate(topo, FEERATE_DELAYED_TO_US); } u32 htlc_resolution_feerate(struct chain_topology *topo) { - return try_get_feerate(topo, FEERATE_NORMAL); + return try_get_feerate(topo, FEERATE_HTLC_RESOLUTION); } u32 penalty_feerate(struct chain_topology *topo) { - return try_get_feerate(topo, FEERATE_NORMAL); + return try_get_feerate(topo, FEERATE_PENALTY); } u32 feerate_from_style(u32 feerate, enum feerate_style style) @@ -525,7 +520,8 @@ static struct command_result *json_feerates(struct command *cmd, response = json_stream_success(cmd); json_object_start(response, json_feerate_style_name(*style)); for (size_t i = 0; i < ARRAY_SIZE(feerates); i++) { - if (!feerates[i]) + if (!feerates[i] || feerates[i] == FEERATE_MIN + || feerates[i] == FEERATE_MAX) continue; json_add_num(response, feerate_name(i), feerate_to_style(feerates[i], *style)); @@ -534,6 +530,23 @@ 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)); + + if (deprecated_apis) { + /* urgent feerate was CONSERVATIVE/2, i.e. what bcli gives us + * now for unilateral close feerate */ + json_add_u64(response, "urgent", + feerate_to_style(unilateral_feerate(cmd->ld->topology), *style)); + /* normal feerate was ECONOMICAL/4, i.e. what bcli gives us + * now for opening feerate */ + json_add_u64(response, "normal", + feerate_to_style(opening_feerate(cmd->ld->topology), *style)); + /* slow feerate was ECONOMICAL/100, i.e. what bcli gives us + * now for min feerate, but doubled (the min was slow/2 but now + * the Bitcoin plugin directly gives the real min acceptable). */ + json_add_u64(response, "slow", + feerate_to_style(feerate_min(cmd->ld, NULL) * 2, *style)); + } + json_object_end(response); if (missing) @@ -827,20 +840,18 @@ u32 feerate_min(struct lightningd *ld, bool *unknown) if (ld->config.ignore_fee_limits) min = 1; else { - min = try_get_feerate(ld->topology, FEERATE_SLOW); + min = try_get_feerate(ld->topology, FEERATE_MIN); if (!min) { if (unknown) *unknown = true; } else { - const u32 *hist = ld->topology->feehistory[FEERATE_SLOW]; + 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]; } - /* Normally, we use half of slow rate. */ - min /= 2; } } @@ -858,7 +869,7 @@ 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_URGENT]; + const u32 *feehistory = ld->topology->feehistory[FEERATE_MAX]; if (unknown) *unknown = false; @@ -867,7 +878,7 @@ u32 feerate_max(struct lightningd *ld, bool *unknown) return UINT_MAX; /* If we don't know feerate, don't limit other side. */ - feerate = try_get_feerate(ld->topology, FEERATE_URGENT); + feerate = try_get_feerate(ld->topology, FEERATE_MAX); if (!feerate) { if (unknown) *unknown = true; diff --git a/lightningd/chaintopology.h b/lightningd/chaintopology.h index b20c3adc0..456296a09 100644 --- a/lightningd/chaintopology.h +++ b/lightningd/chaintopology.h @@ -22,11 +22,16 @@ struct txwatch; /* FIXME: move all feerate stuff out to new lightningd/feerate.[ch] files */ enum feerate { - FEERATE_URGENT, /* Aka: aim for next block. */ - FEERATE_NORMAL, /* Aka: next 4 blocks or so. */ - FEERATE_SLOW, /* Aka: next 100 blocks or so. */ + FEERATE_OPENING, + FEERATE_MUTUAL_CLOSE, + FEERATE_UNILATERAL_CLOSE, + FEERATE_DELAYED_TO_US, + FEERATE_HTLC_RESOLUTION, + FEERATE_PENALTY, + FEERATE_MIN, + FEERATE_MAX, }; -#define NUM_FEERATES (FEERATE_SLOW+1) +#define NUM_FEERATES (FEERATE_MAX+1) /* We keep the last three in case there are outliers (for min/max) */ #define FEE_HISTORY_NUM 3 diff --git a/lightningd/json.c b/lightningd/json.c index 9d1146fcb..c1752556e 100644 --- a/lightningd/json.c +++ b/lightningd/json.c @@ -115,6 +115,15 @@ struct command_result *param_feerate(struct command *cmd, const char *name, if (json_tok_streq(buffer, tok, feerate_name(i))) return param_feerate_estimate(cmd, feerate, i); } + /* 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); /* We have to split the number and suffix. */ suffix.start = suffix.end; diff --git a/tests/test_closing.py b/tests/test_closing.py index 13ab6b144..129574a69 100644 --- a/tests/test_closing.py +++ b/tests/test_closing.py @@ -1094,7 +1094,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((100000, 100000, 100000)) - l1.daemon.wait_for_log('Feerate estimate for normal set to [56789][0-9]{4}') + l1.daemon.wait_for_log('Feerate estimate for unilateral_close set 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 e82935ff0..4fdf89c73 100644 --- a/tests/test_misc.py +++ b/tests/test_misc.py @@ -1359,6 +1359,10 @@ def test_feerates(node_factory): }) l1.start() + # All estimation types + types = ["opening", "mutual_close", "unilateral_close", "delayed_to_us", + "htlc_resolution", "penalty"] + # Query feerates (shouldn't give any!) wait_for(lambda: len(l1.rpc.feerates('perkw')['perkw']) == 2) feerates = l1.rpc.feerates('perkw') @@ -1366,6 +1370,8 @@ def test_feerates(node_factory): assert 'perkb' not in feerates assert feerates['perkw']['max_acceptable'] == 2**32 - 1 assert feerates['perkw']['min_acceptable'] == 253 + 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') @@ -1373,42 +1379,50 @@ 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 + for t in types: + assert t not in feerates['perkb'] # Now try setting them, one at a time. + # Set CONSERVATIVE/2 feerate, for max and unilateral_close l1.set_feerates((15000, 0, 0), True) wait_for(lambda: len(l1.rpc.feerates('perkw')['perkw']) == 3) feerates = l1.rpc.feerates('perkw') - assert feerates['perkw']['urgent'] == 15000 + assert feerates['perkw']['unilateral_close'] == 15000 assert feerates['warning'] == 'Some fee estimates unavailable: bitcoind startup?' assert 'perkb' not in feerates assert feerates['perkw']['max_acceptable'] == 15000 * 10 assert feerates['perkw']['min_acceptable'] == 253 + # Set ECONOMICAL/4 feerate, for all but min l1.set_feerates((15000, 6250, 0), True) - wait_for(lambda: len(l1.rpc.feerates('perkb')['perkb']) == 4) + wait_for(lambda: len(l1.rpc.feerates('perkb')['perkb']) == len(types) + 2) feerates = l1.rpc.feerates('perkb') - assert feerates['perkb']['urgent'] == 15000 * 4 - assert feerates['perkb']['normal'] == 25000 + assert feerates['perkb']['unilateral_close'] == 15000 * 4 + for t in types: + if t != "unilateral_close": + assert feerates['perkb'][t] == 25000 assert feerates['warning'] == 'Some fee estimates unavailable: bitcoind startup?' assert 'perkw' not in feerates assert feerates['perkb']['max_acceptable'] == 15000 * 4 * 10 assert feerates['perkb']['min_acceptable'] == 253 * 4 + # Set ECONOMICAL/100 feerate for min l1.set_feerates((15000, 6250, 5000), True) - wait_for(lambda: len(l1.rpc.feerates('perkw')['perkw']) == 5) + wait_for(lambda: len(l1.rpc.feerates('perkw')['perkw']) >= len(types) + 2) feerates = l1.rpc.feerates('perkw') - assert feerates['perkw']['urgent'] == 15000 - assert feerates['perkw']['normal'] == 25000 // 4 - assert feerates['perkw']['slow'] == 5000 + assert feerates['perkw']['unilateral_close'] == 15000 + for t in types: + if t != "unilateral_close": + assert feerates['perkw'][t] == 25000 // 4 assert 'warning' not in feerates assert 'perkb' not in feerates assert feerates['perkw']['max_acceptable'] == 15000 * 10 assert feerates['perkw']['min_acceptable'] == 5000 // 2 assert len(feerates['onchain_fee_estimates']) == 3 - assert feerates['onchain_fee_estimates']['opening_channel_satoshis'] == feerates['perkw']['normal'] * 702 // 1000 - assert feerates['onchain_fee_estimates']['mutual_close_satoshis'] == feerates['perkw']['normal'] * 673 // 1000 - assert feerates['onchain_fee_estimates']['unilateral_close_satoshis'] == feerates['perkw']['urgent'] * 598 // 1000 + 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 def test_logging(node_factory): diff --git a/wallet/walletrpc.c b/wallet/walletrpc.c index d5b75dcfb..6ec384873 100644 --- a/wallet/walletrpc.c +++ b/wallet/walletrpc.c @@ -297,8 +297,10 @@ static struct command_result *json_prepare_tx(struct command *cmd, } if (!feerate_per_kw) { + /* We mainly use `txprepare` for opening transactions, and FEERATE_OPENING + * is kind of the new FEERATE_NORMAL so it fits well `withdraw` too. */ result = param_feerate_estimate(cmd, &feerate_per_kw, - FEERATE_NORMAL); + FEERATE_OPENING); if (result) return result; }