From b2110847c69951027289f1746d8e5ab3e70f8e47 Mon Sep 17 00:00:00 2001 From: ZmnSCPxj jxPCSnmZ Date: Thu, 23 Jul 2020 18:08:16 +0800 Subject: [PATCH] plugins/bcli.c: `sendrawtransaction` now has a required `allowhighfees` argument. Changelog-Deprecated: plugin: `bcli` replacements should note that `sendrawtransaction` now has a second required Boolean argument, `allowhighfees`, which if `true`, means ignore any fee limits and just broadcast the transaction. Use `--deprecated-apis` to use older `bcli` replacement plugins that only support a single argument. --- doc/PLUGINS.md | 7 ++- lightningd/bitcoind.c | 99 +++++++++++++++++++++++++++++++++--- lightningd/bitcoind.h | 15 ++++++ lightningd/chaintopology.c | 24 ++++++--- lightningd/chaintopology.h | 8 +++ lightningd/onchain_control.c | 8 ++- plugins/bcli.c | 16 ++++++ tests/test_plugin.py | 2 +- 8 files changed, 162 insertions(+), 17 deletions(-) diff --git a/doc/PLUGINS.md b/doc/PLUGINS.md index e03b138945e1..c757dfd7e045 100644 --- a/doc/PLUGINS.md +++ b/doc/PLUGINS.md @@ -1164,8 +1164,11 @@ The plugin must respond to `gettxout` with the following fields: ### `sendrawtransaction` -This call takes one parameter, a string representing a hex-encoded Bitcoin -transaction. +This call takes two parameters, +a string `tx` representing a hex-encoded Bitcoin transaction, +and a boolean `allowhighfees`, which if set means suppress +any high-fees check implemented in the backend, since the given +transaction may have fees that are very high. The plugin must broadcast it and respond with the following fields: - `success` (boolean), which is `true` if the broadcast succeeded diff --git a/lightningd/bitcoind.c b/lightningd/bitcoind.c index 2a4b28a1887b..c3cd31141209 100644 --- a/lightningd/bitcoind.c +++ b/lightningd/bitcoind.c @@ -21,6 +21,7 @@ #include #include #include +#include #include #include #include @@ -252,6 +253,7 @@ void bitcoind_estimate_fees_(struct bitcoind *bitcoind, struct sendrawtx_call { struct bitcoind *bitcoind; + const char *hextx; void (*cb)(struct bitcoind *bitcoind, bool success, const char *err_msg, @@ -293,28 +295,113 @@ static void sendrawtx_callback(const char *buf, const jsmntok_t *toks, tal_free(call); } -void bitcoind_sendrawtx_(struct bitcoind *bitcoind, - const char *hextx, - void (*cb)(struct bitcoind *bitcoind, - bool success, const char *err_msg, void *), - void *cb_arg) +/* For compatibility with `sendrawtransaction` plugins written for + * 0.9.0 or earlier. + * Once this deprecated API is removed, we can just delete this function + * and use sendrawtx_callback directly in bitcoind_sendrawtx_ahf_. + */ +static void sendrawtx_compatv090_callback(const char *buf, + const jsmntok_t *toks, + const jsmntok_t *idtok, + struct sendrawtx_call *call) +{ + const jsmntok_t *errtok; + const jsmntok_t *codetok; + errcode_t code; + struct jsonrpc_request *req; + + static bool warned = false; + + + /* If deprecated APIs not enabled, fail it outright. */ + if (!deprecated_apis) + goto fallback; + + /* If we got a JSONRPC2_INVALID_PARAMS error, assume it is + * because it is an old plugin which does not support the + * new `allowhighfees` parameter. + */ + errtok = json_get_member(buf, toks, "error"); + if (!errtok) + goto fallback; + + codetok = json_get_member(buf, errtok, "code"); + if (!codetok) + goto fallback; + + if (!json_to_errcode(buf, codetok, &code)) + goto fallback; + + if (code != JSONRPC2_INVALID_PARAMS) + goto fallback; + + /* Possibly it is because `allowhighfees` is not understood + * by the plugin. */ + if (!warned) { + warned = true; + log_unusual(call->bitcoind->log, + "`sendrawtransaction` failed when given two " + "arguments, will retry with single-argument " + "deprecated API."); + } + + /* Retry with a single argument, hextx. */ + req = jsonrpc_request_start(call->bitcoind, "sendrawtransaction", + call->bitcoind->log, + &sendrawtx_callback, call); + json_add_string(req->stream, "tx", call->hextx); + jsonrpc_request_end(req); + bitcoin_plugin_send(call->bitcoind, req); + + return; + +fallback: + sendrawtx_callback(buf, toks, idtok, call); + return; +} + +void bitcoind_sendrawtx_ahf_(struct bitcoind *bitcoind, + const char *hextx, + bool allowhighfees, + void (*cb)(struct bitcoind *bitcoind, + bool success, const char *msg, void *), + void *cb_arg) { struct jsonrpc_request *req; struct sendrawtx_call *call = tal(bitcoind, struct sendrawtx_call); call->bitcoind = bitcoind; + /* For compatibility with 0.9.0 or earlier. + * Once removed, we can remove the hextx field in + * struct sendrawtx_call as well. + */ + if (deprecated_apis) + call->hextx = tal_strdup(call, hextx); + else + call->hextx = NULL; call->cb = cb; call->cb_arg = cb_arg; log_debug(bitcoind->log, "sendrawtransaction: %s", hextx); req = jsonrpc_request_start(bitcoind, "sendrawtransaction", - bitcoind->log, sendrawtx_callback, + bitcoind->log, + sendrawtx_compatv090_callback, call); json_add_string(req->stream, "tx", hextx); + json_add_bool(req->stream, "allowhighfees", allowhighfees); jsonrpc_request_end(req); bitcoin_plugin_send(bitcoind, req); } +void bitcoind_sendrawtx_(struct bitcoind *bitcoind, + const char *hextx, + void (*cb)(struct bitcoind *bitcoind, + bool success, const char *msg, void *), + void *arg) +{ + return bitcoind_sendrawtx_ahf_(bitcoind, hextx, false, cb, arg); +} + /* `getrawblockbyheight` * * If no block were found at that height, will set each field to `null`. diff --git a/lightningd/bitcoind.h b/lightningd/bitcoind.h index 1bc105120b5e..8fb275e342cf 100644 --- a/lightningd/bitcoind.h +++ b/lightningd/bitcoind.h @@ -74,6 +74,21 @@ void bitcoind_estimate_fees_(struct bitcoind *bitcoind, const u32 *), \ (arg)) +void bitcoind_sendrawtx_ahf_(struct bitcoind *bitcoind, + const char *hextx, + bool allowhighfees, + void (*cb)(struct bitcoind *bitcoind, + bool success, const char *msg, void *), + void *arg); +#define bitcoind_sendrawtx_ahf(bitcoind_, hextx, allowhighfees, cb, arg)\ + bitcoind_sendrawtx_ahf_((bitcoind_), (hextx), \ + (allowhighfees), \ + typesafe_cb_preargs(void, void *, \ + (cb), (arg), \ + struct bitcoind *, \ + bool, const char *),\ + (arg)) + void bitcoind_sendrawtx_(struct bitcoind *bitcoind, const char *hextx, void (*cb)(struct bitcoind *bitcoind, diff --git a/lightningd/chaintopology.c b/lightningd/chaintopology.c index a09a27f70a84..5378ec2753e2 100644 --- a/lightningd/chaintopology.c +++ b/lightningd/chaintopology.c @@ -218,10 +218,12 @@ static void broadcast_done(struct bitcoind *bitcoind, } } -void broadcast_tx(struct chain_topology *topo, - struct channel *channel, const struct bitcoin_tx *tx, - void (*failed_or_success)(struct channel *channel, - bool success, const char *err)) +void broadcast_tx_ahf(struct chain_topology *topo, + struct channel *channel, const struct bitcoin_tx *tx, + bool allowhighfees, + void (*failed)(struct channel *channel, + bool success, + const char *err)) { /* Channel might vanish: topo owns it to start with. */ struct outgoing_tx *otx = tal(topo, struct outgoing_tx); @@ -230,7 +232,7 @@ void broadcast_tx(struct chain_topology *topo, otx->channel = channel; bitcoin_txid(tx, &otx->txid); otx->hextx = tal_hex(otx, rawtx); - otx->failed_or_success = failed_or_success; + otx->failed_or_success = failed; tal_free(rawtx); tal_add_destructor2(channel, clear_otx_channel, otx); @@ -238,8 +240,18 @@ void broadcast_tx(struct chain_topology *topo, type_to_string(tmpctx, struct bitcoin_txid, &otx->txid)); wallet_transaction_add(topo->ld->wallet, tx->wtx, 0, 0); - bitcoind_sendrawtx(topo->bitcoind, otx->hextx, broadcast_done, otx); + bitcoind_sendrawtx_ahf(topo->bitcoind, otx->hextx, allowhighfees, + broadcast_done, otx); } +void broadcast_tx(struct chain_topology *topo, + struct channel *channel, const struct bitcoin_tx *tx, + void (*failed)(struct channel *channel, + bool success, + const char *err)) +{ + return broadcast_tx_ahf(topo, channel, tx, false, failed); +} + static enum watch_result closeinfo_txid_confirmed(struct lightningd *ld, struct channel *channel, diff --git a/lightningd/chaintopology.h b/lightningd/chaintopology.h index 8fd651283260..02fc157abc4c 100644 --- a/lightningd/chaintopology.h +++ b/lightningd/chaintopology.h @@ -181,6 +181,14 @@ void broadcast_tx(struct chain_topology *topo, void (*failed)(struct channel *channel, bool success, const char *err)); +/* Like the above, but with an additional `allowhighfees` parameter. + * If true, suppress any high-fee checks in the backend. */ +void broadcast_tx_ahf(struct chain_topology *topo, + struct channel *channel, const struct bitcoin_tx *tx, + bool allowhighfees, + void (*failed)(struct channel *channel, + bool success, + const char *err)); struct chain_topology *new_topology(struct lightningd *ld, struct log *log); void setup_topology(struct chain_topology *topology, struct timers *timers, diff --git a/lightningd/onchain_control.c b/lightningd/onchain_control.c index 1beca42c8a0d..dc76dc9bb573 100644 --- a/lightningd/onchain_control.c +++ b/lightningd/onchain_control.c @@ -284,8 +284,12 @@ static void handle_onchain_broadcast_tx(struct channel *channel, wallet_transaction_annotate(w, &txid, type, channel->dbid); /* We don't really care if it fails, we'll respond via watch. */ - broadcast_tx(channel->peer->ld->topology, channel, tx, - is_rbf ? &handle_onchain_broadcast_rbf_tx_cb : NULL); + /* If the onchaind signals this as RBF-able, then we also + * set allowhighfees, as the transaction may be RBFed into + * high feerates as protection against the MAD-HTLC attack. */ + broadcast_tx_ahf(channel->peer->ld->topology, channel, + tx, is_rbf, + is_rbf ? &handle_onchain_broadcast_rbf_tx_cb : NULL); } static void handle_onchain_unwatch_tx(struct channel *channel, const u8 *msg) diff --git a/plugins/bcli.c b/plugins/bcli.c index db6a3ae68c6b..5efbe21f6240 100644 --- a/plugins/bcli.c +++ b/plugins/bcli.c @@ -793,13 +793,29 @@ static struct command_result *sendrawtransaction(struct command *cmd, const jsmntok_t *toks) { const char **params = tal_arr(cmd, const char *, 1); + bool *allowhighfees; /* bitcoin-cli wants strings. */ if (!param(cmd, buf, toks, p_req("tx", param_string, ¶ms[0]), + p_req("allowhighfees", param_bool, &allowhighfees), NULL)) return command_param_failed(); + if (*allowhighfees) { + if (bitcoind->version >= 190001) + /* Starting in 19.0.1, second argument is + * maxfeerate, which when set to 0 means + * no max feerate. + */ + tal_arr_expand(¶ms, "0"); + else + /* in older versions, second arg is allowhighfees, + * set to true to allow high fees. + */ + tal_arr_expand(¶ms, "true"); + } + start_bitcoin_cli(NULL, cmd, process_sendrawtransaction, true, BITCOIND_HIGH_PRIO, "sendrawtransaction", params, NULL); diff --git a/tests/test_plugin.py b/tests/test_plugin.py index d52e6340df0b..f99d42fcc2ff 100644 --- a/tests/test_plugin.py +++ b/tests/test_plugin.py @@ -1222,7 +1222,7 @@ def test_bcli(node_factory, bitcoind, chainparams): "vout": fc['outnum'] })['amount'] is None) - resp = l1.rpc.call("sendrawtransaction", {"tx": "dummy"}) + resp = l1.rpc.call("sendrawtransaction", {"tx": "dummy", "allowhighfees": False}) assert not resp["success"] and "decode failed" in resp["errmsg"]