psbt: Actually use SIGHASH_DEFAULT for PSBT signing

achow101 commented at 2:17 am on July 21, 2021: member

Make the behavior align with the help text by actually using SIGHASH_DEFAULT as the default sighash for signing PSBTs.

achow101 renamed this:
~~rpc: Acutally use SIGHASH_DEFAULT for PSBT signing~~
rpc: Actually use SIGHASH_DEFAULT for PSBT signing
on Jul 21, 2021

achow101 force-pushed on Jul 21, 2021

fanquake added the label PSBT on Jul 21, 2021

fanquake renamed this:
~~rpc: Actually use SIGHASH_DEFAULT for PSBT signing~~
psbt: Actually use SIGHASH_DEFAULT for PSBT signing
on Jul 21, 2021

MarcoFalke commented at 6:18 am on July 21, 2021: member

Is this for backport?

DrahtBot commented at 10:26 am on July 21, 2021: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#21283 (Implement BIP 370 PSBTv2 by achow101)
#17034 ([BIP 174] PSBT version, proprietary, and xpub fields 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.

achow101 commented at 4:37 pm on July 21, 2021: member

Is this for backport?

There is no explicit PSBT support for Taproot yet, so I don’t think so. Although we do still sign PSBTs that have Taproot inputs, but only if we make those inputs final.

achow101 force-pushed on Jul 21, 2021

GeneFerneau commented at 5:28 pm on August 5, 2021: none

Concept + code review ACK 96f78e

meshcollider commented at 1:40 am on September 30, 2021: contributor

utACK 96f78ed1fdd9c5de2fa5ff125f79b37bd75af9c1

in src/wallet/wallet.h:584 in 96f78ed1fd outdated

