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

#32532 (script: short-circuit GetLegacySigOpCount for known scripts by l0rinc)
#26398 (Replace MIN_STANDARD_TX_NONWITNESS_SIZE to preclude 64 non-witness bytes only by instagibbs)
#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:1331 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:487 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: contributor

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.

rkrux commented at 1:13 pm on March 4, 2025:

The BIP states that the sighash field is merely a suggestion

Can you share which BIP is this that states it’s a suggestion? I found a contradicting statement in BIP 174, which is confusing.

Signatures for this input must use the sighash type, finalizers must fail to finalize inputs which have signatures that do not match the specified sighash type.

achow101 commented at 5:21 pm on March 4, 2025:

Huh, it does say that. Perhaps I’m remembering an old draft where it was different.

Changed the test to check that it doesn’t finalize, and added a commit to implement that behavior.

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: contributor

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

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: contributor

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

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

achow101 force-pushed on Feb 20, 2025

in src/psbt.cpp:417 in ecb90f165e outdated

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: contributor

re-ACK 61885ad

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

rkrux commented at 2:34 pm on February 25, 2025: contributor

I’m reviewing this PR again and will leave a review later.

Re: #31622 (review)

I still can’t wrap my head around the logic of how and why we drop non_witness_utxos. @Sjors From what I understand, it’s for efficiency. If a PSBT contains only witness v1 utxos, then the PSBT doesn’t need (and can be removed) information for the previous transactions (non_witness_utxos) since signing the segwit v1 inputs commits to the values of the ALL the UTXOs involved in the transaction. Even if an attacker gave incorrect value of a UTXO to the signer, that signed transaction would later be rejected by the full nodes. This is related to the fee overpayment attack on multi input transactions for which I found this 5 year old Optech newsletter helpful.

in test/functional/rpc_psbt.py:259 in b1515fef9e outdated

254+        # Make sure we can still finalize the transaction
255+        fin_res = self.nodes[0].finalizepsbt(psbt)
256+        assert_equal(fin_res["complete"], True)
257+        fin_hex = fin_res["hex"]
258+
259+        # Change the sighash field to a different value and make sure we still finalize to the same thing

rkrux commented at 1:33 pm on March 4, 2025:

What scenario is not tested if this flow is not done and the transaction generated above is mined directly?

achow101 commented at 5:21 pm on March 4, 2025:

Nothing, it just helps to mutate one which we know is definitely good.

in test/functional/rpc_psbt.py:262 in b1515fef9e outdated

242+        def_wallet.send(outputs)
243+        self.generate(self.nodes[0], 6)
244+
245+        # Make a PSBT
246+        psbt = wallet.walletcreatefundedpsbt(wallet.listunspent(), [{def_wallet.getnewaddress(): 0.5}])["psbt"]
247+        psbt = wallet.walletprocesspsbt(psbt=psbt, sighashtype="ALL|ANYONECANPAY", finalize=False)["psbt"]

rkrux commented at 1:38 pm on March 4, 2025:

The descriptorprocesspsbt RPC uses a different flow than the walletprocesspsbt RPC because it calls ProcessPSBT() instead of FillPSBT(). I think it’d be nice if this tests also ensures the sighash is added via the descriptorprocesspsbt RPC as well.

achow101 commented at 5:48 pm on March 4, 2025:

Done

in src/psbt.cpp:490 in a14daadcf1 outdated

466@@ -467,37 +467,41 @@ PSBTError SignPSBTInput(const SigningProvider& provider, PartiallySignedTransact
467     return sig_complete ? PSBTError::OK : PSBTError::INCOMPLETE;
468 }
469 
470-void RemoveUnnecessaryTransactions(PartiallySignedTransaction& psbtx, std::optional<int> sighash_type)
471+void RemoveUnnecessaryTransactions(PartiallySignedTransaction& psbtx)

rkrux commented at 1:44 pm on March 4, 2025:

Re: #31622 (review)

