psbt: Actually use SIGHASH_DEFAULT for PSBT signing #22514

achow101 · 2021-07-21T02:17:07Z

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

maflcko · 2021-07-21T06:18:53Z

Is this for backport?

DrahtBot · 2021-07-21T10:26:31Z

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 · 2021-07-21T16:37:31Z

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.

GeneFerneau · 2021-08-05T17:28:28Z

Concept + code review ACK 96f78e

meshcollider

utACK 96f78ed

theStack

Concept ACK

src/wallet/wallet.h

achow101 · 2021-11-12T22:36:34Z

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

benthecarman

crACK ed96401

src/wallet/scriptpubkeyman.cpp

Sjors · 2021-11-19T15:08:10Z

Mmm, the help text was changed from defaulting to ALL to SIGHASH_DEFAULT in 458a345, 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 · 2021-11-19T18:14:08Z

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

Added a commit for that.

Sjors · 2021-11-22T17:00:19Z

utACK 322d117

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

sipa · 2021-11-22T17:02:33Z

src/psbt.h

@@ -86,9 +86,9 @@ struct PSBTInput
            }

            // Write the sighash type
-            if (sighash_type > 0) {
+            if (sighash_type != std::nullopt) {


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

sipa · 2021-11-22T17:04:58Z

test/functional/rpc_psbt.py

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


In commit "psbt: Actually use SIGHASH_DEFAULT"

What is this change for?

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 · 2021-11-22T17:05:33Z

utACK 322d117

Sjors · 2021-12-06T03:08:13Z

re-utACK 3624dc5 after rebase

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

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

Sjors · 2021-12-09T03:07:31Z

re-utACK c0405ee

To my surprise, this still worked despite the rename:

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

c0405ee rpc: Document that DEFAULT is for Taproot, ALL for everything else (Andrew Chow) d399266 psbt: Actually use SIGHASH_DEFAULT (Andrew Chow) eb9a1a2 psbt: Make sighash_type std::optional<int> (Andrew Chow) Pull request description: Make the behavior align with the help text by actually using SIGHASH_DEFAULT as the default sighash for signing PSBTs. ACKs for top commit: Sjors: re-utACK c0405ee Tree-SHA512: 5199fb41de416b2f10ac451f824e7c94b428ba11fdb9e50f0027c692e959ce5813a340c34a4e52d7aa128e12008303d80939a693eff36a869720e45442119828

…SBT signing 5b0c5b8 rpc: Document that DEFAULT is for Taproot, ALL for everything else (Andrew Chow) 61dde4b psbt: Actually use SIGHASH_DEFAULT (Andrew Chow) 94c5107 psbt: Make sighash_type std::optional<int> (Andrew Chow) Pull request description: Make the behavior align with the help text by actually using SIGHASH_DEFAULT as the default sighash for signing PSBTs. ACKs for top commit: Sjors: re-utACK 5b0c5b8 Tree-SHA512: 5199fb41de416b2f10ac451f824e7c94b428ba11fdb9e50f0027c692e959ce5813a340c34a4e52d7aa128e12008303d80939a693eff36a869720e45442119828

PSBT signing was changed to use SIGHASH_DEFAULT by default in bitcoin#22514. The signet miner script sets the sighash type of the created PSBT to SIGHASH_ALL, hence this leads to a sighash mismatch when the `walletprocesspsbt` RPC is called. Fix this by explicitly passing the correct sighash type. Note that the same change was needed in one of our functional tests, see commit d399266. Reported by gruve-pu.

PSBT signing was changed to use SIGHASH_DEFAULT by default in bitcoin#22514. The signet miner script sets the sighash type of the created PSBT to SIGHASH_ALL, hence this leads to a sighash mismatch when the `walletprocesspsbt` RPC is called. Fix this by explicitly passing the correct sighash type. Note that the same change was needed in one of our functional tests, see commit d399266. Reported by gruve-p.

12cc020 contrib: fix signet miner (sighash mismatch) (Sebastian Falbesoner) Pull request description: gruve-p reported that the signet miner doesn't work anymore (see bitcoin#24501 (comment)), failing with the following error of the `walletprocesspsbt` RPC: ``` error code: -22 error message: Specified sighash value does not match value stored in PSBT ..... subprocess.CalledProcessError: Command '['bitcoin-cli', '-signet', '-stdin', 'walletprocesspsbt']' returned non-zero exit status 22 ``` PSBT signing was changed to use SIGHASH_DEFAULT by default in bitcoin#22514. The signet miner script sets the sighash type of the created PSBT to SIGHASH_ALL (3 is the per-input type PSBT_IN_SIGHASH_TYPE, following a little-endian 32 unsigned integer of the sighash type): https://github.com/bitcoin/bitcoin/blob/e04720ec3336e3df7fce522e3b1da972aa65ff62/contrib/signet/miner#L169-L170 hence this leads to a sighash mismatch when the `walletprocesspsbt` RPC is called. Fix this by explicitly passing the correct sighash type. The same change was needed in one of our functional tests, see commit d399266. Note that instead of feeding the PSBT via `-stdin` it is directly passed as parameter, as I couldn't figure out a way to pass multiple parameters otherwise (separating by newline also didn't work). ACKs for top commit: kallewoof: ACK 12cc020 ajtowns: ACK 12cc020 ; code review only Tree-SHA512: 8509e768e96f85e28c0ca0dc2d35874aa29623febddc46bf90472ec38f38cb3a1b5407c563fd9101d07088775d0fdb18e9137cc38955e847885b83c16591c736

PSBT signing was changed to use SIGHASH_DEFAULT by default in bitcoin#22514. The signet miner script sets the sighash type of the created PSBT to SIGHASH_ALL, hence this leads to a sighash mismatch when the `walletprocesspsbt` RPC is called. Fix this by explicitly passing the correct sighash type. Note that the same change was needed in one of our functional tests, see commit d399266. Reported by gruve-p. Github-Pull: bitcoin#24553 Rebased-From: 12cc020

darosior · 2022-06-13T10:13:41Z

src/wallet/rpc/spend.cpp

@@ -1167,7 +1167,7 @@ RPCHelpMan walletprocesspsbt()
                {
                    {"psbt", RPCArg::Type::STR, RPCArg::Optional::NO, "The transaction base64 string"},
                    {"sign", RPCArg::Type::BOOL, RPCArg::Default{true}, "Also sign the transaction when updating (requires wallet to be unlocked)"},
-                    {"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"
+                    {"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"


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.

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

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

You changed ParseSighashString in this PR setting DEFAULT for all types, not only Taproot.

bitcoin/src/core_read.cpp

Lines 255 to 277 in 19526d9

int ParseSighashString(const UniValue& sighash)

{

int hash_type = SIGHASH_DEFAULT;

if (!sighash.isNull()) {

static std::map<std::string, int> map_sighash_values = {

{std::string("DEFAULT"), int(SIGHASH_DEFAULT)},

{std::string("ALL"), int(SIGHASH_ALL)},

{std::string("ALL|ANYONECANPAY"), int(SIGHASH_ALL|SIGHASH_ANYONECANPAY)},

{std::string("NONE"), int(SIGHASH_NONE)},

{std::string("NONE|ANYONECANPAY"), int(SIGHASH_NONE|SIGHASH_ANYONECANPAY)},

{std::string("SINGLE"), int(SIGHASH_SINGLE)},

{std::string("SINGLE|ANYONECANPAY"), int(SIGHASH_SINGLE|SIGHASH_ANYONECANPAY)},

};

const std::string& strHashType = sighash.get_str();

const auto& it = map_sighash_values.find(strHashType);

if (it != map_sighash_values.end()) {

hash_type = it->second;

} else {

throw std::runtime_error(strHashType + " is not a valid sighash parameter.");

}

}

return hash_type;

}

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

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

…nd` version for CI testing cd21ab8 Bump in `bitcoind` version used in CI. (Zshan0) 9dbda27 Made `sighash_type` explicit (Zshan0) Pull request description: Since the Bitcoin Core ignores the sighash type specified inside the PSBT inputs, `sign_psbt` relied on the default value of `sighash_type` which aligned with our requirements. bitcoin/bitcoin#22514 changed the default value from `SIGHASH_ALL` to `SIGHASH_DEFAULT`. Making `sighash_type` explicit fixes the minor breaking change caused by the update in the bitcoin wallet. Note that since `sighashtype` parameter comes after the `sign` parameter, the value of `sign` has been set to the default value, `true`. Along with the fix, `bitcoind` version used in CI has been bumped from `22.0` to `23.0`. This issue closes #414. ACKs for top commit: darosior: ACK cd21ab8 Tree-SHA512: fe89d31c06be74221f761349e7652ce3a4ce4cceb950e8f793e68eeb51f46d41771562c300581f403d0c9cd6585b0fb838cec4401f8f95458657ff0c6564face

achow101 changed the title ~~rpc: Acutally use SIGHASH_DEFAULT for PSBT signing~~ rpc: Actually use SIGHASH_DEFAULT for PSBT signing Jul 21, 2021

achow101 force-pushed the psbt-sighash-default branch from de515bc to 07d0ce7 Compare July 21, 2021 02:17

fanquake added the PSBT label Jul 21, 2021

fanquake changed the title ~~rpc: Actually use SIGHASH_DEFAULT for PSBT signing~~ psbt: Actually use SIGHASH_DEFAULT for PSBT signing Jul 21, 2021

DrahtBot mentioned this pull request Jul 21, 2021

rpc: Allow walletprocesspsbt to sign without finalizing #22513

Merged

achow101 force-pushed the psbt-sighash-default branch 2 times, most recently from 352e00f to 96f78ed Compare July 21, 2021 19:10

achow101 mentioned this pull request Jul 26, 2021

psbt: Taproot fields for PSBT #22558

Merged

meshcollider reviewed Sep 30, 2021

View reviewed changes

theStack reviewed Nov 12, 2021

View reviewed changes

src/wallet/wallet.h Outdated Show resolved Hide resolved

achow101 force-pushed the psbt-sighash-default branch from 96f78ed to ed96401 Compare November 12, 2021 17:05

achow101 mentioned this pull request Nov 13, 2021

wallet: Avoid underpaying transaction fees when signing taproot spends #23502

Closed

benthecarman reviewed Nov 13, 2021

View reviewed changes

sipa reviewed Nov 15, 2021

View reviewed changes

src/wallet/scriptpubkeyman.cpp Outdated Show resolved Hide resolved

achow101 force-pushed the psbt-sighash-default branch from ed96401 to 32c9ab7 Compare November 15, 2021 19:05

This was referenced Nov 17, 2021

[BIP 174] PSBT version, proprietary, and xpub fields #17034

Merged

Implement BIP 370 PSBTv2 #21283

Open

achow101 force-pushed the psbt-sighash-default branch from c592939 to 322d117 Compare November 19, 2021 18:15

sipa reviewed Nov 22, 2021

View reviewed changes

This was referenced Nov 23, 2021

Add external signer taproot support #23578

Merged

wallet: Split stuff from rpcwallet #23602

Merged

DrahtBot removed the Needs rebase label Dec 5, 2021

DrahtBot mentioned this pull request Dec 6, 2021

Split up rpcwallet #23667

Merged

DrahtBot added the Needs rebase label Dec 8, 2021

achow101 added 3 commits December 8, 2021 09:43

psbt: Make sighash_type std::optional<int>

eb9a1a2

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

psbt: Actually use SIGHASH_DEFAULT

d399266

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

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

c0405ee

achow101 force-pushed the psbt-sighash-default branch from 3624dc5 to c0405ee Compare December 8, 2021 15:00

DrahtBot removed the Needs rebase label Dec 8, 2021

maflcko merged commit 011d6e4 into bitcoin:master Dec 10, 2021

gruve-p mentioned this pull request Mar 8, 2022

v23.0 testing #24501

Closed

5 tasks

theStack mentioned this pull request Mar 13, 2022

contrib: fix signet miner (sighash mismatch) #24553

Merged

darosior reviewed Jun 13, 2022

View reviewed changes

darosior mentioned this pull request Jun 13, 2022

Minor API break by Bitcoin Core wallet revault/revaultd#414

Closed

Zshan0 mentioned this pull request Jun 13, 2022

sighash_type made explicit along with a bump in bitcoind version for CI testing revault/revaultd#415

Merged

darosior mentioned this pull request Aug 19, 2022

docfix: sighash parameter for walletprocesspsbt #25876

Closed

jamesdorfman added a commit to jamesdorfman/elements that referenced this pull request Jun 13, 2023

Merge 011d6e4 into merged_master (Bitcoin PR bitcoin/bitcoin#22514)

3ef3a3b

bitcoin locked and limited conversation to collaborators Sep 16, 2023

	int ParseSighashString(const UniValue& sighash)
	{
	int hash_type = SIGHASH_DEFAULT;
	if (!sighash.isNull()) {
	static std::map<std::string, int> map_sighash_values = {
	{std::string("DEFAULT"), int(SIGHASH_DEFAULT)},
	{std::string("ALL"), int(SIGHASH_ALL)},
	{std::string("ALL\|ANYONECANPAY"), int(SIGHASH_ALL\|SIGHASH_ANYONECANPAY)},
	{std::string("NONE"), int(SIGHASH_NONE)},
	{std::string("NONE\|ANYONECANPAY"), int(SIGHASH_NONE\|SIGHASH_ANYONECANPAY)},
	{std::string("SINGLE"), int(SIGHASH_SINGLE)},
	{std::string("SINGLE\|ANYONECANPAY"), int(SIGHASH_SINGLE\|SIGHASH_ANYONECANPAY)},
	};
	const std::string& strHashType = sighash.get_str();
	const auto& it = map_sighash_values.find(strHashType);
	if (it != map_sighash_values.end()) {
	hash_type = it->second;
	} else {
	throw std::runtime_error(strHashType + " is not a valid sighash parameter.");
	}
	}
	return hash_type;
	}

psbt: Actually use SIGHASH_DEFAULT for PSBT signing #22514

psbt: Actually use SIGHASH_DEFAULT for PSBT signing #22514

Uh oh!

Conversation

achow101 commented Jul 21, 2021

Uh oh!

maflcko commented Jul 21, 2021

Uh oh!

DrahtBot commented Jul 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

achow101 commented Jul 21, 2021

Uh oh!

GeneFerneau commented Aug 5, 2021

Uh oh!

meshcollider left a comment

Choose a reason for hiding this comment

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

achow101 commented Nov 12, 2021

Uh oh!

benthecarman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Sjors commented Nov 19, 2021

Uh oh!

achow101 commented Nov 19, 2021

Uh oh!

Sjors commented Nov 22, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipa commented Nov 22, 2021

Uh oh!

Sjors commented Dec 6, 2021

Uh oh!

Sjors commented Dec 9, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DrahtBot commented Jul 21, 2021 •

edited

Loading