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

Sjors commented at 12:40 PM on July 4, 2025: member

Replace the sign, finalize , bip32derivs and sighash_type arguments that are passed to FillPSBT() and SignPSBTInput() with a PSBTFillOptions struct.

struct PSBTFillOptions {
    bool sign{true};
    std::optional<int> sighash_type{std::nullopt};
    bool finalize{true};
    bool bip32_derivs{true};
};

Additionally this PR introduces:

struct SignOptions {
    int sighash_type{SIGHASH_DEFAULT};
};

This is used by SignTransaction and the MutableTransactionSignatureCreator constructor.

These changes make it easier to add additional options later without large code churn, such as avoid_script_path proposed in #32857. It also makes the use of default boolean options safer compared to positional arguments that can easily get mixed up.

DrahtBot added the label Refactoring on Jul 4, 2025

DrahtBot commented at 12:40 PM on July 4, 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/32876.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	w0xlt, optout21, sedited
Stale ACK	rkrux

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34969 (fuzz: several improvements to scriptpubkeyman harness by brunoerg)
#33008 (wallet: support bip388 policy with external signer by Sjors)
#32958 (wallet/refactor: Update SignPSBTInput to return util::Expected<void, PSBTError> and remove PSBTError:Ok by kevkevinpal)
#32857 (wallet: allow skipping script paths by Sjors)
#21283 (Implement BIP 370 PSBTv2 by achow101)

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 4, 2025

DrahtBot commented at 12:45 PM on July 4, 2025: contributor

🚧 At least one of the CI tasks failed. Task tidy: https://github.com/bitcoin/bitcoin/runs/45364866454 LLM reason (✨ experimental): The CI failure is due to a compilation error caused by incorrect field initialization order in a C++ struct.

<details><summary>Hints</summary>

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.

</details>

Sjors force-pushed on Jul 4, 2025

DrahtBot removed the label CI failed on Jul 4, 2025

Sjors renamed this:
~~refactor: use PSBTOptions for filling and signing~~
refactor: use options struct for signing and PSBT operations
on Jul 4, 2025

Sjors force-pushed on Jul 4, 2025

DrahtBot added the label CI failed on Jul 4, 2025

DrahtBot commented at 3:59 PM on July 4, 2025: contributor

🚧 At least one of the CI tasks failed. Task no wallet, libbitcoinkernel: https://github.com/bitcoin/bitcoin/runs/45374155547 LLM reason (✨ experimental): The build failed due to a compilation error caused by incompatible template instantiation involving 'void' in the string header.

<details><summary>Hints</summary>

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.

</details>

Sjors commented at 4:08 PM on July 4, 2025: member

Some jobs get confused without this:

diff --git a/src/rpc/rawtransaction_util.cpp b/src/rpc/rawtransaction_util.cpp
index 54f3d2d199..db8f223c7a 100644
--- a/src/rpc/rawtransaction_util.cpp
+++ b/src/rpc/rawtransaction_util.cpp
@@ -312,7 +312,8 @@ void SignTransaction(CMutableTransaction& mtx, const SigningProvider* keystore,
     // Script verification errors
     std::map<int, bilingual_str> input_errors;

-    bool complete = SignTransaction(mtx, keystore, coins, {.sighash_type = *nHashType}, input_errors);
+    SignOptions options{.sighash_type = *nHashType};
+    bool complete = SignTransaction(mtx, keystore, coins, options, input_errors);
     SignTransactionResultToJSON(mtx, complete, coins, input_errors, result);
 }

See e.g. https://cirrus-ci.com/task/6541653541912576:

[11:28:05.798] /usr/lib/llvm-16/bin/../include/c++/v1/string:650:31: error: cannot form a reference to 'void'
[11:28:05.798]       is_convertible<const _Tp&, basic_string_view<_CharT, _Traits> >::value &&
[11:28:05.798]                               ^
[11:28:05.798] /usr/lib/llvm-16/bin/../include/c++/v1/string:967:35: note: in instantiation of template class 'std::__can_be_converted_to_string_view<char, std::char_traits<char>, void>' requested here
[11:28:05.798]             class = __enable_if_t<__can_be_converted_to_string_view<_CharT, _Traits, _Tp>::value &&
[11:28:05.798]                                   ^
[11:28:05.798] /usr/lib/llvm-16/bin/../include/c++/v1/string:969:93: note: in instantiation of default argument for 'basic_string<void>' required here
[11:28:05.798]   _LIBCPP_METHOD_TEMPLATE_IMPLICIT_INSTANTIATION_VIS _LIBCPP_CONSTEXPR_SINCE_CXX20 explicit basic_string(
[11:28:05.798]                                                                                             ^~~~~~~~~~~~~
[11:28:05.798] /usr/lib/llvm-16/bin/../include/c++/v1/__type_traits/is_constructible.h:28:44: note: while substituting deduced template arguments into function template 'basic_string' [with _Tp = void, $1 = (no value)]
[11:28:05.798] inline constexpr bool is_constructible_v = __is_constructible(_Tp, _Args...);
[11:28:05.798]                                            ^
[11:28:05.798] /ci_container_base/src/univalue/include/univalue.h:37:41: note: in instantiation of variable template specialization 'std::is_constructible_v<std::string, void>' requested here
[11:28:05.798]                                    std::is_constructible_v<std::string, T>,        // setStr
[11:28:05.798]                                         ^
[11:28:05.798] /ci_container_base/src/univalue/include/univalue.h:39:5: note: while substituting prior template arguments into non-type template parameter [with Ref = void &, T = std::remove_cv_t<std::remove_reference_t<void &>>]
[11:28:05.798]     UniValue(Ref&& val)
[11:28:05.798]     ^~~~~~~~~~~~~~~~~~~
[11:28:05.798] /ci_container_base/src/rpc/rawtransaction_util.cpp:315:59: note: while substituting deduced template arguments into function template 'UniValue' [with Ref = void &, T = (no value), $2 = (no value)]
[11:28:05.798]     bool complete = SignTransaction(mtx, keystore, coins, {.sighash_type = *nHashType}, input_errors);
[11:28:05.798]                                                           ^
[11:28:05.798] 1 error generated.

DrahtBot removed the label CI failed on Jul 4, 2025

in src/rpc/rawtransaction_util.cpp:316 in f9cf412e7c outdated

 311 | @@ -312,7 +312,8 @@ void SignTransaction(CMutableTransaction& mtx, const SigningProvider* keystore,
 312 |      // Script verification errors
 313 |      std::map<int, bilingual_str> input_errors;
 314 |  
 315 | -    bool complete = SignTransaction(mtx, keystore, coins, *nHashType, input_errors);
 316 | +    SignOptions options{.sighash_type = *nHashType};
 317 | +    bool complete = SignTransaction(mtx, keystore, coins, options, input_errors);

optout21 commented at 5:40 PM on July 6, 2025:

Nit: Maybe options could be inlined here?

Sjors commented at 7:09 AM on July 7, 2025:

@optout21 see #32876 (comment) (also added a comment to the PR description, will add a code comment on the next push)

maflcko commented at 8:34 AM on November 4, 2025:

nit: now that the clang minimum is clang-17, you could revert the workaround again, if you want

Sjors commented at 8:50 AM on November 4, 2025:

Done!

Sjors force-pushed on Jul 8, 2025

in src/common/types.h:31 in 2923087479 outdated

  24 | @@ -23,6 +25,31 @@ enum class PSBTError {
  25 |      INCOMPLETE,
  26 |      OK,
  27 |  };
  28 | +/**
  29 | + * Instructions for how a PSBT should be signed or filled with information.
  30 | + */
  31 | +struct PSBTOptions {

rkrux commented at 10:18 AM on July 8, 2025:

In 2923087479cce6117aa3f88a5476942d4af757b9 "refactor: use PSBTOptions for filling and signing"

PSBTOptions seems quite generic to me, maybe PSBTFillingOptions? The doc above also states something similar.

Sjors commented at 11:26 AM on July 8, 2025:

Renamed to PSBTFillOptions

rkrux commented at 10:21 AM on July 8, 2025: contributor

Concept ACK 99e7ec91322abc809d343a83b31d82307f827f7f

I, too, usually prefer grouping together related items in a struct. In favour of PSBTOptions grouping as ISTM that it cleans up the code a bit. SignOptions could also prove to be helpful with the incoming changes in future PRs.

Sjors force-pushed on Jul 8, 2025

in src/node/psbt.cpp:127 in 576da4c7dd outdated

 123 | @@ -124,7 +124,7 @@ PSBTAnalysis AnalyzePSBT(PartiallySignedTransaction psbtx)
 124 |              PSBTInput& input = psbtx.inputs[i];
 125 |              Coin newcoin;
 126 |  
 127 | -            if (SignPSBTInput(DUMMY_SIGNING_PROVIDER, psbtx, i, nullptr, std::nullopt) != PSBTError::OK || !psbtx.GetInputUTXO(newcoin.out, i)) {
 128 | +            if (SignPSBTInput(DUMMY_SIGNING_PROVIDER, psbtx, i, nullptr, {}) != PSBTError::OK || !psbtx.GetInputUTXO(newcoin.out, i)) {

rkrux commented at 8:15 AM on July 11, 2025:

In 576da4c7dd83f7ab871bac9d76685d112e60e4ab "refactor: use PSBTFillOptions for filling and signing"

Nit if retouched:

- if (SignPSBTInput(DUMMY_SIGNING_PROVIDER, psbtx, i, nullptr, std::nullopt) != PSBTError::OK || !psbtx.GetInputUTXO(newcoin.out, i)) {
+ if (SignPSBTInput(DUMMY_SIGNING_PROVIDER, psbtx, i, nullptr, /*options=*/{}) != PSBTError::OK || !psbtx.GetInputUTXO(newcoin.out, i)) {

Sjors commented at 9:56 AM on July 11, 2025:

Done

in src/common/types.h:50 in 576da4c7dd outdated

  45 | +    bool finalize{true};
  46 | +
  47 | +    /**
  48 | +     * Whether to fill in bip32 derivation information if available.
  49 | +     */
  50 | +    bool bip32_derivs{true};

rkrux commented at 8:22 AM on July 11, 2025:

In 576da4c7dd83f7ab871bac9d76685d112e60e4ab "refactor: use PSBTFillOptions for filling and signing"

Opinionated supernit if retouched: For variables that are supposed to take action, names starting with a verb is easier to relate to imho. Eg: sign & finalize in this struct. So, maybe derive_bip32.

Sjors commented at 9:37 AM on July 11, 2025:

Although I agree that's a better name, renaming would introduce (even) more churn. And it would either be a breaking change with the RPC or introduce two different names for the same thing.

rkrux commented at 3:05 PM on July 11, 2025:

introduce two different names for the same thing.

Yeah, this is what I had in mind & would prefer this over breaking change with the RPC. But fine to leave it as-is to avoid more churn.

in src/wallet/external_signer_scriptpubkeyman.h:39 in 576da4c7dd outdated

  35 | @@ -36,7 +36,7 @@ class ExternalSignerScriptPubKeyMan : public DescriptorScriptPubKeyMan
  36 |    */
  37 |   util::Result<void> DisplayAddress(const CTxDestination& dest, const ExternalSigner& signer) const;
  38 |  
  39 | -  std::optional<common::PSBTError> FillPSBT(PartiallySignedTransaction& psbt, const PrecomputedTransactionData& txdata, std::optional<int> sighash_type = std::nullopt, bool sign = true, bool bip32derivs = false, int* n_signed = nullptr, bool finalize = true) const override;
  40 | +  std::optional<common::PSBTError> FillPSBT(PartiallySignedTransaction& psbt, const PrecomputedTransactionData& txdata, common::PSBTFillOptions options, int* n_signed = nullptr) const override;

rkrux commented at 8:26 AM on July 11, 2025:

In 576da4c7dd83f7ab871bac9d76685d112e60e4ab "refactor: use PSBTFillOptions for filling and signing"

ISTM that the default value of bip32derivs is switched from false to true with the use of PSBTFillOptions here. Is this intended?

Sjors commented at 9:56 AM on July 11, 2025:

Every call to FillPSBT( sets bip32derivs explicitly so this change shouldn't matter.

Except in src/wallet/feebumper.cpp where I originally dropped /*bip32derivs=*/true. I brought that back for clarity.

in src/wallet/scriptpubkeyman.h:376 in 576da4c7dd outdated

 372 | @@ -373,7 +373,7 @@ class DescriptorScriptPubKeyMan : public ScriptPubKeyMan
 373 |  
 374 |      bool SignTransaction(CMutableTransaction& tx, const std::map<COutPoint, Coin>& coins, int sighash, std::map<int, bilingual_str>& input_errors) const override;
 375 |      SigningResult SignMessage(const std::string& message, const PKHash& pkhash, std::string& str_sig) const override;
 376 | -    std::optional<common::PSBTError> FillPSBT(PartiallySignedTransaction& psbt, const PrecomputedTransactionData& txdata, std::optional<int> sighash_type = std::nullopt, bool sign = true, bool bip32derivs = false, int* n_signed = nullptr, bool finalize = true) const override;
 377 | +    std::optional<common::PSBTError> FillPSBT(PartiallySignedTransaction& psbt, const PrecomputedTransactionData& txdata, common::PSBTFillOptions options, int* n_signed = nullptr) const override;

rkrux commented at 8:40 AM on July 11, 2025:

In 576da4c7dd83f7ab871bac9d76685d112e60e4ab "refactor: use PSBTFillOptions for filling and signing"

This also does the same change for bip32derivs default value in SPKMs. Intended?

in src/rpc/rawtransaction.cpp:700 in d94c02e109 outdated

 696 | @@ -697,7 +697,7 @@ static RPCHelpMan combinerawtransaction()
 697 |                  sigdata.MergeSignatureData(DataFromTransaction(txv, i, coin.out));
 698 |              }
 699 |          }
 700 | -        ProduceSignature(DUMMY_SIGNING_PROVIDER, MutableTransactionSignatureCreator(mergedTx, i, coin.out.nValue, 1), coin.out.scriptPubKey, sigdata);
 701 | +        ProduceSignature(DUMMY_SIGNING_PROVIDER, MutableTransactionSignatureCreator(mergedTx, i, coin.out.nValue, {.sighash_type = 1}), coin.out.scriptPubKey, sigdata);

rkrux commented at 8:57 AM on July 11, 2025:

In d94c02e109617a7a4d31a5f03d6f5221f2faf878 "refactor: use SignOptions for MutableTransactionSignatureCreator"

Possible to use SIGHASH_ALL here instead of 1 now?

rkrux commented at 9:00 AM on July 11, 2025: contributor

Code review d94c02e109617a7a4d31a5f03d6f5221f2faf878

Mostly lgtm, asked couple questions along with couple nits.

Using SIGHASH_DEFAULT as the default value in this PR instead of relying on int sighash being 0 as done previously is much better from readability POV.

Sjors force-pushed on Jul 11, 2025

Sjors commented at 10:00 AM on July 11, 2025: member

Rebased (just in case after ##32930) and addressed comments, mainly:

using SIGHASH_ALL instead of 1: see #32876 (review)
making sure bip32derivs is explicitly set everywhere (added SignTransaction in fee_bumper.cpp) because the default changed: see #32876 (review)

rkrux approved

rkrux commented at 3:07 PM on July 11, 2025: contributor

LGTM ACK 70925416317683b0a62a9c3b45df6385e9c5b292

Reviewed range-diff this time, thanks for incorporating minor points:

git range-diff d94c02e...7092541

DrahtBot added the label Needs rebase on Oct 14, 2025

Sjors force-pushed on Oct 16, 2025

Sjors commented at 6:45 AM on October 16, 2025: member

Rebased after #29675.

DrahtBot removed the label Needs rebase on Oct 16, 2025

Sjors force-pushed on Nov 4, 2025

Sjors commented at 8:50 AM on November 4, 2025: member

Rebased past #33555 to get rid of the ancient clang workaround, see #32876 (review).

DrahtBot added the label CI failed on Nov 4, 2025

DrahtBot removed the label CI failed on Nov 4, 2025

Sjors force-pushed on Jan 5, 2026

Sjors force-pushed on Jan 22, 2026

Sjors commented at 3:16 PM on January 22, 2026: member

Rebased to pre-empt tidy complaints after #31650.

in src/wallet/scriptpubkeyman.cpp:1379 in 56cd31ee3c outdated

1375 | @@ -1376,13 +1376,13 @@ std::optional<PSBTError> DescriptorScriptPubKeyMan::FillPSBT(PartiallySignedTran
1376 |              }
1377 |          }
1378 |  
1379 | -        PSBTError res = SignPSBTInput(HidingSigningProvider(keys.get(), /*hide_secret=*/!sign, /*hide_origin=*/!bip32derivs), psbtx, i, &txdata, sighash_type, nullptr, finalize);
1380 | +        PSBTError res = SignPSBTInput(HidingSigningProvider(keys.get(), /*hide_secret=*/!options.sign, /*hide_origin=*/!options.bip32_derivs), psbtx, i, &txdata, options, nullptr);

w0xlt commented at 8:47 PM on March 3, 2026:

nit:

        PSBTError res = SignPSBTInput(HidingSigningProvider(keys.get(), /*hide_secret=*/!options.sign, /*hide_origin=*/!options.bip32_derivs), psbtx, i, &txdata, options, /*out_sigdata=*/nullptr);

in src/psbt.cpp:561 in 56cd31ee3c outdated

 557 | @@ -558,7 +558,7 @@ bool FinalizePSBT(PartiallySignedTransaction& psbtx)
 558 |      const PrecomputedTransactionData txdata = PrecomputePSBTData(psbtx);
 559 |      for (unsigned int i = 0; i < psbtx.tx->vin.size(); ++i) {
 560 |          PSBTInput& input = psbtx.inputs.at(i);
 561 | -        complete &= (SignPSBTInput(DUMMY_SIGNING_PROVIDER, psbtx, i, &txdata, input.sighash_type, nullptr, true) == PSBTError::OK);
 562 | +        complete &= (SignPSBTInput(DUMMY_SIGNING_PROVIDER, psbtx, i, &txdata, {.sighash_type = input.sighash_type, .finalize = true}, nullptr) == PSBTError::OK);

w0xlt commented at 8:48 PM on March 3, 2026:

nit:

        complete &= (SignPSBTInput(DUMMY_SIGNING_PROVIDER, psbtx, i, &txdata, {.sighash_type = input.sighash_type, .finalize = true}, /*out_sigdata=*/nullptr) == PSBTError::OK);

w0xlt commented at 8:48 PM on March 3, 2026: contributor

ACK 56cd31ee3ce4391a01e9937d1399f1d4d50c0b4b

DrahtBot requested review from rkrux on Mar 3, 2026

DrahtBot added the label Needs rebase on Mar 25, 2026

Sjors force-pushed on Mar 26, 2026

Sjors commented at 2:09 PM on March 26, 2026: member

Rebased after #34920.

Sjors marked this as a draft on Mar 26, 2026

Sjors force-pushed on Mar 26, 2026

DrahtBot added the label CI failed on Mar 26, 2026

Sjors marked this as ready for review on Mar 26, 2026

Sjors commented at 2:59 PM on March 26, 2026: member

Also fixed silent conflict with #34472

DrahtBot removed the label CI failed on Mar 26, 2026

DrahtBot removed the label Needs rebase on Mar 26, 2026

DrahtBot added the label Needs rebase on Apr 19, 2026

Sjors commented at 8:26 AM on April 22, 2026: member

Rebased after #35038. Also applied @w0xlt's suggestions from #32876#pullrequestreview-3884964887.

Sjors force-pushed on Apr 22, 2026

DrahtBot removed the label Needs rebase on Apr 23, 2026

sedited requested review from w0xlt on Apr 26, 2026

sedited requested review from optout21 on Apr 26, 2026

in src/script/sign.cpp:1009 in 8bb71b7de3

1005 | @@ -1006,7 +1006,7 @@ bool IsSegWitOutput(const SigningProvider& provider, const CScript& script)
1006 |      return false;
1007 |  }
1008 |  
1009 | -bool SignTransaction(CMutableTransaction& mtx, const SigningProvider* keystore, const std::map<COutPoint, Coin>& coins, const SignOptions options, std::map<int, bilingual_str>& input_errors)
1010 | +bool SignTransaction(CMutableTransaction& mtx, const SigningProvider* keystore, const std::map<COutPoint, Coin>& coins, SignOptions options, std::map<int, bilingual_str>& input_errors)

optout21 commented at 11:03 AM on April 28, 2026:

8bb71b7 refactor: use SignOptions for MutableTransactionSignatureCreator:

I would prefer to see const SignOptions& parameter type here (and in all other usages) as well. The struct has only an int field, but it may be expanded later.

in src/interfaces/wallet.h:206 in b3788bf2fc

 202 | @@ -202,9 +203,7 @@ class Wallet
 203 |          int& num_blocks) = 0;
 204 |  
 205 |      //! Fill PSBT.
 206 | -    virtual std::optional<common::PSBTError> fillPSBT(std::optional<int> sighash_type,
 207 | -        bool sign,
 208 | -        bool bip32derivs,
 209 | +    virtual std::optional<common::PSBTError> fillPSBT(common::PSBTFillOptions options,

optout21 commented at 11:05 AM on April 28, 2026:

b3788bf refactor: use PSBTFillOptions for filling and signing:

I would prefer to see const common::PSBTFillOptions& parameter type here (and in all other usages as well). The struct has several fields, passing by reference is more efficient.

optout21 commented at 11:16 AM on April 28, 2026: contributor

Reviewed 8bb71b7de3ad6951cb6deb55a17decfb9a73cecf, LGTM! Left comments about passing the new structs by reference. Happy to ack if this concern is responded to or adapted!

refactor: use PSBTFillOptions for filling and signing

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 #32857. It also makes the
use of default boolean options safer compared to positional arguments
that can easily get mixed up.

dc4a5d1270

refactor: use SignOptions for SignTransaction 5ed41752c5

refactor: use SignOptions for MutableTransactionSignatureCreator eab72d14d7

Sjors force-pushed on Apr 28, 2026

Sjors commented at 3:45 PM on April 28, 2026: member

Rebased (just in case) and implemented @optout21's suggestion for const reference. I doubt this struct will ever get big, but it doesn't hurt either.

w0xlt commented at 5:26 PM on April 28, 2026: contributor

reACK eab72d14d79a0ec3f66f247c083d6acfda2ea5b0

optout21 commented at 7:07 AM on April 29, 2026: contributor

ACK eab72d14d79a0ec3f66f247c083d6acfda2ea5b0

A relatively straightforward (logically, at least) refactor, replacing a group of method arguments with a struct. Benefits:

more readable method signatures
less verbose passing the same parameters to the next methods (also more efficient)
intent is more clearly visible in cases where only one or two parameters are set to non-default values
easier extension for future needs (only struct is touched, not all method signatures).

sedited approved

sedited commented at 1:44 PM on April 29, 2026: contributor

ACK eab72d14d79a0ec3f66f247c083d6acfda2ea5b0

sedited merged this on Apr 29, 2026

sedited closed this on Apr 29, 2026

Sjors deleted the branch on Apr 30, 2026