Earlier I thought we should add a test that checks the removal of non_witness_utxo in case of segwit v1 inputs only but I see there is one already here test_utxo_conversion in rpc_psbt. However, it works using walletprocesspsbt RPC only. I think this PR can test for the same functionality via descriptorprocesspsbt RPC too because it uses a different flow internally. Either of test_utxo_conversion or test_sighash_adding can be updated for this.

achow101 commented at 5:50 pm on March 4, 2025:

That’s orthogonal to this PR and can be done separately.

rkrux commented at 2:25 pm on March 6, 2025:

Hmm I think you might be correct, I will make a note of this for it to be done separately. If there was a bug in the newer implementation of RemoveUnnecessaryTransactions the existing test using walletprocesspsbt would have caught it.

rkrux commented at 11:51 am on May 3, 2025:

I attempted this^ in PR #32413.

in test/functional/rpc_psbt.py:316 in b1515fef9e outdated

229@@ -230,6 +230,47 @@ def test_sighash_mismatch(self):
230 
231         wallet.unloadwallet()
232 
233+    def test_sighash_adding(self):

rkrux commented at 1:57 pm on March 4, 2025:

Let’s add a TODO in this file to write a test that checks PSBT_GLOBAL_TX_MODIFIABLE field is correctly set if SIGHASH_SINGLE is used in any of the inputs once the PSBT V2 PR #21283 is merged?

achow101 commented at 5:51 pm on March 4, 2025:

I don’t think it makes sense to do that here since PSBTv2 is still entirely unparseable.

rkrux commented at 2:01 pm on March 6, 2025:

I know it’s not parseable right now, that’s why suggested only a TODO that’d be done after PSBTv2 gets merged. The assumption I have is this PR will get merged before #21283. I didn’t notice any such test in the PSBT PR, which is fine because that PR is already big enough, and adding a TODO here can be a reminder for later.

rkrux commented at 1:58 pm on March 4, 2025: contributor

Few comments.

DrahtBot requested review from rkrux on Mar 4, 2025

achow101 force-pushed on Mar 4, 2025

DrahtBot commented at 7:03 pm on March 4, 2025: contributor

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

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 added the label CI failed on Mar 4, 2025

Sjors commented at 2:02 pm on March 5, 2025: member

From what I understand, it’s for efficiency.

I understand the general reason, but the code itself and the nuance for different sighash flags is hard to follow.

achow101 commented at 5:20 pm on March 5, 2025: member

The fuzzer CI tasks are failing since the change in sighash type enforcement. Presumably this is because the fuzz corpus contains seeds that generate psbts which violate the new constraint. How do we move forward with resolving that?

in src/psbt.cpp:451 in 63ee89c1fb outdated

