refactor: use options struct for signing and PSBT operations

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.

0struct PSBTFillOptions {
1    bool sign{true};
2    std::optional<int> sighash_type{std::nullopt};
3    bool finalize{true};
4    bool bip32_derivs{true};
5};

Additionally this PR introduces:

0struct SignOptions {
1    int sighash_type{SIGHASH_DEFAULT};
2};

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

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

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

Some jobs get confused without this:

 0diff --git a/src/rpc/rawtransaction_util.cpp b/src/rpc/rawtransaction_util.cpp
 1index 54f3d2d199..db8f223c7a 100644
 2--- a/src/rpc/rawtransaction_util.cpp
 3+++ b/src/rpc/rawtransaction_util.cpp
 4@@ -312,7 +312,8 @@ void SignTransaction(CMutableTransaction& mtx, const SigningProvider* keystore,
 5     // Script verification errors
 6     std::map<int, bilingual_str> input_errors;
 7
 8-    bool complete = SignTransaction(mtx, keystore, coins, {.sighash_type = *nHashType}, input_errors);
 9+    SignOptions options{.sighash_type = *nHashType};
10+    bool complete = SignTransaction(mtx, keystore, coins, options, input_errors);
11     SignTransactionResultToJSON(mtx, complete, coins, input_errors, result);
12 }

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

 0[11:28:05.798] /usr/lib/llvm-16/bin/../include/c++/v1/string:650:31: error: cannot form a reference to 'void'
 1[11:28:05.798]       is_convertible<const _Tp&, basic_string_view<_CharT, _Traits> >::value &&
 2[11:28:05.798]                               ^
 3[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
 4[11:28:05.798]             class = __enable_if_t<__can_be_converted_to_string_view<_CharT, _Traits, _Tp>::value &&
 5[11:28:05.798]                                   ^
 6[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
 7[11:28:05.798]   _LIBCPP_METHOD_TEMPLATE_IMPLICIT_INSTANTIATION_VIS _LIBCPP_CONSTEXPR_SINCE_CXX20 explicit basic_string(
 8[11:28:05.798]                                                                                             ^~~~~~~~~~~~~
 9[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)]
10[11:28:05.798] inline constexpr bool is_constructible_v = __is_constructible(_Tp, _Args...);
11[11:28:05.798]                                            ^
12[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
13[11:28:05.798]                                    std::is_constructible_v<std::string, T>,        // setStr
14[11:28:05.798]                                         ^
15[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 &>>]
16[11:28:05.798]     UniValue(Ref&& val)
17[11:28:05.798]     ^~~~~~~~~~~~~~~~~~~
18[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)]
19[11:28:05.798]     bool complete = SignTransaction(mtx, keystore, coins, {.sighash_type = *nHashType}, input_errors);
20[11:28:05.798]                                                           ^
21[11:28:05.798] 1 error generated.

DrahtBot removed the label CI failed on Jul 4, 2025

in src/rpc/rawtransaction_util.cpp:317 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)

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:

0- if (SignPSBTInput(DUMMY_SIGNING_PROVIDER, psbtx, i, nullptr, std::nullopt) != PSBTError::OK || !psbtx.GetInputUTXO(newcoin.out, i)) {
1+ 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.

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.

8660b55c88

refactor: use SignOptions for SignTransaction 7b6efb3e98

refactor: use SignOptions for MutableTransactionSignatureCreator 7092541631

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:

0git range-diff d94c02e...7092541

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

Code Coverage & Benchmarks

Reviews

Conflicts