contrib: fix signet miner (sighash mismatch) #24553

theStack · 2022-03-13T21:27:25Z

gruve-p reported that the signet miner doesn't work anymore (see #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 #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):

bitcoin/contrib/signet/miner

Lines 169 to 170 in e04720e

    
           psbt.i = [ PSBTMap( {0: spendme.serialize(), 
        
                                3: bytes([1,0,0,0])})

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

theStack · 2022-03-13T21:28:11Z

pinging @achow101 @gruve-p

gruve-p · 2022-03-13T22:18:58Z

Just tested and this fixes the issue

achow101 · 2022-03-14T10:18:43Z

Code Review ACK 4e1503d

ajtowns · 2022-03-14T10:20:07Z

This doesn't seem right? The walletprocesspsbt docs say

sighashtype (string, optional, default="DEFAULT for Taproot, ALL otherwise") The signature hash type to sign with if not specified by the PSBT.

but in this case the signature hash type is specified by the PSBT, so we're just getting a redundant error?

If we do have to specify the sighash type via RPC as well as the PSBT, you don't need to remove the -stdin to do it; this works:

-        psbt_signed = json.loads(args.bcli("-stdin", "walletprocesspsbt", input=psbt.encode('utf8')))
+        psbt_signed = json.loads(args.bcli("-stdin", "walletprocesspsbt", input=(psbt.encode('utf8') + b"\ntrue\nALL")))

(Passing the psbt via stdin is necessary, as otherwise the block size is limited by your shell's maximum argument length)

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.

theStack · 2022-03-14T11:12:35Z

This doesn't seem right? The walletprocesspsbt docs say

sighashtype (string, optional, default="DEFAULT for Taproot, ALL otherwise") The signature hash type to sign with if not specified by the PSBT.

but in this case the signature hash type is specified by the PSBT, so we're just getting a redundant error?

Good point. Looking at the code, the sighash passed for the walletprocesspsbt (or, the default value SIGHASH_DEFAULT) is always compared to each input's sighash if it is specified in the PSBT:

bitcoin/src/wallet/scriptpubkeyman.cpp

Lines 643 to 645 in 25d045a

    
           if (sign && input.sighash_type != std::nullopt && *input.sighash_type != sighash_type) { 
        
               return TransactionError::SIGHASH_MISMATCH; 
        
           }

So it seems the help text should be changed?

If we do have to specify the sighash type via RPC as well as the PSBT, you don't need to remove the -stdin to do it; this works:
-        psbt_signed = json.loads(args.bcli("-stdin", "walletprocesspsbt", input=psbt.encode('utf8')))
+        psbt_signed = json.loads(args.bcli("-stdin", "walletprocesspsbt", input=(psbt.encode('utf8') + b"\ntrue\nALL")))
(Passing the psbt via stdin is necessary, as otherwise the block size is limited by your shell's maximum argument length)

Thanks, that works indeed. I think yesterday when I tried this I accidentially swapped the argument order of "walletprocesspsbt" and "-stdin" and wondered why it doesn't work - d'oh! Done; used os.linesep instead of \n directly in order to be more portable (OTOH I don't know if the mining script even works on a non-unixoid system).

ajtowns · 2022-03-14T15:35:30Z

#24563 has a patch to make walleprocesspsbt work the way I thought it was meant to, which seems like it's also enough to get contrib/signet/miner working again

kallewoof

ACK 12cc020

gruve-p · 2022-03-16T20:58:25Z

Do I need to retest this or do we prefer the patch approach on #24563 (in any case either needs a milestone 23.0 and backport label) as the signet miner is currently broken on the rc

Also does #24559 superseed this PR? In that case this one can get closed?

ajtowns

ACK 12cc020 ; code review only

Fine to merge this whether walletprocesspsbt behaviour is changed or not.

gruve-p · 2022-03-17T17:41:21Z

Post-merge ACK 12cc020

theStack · 2022-03-17T17:50:33Z

Also does #24559 superseed this PR? In that case this one can get closed?

#24559 adds a functional test for the signet mining script, in order to find potential bugs automatically in the future, rather than by accident months after its introduction (thanks again for reporting!). Including this PR's commit in #24559 was necessary to make the CI tests pass; now that the bugfix is merged, #24559 is rebased on master, solely consisting of the commit adding the test. Review/testing would of course be much appreciated!

laanwj · 2022-03-17T21:42:57Z

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

~~It's strange that newline didn't work. This is the supposed way to pass multiple arguments using -stdin. E.g. see the documentation:~~

Read extra arguments from standard input, one per line until EOF/Ctrl-D

~~If it doesn't work work anymore, that's a bug. Note that it only works for positional arguments, not named ones.~~

Edit: oh, I see you did get it to work, but didn't update the OP that ended up in the commit message.

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

jonatack · 2022-03-28T11:15:26Z

Backported to v23.0 in #24512.

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

174af33 util: Add inotify_rm_watch to syscall sandbox (AllowFileSystem) (Hennadii Stepanov) ded10fe build: Fix Boost.Process test for Boost 1.78 (Hennadii Stepanov) 26c2f23 build: Fix Boost.Process detection on macOS arm64 (Hennadii Stepanov) 85f85c7 util: add linkat to syscall sandbox (AllowFileSystem) (fanquake) eaa0419 contrib: fix signet miner (sighash mismatch) (Sebastian Falbesoner) 235b042 rpc: Exclude descriptor when address is excluded (MarcoFalke) b05a59b ci: Temporarily use clang-13 to work around clang-14 TSan bug (MarcoFalke) 65b9667 doc, init: add links to doc/cjdns.md (Jon Atack) 7a553d4 doc: update i2p.md with cjdns, improve local addresses section (Jon Atack) 4148396 doc: update tor.md with cjdns and getnodeaddresses, fix tor grep, (Jon Atack) 4690e8a doc: create initial doc/cjdns.md for cjdns how-to documentation (Jon Atack) 5d24f61 Clarify in -maxtimeadjustment that only outbound peers influence time data (Jon Atack) b1646f1 test: set segwit height back to 0 on regtest (Martin Zumsande) ef6a37b rpc: rename getdeploymentinfo status-next to status_next (Jon Atack) 2a6fcf9 init, doc: improve -onlynet help and tor/i2p documentation (Jon Atack) Pull request description: Backport the following to 23.x: - #24468 - #24528 - #24527 - #24609 - #24555 - #24663 - #24572 - #24636 - #24553 - #24659 - #24521 - #24523 - #24690 - #24710 Possibly also: - #24579 - #24691 ACKs for top commit: laanwj: List-of-commits ACK 174af33, I think we should merge this and move forward with rc3.. hebasto: ACK 174af33 Tree-SHA512: 5a493e1652b780b527767d6ca9e67012abd2fa5573496e85e0d8aa4bed3eb332bfcd72610b8dfb954ff274d42450623233c96c479de2085b9c8344ba5abf1935

038d2a6 test: add test for signet miner script (Sebastian Falbesoner) 449b96e test: add `is_bitcoin_util_compiled` helper (Sebastian Falbesoner) dde33ec test: determine path to `bitcoin-util` in test framework (Sebastian Falbesoner) Pull request description: This PR adds a very basic test for the signet miner script (contrib/signet/miner). ~~It was based on #24553 (merged by now) which fixes a bug (and was also the motivation to write this test).~~ The test roughly follows the steps from https://en.bitcoin.it/wiki/Signet#Custom_Signet, except that the challenge key-pair is created solely with the test framework. Calibration is also skipped, the difficulty is simply set to the first mainnet target `0x1d00ffff` (see also https://bitcoin.stackexchange.com/a/57186). ACKs for top commit: laanwj: re-ACK 038d2a6 Tree-SHA512: 150698a3c0cda3679661b47688e3b932c9761e777fdd284776b867b485db6a8895960177bd02a53f838a4c9b9bbe6a9beea8d7a5b14825b38e4e43b3177821b3

DrahtBot added the Scripts and tools label Mar 13, 2022

fanquake requested review from ajtowns and kallewoof March 14, 2022 05:47

theStack force-pushed the 202203-contrib-fix_signet_miner_sighash_mismatch branch from 4e1503d to 12cc020 Compare March 14, 2022 11:03

theStack mentioned this pull request Mar 14, 2022

test: add test for signet miner script #24559

Merged

kallewoof reviewed Mar 15, 2022

View reviewed changes

ajtowns approved these changes Mar 16, 2022

View reviewed changes

maflcko merged commit 74f8c55 into bitcoin:master Mar 17, 2022

maflcko added the Needs backport (23.x) label Mar 17, 2022

maflcko added this to the 23.0 milestone Mar 17, 2022

theStack deleted the 202203-contrib-fix_signet_miner_sighash_mismatch branch March 17, 2022 07:24

fanquake removed the Needs backport (23.x) label Mar 28, 2022

jonatack mentioned this pull request Mar 28, 2022

23.x backports #24512

Merged

bitcoin locked and limited conversation to collaborators Mar 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

contrib: fix signet miner (sighash mismatch) #24553

contrib: fix signet miner (sighash mismatch) #24553

Uh oh!

theStack commented Mar 13, 2022

Uh oh!

theStack commented Mar 13, 2022

Uh oh!

gruve-p commented Mar 13, 2022

Uh oh!

achow101 commented Mar 14, 2022

Uh oh!

ajtowns commented Mar 14, 2022

Uh oh!

theStack commented Mar 14, 2022 •

edited

Loading

Uh oh!

ajtowns commented Mar 14, 2022

Uh oh!

kallewoof left a comment

Uh oh!

gruve-p commented Mar 16, 2022 •

edited

Loading

Uh oh!

ajtowns left a comment

Uh oh!

gruve-p commented Mar 17, 2022

Uh oh!

theStack commented Mar 17, 2022

Uh oh!

laanwj commented Mar 17, 2022 •

edited

Loading

Uh oh!

jonatack commented Mar 28, 2022

Uh oh!

Uh oh!

	psbt.i = [ PSBTMap( {0: spendme.serialize(),
	3: bytes([1,0,0,0])})

contrib: fix signet miner (sighash mismatch) #24553

contrib: fix signet miner (sighash mismatch) #24553

Uh oh!

Conversation

theStack commented Mar 13, 2022

Uh oh!

theStack commented Mar 13, 2022

Uh oh!

gruve-p commented Mar 13, 2022

Uh oh!

achow101 commented Mar 14, 2022

Uh oh!

ajtowns commented Mar 14, 2022

Uh oh!

theStack commented Mar 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ajtowns commented Mar 14, 2022

Uh oh!

kallewoof left a comment

Choose a reason for hiding this comment

Uh oh!

gruve-p commented Mar 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ajtowns left a comment

Choose a reason for hiding this comment

Uh oh!

gruve-p commented Mar 17, 2022

Uh oh!

theStack commented Mar 17, 2022

Uh oh!

laanwj commented Mar 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack commented Mar 28, 2022

Uh oh!

Uh oh!

theStack commented Mar 14, 2022 •

edited

Loading

gruve-p commented Mar 16, 2022 •

edited

Loading

laanwj commented Mar 17, 2022 •

edited

Loading