447+        }
448+        for (const auto& [_, sig] : input.m_tap_script_sigs) {
449+            if (sig.back() != *sighash) return PSBTError::SIGHASH_MISMATCH;
450+        }
451+        for (const auto& [_, sig] : input.partial_sigs) {
452+            if (sig.second.back() != *sighash) return PSBTError::SIGHASH_MISMATCH;

dergoegge commented at 5:39 pm on March 5, 2025:

The fuzz test decodes a psbt from the fuzz input and passes it to the various psbt functions. Looks like it crashes here because the signature is empty? (could be wrong, just glanced at this)

achow101 commented at 7:35 pm on March 5, 2025:

Fixed

rkrux commented at 5:45 pm on March 5, 2025: contributor

I don’t have much idea about fuzz test cases currently but can’t we exclude those seeds that generate PSBTs violating the new constraint? Because the new constraint renders those seeds invalid now.

achow101 force-pushed on Mar 5, 2025

achow101 commented at 7:35 pm on March 5, 2025: member

Added a commit that should fix the fuzz failure.

DrahtBot removed the label CI failed on Mar 5, 2025

rkrux commented at 2:13 pm on March 6, 2025: contributor

The functional test failure seems unrelated to me. A re-run should fix it?

test_preferred_tiebreaker_inv() in p2p_tx_download.py

in test/functional/rpc_psbt.py:350 in a7f76b28a1 outdated

280+            # Make sure we can still finalize the transaction
281+            fin_res = self.nodes[0].finalizepsbt(psbt)
282+            assert_equal(fin_res["complete"], True)
283+            fin_hex = fin_res["hex"]
284+
285+            # Change the sighash field to a different value and make sure we can no longer finalize

rkrux commented at 2:38 pm on March 6, 2025:

Relieved to see this now, it was bugging me earlier (but could not prove to myself until I found it in the spec) that how it can finalise to the same hex with a different sighash when underneath PSBTInputSignedAndVerified uses the same interpreter.cpp::VerifyScript() that is used in other script validation flows as well.

in src/psbt.h:527 in 14e35e675c outdated

428@@ -429,6 +429,11 @@ struct PSBTInput
429                     std::vector<unsigned char> sig;
430                     s >> sig;
431 
432+                    // Check that the signature is validly encoded
433+                    if (sig.empty() || !CheckSignatureEncoding(sig, SCRIPT_VERIFY_DERSIG | SCRIPT_VERIFY_STRICTENC, nullptr)) {

theStack commented at 1:20 pm on April 20, 2025:

nit: CheckSignatureEncoding already checks for empty sig, so the first check isn’t needed

0                    if (!CheckSignatureEncoding(sig, SCRIPT_VERIFY_DERSIG | SCRIPT_VERIFY_STRICTENC, nullptr)) {

any reason why the verification flags don’t also include SCRIPT_VERIFY_LOW_S?

rkrux commented at 10:39 am on April 25, 2025:

nit: CheckSignatureEncoding already checks for empty sig, so the first check isn’t needed

I checked that CheckSignatureEncoding returns true for an empty signature. https://github.com/bitcoin/bitcoin/blob/ff69046e664f2460bb2ef595c5bd16eeb4a13f58/src/script/interpreter.cpp#L200-L205

But here the requirement is to throw error in that case because of what’s stated in BIP 174.

The signature as would be pushed to the stack from a scriptSig or witness. The signature should be a valid ECDSA signature corresponding to the pubkey that would return true when verified and not a value that would return false or be invalid otherwise (such as a NULLDUMMY).

Just like it’s handled in CheckECDSASignature: https://github.com/bitcoin/bitcoin/blob/ff69046e664f2460bb2ef595c5bd16eeb4a13f58/src/script/interpreter.cpp#L1655-L1657

Or in VerifyECDSASignature via CPubKey::Verify: https://github.com/bitcoin/bitcoin/blob/971952588daebf0d4d08a7e0eeef1734fcd8a9b1/src/pubkey.cpp#L56-L58

That’s why I believe sig.empty() is added separately.

rkrux commented at 3:30 pm on April 28, 2025:

any reason why the verification flags don’t also include SCRIPT_VERIFY_LOW_S?

I had been wondering about this as well. I think this flag should be included as the core wallet always produces low R/S values ECDSA signatures already ever since this change: #13666.

If I am not missing anything, might as well the wallet reject such PSBTs while unserializing?

rkrux commented at 3:35 pm on April 28, 2025:

Also, I realise that it would be cool to pass a reference to the last argument to CheckSignatureEncoding that captures the error so that we can relay it forward to the user; the current error message (or its variation) can remain as a prefix.

achow101 commented at 8:51 pm on April 28, 2025:

That’s why I believe sig.empty() is added separately.

Yes, that’s why.

any reason why the verification flags don’t also include SCRIPT_VERIFY_LOW_S?

Low S is a standardness rule, not a consensus rule. PSBTs may be produced by software other than Bitcoin Core, and these may sign with high S, for whatever reason. Since these are consensus valid signatures, we need to also accept them when decoding a PSBT.

theStack commented at 1:20 pm on April 20, 2025: contributor

Concept ACK

rkrux commented at 2:17 pm on April 25, 2025: contributor

I’m reviewing the PR again and will finalise my review(s) in the coming days.

in src/psbt.cpp:433 in 47bec7af45 outdated

428+    // Note that signing already aliases DEFAULT to ALL for non-taproot inputs.
429+    if (utxo.scriptPubKey.IsPayToTaproot() ? sighash != SIGHASH_DEFAULT :
430+                                            (sighash != SIGHASH_DEFAULT && sighash != SIGHASH_ALL)) {
431+        input.sighash_type = sighash;
432+    }
433     Assert(sighash.has_value());

rkrux commented at 9:51 am on April 26, 2025:

I feel this assert should come right after line 421 because that’s where sighash can be set last, after that its value is only read.

achow101 commented at 9:05 pm on April 28, 2025:

Done

in src/psbt.cpp:435 in 94fb539f4e outdated

424@@ -425,6 +425,26 @@ PSBTError SignPSBTInput(const SigningProvider& provider, PartiallySignedTransact
425     }
426     Assert(sighash.has_value());
427 
428+    // Check all existing signatures use the sighash type

rkrux commented at 9:56 am on April 26, 2025:

IMO it’d be good to extract this if-else piece out in a function called ValidatePSBTInputSigsWithSighash or similar that takes the input and the sighash as arguments.

Though making SignPSBTInput shorter is a benefit, suggesting this mostly to emphasise (to the reader) on what’s happening here.

achow101 commented at 9:06 pm on April 28, 2025:

I don’t think a separate function is necessary. This code is unlikely to be reused.

in src/psbt.cpp:449 in 94fb539f4e outdated

437+        if (!input.m_tap_key_sig.empty() && input.m_tap_key_sig.back() != *sighash) {
438+            return PSBTError::SIGHASH_MISMATCH;
439+        }
440+        for (const auto& [_, sig] : input.m_tap_script_sigs) {
441+            if (sig.back() != *sighash) return PSBTError::SIGHASH_MISMATCH;
442+        }

rkrux commented at 10:10 am on April 26, 2025:

These if blocks here in this else can possibly check for SIGHASH_ALL for taproot inputs, which seems ok to me from what I gather from the functional test, and I have not found anything yet in the BIP 341 that contradicts it. Also, the CheckSchnorrSignature function disallows only SIGHASH_DEFAULT to be explicitly mentioned, so SIGHASH_ALL is fine here I believe.

However, I believe it’d be nice to check the size of the signature is 65 if present in this case.

rkrux commented at 10:12 am on April 26, 2025:

Nit: Maybe can nest these 2 checks within a utxo.scriptPubKey.IsPayToTaproot() check because these are taproot specific sigs. Using SIGHASH_DEFAULT in the above if implies it’s a taproot input but this else can contain either.

achow101 commented at 9:06 pm on April 28, 2025:

I’ve added size checks. I did not gate behind a IsPayToTaproot as the fields should only be set if the output script is taproot anyways.

in src/psbt.cpp:442 in 94fb539f4e outdated

430+        if (!input.m_tap_key_sig.empty() && input.m_tap_key_sig.size() != 64) {
431+            return PSBTError::SIGHASH_MISMATCH;
432+        }
433+        for (const auto& [_, sig] : input.m_tap_script_sigs) {
434+            if (sig.size() != 64) return PSBTError::SIGHASH_MISMATCH;
435+        }

rkrux commented at 10:18 am on April 26, 2025:

I gather from the interpreter code that these 2 checks are testing for the fixed length of 64 in case of Schnorr signatures if sighash is absent, which is ok but it’d be decent to have a constant for 64 to make it explicit to the reader. https://github.com/bitcoin/bitcoin/blob/de90b47ea09826d52043f4ce40c734847187c075/src/script/interpreter.cpp#L1673-L1698

Maybe an enum SCHNORR_SIG_LENGTH?

0enum SCHNORR_SIG_LENGTH: size_t {
1 WITHOUT_HASHTYPE = 64,
2 WITH_HASHTYPE = 65,
3}

achow101 commented at 9:06 pm on April 28, 2025:

Meh, maybe someone can do that in a followup.

in test/functional/rpc_psbt.py:277 in 2a721dcfae outdated

201@@ -201,6 +202,34 @@ def test_input_confs_control(self):
202 
203         wallet.unloadwallet()
204 
205+    def test_sighash_mismatch(self):
206+        self.log.info("Test sighash type mismatches")

rkrux commented at 10:40 am on April 26, 2025:

Nit (& for 2a721dcfae44cb0fef6825e773e5895cb87a91a5 commit message): Because it’s easier to get the context from the log message instead of having to find in the test.

0- self.log.info("Test sighash type mismatches")
1+ self.log.info("Test sighash type mismatches in process psbt rpcs")

achow101 commented at 9:07 pm on April 28, 2025:

I don’t think it’s really necessary as these log lines are in the PSBT test, and the only way to sign with PSBTs is with *processpsbt.

in src/psbt.h:523 in 14e35e675c outdated

428@@ -429,6 +429,11 @@ struct PSBTInput
429                     std::vector<unsigned char> sig;

rkrux commented at 11:13 am on April 26, 2025:

Note for this commit: 14e35e675c576c770641160e982713dda4032bd8

I think this is added because later in the commit 94fb539f4e22ed4ec9a65f90db26cb81e5b8749a, the last element of the partial sig is used to check for the sighash type match while signing the PSBT input for which this signature validity guarantee is needed.

It’d be helpful to mention something around this in the commit message to aid review (& for future reference).

achow101 commented at 9:07 pm on April 28, 2025:

Added a sentence to the commit message.

rkrux commented at 11:22 am on April 26, 2025: contributor

Code review a7f76b28a1293929d05f15e0a9276d4ec3687c92. I reviewed focussing on new commits that are added and will do a subsequent review that finalises my understanding of the whole diff.

achow101 force-pushed on Apr 28, 2025

DrahtBot added the label CI failed on Apr 28, 2025

in src/qt/psbtoperationsdialog.cpp:86 in ea751ce645 outdated

82@@ -83,7 +83,7 @@ void PSBTOperationsDialog::signTransaction()
83 
84     WalletModel::UnlockContext ctx(m_wallet_model->requestUnlock());
85 
86-    const auto err{m_wallet_model->wallet().fillPSBT(SIGHASH_DEFAULT, /*sign=*/true, /*bip32derivs=*/true, &n_signed, m_transaction_data, complete)};

rkrux commented at 1:42 pm on April 29, 2025:

Wondering why this was SIGHASH_DEFAULT before as I don’t see anything Taproot specific here.

achow101 commented at 6:02 pm on April 29, 2025:

DEFAULT becomes ALL for non-taproot. It’s just easier to put DEFAULT everywhere since it covers all cases.

in src/wallet/scriptpubkeyman.cpp:1385 in ea751ce645 outdated

2632@@ -2640,7 +2633,10 @@ std::optional<PSBTError> DescriptorScriptPubKeyMan::FillPSBT(PartiallySignedTran
2633             }
2634         }
2635 
2636-        SignPSBTInput(HidingSigningProvider(keys.get(), /*hide_secret=*/!sign, /*hide_origin=*/!bip32derivs), psbtx, i, &txdata, sighash_type, nullptr, finalize);
2637+        PSBTError res = SignPSBTInput(HidingSigningProvider(keys.get(), /*hide_secret=*/!sign, /*hide_origin=*/!bip32derivs), psbtx, i, &txdata, sighash_type, nullptr, finalize);
2638+        if (res != PSBTError::OK && res != PSBTError::INCOMPLETE) {

rkrux commented at 2:24 pm on April 29, 2025:

This looked slightly odd to me at first glance. Interesting that an incomplete (not signed as per the SignPSBTInput) PSBT input is fine here at this stage because if it’s incomplete then the below if check would accordingly not update the number of inputs signed variable below. Only if the error was due to sighash_mismatch, it needs to be returned right away just like the behaviour prior to this change.

in src/psbt.h:1407 in ea751ce645 outdated

1403@@ -1396,10 +1404,10 @@ bool PSBTInputSignedAndVerified(const PartiallySignedTransaction psbt, unsigned
1404  * txdata should be the output of PrecomputePSBTData (which can be shared across
1405  * multiple SignPSBTInput calls). If it is nullptr, a dummy signature will be created.
1406  **/
1407-bool SignPSBTInput(const SigningProvider& provider, PartiallySignedTransaction& psbt, int index, const PrecomputedTransactionData* txdata, int sighash = SIGHASH_ALL, SignatureData* out_sigdata = nullptr, bool finalize = true);
1408+[[nodiscard]] PSBTError SignPSBTInput(const SigningProvider& provider, PartiallySignedTransaction& psbt, int index, const PrecomputedTransactionData* txdata, std::optional<int> sighash = std::nullopt, SignatureData* out_sigdata = nullptr, bool finalize = true);

rkrux commented at 2:39 pm on April 29, 2025:

+1 for adding the [[nodiscard]] attribute here!

in src/psbt.cpp:454 in ea751ce645 outdated

493 {
494-    // Only drop non_witness_utxos if sighash_type != SIGHASH_ANYONECANPAY
495-    if ((sighash_type & 0x80) != SIGHASH_ANYONECANPAY) {
496-        // Figure out if any non_witness_utxos should be dropped
497-        std::vector<unsigned int> to_drop;
498-        for (unsigned int i = 0; i < psbtx.inputs.size(); ++i) {

rkrux commented at 3:00 pm on April 29, 2025:

It appears that the logic of the removal of unnecessary transactions has become more fine-grained as it checks the sighash of each input now instead of treating all the inputs with the same sighash as done previously.

Overall seems better to me than it was earlier, I can’t think of any adverse consequence of it yet.

rkrux commented at 3:00 pm on April 29, 2025: contributor

I’m done with review ea751ce.

The wallet_backwards_compatibility.py test is failing. Guessing it’s because of the silent merge conflict issue #32369, I don’t see the exact error though in the Cirrus CI.

I will ack once the CI is green.

achow101 force-pushed on Apr 29, 2025

DrahtBot removed the label CI failed on Apr 29, 2025

rkrux approved

rkrux commented at 1:09 pm on April 30, 2025: contributor

tACK b6101b1849fbc2ec510c56cfae39d7de244a50cc

I found this pull more involved because of multiple moving pieces and I think I have covered all the areas that I could. Hence, sharing below my own notes of the review that hopefully would be helpful for anyone else reviewing the PR (& also my future self).

I see the main changes are along 3 lines:

Addition of atypical (non default/all) sighash type in the PSBT (Input).
Checking of the sighash type in the PSBT against the one provided in the RPC/CLI such as descriptorprocesspsbt, walletprocesspsbt.
Checking of all the different types of signatures in the PSBT inputs against the sighash type present in the PSBT input as per BIP 174; it would be helpful to mention in the PR description.

To achieve the above changes, SignPSBTInput is refactored to accommodate the above changes, and now returns a PSBTError instead of a boolean that can’t be discarded by the callers (enforced in compile time) and therefore must handle it.

Since SignPSBTInput is a common function used in multiple places & now checks for sighash as well (which is not always determined by the callers), SignPSBTInput now takes sighash as an optional leading to all the call sites passing in an optional if not being done already. This is what causes the diff to increase a bit as the call sites such as CWallet::FillPSBT -> ScriptPubKeyMan::FillPSBT & ProcessPSBT need to be updated along with their own callers as well that are many & are listed below.

0FillPSBT callers -> walletprocesspsbt; walletcreatefundedpsbt; feebumper; ExternalSignerScriptPubKeyMan::FillPSBT; interfaces::wallet::fillPSBT (includes the GUI models)
1
2ProcessPSBT callers -> descriptorprocesspsbt; utxoupdatepsbt

Another consequence of pushing down the sighash check in the common SignPSBTInput is that the descriptorprocesspsbt RPC also rightly handles it now that was not being done earlier as this check was part of the CWallet::FillPSBT flow, which is separate from the ProcessPSBT flow; this is tested in the sighash_mismatch subtest in the rpc_psbt functional test. Naturally, this check from the CWallet::FillPSBT is removed now to avoid duplication.

One more consequence is that the FinalisePSBT RPC also accounts for the sighash check because it internally uses SignPSBTInput and it’s used while verifying in the sighash_adding functional test. This conforms with what’s mentioned in BIP 174.

Also to verify whether PSBT input signatures actually conform with the PSBT input sighash type, the encoding of the ECDSA signatures is checked while unserialising the PSBT. Notably, PSBTs with high S signatures are allowed to make the core wallet compatible with other signers - fine because these though are non-standard, but not consensus-invalid.

Lastly, the removal of non-witness utxos from the PSBT in case of only segwit v1 inputs is refactored to take into account the newly added sighash type in the PSBT input as opposed to using the one that was passed in from the caller, leading to a more granular checking imo; this particular flow was already tested in test_utxo_conversion functional subtest.

DrahtBot requested review from theStack on Apr 30, 2025

rkrux referenced this in commit 87c8c6c7f5 on May 3, 2025

in test/functional/rpc_psbt.py:359 in 1f85f6911d outdated

269+            psbt = mod_psbt.to_base64()
270+            fin_res = self.nodes[0].finalizepsbt(psbt)
271+            assert_equal(fin_res["complete"], False)
272+
273+        self.nodes[0].sendrawtransaction(fin_hex)
274+        self.generate(self.nodes[0], 1)

theStack commented at 3:14 pm on May 6, 2025:

Rather than sending and mining only one finalized transaction, could instead use testmempoolaccept within the loop above, so the full check if the transaction is valid can be done for both the wallet_psbt and desc_psbt result (currently it’s only done for the latter; the first fin_hex result is overwritten).

achow101 commented at 7:09 pm on May 7, 2025:

Done

in src/wallet/feebumper.cpp:342 in 954d2913ac outdated

337@@ -338,8 +338,8 @@ bool SignTransaction(CWallet& wallet, CMutableTransaction& mtx) {
338         // First fill transaction with our data without signing,
339         // so external signers are not asked to sign more than once.
340         bool complete;
341-        wallet.FillPSBT(psbtx, complete, SIGHASH_ALL, false /* sign */, true /* bip32derivs */);
342-        auto err{wallet.FillPSBT(psbtx, complete, SIGHASH_ALL, true /* sign */, false  /* bip32derivs */)};
343+        wallet.FillPSBT(psbtx, complete, std::nullopt, false /* sign */, true /* bip32derivs */);
344+        auto err{wallet.FillPSBT(psbtx, complete, std::nullopt, true /* sign */, false  /* bip32derivs */)};

theStack commented at 3:20 pm on May 6, 2025:

nit: while touching these lines anyway, could fix the extra-weird formatting of the named argument comments.

achow101 commented at 7:09 pm on May 7, 2025:

Done

in test/functional/rpc_psbt.py:323 in 1f85f6911d outdated

234+        wallet = self.nodes[0].get_wallet_rpc("sighash_adding")
235+        def_wallet = self.nodes[0].get_wallet_rpc(self.default_wallet_name)
236+
237+        addr = wallet.getnewaddress(address_type="bech32")
238+        outputs = [{addr: 1}]
239+        outputs.append({wallet.getnewaddress(address_type="bech32m"): 1})

theStack commented at 3:27 pm on May 6, 2025:

nit: addr is never used anywhere else, could directly construct the outputs object in one go

achow101 commented at 7:09 pm on May 7, 2025:

Done

theStack approved

theStack commented at 3:54 pm on May 6, 2025: contributor

Code-review ACK b6101b1849fbc2ec510c56cfae39d7de244a50cc

Left some nits below, nothing critical. As others have already noted, some more test coverage for certain areas would be nice, but can be done in a follow-up, e.g.:

valid signature encoding check in parsing of PSBT_IN_PARTIAL_SIG (in particular, that a high-s signature is accepted)
PSBTError::INCOMPLETE error condition
unit test for CScript::IsPayToTaproot (trivial)

achow101 referenced this in commit 59d3e4ed34 on May 6, 2025

DrahtBot added the label Needs rebase on May 7, 2025

achow101 force-pushed on May 7, 2025

DrahtBot removed the label Needs rebase on May 7, 2025

achow101 force-pushed on May 7, 2025

rkrux approved

rkrux commented at 10:22 am on May 8, 2025: contributor

re-ACK f09e1f9

0git range-diff b6101b1...f09e1f9

Range diff contains changes from this review & rebase now that #28710 is merged that has led to the removal of LegacyScriptPubKeyMan and naturally all the changes in it from this PR - the overall diff has become marginally smaller.

DrahtBot requested review from theStack on May 8, 2025

in test/functional/rpc_psbt.py:359 in 1a145683cd outdated

269+            psbt = mod_psbt.to_base64()
270+            fin_res = self.nodes[0].finalizepsbt(psbt)
271+            assert_equal(fin_res["complete"], False)
272+
273+        self.nodes[0].sendrawtransaction(fin_hex)
274+        self.generate(self.nodes[0], 1)

theStack commented at 12:33 pm on May 8, 2025:

nitty nit: now that the checks are done via testmempoolaccept above, these two lines could be deleted

rkrux commented at 1:26 pm on May 8, 2025:

I feel it’s fine to keep them - imho the ultimate test is whether the transaction can be mined, gives me more confidence.

theStack approved

theStack commented at 12:36 pm on May 8, 2025: contributor

re-ACK f09e1f943dbd1e5865dece3c35f342e4f0b31b9e :writing_hand: :hash:

DrahtBot added the label Needs rebase on May 14, 2025

psbt: Require ECDSA signatures to be validly encoded

Needed for later validation of sighash types.

5a5d26d612

tests: Test PSBT sighash type mismatch 2adfd81532

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.

e58b680923

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.

d6001dcd4a

script: Add IsPayToTaproot() a118256948

psbt: Check sighash types in SignPSBTInput and take sighash as optional 4c7d767e49

wallet: Remove sighash type enforcement from FillPSBT 1f71cd337a

psbt: Enforce sighash type of signatures matches psbt

BIP 174 states that the sighash type of all signatures must match the
type given by the PSBT, so do that.

15ce1bd73f

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.

28781b5f06

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.

2b7682c372

rpc, psbt: Require sighashes match for descriptorprocesspsbt ee045b61ef

achow101 force-pushed on May 14, 2025

DrahtBot removed the label Needs rebase on May 14, 2025

theStack approved

theStack commented at 10:04 am on May 15, 2025: contributor

re-ACK ee045b61efc1479c1866b786661ef39a863677d0 (as per $ git range-diff f09e1f94...ee045b61)

DrahtBot requested review from rkrux on May 15, 2025

rkrux approved

rkrux commented at 10:36 am on May 15, 2025: contributor

re-ACK ee045b61efc1479c1866b786661ef39a863677d0

Only rebase changes from #32305

0git range-diff f09e1f94...ee045b61

in src/common/types.h:24 in ee045b61ef

19@@ -20,6 +20,8 @@ enum class PSBTError {
20     EXTERNAL_SIGNER_NOT_FOUND,
21     EXTERNAL_SIGNER_FAILED,
22     UNSUPPORTED,
23+    INCOMPLETE,
24+    OK,

fjahr commented at 10:13 pm on May 15, 2025:

nit: Having an OK variant makes this more into an PSBTResult rather than PSBTError

achow101 commented at 5:36 pm on May 20, 2025:

Will leave for a followup

in src/interfaces/wallet.h:207 in d6001dcd4a outdated

203@@ -204,7 +204,7 @@ class Wallet
204         int& num_blocks) = 0;
205 
206     //! Fill PSBT.
207-    virtual std::optional<common::PSBTError> fillPSBT(int sighash_type,
208+    virtual std::optional<common::PSBTError> fillPSBT(std::optional<int> sighash_type,

fjahr commented at 10:19 pm on May 15, 2025:

Since there is now a PSBTError::OK it seems like there is no need for using an std::optional<common::PSBTError> return type. But this can be made more consistent in a follow-up.

achow101 commented at 5:36 pm on May 20, 2025:

Will leave for a followup

fjahr commented at 10:54 pm on May 15, 2025: contributor

Code review ACK ee045b61efc1479c1866b786661ef39a863677d0

My suggestions make more sense as a follow-up than addressing them here.

fanquake merged this on May 21, 2025

fanquake closed this on May 21, 2025

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

Code Coverage & Benchmarks

Reviews

Conflicts