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
ACK	Sjors
Concept ACK	1440000bytes, fjahr
Stale 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:

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

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:426 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;

achow101 commented at 9:24 pm on January 31, 2025:

It’s for compatibility, particularly when signing a transaction that has both taproot and non-taproot inputs.

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.

Sjors commented at 9:46 am on January 21, 2025:

Ah, I see. This is only called by descriptorprocesspsbt and utxoupdatepsbt. The former passes in any private key info it needs, and the latter doesn’t pass private keys at all.

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 9:50 am on January 21, 2025:

I’m still seeing this in CreateSig at the last commit of this PR:

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

Is that redundant after 34c33a747128e1c26b1931ac3fa0c4ed6b6a7bb7?

achow101 commented at 6:19 pm on January 21, 2025:

Is that redundant after 34c33a7?

No. There are other non-PSBT callers of ProduceSignature which may still pass SIGHASH_DEFAULT to legacy input types, notably signrawtranssaction.

Sjors commented at 9:30 am on January 22, 2025:

It might make things easier to follow if those callers pass the right flag as well. If it can be done without making this PR too huge.

Sjors commented at 9:32 am on January 22, 2025:

In other words: for all functions that take a sighash argument, you either pass the correct sighash or nothing.

achow101 commented at 9:29 pm on January 31, 2025:

I don’t think that’s actually correct either as then passing SIGHASH_DEFAULT will be unable to sign for non-taproot at the same time.

It’s also a much larger change that I think is out of scope for this PR.

Sjors commented at 9:14 am on February 3, 2025:

at the same time

You mean a transaction with both a taproot and non-taproot input?

achow101 commented at 6:13 pm on February 10, 2025:

You mean a transaction with both a taproot and non-taproot input?

Yes

Sjors commented at 3:19 pm on February 17, 2025:

In 59186f20f24c8ab14ac34a69c61ed0c40793cea6 “wallet: change FillPSBT to take sighash as optional”: I think I get it now, see comment on 293fb5d16b51cf259c9a8fe379954d1d9f303029 “psbt: Add sighash types to PSBT when not DEFAULT or ALL”

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.

achow101 force-pushed on Jan 20, 2025

achow101 force-pushed on Jan 21, 2025

DrahtBot removed the label CI failed on Jan 22, 2025

in src/psbt.cpp:417 in 1a6d29212e outdated

412@@ -413,12 +413,25 @@ PSBTError SignPSBTInput(const SigningProvider& provider, PartiallySignedTransact
413         return PSBTError::MISSING_INPUTS;
414     }
415 
416+    // Get the sighash type
417+    // If both the field and the paramter are provided, they must match

rkrux commented at 8:52 am on January 29, 2025:

parameter

achow101 commented at 10:34 pm on January 31, 2025:

Fixed

in src/psbt.cpp:420 in 1a6d29212e outdated

412@@ -413,12 +413,25 @@ PSBTError SignPSBTInput(const SigningProvider& provider, PartiallySignedTransact
413         return PSBTError::MISSING_INPUTS;
414     }
415 
416+    // Get the sighash type
417+    // If both the field and the paramter are provided, they must match
418+    // If only the psbt field is provided, refuse to sign. For user safety, the desired sighash must
419+    // be provided if the PSBT wants something other than DEFAULT
420+    // If only the paramter is provider, use it and add it to the PSBT if it is other than SIGHASH_DEFAULT

rkrux commented at 8:52 am on January 29, 2025:

If only the parameter is provided

achow101 commented at 10:34 pm on January 31, 2025:

Fixed

in src/wallet/external_signer_scriptpubkeyman.h:38 in 1c980c4207 outdated

34@@ -35,7 +35,7 @@ class ExternalSignerScriptPubKeyMan : public DescriptorScriptPubKeyMan
35   */
36  util::Result<void> DisplayAddress(const CTxDestination& dest, const ExternalSigner& signer) const;
37 
38-  std::optional<common::PSBTError> FillPSBT(PartiallySignedTransaction& psbt, const PrecomputedTransactionData& txdata, int sighash_type = 1 /* SIGHASH_ALL */, bool sign = true, bool bip32derivs = false, int* n_signed = nullptr, bool finalize = true) const override;
39+  std::optional<common::PSBTError> FillPSBT(PartiallySignedTransaction& psbt, const PrecomputedTransactionData& txdata, std::optional<int> sighash_type = 1 /* SIGHASH_ALL */, bool sign = true, bool bip32derivs = false, int* n_signed = nullptr, bool finalize = true) const override;

