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.

The options structs are usually passed inline, except in src/rpc/rawtransaction_util.cpp where otherwise LLVM 16 gets confused.

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
ACK	rkrux

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#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)
#29675 (wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys by achow101)
#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 · 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)

rkrux

Concept ACK 99e7ec9

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.

rkrux · 2025-07-08T10:18:29Z

src/common/types.h

+/**
+ * Instructions for how a PSBT should be signed or filled with information.
+ */
+struct PSBTOptions {


In 2923087 "refactor: use PSBTOptions for filling and signing"

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

Renamed to PSBTFillOptions

rkrux

Code review d94c02e

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 576da4c "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 576da4c "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

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

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

Open

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

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 added 3 commits July 11, 2025 11:56

refactor: use SignOptions for SignTransaction

7b6efb3

refactor: use SignOptions for MutableTransactionSignatureCreator

7092541

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

		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

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!

rkrux left a comment

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!

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!

Uh oh!

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