wallet: allow skipping script paths #32857

Sjors · 2025-07-02T14:13:31Z

While testing MuSig2 integration in #29675 I ran into a Ledger bug LedgerHQ/app-bitcoin-new#329 that was triggered because we signed for a script path in addition to the key path. The bug itself will be fixed, but it seems useful in general to be able to skip script path signing.

For example, you may have a 2-of-2 musig() descriptor with some sort of fallback after coins are N blocks old. For both on chain privacy and fee reasons, you don't want to use this path unless absolutely necessary. But our wallet can't know if our co-signer still has their key, so we sign the script path at all times. This could lead to accidental broadcast.

This PR introduces a keypath_only option (default false for now) to:

the send RPC
the walletprocesspsbt RPC.

The resulting PSBT won't include taproot_script_path_sigs. It does still have taproot_bip32_derivs and musig2_participant_pubkeys.

The wallet_taproot.py tests are expanded to cover this behavior.

It can also be tested with Sjors#91 and a slightly modified Moosig. See LedgerHQ/app-bitcoin-new#329 for the rough steps.

Based on:

refactor: use options struct for signing and PSBT operations #32876

TODO:

maybe refactor MutableTransactionSignatureCreator
add test coverage for send

Potential followups:

make this default for musig() (after wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys #29675)

DrahtBot · 2025-07-02T14:13:36Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32857.

Reviews

See the guideline for information on the review process.
A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34170 (fuzz: Extend scriptpubkeyman coverage by Chand-ra)
#33008 (wallet: support bip388 policy with external signer by Sjors)
#32958 (wallet/refactor: change PSBTError to PSBTResult and remove std::optionalcommon::PSBTResult and return common::PSBTResult by kevkevinpal)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

LLM Linter (✨ experimental)

Possible places where named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):

MutableTransactionSignatureCreator(valid_with_witness_tx, 0, 11 * CENT, {.sighash_type = SIGHASH_DEFAULT}) in src/test/txvalidationcache_tests.cpp
MutableTransactionSignatureCreator(tx, i, 11 * CENT, {.sighash_type = SIGHASH_DEFAULT}) in src/test/txvalidationcache_tests.cpp
walletcreatefundedpsbt([], [{...}], None, {"subtractFeeFromOutputs":[0], "fee_rate": fee_rate, "change_type": address_type}) in test/functional/wallet_taproot.py
walletcreatefundedpsbt([], [{...}], None, {"subtractFeeFromOutputs":[0], "fee_rate": fee_rate, "change_type": address_type}) in test/functional/wallet_taproot.py

^2026-01-05

DrahtBot · 2025-07-02T15:53:11Z