580@@ -581,7 +581,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
581      */
582     TransactionError FillPSBT(PartiallySignedTransaction& psbtx,
583                   bool& complete,
584-                  int sighash_type = 1 /* SIGHASH_ALL */,
585+                  int sighash_type = 0 /* SIGHASH_DEFAULT */,

theStack commented at 12:29 pm on November 12, 2021:

Maybe I’m missing something obvious, but any reason why we use magic numbers with comments here (and also for the interface in src/wallet/scriptpubkeyman.h), rather than the enum directly?

0                  int sighash_type = SIGHASH_DEFAULT,

achow101 commented at 5:05 pm on November 12, 2021:

There was a time a while ago where that didn’t work and this code has just been moved around to various places without changing. I’ve changed it to use SIGHASH_DEFAULT directly.

theStack commented at 12:30 pm on November 12, 2021: contributor

Concept ACK

achow101 force-pushed on Nov 12, 2021

achow101 commented at 10:36 pm on November 12, 2021: member

Apparently without this, we may end up creating final transactions with a lower feerate than expected.

benthecarman commented at 2:02 am on November 13, 2021: contributor

crACK ed964019f7af13028c99381c3c2b9b1b4d060409

in src/wallet/scriptpubkeyman.cpp:627 in ed964019f7 outdated

623@@ -624,7 +624,7 @@ TransactionError LegacyScriptPubKeyMan::FillPSBT(PartiallySignedTransaction& psb
624         }
625 
626         // Get the Sighash type
627-        if (sign && input.sighash_type > 0 && input.sighash_type != sighash_type) {
628+        if (sign && input.sighash_type > 0 && sighash_type > 0 && input.sighash_type != sighash_type) {

sipa commented at 3:41 pm on November 15, 2021:

Is this right? I think we should distinguish based on whether the PSBT having having a PSBT_IN_SIGHASH_TYPE record and not. I think the current code treats there not being a record, and the record being 0 as identical?

achow101 commented at 7:06 pm on November 15, 2021:

Hmm, yes. I’ve changed input.sighash_type to be an optional. The issue this particular change fixed was test related, so I’ve updated the tests instead.

achow101 force-pushed on Nov 15, 2021

Sjors commented at 3:08 pm on November 19, 2021: member

Mmm, the help text was changed from defaulting to ALL to SIGHASH_DEFAULT in https://github.com/bitcoin/bitcoin/commit/458a345b0590fd2fa04c7d8d70beb8d57e34bbc8, but that change also made sure for non-taproot it’s interpreted as ALL.

Let’s make the help text say RPCArg::Default{"DEFAULT for Taproot, ALL otherwise"} and behave accordingly.

The first commit 62621db looks good to me.

achow101 commented at 6:14 pm on November 19, 2021: member

Let’s make the help text say RPCArg::Default{"DEFAULT for Taproot, ALL otherwise"} and behave accordingly.

Added a commit for that.

achow101 force-pushed on Nov 19, 2021

Sjors commented at 5:00 pm on November 22, 2021: member

utACK 322d117f6f39d8534b4ea77cd46fbe0635a7b3c5

Maybe swap the last two commits for clarity (or add “for taproot” to ff1ec7b)

in src/psbt.h:89 in fcc10ca420 outdated

85@@ -86,9 +86,9 @@ struct PSBTInput
86             }
87 
88             // Write the sighash type
89-            if (sighash_type > 0) {
90+            if (sighash_type != std::nullopt) {

sipa commented at 5:02 pm on November 22, 2021:

In commit “psbt: Make signhash_type std::optional”

Nit: I think the ideomatic way of writing this would be if (sighash_type.has_value()) or just if (sighash_type) (here and further).

in test/functional/rpc_psbt.py:460 in ff1ec7b11a outdated

454@@ -455,7 +455,7 @@ def run_test(self):
455             wrpc = self.nodes[2].get_wallet_rpc("wallet{}".format(i))
456             for key in signer['privkeys']:
457                 wrpc.importprivkey(key)
458-            signed_tx = wrpc.walletprocesspsbt(signer['psbt'])['psbt']
459+            signed_tx = wrpc.walletprocesspsbt(signer['psbt'], True, "ALL")['psbt']

sipa commented at 5:04 pm on November 22, 2021:

In commit “psbt: Actually use SIGHASH_DEFAULT”

What is this change for?

achow101 commented at 5:08 pm on November 22, 2021:

At least one of the tests sets the sighash type in the PSBT. However when we sign, if the sighash type given in the RPC does not match the sighash type in the PSBT, we fail to sign. So this change is to make sure that we do sign those PSBTs.

sipa commented at 5:05 pm on November 22, 2021: member

utACK 322d117f6f39d8534b4ea77cd46fbe0635a7b3c5

DrahtBot added the label Needs rebase on Nov 29, 2021

achow101 force-pushed on Dec 5, 2021

DrahtBot removed the label Needs rebase on Dec 5, 2021

Sjors commented at 3:08 am on December 6, 2021: member

re-utACK 3624dc5f974beb0f31800a1828efc46a1dd9f78f after rebase

DrahtBot added the label Needs rebase on Dec 8, 2021

psbt: Make sighash_type std::optional<int>

It is better to ues an optional to determine whether the sighash type
is set rather than using 0 as a magic number.

eb9a1a2c59

psbt: Actually use SIGHASH_DEFAULT

Make the behavior align with the help text by actually using
SIGHASH_DEFAULT as the default sighash for signing PSBTs.

d3992669df

rpc: Document that DEFAULT is for Taproot, ALL for everything else c0405ee27f

achow101 force-pushed on Dec 8, 2021

DrahtBot removed the label Needs rebase on Dec 8, 2021

Sjors commented at 3:07 am on December 9, 2021: member

re-utACK c0405ee27fa23a9868f04c052bdc94d7accc57e2

To my surprise, this still worked despite the rename:

0PREV=3624dc5 N=3 && git range-diff `git merge-base --all HEAD $PREV`...$PREV HEAD~$N...HEAD

MarcoFalke merged this on Dec 10, 2021

MarcoFalke closed this on Dec 10, 2021

sidhujag referenced this in commit 8a73c22689 on Dec 10, 2021

RandyMcMillan referenced this in commit 04eb2b7ea2 on Dec 23, 2021

theStack referenced this in commit 0a5739f09f on Mar 13, 2022

theStack referenced this in commit 4e1503dbc8 on Mar 13, 2022

theStack referenced this in commit 12cc0201c2 on Mar 14, 2022

MarcoFalke referenced this in commit 74f8c551e9 on Mar 17, 2022

sidhujag referenced this in commit 79173fc631 on Mar 18, 2022

jonatack referenced this in commit 44b37e6e57 on Mar 28, 2022

hebasto referenced this in commit cd9cfc9582 on Mar 31, 2022

jonatack referenced this in commit eaa04194b9 on Mar 31, 2022

in src/wallet/rpc/spend.cpp:1170 in c0405ee27f

1166@@ -1167,7 +1167,7 @@ RPCHelpMan walletprocesspsbt()
1167                 {
1168                     {"psbt", RPCArg::Type::STR, RPCArg::Optional::NO, "The transaction base64 string"},
1169                     {"sign", RPCArg::Type::BOOL, RPCArg::Default{true}, "Also sign the transaction when updating (requires wallet to be unlocked)"},
1170-                    {"sighashtype", RPCArg::Type::STR, RPCArg::Default{"DEFAULT"}, "The signature hash type to sign with if not specified by the PSBT. Must be one of\n"
1171+                    {"sighashtype", RPCArg::Type::STR, RPCArg::Default{"DEFAULT for Taproot, ALL otherwise"}, "The signature hash type to sign with if not specified by the PSBT. Must be one of\n"

darosior commented at 10:13 am on June 13, 2022:

How is it “ALL otherwise”? If a user wants ALL, even for non-Taproot descriptor, they now have to explicitly set it. It’s a (minor) breaking change which i think should have had a release note.

More confusing is that even if ALL is set in the PSBT input, this will use DEFAULT. A user has to specify it using the command line for the whole transaction.

achow101 commented at 4:44 pm on September 15, 2022:

ALL is the sighash type that is signed with by default for non-Taproot things. I don’t see how this is incorrect?

darosior commented at 9:18 am on September 16, 2022:

Hmm? That’s the point. It’s not.

You changed ParseSighashString in this PR setting DEFAULT for all types, not only Taproot. https://github.com/bitcoin/bitcoin/blob/19526d937fbd16b3c868f4a7ecd2c076e71eb8fc/src/core_read.cpp#L255-L277

achow101 commented at 3:04 pm on September 16, 2022:

DEFAULT is an alias for ALL internally, and signing non-taproot things will result in a ALL being appended to the signature.

darosior commented at 3:37 pm on September 16, 2022:

Right, i think the issue was because in my repro i had the sighash flag set in the PSBT input. Have i tried to reproduce it before reporting i could have been more precise, sorry about that.

It’s still a (minor) API break since the PSBT was signed without failure in 22.0 but failed in 23.0 (as shown by the necessity to modify the functional test for it to pass in this PR https://github.com/bitcoin/bitcoin/pull/22514/files#r754474555). But since it was released like this already i agree with your comment in #25876 that this doesn’t need “fixing” (not sure there could have been any reasonable fix anyways).

darosior referenced this in commit e72146ecbf on Jun 20, 2022

jamesdorfman referenced this in commit 3ef3a3ba00 on Jun 13, 2023

bitcoin locked this on Sep 16, 2023

psbt: Actually use SIGHASH_DEFAULT for PSBT signing #22514

Conflicts