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.
5 years ago · dce2e87928
7 changed files with 100 additions and 59 deletions
--- 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)"""
--- a/lightningd/chaintopology.c
+++ b/lightningd/chaintopology.c
@ -14,6 +14,7 @@
 #include <ccan/build_assert/build_assert.h>
 #include <ccan/io/io.h>
 #include <ccan/tal/str/str.h>
+#include <common/configdir.h>
 #include <common/json_command.h>
 #include <common/jsonrpc_errors.h>
 #include <common/memleak.h>
@ -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;
--- 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
--- 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;
--- 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')
--- 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):
--- 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;
 	}