Improve validation of txprepare arguments

bitcoin/tx.h

@@ -21,6 +21,12 @@ struct wally_psbt;
 struct bitcoin_txid {
 	struct sha256_double shad;
 };
+
+struct bitcoin_outpoint {
+	struct bitcoin_txid txid;
+	u32 n;
+};
+
 /* Define bitcoin_txid_eq */
 STRUCTEQ_DEF(bitcoin_txid, 0, shad.sha.u);
 

cli/lightning-cli.c

@@ -740,6 +740,14 @@ int main(int argc, char *argv[])
 		tal_append_fmt(&cmd, "] }");
 	}
 
+	toks = json_parse_simple(ctx, cmd, strlen(cmd));
+	if (toks == NULL)
+		errx(ERROR_USAGE,
+		     "Some parameters are malformed, cannot create a valid "
+		     "JSON-RPC request: %s",
+		     cmd);
+	tal_free(toks);
+
 	if (!write_all(fd, cmd, strlen(cmd)))
 		err(ERROR_TALKING_TO_LIGHTNINGD, "Writing command");
 
@@ -823,8 +831,9 @@ int main(int argc, char *argv[])
 		     "Missing 'id' in response '%s'", resp);
 	if (!json_tok_streq(resp, id, idstr))
 		errx(ERROR_TALKING_TO_LIGHTNINGD,
-		     "Incorrect 'id' in response: %.*s",
-		     json_tok_full_len(id), json_tok_full(resp, id));
+		     "Incorrect 'id' (%.*s) in response: %.*s",
+		     json_tok_full_len(id), json_tok_full(resp, id),
+		     json_tok_full_len(toks), json_tok_full(resp, toks));
 
 	if (!error || json_tok_is_null(resp, error)) {
 		switch (format) {

common/json_helpers.c

@@ -93,6 +93,18 @@ bool json_to_txid(const char *buffer, const jsmntok_t *tok,
 				     tok->end - tok->start, txid);
 }
 
+bool json_to_outpoint(const char *buffer, const jsmntok_t *tok,
+		      struct bitcoin_outpoint *op)
+{
+	jsmntok_t t1, t2;
+
+	if (!split_tok(buffer, tok, ':', &t1, &t2))
+		return false;
+
+	return json_to_txid(buffer, &t1, &op->txid)
+		&& json_to_u32(buffer, &t2, &op->n);
+}
+
 bool json_to_channel_id(const char *buffer, const jsmntok_t *tok,
 			struct channel_id *cid)
 {
@@ -168,6 +180,14 @@ void json_add_txid(struct json_stream *result, const char *fieldname,
 	json_add_string(result, fieldname, hex);
 }
 
+void json_add_outpoint(struct json_stream *result, const char *fieldname,
+		       const struct bitcoin_outpoint *out)
+{
+	char hex[hex_str_size(sizeof(out->txid))];
+	bitcoin_txid_to_hex(&out->txid, hex, sizeof(hex));
+	json_add_member(result, fieldname, true, "%s:%d", hex, out->n);
+}
+
 void json_add_short_channel_id(struct json_stream *response,
 			       const char *fieldname,
 			       const struct short_channel_id *scid)

common/json_helpers.h

@@ -62,6 +62,10 @@ bool json_to_msat(const char *buffer, const jsmntok_t *tok,
 bool json_to_txid(const char *buffer, const jsmntok_t *tok,
 		  struct bitcoin_txid *txid);
 
+/* Extract a bitcoin outpoint from this */
+bool json_to_outpoint(const char *buffer, const jsmntok_t *tok,
+		      struct bitcoin_outpoint *op);
+
 /* Extract a channel id from this */
 bool json_to_channel_id(const char *buffer, const jsmntok_t *tok,
 			struct channel_id *cid);
@@ -98,6 +102,10 @@ void json_add_channel_id(struct json_stream *response,
 void json_add_txid(struct json_stream *result, const char *fieldname,
 		   const struct bitcoin_txid *txid);
 
+/* '"fieldname" : "txid:n" */
+void json_add_outpoint(struct json_stream *result, const char *fieldname,
+		       const struct bitcoin_outpoint *out);
+
 /* '"fieldname" : "1234:5:6"' */
 void json_add_short_channel_id(struct json_stream *response,
 			       const char *fieldname,

common/json_tok.c

@@ -501,3 +501,30 @@ struct command_result *param_psbt(struct command *cmd,
 	return command_fail_badparam(cmd, name, buffer, tok,
 				     "Expected a PSBT");
 }
+
+struct command_result *param_outpoint_arr(struct command *cmd,
+					  const char *name,
+					  const char *buffer,
+					  const jsmntok_t *tok,
+					  struct bitcoin_outpoint **outpoints)
+{
+	size_t i;
+	const jsmntok_t *curr;
+	if (tok->type != JSMN_ARRAY) {
+		return command_fail(cmd, JSONRPC2_INVALID_PARAMS,
+				    "Could not decode the outpoint array for %s: "
+				    "\"%s\" is not a valid outpoint array.",
+				    name, json_strdup(tmpctx, buffer, tok));
+	}
+
+	*outpoints = tal_arr(cmd, struct bitcoin_outpoint, tok->size);
+
+	json_for_each_arr(i, curr, tok) {
+		if (!json_to_outpoint(buffer, curr, &(*outpoints)[i]))
+			return command_fail(cmd, JSONRPC2_INVALID_PARAMS,
+					    "Could not decode outpoint \"%.*s\", "
+					    "expected format: txid:output",
+					    json_tok_full_len(curr), json_tok_full(buffer, curr));
+	}
+	return NULL;
+}

common/json_tok.h

@@ -11,6 +11,7 @@
 struct amount_msat;
 struct amount_sat;
 struct bitcoin_txid;
+struct bitcoin_outpoint;
 struct channel_id;
 struct command;
 struct command_result;
@@ -172,4 +173,13 @@ struct command_result *param_psbt(struct command *cmd,
 				  const char *buffer,
 				  const jsmntok_t *tok,
 				  struct wally_psbt **psbt);
+
+/**
+ * Parse a list of `txid:output` outpoints.
+ */
+struct command_result *param_outpoint_arr(struct command *cmd,
+					  const char *name,
+					  const char *buffer,
+					  const jsmntok_t *tok,
+					  struct bitcoin_outpoint **outpoints);
 #endif /* LIGHTNING_COMMON_JSON_TOK_H */

common/param.c

@@ -262,8 +262,6 @@ static struct command_result *param_arr(struct command *cmd, const char *buffer,
 			    "Expected array or object for params");
 }
 
-#include <stdio.h>
-
 const char *param_subcommand(struct command *cmd, const char *buffer,
 			     const jsmntok_t tokens[],
 			     const char *name, ...)

common/test/run-param.c

@@ -8,6 +8,7 @@
 #include <common/setup.h>
 #include <setjmp.h>
 #include <signal.h>
+#include <stdio.h>
 #include <unistd.h>
 #include <wire/wire.h>
 
@@ -52,6 +53,10 @@ bool json_to_channel_id(const char *buffer UNNEEDED, const jsmntok_t *tok UNNEED
 bool json_to_node_id(const char *buffer UNNEEDED, const jsmntok_t *tok UNNEEDED,
 			       struct node_id *id UNNEEDED)
 { fprintf(stderr, "json_to_node_id called!\n"); abort(); }
+/* Generated stub for json_to_outpoint */
+bool json_to_outpoint(const char *buffer UNNEEDED, const jsmntok_t *tok UNNEEDED,
+		      struct bitcoin_outpoint *op UNNEEDED)
+{ fprintf(stderr, "json_to_outpoint called!\n"); abort(); }
 /* Generated stub for json_to_pubkey */
 bool json_to_pubkey(const char *buffer UNNEEDED, const jsmntok_t *tok UNNEEDED,
 		    struct pubkey *pubkey UNNEEDED)

contrib/pyln-client/pyln/client/lightning.py

@@ -1172,19 +1172,7 @@ def withdraw(self, destination, satoshi, feerate=None, minconf=None, utxos=None)
 
         return self.call("withdraw", payload)
 
-    def _deprecated_txprepare(self, destination, satoshi, feerate=None, minconf=None):
-        warnings.warn("txprepare now takes output arg: expect removal"
-                      " in Mid-2020",
-                      DeprecationWarning)
-        payload = {
-            "destination": destination,
-            "satoshi": satoshi,
-            "feerate": feerate,
-            "minconf": minconf,
-        }
-        return self.call("txprepare", payload)
-
-    def txprepare(self, *args, **kwargs):
+    def txprepare(self, outputs, feerate=None, minconf=None, utxos=None):
         """
         Prepare a Bitcoin transaction which sends to [outputs].
         The format of output is like [{address1: amount1},
@@ -1194,22 +1182,13 @@ def txprepare(self, *args, **kwargs):
         Outputs will be reserved until you call txdiscard or txsend, or
         lightningd restarts.
         """
-        if 'destination' in kwargs or 'satoshi' in kwargs:
-            return self._deprecated_txprepare(*args, **kwargs)
-
-        if len(args) and not isinstance(args[0], list):
-            return self._deprecated_txprepare(*args, **kwargs)
-
-        def _txprepare(outputs, feerate=None, minconf=None, utxos=None):
-            payload = {
-                "outputs": outputs,
-                "feerate": feerate,
-                "minconf": minconf,
-                "utxos": utxos,
-            }
-            return self.call("txprepare", payload)
-
-        return _txprepare(*args, **kwargs)
+        payload = {
+            "outputs": outputs,
+            "feerate": feerate,
+            "minconf": minconf,
+            "utxos": utxos,
+        }
+        return self.call("txprepare", payload)
 
     def txdiscard(self, txid):
         """

lightningd/jsonrpc.c

@@ -956,8 +956,10 @@ static struct io_plan *read_json(struct io_conn *conn,
 	if (!json_parse_input(&jcon->input_parser, &jcon->input_toks,
 			      jcon->buffer, jcon->used,
 			      &complete)) {
-		json_command_malformed(jcon, "null",
-				       "Invalid token in json input");
+		json_command_malformed(
+		    jcon, "null",
+		    tal_fmt(tmpctx, "Invalid token in json input: '%s'",
+			    tal_strndup(tmpctx, jcon->buffer, jcon->used)));
 		return io_halfclose(conn);
 	}
 

plugins/txprepare.c

@@ -67,6 +67,13 @@ static struct command_result *param_outputs(struct command *cmd,
 	size_t i;
 	const jsmntok_t *t;
 
+	if (tok->type != JSMN_ARRAY) {
+		return command_fail(cmd, JSONRPC2_INVALID_PARAMS,
+				    "Expected an array of outputs in the "
+				    "format '[{\"txid\":0}, ...]', got \"%s\"",
+				    json_strdup(tmpctx, buffer, tok));
+	}
+
 	txp->outputs = tal_arr(txp, struct tx_output, tok->size);
 	txp->output_total = AMOUNT_SAT(0);
 	txp->all_output_idx = -1;
@@ -324,7 +331,7 @@ static struct command_result *txprepare_continue(struct command *cmd,
 						 struct txprepare *txp,
 						 const char *feerate,
 						 unsigned int *minconf,
-						 const char *utxos,
+						 struct bitcoin_outpoint *utxos,
 						 bool is_withdraw)
 {
 	struct out_req *req;
@@ -341,7 +348,11 @@ static struct command_result *txprepare_continue(struct command *cmd,
 		req = jsonrpc_request_start(cmd->plugin, cmd, "utxopsbt",
 					    psbt_created, forward_error,
 					    txp);
-		json_add_jsonstr(req->js, "utxos", utxos);
+		json_array_start(req->js, "utxos");
+		for (size_t i = 0; i < tal_count(utxos); i++) {
+			json_add_outpoint(req->js, NULL, &utxos[i]);
+		}
+		json_array_end(req->js);
 	} else {
 		req = jsonrpc_request_start(cmd->plugin, cmd, "fundpsbt",
 					    psbt_created, forward_error,
@@ -365,14 +376,15 @@ static struct command_result *json_txprepare(struct command *cmd,
 					     const jsmntok_t *params)
 {
 	struct txprepare *txp = tal(cmd, struct txprepare);
-	const char *feerate, *utxos;
+	const char *feerate;
+	struct bitcoin_outpoint *utxos;
 	unsigned int *minconf;
 
 	if (!param(cmd, buffer, params,
 		   p_req("outputs", param_outputs, txp),
 		   p_opt("feerate", param_string, &feerate),
 		   p_opt_def("minconf", param_number, &minconf, 1),
-		   p_opt("utxos", param_string, &utxos),
+		   p_opt("utxos", param_outpoint_arr, &utxos),
 		   NULL))
 		return command_param_failed();
 
@@ -472,7 +484,8 @@ static struct command_result *json_withdraw(struct command *cmd,
 	struct txprepare *txp = tal(cmd, struct txprepare);
 	struct amount_sat *amount;
 	const u8 *scriptpubkey;
-	const char *feerate, *utxos;
+	const char *feerate;
+	struct bitcoin_outpoint *utxos;
 	unsigned int *minconf;
 
 	if (!param(cmd, buffer, params,
@@ -481,7 +494,7 @@ static struct command_result *json_withdraw(struct command *cmd,
 		   p_req("satoshi", param_sat_or_all, &amount),
 		   p_opt("feerate", param_string, &feerate),
 		   p_opt_def("minconf", param_number, &minconf, 1),
-		   p_opt("utxos", param_string, &utxos),
+		   p_opt("utxos", param_outpoint_arr, &utxos),
 		   NULL))
 		return command_param_failed();
 

tests/test_wallet.py

@@ -1173,3 +1173,55 @@ def test_multiwithdraw_simple(node_factory, bitcoind):
     assert only_one(funds3)["address"] == addr3
     assert only_one(funds3)["status"] == "confirmed"
     assert only_one(funds3)["amount_msat"] == amount3
+
+
+@unittest.skipIf(
+    TEST_NETWORK == 'liquid-regtest',
+    'Blinded elementsd addresses are not recognized')
+def test_repro_4258(node_factory, bitcoind):
+    """Reproduces issue #4258, invalid output encoding for txprepare.
+    """
+    l1 = node_factory.get_node()
+    addr = l1.rpc.newaddr()['bech32']
+    bitcoind.rpc.sendtoaddress(addr, 1)
+    bitcoind.generate_block(1)
+
+    wait_for(lambda: l1.rpc.listfunds()['outputs'] != [])
+    out = l1.rpc.listfunds()['outputs'][0]
+
+    addr = bitcoind.rpc.getnewaddress()
+
+    # Missing array parentheses for outputs
+    with pytest.raises(RpcError, match=r"Expected an array of outputs"):
+        l1.rpc.txprepare(
+            outputs="{addr}:all".format(addr=addr),
+            feerate="slow",
+            minconf=1,
+            utxos=["{txid}:{output}".format(**out)]
+        )
+
+    # Missing parentheses on the utxos array
+    with pytest.raises(RpcError, match=r"Could not decode the outpoint array for utxos"):
+        l1.rpc.txprepare(
+            outputs=[{addr: "all"}],
+            feerate="slow",
+            minconf=1,
+            utxos="{txid}:{output}".format(**out)
+        )
+
+    tx = l1.rpc.txprepare(
+        outputs=[{addr: "all"}],
+        feerate="slow",
+        minconf=1,
+        utxos=["{txid}:{output}".format(**out)]
+    )
+
+    tx = bitcoind.rpc.decoderawtransaction(tx['unsigned_tx'])
+
+    assert(len(tx['vout']) == 1)
+    o0 = tx['vout'][0]
+    assert(o0['scriptPubKey']['addresses'] == [addr])
+
+    assert(len(tx['vin']) == 1)
+    i0 = tx['vin'][0]
+    assert([i0['txid'], i0['vout']] == [out['txid'], out['output']])