rkrux commented at 9:45 am on January 29, 2025:

std::optional<int> sighash_type = 1 /* SIGHASH_ALL */ Does it not require std::nullopt as the default like in other cases?

achow101 commented at 10:34 pm on January 31, 2025:

Indeed, Fixed

in src/psbt.cpp:418 in 1a6d29212e outdated

412@@ -413,12 +413,25 @@ PSBTError SignPSBTInput(const SigningProvider& provider, PartiallySignedTransact
413         return PSBTError::MISSING_INPUTS;
414     }
415 
416+    // Get the sighash type
417+    // If both the field and the paramter are provided, they must match
418+    // If only the psbt field is provided, refuse to sign. For user safety, the desired sighash must

rkrux commented at 10:14 am on January 29, 2025:

If only the psbt field is provided, refuse to sign.

How does it refuse to sign in this case?

achow101 commented at 10:35 pm on January 31, 2025:

I’ve removed this sentence since we allow DEFAULT and ALL.

in src/wallet/scriptpubkeyman.cpp:2565 in 083daacb7b outdated

2539@@ -2545,11 +2540,6 @@ std::optional<PSBTError> DescriptorScriptPubKeyMan::FillPSBT(PartiallySignedTran
2540             continue;
2541         }
2542 
2543-        // Get the Sighash type
2544-        if (sign && input.sighash_type != std::nullopt && *input.sighash_type != sighash_type) {
2545-            return PSBTError::SIGHASH_MISMATCH;
2546-        }
2547-

rkrux commented at 10:23 am on January 29, 2025:

Confirming - it’s being removed from here because this check now happens inside SignPSBTInput that is called down below?

achow101 commented at 9:44 pm on January 31, 2025:

Yes

in src/psbt.cpp:431 in 34c33a7471 outdated

