common: don't abort() if wally_psbt_output_taproot_keypath_add() fails.

It fails on duplicates.  It would ideally succeed, but bug reported:

	https://github.com/ElementsProject/libwally-core/issues/509

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Fixed: JSON-RPC: `signpsbt` no longer crashes if asked to sign an already-signed PSBT with taproot paths.

channeld/watchtower.c

@@ -79,8 +79,11 @@ penalty_tx_create(const tal_t *ctx,
 	if (final_index) {
 		size_t script_len = tal_bytelen(final_scriptpubkey);
 		bool is_tr = is_p2tr(final_scriptpubkey, script_len, NULL);
-		psbt_add_keypath_to_last_output(tx, *final_index,
-						final_ext_key, is_tr);
+		if (!psbt_add_keypath_to_last_output(tx, *final_index,
+						     final_ext_key, is_tr))
+			status_failed(STATUS_FAIL_INTERNAL_ERROR,
+				      "Cannot add final keypath?");
+
         }
 
 	/* Worst-case sig is 73 bytes */

common/close_tx.c

@@ -54,9 +54,11 @@ struct bitcoin_tx *create_close_tx(const tal_t *ctx,
 		assert((local_wallet_index == NULL) == (local_wallet_ext_key == NULL));
 		if (local_wallet_index) {
 			size_t script_len = tal_bytelen(script);
-			psbt_add_keypath_to_last_output(
+			/* Should not happen! */
+			if (!psbt_add_keypath_to_last_output(
 				    tx, *local_wallet_index, local_wallet_ext_key,
-				is_p2tr(script, script_len, NULL));
+				    is_p2tr(script, script_len, NULL)))
+				return tal_free(tx);
                 }
 		num_outputs++;
 	}

common/psbt_keypath.c

@@ -3,7 +3,11 @@
 #include <common/psbt_keypath.h>
 #include <common/utils.h>
 
-void psbt_output_set_keypath(u32 index, const struct ext_key *ext, bool is_taproot, struct wally_psbt_output *output) {
+bool psbt_output_set_keypath(u32 index,
+			     const struct ext_key *ext,
+			     bool is_taproot,
+			     struct wally_psbt_output *output)
+{
 	u8 fingerprint[BIP32_KEY_FINGERPRINT_LEN];
 	if (bip32_key_get_fingerprint(
 		    (struct ext_key *) ext, fingerprint, sizeof(fingerprint)) != WALLY_OK)
@@ -18,24 +22,29 @@ void psbt_output_set_keypath(u32 index, const struct ext_key *ext, bool is_tapro
 							  NULL, 0,
 							  fingerprint, sizeof(fingerprint),
 							  path, 1) != WALLY_OK)
-			abort();
+			return false;
 	} else {
 		if (wally_psbt_output_keypath_add(output,
 						  ext->pub_key, sizeof(ext->pub_key),
 						  fingerprint, sizeof(fingerprint),
 						  path, 1) != WALLY_OK)
-			abort();
+			return false;
 	}
 
+	return true;
 }
 
