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
Stale ACK	rkrux, w0xlt

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)
#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 typos and grammar issues:

// If sign is true, transaction must previously have been filled -> // If sign is true, the transaction must have previously been filled [Fixes awkward grammar (“previously have been”) to keep the meaning clear.]
* them. Tries to sign if options.sign=true. -> * them. Tries to sign if options.sign is true. [Clarifies the boolean expression format for readability.]

2026-03-26 14:20:47

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

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.

66ce046ab6

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

refactor: use SignOptions for SignTransaction 3d2916607a

refactor: use SignOptions for MutableTransactionSignatureCreator bf58f912e0

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