common: don't abort() if wally_psbt_output_taproot_keypath_add() fails.
What changed, and why it matters
This update fixes a crash in Core Lightning's wallet command `signpsbt`. When asked to sign a PSBT (a Bitcoin transaction format) that already contains taproot signatures, the underlying library would fail and the program would call `abort()`, killing the node. The patch turns that fatal crash into a normal error message. It also removes a test marker that expected the crash, confirming the bug is now fixed.
Apply the patch. It is a targeted fix with low regression risk. Consider also updating libwally-core once ElementsProject/libwally-core#509 is resolved, since the current behavior still rejects duplicate keypaths and the patch only handles that failure gracefully.
Security signals we found
Denial-of-service vector: JSON-RPC `signpsbt` on an already-signed taproot PSBT caused node abort()
Crash-to-error conversion: fatal abort() replaced with boolean return and graceful failure
Library bug dependency: failure caused by libwally-core rejecting duplicate taproot keypaths
Test re-enabled: `test_sign_signed_psbt` no longer marked as expected failure
Evidence from the diff
The commit changes psbt_output_set_keypath() and psbt_add_keypath_to_last_output() from void to bool, returning failure instead of calling abort() when wally_psbt_output_taproot_keypath_add() returns an error. The libwally function fails on duplicate taproot keypath entries, which occurs when signpsbt is called on an already-signed PSBT with taproot paths. Callers in wallet/walletrpc.c, lightningd/onchain_control.c, common/close_tx.c, and channeld/watchtower.c now check the return value and propagate an error instead of crashing. A previously xfail test (test_sign_signed_psbt) is re-enabled.
Changed components
wallet/walletrpc.c: json_signpsbt and match_psbt_outputs_to_walletcommon/psbt_keypath.c: psbt_output_set_keypath, psbt_add_keypath_to_last_outputcommon/psbt_keypath.h: function signatures and WARN_UNUSED_RESULT annotationslightningd/onchain_control.c: onchaind_tx_unsignedcommon/close_tx.c: create_close_txchanneld/watchtower.c: penalty_tx_createtests/test_wallet.py: test_sign_signed_psbtInspect captured patch +58 / −27
diff --git a/channeld/watchtower.c b/channeld/watchtower.c
index 285cdc4..9c2126d 100644
--- a/channeld/watchtower.c
+++ b/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 */
diff --git a/common/close_tx.c b/common/close_tx.c
index ee54913..8d6805b 100644
--- a/common/close_tx.c
+++ b/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(
- tx, *local_wallet_index, local_wallet_ext_key,
- is_p2tr(script, script_len, NULL));
+ /* Should not happen! */
+ if (!psbt_add_keypath_to_last_output(
+ tx, *local_wallet_index, local_wallet_ext_key,
+ is_p2tr(script, script_len, NULL)))
+ return tal_free(tx);
}
num_outputs++;
}
diff --git a/common/psbt_keypath.c b/common/psbt_keypath.c
index dcd89a9..c137eff 100644
--- a/common/psbt_keypath.c
+++ b/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) {
+ const struct ext_key *ext,
+ 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;
}
diff --git a/common/psbt_keypath.h b/common/psbt_keypath.h
index ddafa03..5af5a94 100644
--- a/common/psbt_keypath.h
+++ b/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,11 +16,14 @@ 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,
- const struct ext_key *ext,
- bool is_taproot,
- struct wally_psbt_output *output);
+WARN_UNUSED_RESULT
+bool psbt_output_set_keypath(u32 index,
+ const struct ext_key *ext,
+ bool is_taproot,
+ struct wally_psbt_output *output);
/* psbt_add_keypath_to_last_output - augment the last output with the
* given wallet keypath
@@ -29,9 +33,10 @@ 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);
+ bool is_taproot);
#endif /* LIGHTNING_COMMON_PSBT_KEYPATH_H */
diff --git a/lightningd/onchain_control.c b/lightningd/onchain_control.c
index 192450e..f62a673 100644
--- a/lightningd/onchain_control.c
+++ b/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);
diff --git a/tests/test_wallet.py b/tests/test_wallet.py
index c34c8ae..0dfa66c 100644
--- a/tests/test_wallet.py
+++ b/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)
diff --git a/wallet/walletrpc.c b/wallet/walletrpc.c
index e3ecb56..6bb111e 100644
--- a/wallet/walletrpc.c
+++ b/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,
- is_p2tr(script, script_len, NULL),
- &psbt->outputs[outndx]);
+ if (!psbt_output_set_keypath(index, &ext,
+ is_p2tr(script, script_len, NULL),
+ &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. */
Why this scored 60/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.