-void psbt_add_keypath_to_last_output(struct bitcoin_tx *tx,
+bool psbt_add_keypath_to_last_output(struct bitcoin_tx *tx,
 				     u32 key_index,
 				     const struct ext_key *ext,
-					 bool is_taproot) {
+				     bool is_taproot)
+{
 	size_t outndx = tx->psbt->num_outputs - 1;
+	bool ok;
 
 	tal_wally_start();
-	psbt_output_set_keypath(key_index, ext, is_taproot, &tx->psbt->outputs[outndx]);
+	ok = psbt_output_set_keypath(key_index, ext, is_taproot, &tx->psbt->outputs[outndx]);
 	tal_wally_end(tx->psbt);
+
+	return ok;
 }

common/psbt_keypath.h

@@ -2,6 +2,7 @@
 #define LIGHTNING_COMMON_PSBT_KEYPATH_H
 
 #include "config.h"
+#include <ccan/compiler/compiler.h>
 #include <ccan/short_types/short_types.h>
 #include <wally_psbt.h>
 
@@ -15,8 +16,11 @@ struct wally_map;
  * @ext - extended public key of the immediate parent of the wallet key
  * @is_taproot - PSBT output has taproot script
  * @output - PSBT output to set
+ *
+ * This can fail, if it's adding the same thing twice (taproot only)
  */
-void psbt_output_set_keypath(u32 index,
+WARN_UNUSED_RESULT
+bool psbt_output_set_keypath(u32 index,
 			     const struct ext_key *ext,
 			     bool is_taproot,
 			     struct wally_psbt_output *output);
@@ -29,7 +33,8 @@ void psbt_output_set_keypath(u32 index,
  * @ext - extended public key of the immediate parent of the wallet key
  * @is_taproot - if the output is taproot
  */
-void psbt_add_keypath_to_last_output(struct bitcoin_tx *tx,
+WARN_UNUSED_RESULT
+bool psbt_add_keypath_to_last_output(struct bitcoin_tx *tx,
 				     u32 index,
 				     const struct ext_key *ext,
 				     bool is_taproot);

lightningd/onchain_control.c

@@ -880,6 +880,7 @@ static struct bitcoin_tx *onchaind_tx_unsigned(const tal_t *ctx,
 	struct ext_key final_wallet_ext_key;
 	u64 block_target;
 	struct lightningd *ld = channel->peer->ld;
+	bool keypath_ok;
 
 	bip32_pubkey(ld, &final_key, channel->final_key_idx);
 	if (bip32_key_from_parent(ld->bip32_base,
@@ -899,13 +900,16 @@ static struct bitcoin_tx *onchaind_tx_unsigned(const tal_t *ctx,
 	if (chainparams->is_elements) {
 		bitcoin_tx_add_output(
 			tx, scriptpubkey_p2wpkh(tmpctx, &final_key), NULL, info->out_sats);
-		psbt_add_keypath_to_last_output(tx, channel->final_key_idx, &final_wallet_ext_key, false /* is_taproot */);
+		keypath_ok = psbt_add_keypath_to_last_output(tx, channel->final_key_idx, &final_wallet_ext_key, false /* is_taproot */);
 	} else {
 		bitcoin_tx_add_output(
 			tx, scriptpubkey_p2tr(tmpctx, &final_key), NULL, info->out_sats);
-		psbt_add_keypath_to_last_output(tx, channel->final_key_idx, &final_wallet_ext_key, true /* is_taproot */);
+		keypath_ok = psbt_add_keypath_to_last_output(tx, channel->final_key_idx, &final_wallet_ext_key, true /* is_taproot */);
+	}
+	if (!keypath_ok) {
+ 		channel_internal_error(channel, "Could not add keypath?");
+		return tal_free(tx);
 	}
-
 	/* Worst-case sig is 73 bytes */
 	weight = bitcoin_tx_weight(tx) + 1 + 3 + 73 + 0 + tal_count(info->wscript);
 	weight += elements_tx_overhead(chainparams, 1, 1);

tests/test_wallet.py

@@ -948,7 +948,6 @@ def test_sign_external_psbt(node_factory, bitcoind, chainparams):
     l1.rpc.signpsbt(psbt)
 
 
-@pytest.mark.xfail(strict=True)
 def test_sign_signed_psbt(node_factory, bitcoind, chainparams):
     l1 = node_factory.get_node()
     l1.fundwallet(10**6)

wallet/walletrpc.c

@@ -738,9 +738,11 @@ static struct command_result *match_psbt_inputs_to_utxos(struct command *cmd,
 	return NULL;
 }
 
-static void match_psbt_outputs_to_wallet(struct wally_psbt *psbt,
+static bool match_psbt_outputs_to_wallet(struct wally_psbt *psbt,
 				  struct wallet *w)
 {
+	bool ok = true;
+
 	tal_wally_start();
 	for (size_t outndx = 0; outndx < psbt->num_outputs; ++outndx) {
 		struct ext_key ext;
@@ -756,11 +758,16 @@ static void match_psbt_outputs_to_wallet(struct wally_psbt *psbt,
 			abort();
 		}
 
-		psbt_output_set_keypath(index, &ext,
+		if (!psbt_output_set_keypath(index, &ext,
 					     is_p2tr(script, script_len, NULL),
-					&psbt->outputs[outndx]);
+					     &psbt->outputs[outndx])) {
+			ok = false;
+			break;
+		}
 	}
 	tal_wally_end(psbt);
+
+	return ok;
 }
 
 static struct command_result *param_input_numbers(struct command *cmd,
@@ -842,7 +849,9 @@ static struct command_result *json_signpsbt(struct command *cmd,
 		return command_check_done(cmd);
 
 	/* Update the keypaths on any outputs that are in our wallet (change addresses). */
-	match_psbt_outputs_to_wallet(psbt, cmd->ld->wallet);
+	if (!match_psbt_outputs_to_wallet(psbt, cmd->ld->wallet))
+		return command_fail(cmd, JSONRPC2_INVALID_PARAMS,
+				    "Could not add keypaths to PSBT?");
 
 	/* FIXME: hsm will sign almost anything, but it should really
 	 * fail cleanly (not abort!) and let us report the error here. */