🚧 At least one of the CI tasks failed.
_{Task ARM, unit tests, no functional tests: https://github.com/bitcoin/bitcoin/runs/45219351756}
_{LLM reason (✨ experimental): Compilation error due to passing a bool where an int* is expected in fillPSBT function.}

Hints

Try to run the tests locally, according to the documentation. However, a CI failure may still
happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the
affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

fanquake · 2025-07-02T16:04:58Z

Why is one of the native Windows jobs failing to compile: https://github.com/bitcoin/bitcoin/actions/runs/16027562790/job/45219347987?pr=32857#step:10:553, but not the other: https://github.com/bitcoin/bitcoin/actions/runs/16027562790/job/45219348012?pr=32857 ?

Sjors · 2025-07-02T16:35:47Z

@fanquake because it's in the fuzzer code, I incorrectly changed one of the function signatures there.

Sjors · 2025-07-02T16:53:37Z

From the original description:

Under the hood it adds m_hide_script_paths to the HidingSigningProvider which triggers an early return in GetTaprootSpendData() when set.

It's also missing taproot_scripts which might be a bit too much. At least in initial testing Ledger wouldn't sign it.

I probably need to implement this at the SigningProvider level rather than HidingSigningProvider. The early return needs to be inside SignTaproot() right before // Try script path spending rather than in GetTaprootSpendData(). The goal isn't to hide leaf scripts from co-signers, only to make sure we don't sign them ourselves.

Sjors · 2025-07-02T18:31:02Z

Shoehorning this into SigningProvider got ugly real fast. So instead I endup up passing avoid_script_path along the call stack from SignPSBTInput to SignTaproot. Which is also ugly...

Well, it compiles and works. Thoughts on how to implement this elegantly?

Sjors · 2025-07-03T12:48:06Z

The walletprocesspsbt RPC now also has this option. Added test coverage by expanding wallet_taproot.py.

Sjors · 2025-07-03T14:01:34Z

I split off a non-behavior-changing commit that adds avoid_script_path to FillPSBT. I think it makes sense to pass this option here, although in general we may want to refactor FillPSBT to take an options struct instead of this ever expanding argument list. That could be an independent or prerequisite PR.

I then did the same for SignPSBTInput, where I also think struct would be better.

Rinse and repeat for ::SignTransaction.

Will continue later to further split 09334ae0bd7939ed7476eb2b36f3bf8085885fe7. It might make sense to add a property to BaseSignatureCreator or it subclasses?

Sjors · 2025-07-04T13:45:56Z

I opened #32876 and rebased this PR on top of it. That allowed me to drop 1486f05c8eded02d72690bf34d983a03e4ad748b + efc34a514b59cc1bab6e4d0f80c197481561c429.

Sjors · 2025-07-04T16:02:42Z

I expanded #32876 with SignOptions. This shrinks 93fb607af3141ff1e22693588f498764401fc2c4 to just e2a151c3dc35997ac0f515feb292fc4543093593 which is very small ... but very ugly: casting BaseSignatureCreator to MutableTransactionSignatureCreator to access its avoid_script_path option.

Replace the sign, finalize , bip32derivs and sighash_type arguments which are passed to FillPSBT() and SignPSBTInput() with a PSBTFillOptions struct. This makes it easier to add additional options later without large code churn, such as avoid_script_path proposed in bitcoin#32857. It also makes the use of default boolean options safer compared to positional arguments that can easily get mixed up.

Sjors · 2025-08-25T11:51:26Z

Holding off on rebasing until #32876 is merged or I have to touch it.

rkrux · 2025-10-06T12:39:52Z

Sidenote: currently it's unlikely you'll accidentally spend via an older() or after() script path. This is because you need to manually set the input sequence field in the send (etc.) RPC (and manually adjust the fee rate). Found out the hard way while testing 🤦 (there's currently no useful feedback when this happens).

Scenario that I have not tested but expect to behave in this way: 3 signers (A, B, C) form a MuSig (ABC) in the keypath and also have 3 non-Musig multi-sig script paths (AB, BC, AC) without any timelocks. They intend to MuSig sign in order from A to C to spend via keypath and start the signing flow first by producing nonces in the mentioned order, and then by adding partial signatures. By the time B adds its nonce, there'd would be a fully signed spendable script path? If yes, seems unusual to have this when the intent of the signers was for keypath spending, which makes a good case for this PR.

I did notice unconditional key and script path signing within SignTaproot function when I was reviewing #29675. We tried to simulate an intentional script path spending like above in #29675 by making A a no-signing wallet in the last test case in functional tests: ac599c4#diff-c94f9555a2569240d362a00a82764f4714f6c65283e3f96725cf2a1043a296b5R238. Though the script path in that case is using MuSig but the overall unconditional script signing principle is still applicable.

rkrux · 2025-10-06T12:42:43Z

If #32876 is not garnering reviews, can consider undrafting this PR itself because it contains 7 commits only.
Allowing skipping script path spending optionally seems reasonable to me and a ready-to-review PR is more inviting than a draft one.

Sjors · 2025-10-07T08:10:55Z

@rkrux I'm not too worried about reviews just yet, let's focus on getting #29675 in.

Replace the sign, finalize , bip32derivs and sighash_type arguments which are passed to FillPSBT() and SignPSBTInput() with a PSBTFillOptions struct. This makes it easier to add additional options later without large code churn, such as avoid_script_path proposed in bitcoin#32857. It also makes the use of default boolean options safer compared to positional arguments that can easily get mixed up.

Expand taproot tests to cover avoid_script_path in walletprocesspsbt. When avoiding script paths, there's no need for the workaround that increases fee_rate to compensate for the wallet's inability to estimate fees for script path spends. We use this to indirectly test that key path was used. We also check that taproot_script_path_sigs is not set. Finally, for transactions that can't be signed using their key path, we try again by allowing the script path. Additional test extended private keys were extracted from other tests.

DrahtBot added the Wallet label Jul 2, 2025

Sjors mentioned this pull request Jul 2, 2025

Fix failure case for MuSig2 signing LedgerHQ/app-bitcoin-new#340

Merged

DrahtBot added the CI failed label Jul 2, 2025

Sjors force-pushed the 2025/07/no_script_path branch from 967931f to 22013ba Compare July 2, 2025 16:44

Sjors force-pushed the 2025/07/no_script_path branch from 22013ba to 370081a Compare July 2, 2025 18:30

DrahtBot mentioned this pull request Jul 2, 2025

wallet: Remove ISMINE_WATCHONLY and watchonly from RPCs #32618

Merged

Sjors mentioned this pull request Jul 2, 2025

MuSig2 power Sjors/bitcoin#91

Open

This was referenced Jul 2, 2025

wallet: Remove isminetypes #32523

Merged

wallet, rpc: add anti-fee-sniping to send and sendall #28944

Merged

Sjors force-pushed the 2025/07/no_script_path branch from 370081a to 1c4dfcb Compare July 3, 2025 12:47

Sjors force-pushed the 2025/07/no_script_path branch from 1c4dfcb to 5542074 Compare July 3, 2025 13:55

DrahtBot removed the CI failed label Jul 3, 2025

Sjors mentioned this pull request Jul 4, 2025

refactor: use options struct for signing and PSBT operations #32876

Open

Sjors force-pushed the 2025/07/no_script_path branch from 5542074 to 4c6a1c7 Compare July 4, 2025 13:45

Sjors force-pushed the 2025/07/no_script_path branch from 4c6a1c7 to 5192880 Compare July 4, 2025 16:02

This was referenced Jul 4, 2025

wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys #29675

Merged

Implement BIP 370 PSBTv2 #21283

Open

DrahtBot mentioned this pull request Jul 29, 2025

wallet, rpc: add v3 transaction creation and wallet support #32896

Merged

9 tasks

DrahtBot added the Needs rebase label Jul 29, 2025

Sjors force-pushed the 2025/07/no_script_path branch from 50023a9 to db07d06 Compare July 29, 2025 18:52

DrahtBot removed the Needs rebase label Jul 29, 2025

DrahtBot added the Needs rebase label Aug 19, 2025

Sjors force-pushed the 2025/07/no_script_path branch from db07d06 to 0e70253 Compare October 16, 2025 07:09

DrahtBot removed the Needs rebase label Oct 16, 2025

This was referenced Oct 17, 2025

rpc: Handle -named argument parsing where '=' character is used #32821

Merged

wallet: remove redundant sighash calculation in Musig2 signing flow #33665

Closed

Sjors force-pushed the 2025/07/no_script_path branch from 0e70253 to 9b5ee07 Compare November 4, 2025 09:19

Sjors mentioned this pull request Nov 4, 2025

ci: spurious linter failure? #33773

Closed

DrahtBot added the Needs rebase label Nov 18, 2025

Sjors added 7 commits January 5, 2026 11:40

refactor: use SignOptions for SignTransaction

9c69ba1

refactor: use SignOptions for MutableTransactionSignatureCreator

27baec7

wallet: add option to avoid script path spends

92afe0e

rpc: add keypath_only to walletprocesspsbt

52b4ac7

rpc: add keypath_only to send

afe5cfa

Sjors force-pushed the 2025/07/no_script_path branch from 9b5ee07 to afe5cfa Compare January 5, 2026 04:47

DrahtBot removed the Needs rebase label Jan 5, 2026

DrahtBot mentioned this pull request Jan 5, 2026

fuzz: Extend scriptpubkeyman coverage #34170

Open

wallet: allow skipping script paths #32857

Are you sure you want to change the base?

wallet: allow skipping script paths #32857

Uh oh!

Conversation

Sjors commented Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

LLM Linter (✨ experimental)

Uh oh!

DrahtBot commented Jul 2, 2025

Uh oh!

fanquake commented Jul 2, 2025

Uh oh!

Sjors commented Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sjors commented Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sjors commented Jul 2, 2025

Uh oh!

Sjors commented Jul 3, 2025

Uh oh!

Sjors commented Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sjors commented Jul 4, 2025

Uh oh!

Sjors commented Jul 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sjors commented Aug 25, 2025

Uh oh!

rkrux commented Oct 6, 2025

Uh oh!

rkrux commented Oct 6, 2025

Uh oh!

Sjors commented Oct 7, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Sjors commented Jul 2, 2025 •

edited

Loading

DrahtBot commented Jul 2, 2025 •

edited

Loading

Sjors commented Jul 2, 2025 •

edited

Loading

Sjors commented Jul 2, 2025 •

edited

Loading

Sjors commented Jul 3, 2025 •

edited

Loading

Sjors commented Jul 4, 2025 •

edited

Loading