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

Sjors · 2025-07-04T12:40:44Z

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 · 2025-07-04T12:40:49Z

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

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

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{spend, 0, CAmount{0}, &txdata, {.sighash_type = SIGHASH_DEFAULT}} in src/test/descriptor_tests.cpp
SignPSBTInput(HidingSigningProvider(keys.get(), /hide_secret=/!options.sign, /hide_origin=/!options.bip32_derivs), psbtx, i, &txdata, options, nullptr) in src/wallet/scriptpubkeyman.cpp

^2026-01-05

DrahtBot · 2025-07-04T12:45:30Z

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

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.

DrahtBot · 2025-07-04T15:59:38Z

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

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.

Sjors · 2025-07-04T16:08:07Z

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.

optout21 · 2025-07-06T17:40:58Z

src/rpc/rawtransaction_util.cpp

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


Nit: Maybe options could be inlined here?

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

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

rkrux

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.

src/common/types.h

rkrux

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.

src/node/psbt.cpp

rkrux · 2025-07-11T08:22:17Z

src/common/types.h

+    /**
+     * Whether to fill in bip32 derivation information if available.
+     */
+    bool bip32_derivs{true};


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.

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.

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.

rkrux · 2025-07-11T08:26:57Z

src/wallet/external_signer_scriptpubkeyman.h

-  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;
+  std::optional<common::PSBTError> FillPSBT(PartiallySignedTransaction& psbt, const PrecomputedTransactionData& txdata, common::PSBTFillOptions options, int* n_signed = nullptr) const override;


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?

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.

src/wallet/scriptpubkeyman.h

src/rpc/rawtransaction.cpp

Sjors · 2025-07-11T10:00:14Z

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

using SIGHASH_ALL instead of 1: see refactor: use options struct for signing and PSBT operations #32876 (comment)
making sure bip32derivs is explicitly set everywhere (added SignTransaction in fee_bumper.cpp) because the default changed: see refactor: use options struct for signing and PSBT operations #32876 (comment)

rkrux

LGTM ACK 7092541

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

git range-diff d94c02e...7092541

rkrux · 2025-07-11T15:05:38Z

src/common/types.h

+    /**
+     * Whether to fill in bip32 derivation information if available.
+     */
+    bool bip32_derivs{true};


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.

Sjors · 2025-10-16T06:45:14Z

Rebased after #29675.

Sjors · 2025-11-04T08:50:15Z

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

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.

DrahtBot added the Refactoring label Jul 4, 2025

DrahtBot added the CI failed label Jul 4, 2025

Sjors force-pushed the 2025/07/fill-psbt-struct branch 2 times, most recently from edbb5d8 to 2829cb7 Compare July 4, 2025 13:09

Sjors mentioned this pull request Jul 4, 2025

wallet: allow skipping script paths #32857

Draft

3 tasks

DrahtBot removed the CI failed label Jul 4, 2025

Sjors changed the title ~~refactor: use PSBTOptions for filling and signing~~ refactor: use options struct for signing and PSBT operations Jul 4, 2025

Sjors force-pushed the 2025/07/fill-psbt-struct branch from 22ceffe to f9cf412 Compare July 4, 2025 15:59

DrahtBot added the CI failed label Jul 4, 2025

DrahtBot removed the CI failed label Jul 4, 2025

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

optout21 reviewed Jul 6, 2025

View reviewed changes

Sjors force-pushed the 2025/07/fill-psbt-struct branch from f9cf412 to 99e7ec9 Compare July 8, 2025 07:12

rkrux reviewed Jul 8, 2025

View reviewed changes

src/common/types.h Outdated Show resolved Hide resolved

Sjors force-pushed the 2025/07/fill-psbt-struct branch from 99e7ec9 to d94c02e Compare July 8, 2025 11:26

DrahtBot mentioned this pull request Jul 10, 2025

Introduce per-txin sighash midstate cache for legacy/p2sh/segwitv0 scripts #32473

Merged

rkrux reviewed Jul 11, 2025

View reviewed changes

Sjors force-pushed the 2025/07/fill-psbt-struct branch from d94c02e to 7092541 Compare July 11, 2025 09:56

rkrux approved these changes Jul 11, 2025

View reviewed changes

This was referenced Jul 13, 2025

wallet/refactor: change PSBTError to PSBTResult and remove std::optional<common::PSBTResult> and return common::PSBTResult #32958

Open

wallet: support bip388 policy with external signer #33008

Draft

DrahtBot added the Needs rebase label Oct 14, 2025

Sjors force-pushed the 2025/07/fill-psbt-struct branch from 7092541 to c4fb58a Compare October 16, 2025 06:45

DrahtBot removed the Needs rebase label Oct 16, 2025

DrahtBot mentioned this pull request Oct 21, 2025

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

Closed

Sjors force-pushed the 2025/07/fill-psbt-struct branch from c4fb58a to a4169a3 Compare November 4, 2025 08:49

Sjors mentioned this pull request Nov 4, 2025

ci: spurious linter failure? #33773

Closed

DrahtBot added CI failed and removed CI failed labels Nov 4, 2025

DrahtBot mentioned this pull request Dec 29, 2025

fuzz: Extend scriptpubkeyman coverage #34170

Open

Sjors added 3 commits January 5, 2026 11:40

refactor: use SignOptions for SignTransaction

9c69ba1

refactor: use SignOptions for MutableTransactionSignatureCreator

27baec7

Sjors force-pushed the 2025/07/fill-psbt-struct branch from a4169a3 to 27baec7 Compare January 5, 2026 04:40

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

		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;
		std::optional<common::PSBTError> FillPSBT(PartiallySignedTransaction& psbt, const PrecomputedTransactionData& txdata, common::PSBTFillOptions options, int* n_signed = nullptr) const override;

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

Are you sure you want to change the base?

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

Uh oh!

Conversation

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

Uh oh!

Uh oh!

DrahtBot commented Jul 4, 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 4, 2025

Uh oh!

DrahtBot 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!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rkrux left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rkrux left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

rkrux left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sjors commented Oct 16, 2025

Uh oh!

Sjors commented Nov 4, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Sjors commented Jul 4, 2025 •

edited

Loading

DrahtBot commented Jul 4, 2025 •

edited

Loading

Sjors commented Jul 4, 2025 •

edited

Loading

Sjors Jul 7, 2025 •

edited

Loading

Sjors Jul 11, 2025 •

edited

Loading

Sjors commented Jul 11, 2025 •

edited

Loading