psbt: add non-default sighash types to PSBTs and unify sighash type match checking

achow101 commented at 2:09 am on January 9, 2025: member

Currently, we do not add the sighash field to PSBTs at all, even when we have signed with a non-default sighash. This PR changes the behavior such that when we (attempt to) sign with a sighash other than DEFAULT or ALL, the sighash type field will be added to the PSBT to inform the later signers that a different sighash type was used by a signer. Notably, this is necessary for MuSig2 support as all signers must sign using the same sighash type, but the sighash is not provided in partial signatures.

Furthermore, because the sighash type can also be provided on the command line, we require that if both a command line sighash type and the sighash field is present, they must specify the same sighash type. However, this was being checked by the wallet, rather than the signing code, so the descriptorprocesspsbt RPC was not enforcing this restriction at all, and in fact ignored the sighash field entirely. This PR refactors the checking code so that the underlying PSBT signing function SignPSBTInput does the check.

DrahtBot commented at 2:09 am on January 9, 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/31622.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	1440000bytes, Sjors

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:

#bitcoin-core/gui/850 (gui, psbt: Use SIGHASH_DEFAULT when signing PSBTs by achow101)
#bitcoin-core/gui/832 (Improve user dialog when signing multisig psbts by mjdietzx)
#31247 (psbt: MuSig2 Fields by achow101)
#30886 (rpc: Add support to populate PSBT input utxos via rpc by instagibbs)
#29675 (wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys by achow101)
#24963 (RPC/Wallet: Convert walletprocesspsbt to use options parameter by luke-jr)
#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 PSBT on Jan 9, 2025

DrahtBot added the label CI failed on Jan 9, 2025

DrahtBot commented at 3:25 am on January 9, 2025: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/35347529485

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.

tests: Test PSBT sighash type mismatch cc870ac342

achow101 force-pushed on Jan 9, 2025

fanquake commented at 3:47 pm on January 10, 2025: member

