wallet: allow skipping script paths

Sjors commented at 2:13 pm on July 2, 2025: member

While testing MuSig2 integration in #29675 I ran into a Ledger bug https://github.com/LedgerHQ/app-bitcoin-new/issues/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 and 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 https://github.com/Sjors/bitcoin/pull/91 and a slightly modified Moosig. See https://github.com/LedgerHQ/app-bitcoin-new/issues/329 for the rough steps.

TODO:

refactor (how?)
add test coverage for send

Potential followups:

make this default for musig() (after #29675)

DrahtBot added the label Wallet on Jul 2, 2025

DrahtBot commented at 2:13 pm on July 2, 2025: contributor

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:

#32618 (wallet: Remove ISMINE_WATCHONLY and watchonly from RPCs by achow101)
#32523 (wallet: Remove watchonly behavior and isminetypes by achow101)
#28944 (wallet, rpc: add anti-fee-sniping to send and sendall by ishaanam)

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.

DrahtBot added the label CI failed on Jul 2, 2025

DrahtBot commented at 3:53 pm on July 2, 2025: contributor

🚧 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.

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 commented at 4:04 pm on July 2, 2025: member

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 commented at 4:35 pm on July 2, 2025: member

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

Sjors force-pushed on Jul 2, 2025

Sjors commented at 4:53 pm on July 2, 2025: member

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 force-pushed on Jul 2, 2025

Sjors commented at 6:31 pm on July 2, 2025: member

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 force-pushed on Jul 3, 2025

Sjors commented at 12:48 pm on July 3, 2025: member

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

wallet: add avoid_script_path to FillPSBT()

This commit does not change behavior.

1486f05c8e

wallet: add avoid_script_path to SignPSBTInput()

This commit does not change behavior.

efc34a514b

Add avoid_script_path to ::SignTransaction

This commit does not change behavior.

ac2854456d

wallet: add option to avoid script path spends 09334ae0bd

rpc: add keypath_only to walletprocesspsbt 87a9768793

test: cover keypath_only in wallet_taproot.py

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.

99bab2b501

rpc: add keypath_only to send 5542074a00

Sjors force-pushed on Jul 3, 2025

Sjors commented at 2:01 pm on July 3, 2025: member

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?

DrahtBot removed the label CI failed on Jul 3, 2025

wallet: allow skipping script paths #32857

Code Coverage & Benchmarks

Reviews

Conflicts