contrib: fix signet miner (sighash mismatch)

theStack commented at 9:27 pm on March 13, 2022: member

gruve-p reported that the signet miner doesn’t work anymore (see #24501 (comment)), failing with the following error of the walletprocesspsbt RPC:

0error code: -22
1error message:
2Specified sighash value does not match value stored in PSBT
3.....
4subprocess.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):

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

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 commented at 9:28 pm on March 13, 2022: member

pinging @achow101 @gruve-p

DrahtBot added the label Scripts and tools on Mar 13, 2022

gruve-p commented at 10:18 pm on March 13, 2022: contributor

Just tested and this fixes the issue

fanquake requested review from ajtowns on Mar 14, 2022

fanquake requested review from kallewoof on Mar 14, 2022

achow101 commented at 10:18 am on March 14, 2022: member

Code Review ACK 4e1503dbc84b712fcbfdae3ba667ecc4ec8b710c

ajtowns commented at 10:20 am on March 14, 2022: member

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:

0-        psbt_signed = json.loads(args.bcli("-stdin", "walletprocesspsbt", input=psbt.encode('utf8')))
1+        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)

contrib: fix signet miner (sighash mismatch)

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, 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 d3992669df826899a3de78a77a366dab46028026.

Reported by gruve-p.

12cc0201c2

theStack force-pushed on Mar 14, 2022

theStack commented at 11:12 am on March 14, 2022: member

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: https://github.com/bitcoin/bitcoin/blob/25d045a9ecc24a2752685d176c28f8f75bb100f6/src/wallet/scriptpubkeyman.cpp#L643-L645

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:
0-        psbt_signed = json.loads(args.bcli("-stdin", "walletprocesspsbt", input=psbt.encode('utf8')))
1+        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 commented at 3:35 pm on March 14, 2022: member

#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 commented at 6:02 am on March 15, 2022: member

ACK 12cc0201c26f4214d9e1dee1e6d0ddb97b7ab20f

gruve-p commented at 8:58 pm on March 16, 2022: contributor

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 https://github.com/bitcoin/bitcoin/pull/24559/ superseed this PR? In that case this one can get closed?

ajtowns approved

ajtowns commented at 9:35 pm on March 16, 2022: member

ACK 12cc0201c26f4214d9e1dee1e6d0ddb97b7ab20f ; code review only

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

MarcoFalke merged this on Mar 17, 2022

MarcoFalke closed this on Mar 17, 2022

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

MarcoFalke added this to the milestone 23.0 on Mar 17, 2022

theStack deleted the branch on Mar 17, 2022

gruve-p commented at 5:41 pm on March 17, 2022: contributor

Post-merge ACK https://github.com/bitcoin/bitcoin/pull/24553/commits/12cc0201c26f4214d9e1dee1e6d0ddb97b7ab20f

theStack commented at 5:50 pm on March 17, 2022: member

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 commented at 9:42 pm on March 17, 2022: member

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.

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

jonatack commented at 11:15 am on March 28, 2022: member

Backported to v23.0 in #24512.

fanquake removed the label Needs backport (23.x) on Mar 28, 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

fanquake referenced this in commit c243e08351 on Mar 31, 2022

laanwj referenced this in commit cf0a8b9c48 on Apr 14, 2022

DrahtBot locked this on Mar 28, 2023

contrib: fix signet miner (sighash mismatch) #24553