lightning/bitcoind: adapt and batch fees estimations

This adapts our fee estimations requests to the Bitcoin backend to the
new semantic, and batch the requests.

This makes our request for fees much simpler, and leaves some more
flexibility for a plugin to do something smart (it could still lie before
but now it's explicit, at least.) as we don't explicitly request
estimation for a specific mode and a target.

Changelog-Changed: We now batch the requests for fee estimation to our Bitcoin backend.
Changelog-Changed: We now get more fine-grained fee estimation from our Bitcoin backend.

lightningd/bitcoind.c

@@ -38,7 +38,7 @@
 /* The names of the requests we can make to our Bitcoin backend. */
 static const char *methods[] = {"getchaininfo", "getrawblockbyheight",
                                 "sendrawtransaction", "getutxout",
-                                "getfeerate"};
+                                "estimatefees"};
 
 static void plugin_config_cb(const char *buffer,
 			     const jsmntok_t *toks,
@@ -113,9 +113,17 @@ void bitcoind_check_commands(struct bitcoind *bitcoind)
 /* Our Bitcoin backend plugin gave us a bad response. We can't recover. */
 static void bitcoin_plugin_error(struct bitcoind *bitcoind, const char *buf,
 				 const jsmntok_t *toks, const char *method,
-				 const char *reason)
+				 const char *fmt, ...)
 {
-	struct plugin *p = strmap_get(&bitcoind->pluginsmap, method);
+	va_list ap;
+	char *reason;
+	struct plugin *p;
+
+	va_start(ap, fmt);
+	reason = tal_vfmt(NULL, fmt, ap);
+	va_end(ap);
+
+	p = strmap_get(&bitcoind->pluginsmap, method);
 	fatal("%s error: bad response to %s (%s), response was %.*s",
 	      p->cmd, method, reason,
 	      toks->end - toks->start, buf + toks->start);
@@ -133,118 +141,111 @@ static void bitcoin_plugin_send(struct bitcoind *bitcoind,
 	plugin_request_send(plugin, req);
 }
 
-/* `getfeerate`
+/* `estimatefees`
  *
  * Gather feerate from our Bitcoin backend. Will set the feerate to `null`
  * if estimation failed.
  *
+ *   - `opening` is used for funding and also misc transactions
+ *   - `mutual_close` is used for the mutual close transaction
+ *   - `unilateral_close` is used for unilateral close (commitment transactions)
+ *   - `delayed_to_us` is used for resolving our output from our unilateral close
+ *   - `htlc_resolution` is used for resolving onchain HTLCs
+ *   - `penalty` is used for resolving revoked transactions
+ *   - `min` is the minimum acceptable feerate
+ *   - `max` is the maximum acceptable feerate, note that it will be multiplied by the
+ *   config's  `max_fee_multiplier`.
+ *
  * Plugin response:
  * {
- *	"feerate": <fee per KB>,
+ *	"opening": <sat per kVB>,
+ *	"mutual_close": <sat per kVB>,
+ *	"unilateral_close": <sat per kVB>,
+ *	"delayed_to_us": <sat per kVB>,
+ *	"htlc_resolution": <sat per kVB>,
+ *	"penalty": <sat per kVB>,
+ *	"min_acceptable": <sat per kVB>,
+ *	"max_acceptable": <sat per kVB>,
  * }
  */
 
 struct estimatefee_call {
 	struct bitcoind *bitcoind;
-	size_t i;
-	const u32 *blocks;
-	const char **estmode;
-
 	void (*cb)(struct bitcoind *bitcoind, const u32 satoshi_per_kw[],
 		   void *);
 	void *arg;
-	u32 *satoshi_per_kw;
 };
 
-static void do_one_estimatefee(struct bitcoind *bitcoind,
-			       struct estimatefee_call *call);
-
-static void getfeerate_callback(const char *buf, const jsmntok_t *toks,
-				const jsmntok_t *idtok,
-				struct estimatefee_call *call)
+static void estimatefees_callback(const char *buf, const jsmntok_t *toks,
+				  const jsmntok_t *idtok,
+				  struct estimatefee_call *call)
 {
 	const jsmntok_t *resulttok, *feeratetok;
-	u64 feerate;
+	u32 *feerates = tal_arr(call, u32, NUM_FEERATES);
 
 	resulttok = json_get_member(buf, toks, "result");
 	if (!resulttok)
 		bitcoin_plugin_error(call->bitcoind, buf, toks,
-				     "getfeerate",
+				     "estimatefees",
 				     "bad 'result' field");
 
-	feeratetok = json_get_member(buf, resulttok, "feerate");
-	if (!feeratetok)
-		bitcoin_plugin_error(call->bitcoind, buf, toks,
-				     "getfeerate",
-				     "bad 'feerate' field");
+	for (enum feerate 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));
 
-	/* FIXME: We could trawl recent blocks for median fee... */
-	if (!json_to_u64(buf, feeratetok, &feerate)) {
-		log_unusual(call->bitcoind->log, "Unable to estimate %s/%u fee",
-			    call->estmode[call->i], call->blocks[call->i]);
+		/* FIXME: We could trawl recent blocks for median fee... */
+		if (!json_to_u32(buf, feeratetok, &feerates[f])) {
+			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 */
-		call->satoshi_per_kw[call->i] = 0;
+			/* 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)
-			call->satoshi_per_kw[call->i] = FEERATE_FLOOR;
-		else
-			call->satoshi_per_kw[call->i] = 0;
+			/* 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
-		/* Rate in satoshi per kw. */
-		call->satoshi_per_kw[call->i]
-			= feerate_from_style(feerate, FEERATE_PER_KBYTE);
-
-	call->i++;
-	if (call->i == tal_count(call->satoshi_per_kw)) {
-		call->cb(call->bitcoind, call->satoshi_per_kw, call->arg);
-		tal_free(call);
-	} else {
-		/* Next */
-		do_one_estimatefee(call->bitcoind, call);
+		} else
+			/* Rate in satoshi per kw. */
+			feerates[f] = feerate_from_style(feerates[f],
+							 FEERATE_PER_KBYTE);
 	}
-}
 
-static void do_one_estimatefee(struct bitcoind *bitcoind,
-			       struct estimatefee_call *call)
-{
-	struct jsonrpc_request *req;
-
-	req = jsonrpc_request_start(bitcoind, "getfeerate",
-				    bitcoind->log, getfeerate_callback,
-				    call);
-	json_add_num(req->stream, "blocks", call->blocks[call->i]);
-	json_add_string(req->stream, "mode", call->estmode[call->i]);
-	jsonrpc_request_end(req);
-	bitcoin_plugin_send(bitcoind, req);
+	call->cb(call->bitcoind, feerates, call->arg);
+	tal_free(call);
 }
 
 void bitcoind_estimate_fees_(struct bitcoind *bitcoind,
-			     const u32 blocks[], const char *estmode[],
 			     size_t num_estimates,
 			     void (*cb)(struct bitcoind *bitcoind,
 					const u32 satoshi_per_kw[], void *),
 			     void *arg)
 {
-	struct estimatefee_call *efee = tal(bitcoind, struct estimatefee_call);
-
-	efee->bitcoind = bitcoind;
-	efee->i = 0;
-	efee->blocks = tal_dup_arr(efee, u32, blocks, num_estimates, 0);
-	efee->estmode = tal_dup_arr(efee, const char *, estmode, num_estimates,
-				    0);
-	efee->cb = cb;
-	efee->arg = arg;
-	efee->satoshi_per_kw = tal_arr(efee, u32, num_estimates);
-
-	do_one_estimatefee(bitcoind, efee);
+	struct jsonrpc_request *req;
+	struct estimatefee_call *call = tal(bitcoind, struct estimatefee_call);
+
+	call->bitcoind = bitcoind;
+	call->cb = cb;
+	call->arg = arg;
+
+	/* No parameter needed, we always want an urgent, normal and slow
+	 * feerate. This gives computation flexibility to the plugin. */
+	req = jsonrpc_request_start(bitcoind, "estimatefees", bitcoind->log,
+				    estimatefees_callback, call);
+	jsonrpc_request_end(req);
+	plugin_request_send(strmap_get(&bitcoind->pluginsmap,
+				       "estimatefees"), req);
 }
 
 /* `sendrawtransaction`

lightningd/bitcoind.h

@@ -61,14 +61,13 @@ struct bitcoind *new_bitcoind(const tal_t *ctx,
 			      struct log *log);
 
 void bitcoind_estimate_fees_(struct bitcoind *bitcoind,
-			     const u32 blocks[], const char *estmode[],
 			     size_t num_estimates,
 			     void (*cb)(struct bitcoind *bitcoind,
 					const u32 satoshi_per_kw[], void *),
 			     void *arg);
 
-#define bitcoind_estimate_fees(bitcoind_, blocks, estmode, num, cb, arg) \
-	bitcoind_estimate_fees_((bitcoind_), (blocks), (estmode), (num), \
+#define bitcoind_estimate_fees(bitcoind_, num, cb, arg) \
+	bitcoind_estimate_fees_((bitcoind_), (num), \
 				typesafe_cb_preargs(void, void *,	\
 						    (cb), (arg),	\
 						    struct bitcoind *,	\

lightningd/chaintopology.c

@@ -429,13 +429,9 @@ static void update_feerates(struct bitcoind *bitcoind,
 
 static void start_fee_estimate(struct chain_topology *topo)
 {
-	const char *estmodes[] = { "CONSERVATIVE", "ECONOMICAL", "ECONOMICAL" };
-	const u32 blocks[] = { 2, 4, 100 };
-
-
 	/* Once per new block head, update fee estimates. */
-	bitcoind_estimate_fees(topo->bitcoind, blocks, estmodes, NUM_FEERATES,
-			       update_feerates, topo);
+	bitcoind_estimate_fees(topo->bitcoind, NUM_FEERATES, update_feerates,
+			       topo);
 }
 
 u32 opening_feerate(struct chain_topology *topo)

tests/plugins/bitcoin/part1.py

@@ -11,7 +11,7 @@
 plugin = Plugin()
 
 
-@plugin.method("getfeerate")
+@plugin.method("estimatefees")
 def getfeerate(plugin, **kwargs):
     time.sleep(1)
     return {}

tests/test_misc.py

@@ -1681,7 +1681,7 @@ def mock_fail(*args):
     wait_for(lambda: l1.daemon.is_in_log(
         r'getblockhash [a-z0-9]* exited with status 1'))
     wait_for(lambda: l1.daemon.is_in_log(
-        r'Unable to estimate CONSERVATIVE/2 fee'))
+        r'Unable to estimate opening fees'))
 
     # Now unset the mock, so calls go through again
     l1.daemon.rpcproxy.mock_rpc('getblockhash', None)