422@@ -423,6 +423,12 @@ PSBTError SignPSBTInput(const SigningProvider& provider, PartiallySignedTransact
423     if (!sighash) sighash = utxo.scriptPubKey.IsPayToTaproot() ? SIGHASH_DEFAULT : SIGHASH_ALL;
424     if (input.sighash_type && input.sighash_type != sighash) {
425         return PSBTError::SIGHASH_MISMATCH;
426+    } else {
427+        if ((!utxo.scriptPubKey.IsPayToTaproot() && (sighash != SIGHASH_ALL && sighash != SIGHASH_DEFAULT)) ||
428+            (utxo.scriptPubKey.IsPayToTaproot() && sighash != SIGHASH_DEFAULT)
429+        ) {
430+            input.sighash_type = sighash;

rkrux commented at 10:48 am on January 29, 2025:

Is the only downside of adding sighash in PSBT in case of default or all is that it increases the PSBT size?

achow101 commented at 9:45 pm on January 31, 2025:

Yes. Additionally, specifying ALL for taproot means adding another byte to the signature and also changes the sighash.

in src/psbt.cpp:467 in 849877c6f7 outdated

466@@ -467,36 +467,41 @@ PSBTError SignPSBTInput(const SigningProvider& provider, PartiallySignedTransact
467     return sig_complete ? PSBTError::OK : PSBTError::INCOMPLETE;

rkrux commented at 11:18 am on January 29, 2025:

Can this commit 849877c6f726e230ca7fecfeba78ed0c8ec8c8a7 be a separate PR? - because it’s an application of the underlying addition of the sighash type in the PSBT and removing it from the PR won’t break the sighash type addition functionality. It would be nice to have a test that checks non witness utxos are dropped if sighash type is not SIGHASH_ANYONECANPAY. It can be added in this PR as well at the cost of a larger diff.

achow101 commented at 9:46 pm on January 31, 2025:

No, this was added because without it there were test failures.

in test/functional/rpc_psbt.py:267 in 34c33a7471 outdated

252+        psbt = wallet.walletprocesspsbt(psbt=psbt, sighashtype="ALL|ANYONECANPAY", finalize=False)["psbt"]
253+
254+        # Check that the PSBT has a sighash field on all inputs
255+        dec_psbt = self.nodes[0].decodepsbt(psbt)
256+        for input in dec_psbt["inputs"]:
257+            assert_equal(input["sighash"], "ALL|ANYONECANPAY")

rkrux commented at 11:42 am on January 29, 2025:

Atm, there is only 1 input in the PSBT. It would be nice to have another input that is Taproot and check for its sighash.

achow101 commented at 10:35 pm on January 31, 2025:

Done

in test/functional/rpc_psbt.py:251 in fefe0636d4 outdated

246+        assert_raises_rpc_error(-22, "Specified sighash value does not match value stored in PSBT", self.nodes[0].descriptorprocesspsbt, psbt, descs, "DEFAULT")
247+        assert_raises_rpc_error(-22, "Specified sighash value does not match value stored in PSBT", self.nodes[0].descriptorprocesspsbt, psbt, descs, "ALL")
248+        assert_raises_rpc_error(-22, "Specified sighash value does not match value stored in PSBT", self.nodes[0].descriptorprocesspsbt, psbt, descs, "NONE")
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")

rkrux commented at 11:44 am on January 29, 2025:

Nit feel free to ignore: I ignored this duplication in the first commit but now that it’s being done twice, a refactor could be to just iterate this in a loop of sighash types.

achow101 commented at 10:35 pm on January 31, 2025:

Done

rkrux commented at 11:45 am on January 29, 2025: none

Concept ACK fefe0636d4ae7c246042276cacd60b22f5fc6bb9

Good PR with enough context to unpack for me. Left few comments, will review again soon.

Build and functional tests successful.

achow101 force-pushed on Jan 31, 2025

in src/common/messages.cpp:120 in c99e1bc359 outdated

115@@ -116,6 +116,10 @@ bilingual_str PSBTErrorString(PSBTError err)
116             return Untranslated("External signer failed to sign");
117         case PSBTError::UNSUPPORTED:
118             return Untranslated("Signer does not support PSBT");
119+        case PSBTError::INCOMPLETE:
120+            return Untranslated("Input needs additional signatures or other data");

fjahr commented at 11:38 pm on February 1, 2025:

It looks like there is no test coverage for this yet. Is there coverage for this in a follow-up PR (maybe Musig2) or are you writing a test? Otherwise I can give it a shot.

achow101 commented at 2:38 am on February 2, 2025:

I don’t think there’s any specific coverage for this, feel free to tackle it.

fjahr commented at 11:40 pm on February 1, 2025: contributor

Concept ACK

in test/functional/rpc_psbt.py:282 in e741762081 outdated

262+        if self.options.descriptors:
263+            mod_psbt.i[1].map[PSBT_IN_SIGHASH_TYPE] = (SIGHASH_ALL).to_bytes(4, byteorder="little")
264+        psbt = mod_psbt.to_base64()
265+        fin_res = self.nodes[0].finalizepsbt(psbt)
266+        assert_equal(fin_res["complete"], True)
267+        assert_equal(fin_res["hex"], fin_hex)

rkrux commented at 8:22 am on February 3, 2025:

This is the final hex when I run the test, which is same as the one before modifying the sighash type as per the test:

00200000000010208bd94fdd97de6bc3ec454b8e0088a5341f9e9611bfbe412387e7e0df35207960000000000fdffffff08bd94fdd97de6bc3ec454b8e0088a5341f9e9611bfbe412387e7e0df35207960100000000fdffffff0280f0fa0200000000160014f57919cd765629cd0d2ba39021a146ee065d697508c2f00800000000160014c7109263e9f5db5d2162ce40a1f898f6ac3933ad02473044022067caa211634d7f25bcac48704165ca7d257ff80a7d0b764825877c7a4a137eff02204ef676f27c9f47813be48a0d2cce9b1dfa2b6fb0ac44eb1d4273781299ad318c81210362cd5a24c1978e204d78b15e5b303c4a8bbb8234591d94b1b8faa27cc72fdeed01412427cd2bcb8d07bc16ccb73a4cffeb996f89a6ee80a39d8ca4cf80f342039e0ec90d12dd0bcb1cdc392c05b1b9e2006fb23fb5fb9ade3a30f9c7b40cd3a581a78100000000

These are the 2 signatures of the 2 inputs:

03044022067caa211634d7f25bcac48704165ca7d257ff80a7d0b764825877c7a4a137eff02204ef676f27c9f47813be48a0d2cce9b1dfa2b6fb0ac44eb1d4273781299ad318c81
12427cd2bcb8d07bc16ccb73a4cffeb996f89a6ee80a39d8ca4cf80f342039e0ec90d12dd0bcb1cdc392c05b1b9e2006fb23fb5fb9ade3a30f9c7b40cd3a581a781

I notice the 81 at the end in both of them signifying ALL|ANYONECANPAY.

Why does it not change to 01 when the transaction inputs are signed using SIGHASH_ALL later, and subsequently leading to changing of actual signature as well because all inputs are signed now?

Is it because the finalizepsbt doesn’t actually sign the inputs again because there are signatures already in the PSBT albeit using a different sighash type?

Shouldn’t PSBTInputSignedAndVerified() fail here because the sighash type doesn’t match with the signature?

achow101 commented at 6:16 pm on February 10, 2025:

Is it because the finalizepsbt doesn’t actually sign the inputs again because there are signatures already in the PSBT albeit using a different sighash type?

finalizepsbt does not actually sign even though it calls the sign functions. It does not have access to private keys.

Shouldn’t PSBTInputSignedAndVerified() fail here because the sighash type doesn’t match with the signature?

The BIP states that the sighash field is merely a suggestion and signers are allowed to ignore it. Thus signatures can be produced with a different sighash.

rkrux commented at 10:11 am on February 13, 2025:

Thanks, I get it now. I asked because I noticed SignPSBTInput being called but I see now the DUMMY_SIGNING_PROVIDER argument passed to it as well. I see the reason why the test is contrived such that it requires manually updating the sighash and calling finalisepsbt again.

in src/psbt.cpp:419 in c99e1bc359 outdated

415+    }
416+
417+    // Get the sighash type
418+    // If both the field and the parameter are provided, they must match
419+    // For user safety, the desired sighash must be provided if the PSBT wants something other than DEFAULT
420+    // If only the parameter is provider, use it and add it to the PSBT if it is other than SIGHASH_DEFAULT

rkrux commented at 8:23 am on February 3, 2025:

provided

achow101 commented at 6:26 pm on February 10, 2025:

Done

rkrux commented at 8:27 am on February 3, 2025: none

I reviewed range-diff, asked a question.

0git range-diff fefe0636d4ae7c246042276cacd60b22f5fc6bb9...c99e1bc359263d44a85460032c04f7c1f3c688c7

All the new changes address the previous review comments, namely: removing duplication in the functional test rpc_psbt, adding Taproot input in the test, fixing comments, and changing FillPSBT() argument default.

Sjors commented at 12:32 pm on February 4, 2025: member

Silent merge conflict with bitcoin-core/gui#850.

DrahtBot added the label Needs rebase on Feb 4, 2025

tests: Test PSBT sighash type mismatch 75778d4cbe

achow101 force-pushed on Feb 10, 2025

DrahtBot removed the label Needs rebase on Feb 12, 2025

rkrux commented at 10:26 am on February 13, 2025: none

ACK 9911ba82be9ec2243b86bb149d6ec8509d9717d7

I reviewed the range diff this time, my earlier query is answered.

0git range-diff c99e1bc359263d44a85460032c04f7c1f3c688c7...9911ba82be9ec2243b86bb149d6ec8509d9717d7

DrahtBot requested review from fjahr on Feb 13, 2025

DrahtBot requested review from Sjors on Feb 13, 2025

in src/wallet/scriptpubkeyman.cpp:664 in c49cccf9aa outdated

664@@ -665,7 +665,10 @@ std::optional<PSBTError> LegacyScriptPubKeyMan::FillPSBT(PartiallySignedTransact
665             // There's no UTXO so we can just skip this now
666             continue;
667         }
668-        SignPSBTInput(HidingSigningProvider(this, !sign, !bip32derivs), psbtx, i, &txdata, sighash_type, nullptr, finalize);
669+        PSBTError res = SignPSBTInput(HidingSigningProvider(this, !sign, !bip32derivs), psbtx, i, &txdata, sighash_type, nullptr, finalize);
670+        if (res != PSBTError::OK && res != PSBTError::INCOMPLETE) {

Sjors commented at 1:33 pm on February 17, 2025:

In c49cccf9aac2d4e783c633930ceca4534f05c710 “psbt: Return PSBTError from SignPSBTInput”: would be useful if the commit describes this behavior change, especially since there’s no test. It seems that previously SignPSBTInput failures were just ignored, and now they’re not. But this wasn’t a problem before?

achow101 commented at 9:15 pm on February 18, 2025:

Updated the commit message.

It’s not a problem before, and not a problem now, because all of the things SignPSBTInput would fail on are checked by its callers.

in src/psbt.cpp:421 in 121c8fb494 outdated

417+    // If both the field and the parameter are provided, they must match
418+    // For user safety, the desired sighash must be provided if the PSBT wants something other than DEFAULT
419+    // If only the parameter is provided, use it and add it to the PSBT if it is other than SIGHASH_DEFAULT
420+    // for all input types, and not SIGHASH_ALL for non-taproot input types.
421+    // If neither are provided, use SIGHASH_DEFAULT
422+    if (!sighash) sighash = utxo.scriptPubKey.IsPayToTaproot() ? SIGHASH_DEFAULT : SIGHASH_ALL;

Sjors commented at 3:40 pm on February 17, 2025:

In 121c8fb494df4e81566eb0706f7b9500a870ac85 “psbt: Check sighash types in SignPSBTInput and take sighash as optional”: I’m a little worried that burying this in PSBT code will lead to a bug one day when a new soft fork allows e.g. a witness version 1 scriptPubKey of 40 bytes. And then instead of expanding IsPayToTaproot() a new helper is added and not used here.

I don’t have a good suggestion though, other than maybe adding another helper like CanSighashDefault(). But that might be forgotten too.

in src/wallet/wallet.cpp:2205 in 121c8fb494 outdated

2201@@ -2202,7 +2202,6 @@ std::optional<PSBTError> CWallet::FillPSBT(PartiallySignedTransaction& psbtx, bo
2202     if (n_signed) {
2203         *n_signed = 0;
2204     }
2205-    if (!sighash_type) sighash_type = SIGHASH_DEFAULT;

Sjors commented at 4:06 pm on February 17, 2025:

In 121c8fb494df4e81566eb0706f7b9500a870ac85 “psbt: Check sighash types in SignPSBTInput and take sighash as optional”: this makes the call to RemoveUnnecessaryTransactions with *sighash_type below dangerous.

achow101 commented at 9:15 pm on February 18, 2025:

Changed RemoveUnnecessaryTransactions to take sighash type as optional in this commit as well.

in src/psbt.cpp:418 in 121c8fb494 outdated

412@@ -413,12 +413,24 @@ PSBTError SignPSBTInput(const SigningProvider& provider, PartiallySignedTransact
413         return PSBTError::MISSING_INPUTS;
414     }
415 
416+    // Get the sighash type
417+    // If both the field and the parameter are provided, they must match
418+    // For user safety, the desired sighash must be provided if the PSBT wants something other than DEFAULT

Sjors commented at 4:16 pm on February 17, 2025:

In https://github.com/bitcoin/bitcoin/commit/121c8fb494df4e81566eb0706f7b9500a870ac85 “psbt: Check sighash types in SignPSBTInput and take sighash as optional” nit: it would be slightly more clear if you move this comment line directly above if (input.sighash_type && input.sighash_type != sighash).

achow101 commented at 9:16 pm on February 18, 2025:

Done

in src/psbt.cpp:425 in 293fb5d16b outdated

421@@ -422,6 +422,12 @@ PSBTError SignPSBTInput(const SigningProvider& provider, PartiallySignedTransact
422     if (!sighash) sighash = utxo.scriptPubKey.IsPayToTaproot() ? SIGHASH_DEFAULT : SIGHASH_ALL;
423     if (input.sighash_type && input.sighash_type != sighash) {
424         return PSBTError::SIGHASH_MISMATCH;
425+    } else {

Sjors commented at 4:21 pm on February 17, 2025:

In 293fb5d16b51cf259c9a8fe379954d1d9f303029 “psbt: Add sighash types to PSBT when not DEFAULT or ALL” nit: is this really else-worthy?

This boolean juggling is really hard to read, try:

0// Allow SIGHASH_DEFAULT for non-taproot inputs, to support transactions
1// that also have taproot inputs. CreateSig() will adjust it.
2if (utxo.scriptPubKey.IsPayToTaproot() ? sighash != SIGHASH_DEFAULT :
3                                        (sighash != SIGHASH_DEFAULT && sighash != SIGHASH_ALL)) {

Sjors commented at 5:05 pm on February 17, 2025:

(although maybe just indenting the second line with one extra space would do the trick, along with the comment)

achow101 commented at 9:16 pm on February 18, 2025:

This is all explained by the comment 2 lines up.

Took the suggestion, changed the comment to be less repetitive of things already said.

Sjors commented at 10:59 am on February 20, 2025:

In 9917a47ca49456172b03fee1e75c79ee72c381a6 “psbt: Add sighash types to PSBT when not DEFAULT or ALL”

Better. Maybe also drop the else, since the SIGHASH_MISMATCH guard makes sense in isolation.

achow101 commented at 4:10 pm on February 20, 2025:

Removed the else.

in src/psbt.cpp:428 in 293fb5d16b outdated

421@@ -422,6 +422,12 @@ PSBTError SignPSBTInput(const SigningProvider& provider, PartiallySignedTransact
422     if (!sighash) sighash = utxo.scriptPubKey.IsPayToTaproot() ? SIGHASH_DEFAULT : SIGHASH_ALL;
423     if (input.sighash_type && input.sighash_type != sighash) {
424         return PSBTError::SIGHASH_MISMATCH;
425+    } else {
426+        if ((!utxo.scriptPubKey.IsPayToTaproot() && (sighash != SIGHASH_ALL && sighash != SIGHASH_DEFAULT)) ||
427+            (utxo.scriptPubKey.IsPayToTaproot() && sighash != SIGHASH_DEFAULT)
428+        ) {

Sjors commented at 4:42 pm on February 17, 2025:

In https://github.com/bitcoin/bitcoin/commit/293fb5d16b51cf259c9a8fe379954d1d9f303029 “psbt: Add sighash types to PSBT when not DEFAULT or ALL”: this is a good place for the commit message:

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

achow101 commented at 9:16 pm on February 18, 2025:

This is already explained by the comment a few lines above.

Added a less repetitive comment.

Sjors commented at 5:02 pm on February 17, 2025: member

Reviewed code for 9911ba82be9ec2243b86bb149d6ec8509d9717d7, but I still need to review the last two commits.

rpc_psbt.py --descriptors consistently fails for me in commit 121c8fb494df4e81566eb0706f7b9500a870ac85 “psbt: Check sighash types in SignPSBTInput and take sighash as optional”, at “Check that non-witness UTXOs are removed for segwit v1+ inputs” with assert not non_witness_utxo at line 136.

But only on Ubuntu, not on macOS, so maybe a gcc vs clang thing?

It’s happy again three commits later at 6ec298a335585ef723d113b94959377d3ff65759 “psbt: use sighash type field to determine whether to remove non-witness utxos”

DrahtBot requested review from Sjors on Feb 17, 2025

in src/psbt.cpp:487 in 6ec298a335 outdated

504+        if (wit_ver == 0) {
505+            // Segwit v0, so we cannot drop any non_witness_utxos
506+            to_drop.clear();
507+            break;
508+        }
509+        // non_witness_utxos can only be dropped if the sighash type does not include SIGHASH_ANYONECANPAY

Sjors commented at 6:28 pm on February 17, 2025:

In 6ec298a335585ef723d113b94959377d3ff65759 “psbt: use sighash type field to determine whether to remove non-witness utxos”:

“can only … if not” is hard to parse, and it seems the code doing the opposite. It also seems to do the opposite of the original behavior.

0// Only drop non_witness_utxos if sighash_type != SIGHASH_ANYONECANPAY

Can you clarify the reason why SIGHASH_ANYONECANPAY requires different treatment in the first place?

achow101 commented at 9:19 pm on February 18, 2025:

Fixed and reworded the comment.

SIGHASH_ANYONECANPAY does not commit to any input information, so none of the protections that allow us to drop the non_witness_utxo apply.

Sjors commented at 11:40 am on February 20, 2025:

Ok, in 795e911ded2563705cec69e43ff622ace22d58d5 psbt: use sighash type field to determine whether to remove non-witness utxos it’s now more clear that this just moves a check inside the for loop (and we no longer need to pass it into the function, since we check it for each input).

I still can’t wrap my head around the logic of how and why we drop non_witness_utxos.

in src/psbt.cpp:478 in 6ec298a335 outdated

495+    for (unsigned int i = 0; i < psbtx.inputs.size(); ++i) {
496+        const auto& input = psbtx.inputs.at(i);
497+        int wit_ver;
498+        std::vector<unsigned char> wit_prog;
499+        if (input.witness_utxo.IsNull() || !input.witness_utxo.scriptPubKey.IsWitnessProgram(wit_ver, wit_prog)) {
500+            // There's a non-segwit input or Segwit v0, so we cannot drop any witness_utxos

Sjors commented at 6:46 pm on February 17, 2025:

In https://github.com/bitcoin/bitcoin/commit/6ec298a335585ef723d113b94959377d3ff65759 “psbt: use sighash type field to determine whether to remove non-witness utxos”: any non_witness_utxos? Also we don’t know if it’s SegWit v0 at this point?

achow101 commented at 9:20 pm on February 18, 2025:

Fixed

Sjors commented at 7:00 pm on February 17, 2025: member

Also looked at the last two commits.

DrahtBot requested review from Sjors on Feb 17, 2025

Sjors commented at 7:02 pm on February 17, 2025: member

@DrahtBot is drunk again? Asking me for a review directly after I give a (partial) review.

maflcko commented at 6:59 am on February 18, 2025: member

@DrahtBot is drunk again? Asking me for a review directly after I give a (partial) review.

It will never be possible to be 100% accurate in a heuristic. If it bugs you, you may be able to work around it by leaving a fresh review comment with a (concept/approach) ack (commit_id), but I skipped over some commits (or similar), according to https://github.com/maflcko/DrahtBot/blob/1cce096df7cb4d89d34bcdbed09b20a38fe2a344/webhook_features/src/features/summary_comment.rs#L318-L320. You may also just ignore it, as it should happen rarely.

psbt: Return PSBTError from SignPSBTInput

SignPSBTInput will need to report the specific things that caused an
error to callers, so change it to return a PSBTError. Additionally some
callers will now check the return value and report an error to the user.

Currently, this should not change any behavior as the things that
SignPBSTInput will error on are all first checked by its callers.

756a0b040f

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.

2dbbd83bf5

script: Add IsPayToTaproot() 4691b92190

psbt: Check sighash types in SignPSBTInput and take sighash as optional 71b6d854eb

wallet: Remove sighash type enforcement from FillPSBT 5d7be1000d

achow101 force-pushed on Feb 18, 2025

Sjors commented at 11:44 am on February 20, 2025: member

Code review ACK 61885ad9d406e25ec3b6523d01918e886996f1c3

DrahtBot requested review from rkrux on Feb 20, 2025

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.

b1515fef9e

psbt: use sighash type field to determine whether to remove non-witness utxos

Since the sighash type field is written for atypical sighash types, we
can look at that field to figure out whether the psbt contains
unnecessary transactions.

a14daadcf1

rpc, psbt: Require sighashes match for descriptorprocesspsbt ecb90f165e

achow101 force-pushed on Feb 20, 2025

in src/psbt.cpp:417 in ecb90f165e

409@@ -407,19 +410,38 @@ bool SignPSBTInput(const SigningProvider& provider, PartiallySignedTransaction&
410         // a witness signature in this situation.
411         require_witness_sig = true;
412     } else {
413-        return false;
414+        return PSBTError::MISSING_INPUTS;
415     }
416 
417+    // Get the sighash type
418+    // If both the field and the parameter are provided, they must match

yancyribbens commented at 5:52 am on February 21, 2025:

nit missing period after match

Sjors commented at 7:57 am on February 21, 2025: member

re-ACK ecb90f165ebade21f3f29f309e408eb1a7aa2e2b

(previously used the wrong hash)

Dropped the else: #31622 (review)

rkrux commented at 1:00 pm on February 21, 2025: none

re-ACK 61885ad

Dropped the else: #31622 (comment) @Sjors 61885ad9d406e25ec3b6523d01918e886996f1c3 commit seems to be outdated now.

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

Code Coverage & Benchmarks

Reviews

Conflicts