https://github.com/bitcoin/bitcoin/actions/runs/12700499877/job/35403281996?pr=31622#step:12:8987:

 0  test  2025-01-10T00:28:24.738000Z TestFramework (ERROR): Assertion failed 
 1                                   Traceback (most recent call last):
 2                                     File "D:\a\bitcoin\bitcoin\build\test\functional\test_framework\test_framework.py", line 135, in main
 3                                       self.run_test()
 4                                     File "D:\a\bitcoin\bitcoin\build\test\functional\wallet_taproot.py", line 411, in run_test
 5                                       self.do_test(
 6                                     File "D:\a\bitcoin\bitcoin\build\test\functional\wallet_taproot.py", line 395, in do_test
 7                                       self.do_test_psbt(comment, pattern, privmap, treefn, keys[2*nkeys:3*nkeys], keys[3*nkeys:4*nkeys])
 8                                     File "D:\a\bitcoin\bitcoin\build\test\functional\wallet_taproot.py", line 363, in do_test_psbt
 9                                       assert "non_witness_utxo" not in psbtin
10                                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
11                                   AssertionError
12 test  2025-01-10T00:28:24.738000Z TestFramework (DEBUG): Closing down network thread

1440000bytes commented at 1:47 am on January 11, 2025: none

Currently, we do not add the sighash field to PSBTs at all, even when we have signed with a non-default sighash. This PR changes the behavior such that when we (attempt to) sign with a sighash other than DEFAULT or ALL, the sighash type field will be added to the PSBT to inform the later signers that a different sighash type was used by a signer. Notably, this is necessary for MuSig2 support as all signers must sign using the same sighash type, but the sighash is not provided in partial signatures.

Concept ACK

in src/psbt.cpp:427 in b70e83da42 outdated

423+    // If neither are provided, use SIGHASH_DEFAULT
424+    if (!sighash) sighash = utxo.scriptPubKey.IsPayToTaproot() ? SIGHASH_DEFAULT : SIGHASH_ALL;
425+    if (input.sighash_type && input.sighash_type != sighash) {
426+        return PSBTError::SIGHASH_MISMATCH;
427+    } else {
428+        if ((!utxo.scriptPubKey.IsPayToTaproot() && (sighash != SIGHASH_ALL && sighash != SIGHASH_DEFAULT)) ||

scgbckbone commented at 3:27 am on January 11, 2025:

is there any specific reason to allow SIGHASH_DEFAULT on non-taproot utxo? Shouldn’t it be error instead of rewriting sighash in sign.cpp

0    // BASE/WITNESS_V0 signatures don't support explicit SIGHASH_DEFAULT, use SIGHASH_ALL instead.
1    const int hashtype = nHashType == SIGHASH_DEFAULT ? SIGHASH_ALL : nHashType;

in test/functional/rpc_psbt.py:254 in b70e83da42 outdated

249+        assert_raises_rpc_error(-22, "Specified sighash value does not match value stored in PSBT", self.nodes[0].descriptorprocesspsbt, psbt, descs, "SINGLE")
250+        assert_raises_rpc_error(-22, "Specified sighash value does not match value stored in PSBT", self.nodes[0].descriptorprocesspsbt, psbt, descs, "NONE|ANYONECANPAY")
251+        assert_raises_rpc_error(-22, "Specified sighash value does not match value stored in PSBT", self.nodes[0].descriptorprocesspsbt, psbt, descs, "SINGLE|ANYONECANPAY")
252+
253+        # No sighash type specified fails
254+        assert_raises_rpc_error(-22, "Specified sighash value does not match value stored in PSBT", self.nodes[0].descriptorprocesspsbt, psbt, descs)

Sjors commented at 11:23 am on January 16, 2025:

b70e83da4210e8098cbf3fc3b2289fc710828455: let’s echo the specified sighash value, otherwise the error is confusing in this default case.

We should probably also echo what’s in the PSBT.

I think it’s a good safety feature that the default behaviour is not to just mimic what’s in the PSBT but to insist on DEFAULT / ALL. The error message could warn against the temptation to call the RPC again with the “right” value.

achow101 commented at 11:30 pm on January 20, 2025:

The function that produces this specific error message does not have access to the arguments nor the PSBT to produce a specific message in each instance. Rewiring everything to allow that is a significant amount of work that I don’t want to do in this PR.

in src/rpc/rawtransaction.cpp:239 in b70e83da42 outdated

234@@ -235,8 +235,10 @@ PartiallySignedTransaction ProcessPSBT(const std::string& psbt_string, const std
235         // Note that SignPSBTInput does a lot more than just constructing ECDSA signatures.
236         // We only actually care about those if our signing provider doesn't hide private
237         // information, as is the case with `descriptorprocesspsbt`
238-        // As such, we ignore the return value as any errors just mean that we do not have enough information.
239-        SignPSBTInput(provider, psbtx, /*index=*/i, &txdata, sighash_type, /*out_sigdata=*/nullptr, finalize);
240+        // Only error for mismatching sighash types as it is critical that the sighash to sign with matches the PSBT's
241+        if (SignPSBTInput(provider, psbtx, /*index=*/i, &txdata, sighash_type, /*out_sigdata=*/nullptr, finalize) == common::PSBTError::SIGHASH_MISMATCH) {

Sjors commented at 12:07 pm on January 16, 2025:

b70e83da4210e8098cbf3fc3b2289fc710828455: in case of a hardware wallet (external signer) I would want this to fail before any call to the device is made.

It’s a bit hard to tell if that’s the case, maybe this specific check can be refactored into its own function? That would make things easier to follow in general.

achow101 commented at 11:32 pm on January 20, 2025:

Nothing in this file has access to hardware signers. Those are wallet only.

in src/psbt.cpp:378 in 0970178f62 outdated

374@@ -372,13 +375,13 @@ PrecomputedTransactionData PrecomputePSBTData(const PartiallySignedTransaction&
375     return txdata;
376 }
377 
378-bool SignPSBTInput(const SigningProvider& provider, PartiallySignedTransaction& psbt, int index, const PrecomputedTransactionData* txdata, int sighash,  SignatureData* out_sigdata, bool finalize)
379+PSBTError SignPSBTInput(const SigningProvider& provider, PartiallySignedTransaction& psbt, int index, const PrecomputedTransactionData* txdata, int sighash,  SignatureData* out_sigdata, bool finalize)

Sjors commented at 12:11 pm on January 16, 2025:

0970178f620acd695c49526d2b375eae489072c1: can we make this nodiscard and then explicitly ignore the result in rpc/rawtransaction.cpp?

achow101 commented at 11:44 pm on January 20, 2025:

Done

in src/wallet/scriptpubkeyman.cpp:640 in e94bb4e5fc outdated

636+std::optional<PSBTError> LegacyScriptPubKeyMan::FillPSBT(PartiallySignedTransaction& psbtx, const PrecomputedTransactionData& txdata, std::optional<int> sighash_type, bool sign, bool bip32derivs, int* n_signed, bool finalize) const
637 {
638     if (n_signed) {
639         *n_signed = 0;
640     }
641+    if (!sighash_type) sighash_type = SIGHASH_DEFAULT;

Sjors commented at 12:19 pm on January 16, 2025:

e94bb4e5fc6dea67844a8495b948b679649b7b65: maybe use SIGHASH_ALL here, since the legacy wallet doesn’t support Taproot.

achow101 commented at 11:43 pm on January 20, 2025:

This is temporary anyways and is removed in a later commit.

in src/wallet/scriptpubkeyman.cpp:2541 in e94bb4e5fc outdated

2537+std::optional<PSBTError> DescriptorScriptPubKeyMan::FillPSBT(PartiallySignedTransaction& psbtx, const PrecomputedTransactionData& txdata, std::optional<int> sighash_type, bool sign, bool bip32derivs, int* n_signed, bool finalize) const
2538 {
2539     if (n_signed) {
2540         *n_signed = 0;
2541     }
2542+    if (!sighash_type) sighash_type = SIGHASH_DEFAULT;

Sjors commented at 12:22 pm on January 16, 2025:

e94bb4e5fc6dea67844a8495b948b679649b7b65: maybe do this inside the psbtx.tx->vin loop below and set SIGHASH_ALL or SIGHASH_DEFAULT depending on whether it’s taproot.

And then replace the magic buried in MutableTransactionSignatureCreator::CreateSig with an assert.

achow101 commented at 11:43 pm on January 20, 2025:

This is temporary and will be done inside of SignPSBTInput in a later commit.

Sjors commented at 12:25 pm on January 16, 2025: member

Concept ACK

The descriptorprocesspsbt documentation for sighashtype currently says:

The signature hash type to sign with if not specified by the PSBT.

Let’s change that to:

The signature hash type to sign with. If the PSBT already specifies a hash type, it must match. Otherwise it is added to the PSBT.

Some comments based on partial code review.

psbt: Return PSBTError from SignPSBTInput ff5aaef07e

wallet: change FillPSBT to take sighash as optional

Instead of having the caller have to figure out the correct sane default
to provide to FillPSBT, have FillPSBT do that by having it take the
sighash type as an optional. This further allows it to distinguish
between an explicit sighash type being provided and expecting the
default value to be used.

1c980c4207

script: Add IsPayToTaproot() 29361d69c8

psbt: Check sighash types in SignPSBTInput and take sighash as optional 1a6d29212e

wallet: Remove sighash type enforcement from FillPSBT 083daacb7b

psbt: Add sighash types to PSBT when not DEFAULT or ALL

When an atypical sighash type is specified by the user, add it to the
PSBT so that further signing can enforce sighash type matching.

34c33a7471

rpc, psbt: Require sighashes match for descriptorprocesspsbt 6df3857fa7

achow101 force-pushed on Jan 20, 2025

psbt: add non-default sighash types to PSBTs and unify sighash type match checking #31622

Code Coverage & Benchmarks

Reviews

Conflicts