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

@@ -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);

plugins/bcli.c

@@ -456,35 +456,46 @@ 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 *
 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);
 }
 

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,

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']