bitcoinhodler commented at 11:25 pm on February 12, 2018: contributor

I’m working on a paper multisig system, improving it to use p2sh-segwit instead of legacy p2sh. I’m using my own Linux build of Bitcoin Core tag v0.16.0rc3 and using bitcoin-cli to build transactions.

I used addmultisigaddress to create a 2-of-4 p2sh-segwit address and funded it on testnet. I created a transaction to spend those coins using createrawtransaction and I’m attempting to sign it.

Actual behavior

If I provide no keys to signrawtransaction and let it use the wallet keys, everything works. But if I provide the keys explicitly, I get the error: “Unable to sign input, invalid stack size (possibly missing key)”.

Expected behavior

I expect signrawtransaction to be able to sign the transaction using the explicltly-provided keys. (For a paper wallet, the bitcoind wallet will not know the keys or the multisig address.)

This flow works as expected when I use type ’legacy’ for the addmultisigaddress. It fails when I use type ‘p2sh-segwit’.

My guess is that it assumes it’s legacy p2sh and therefore the signature fails. Shouldn’t bitcoind be able to figure out (based on the scriptPubKey and the redeemScript) that this is p2sh-segwit, and construct the appropriate signatures? Or is there a different/better way to use the CLI to accomplish this?

To recreate

This script shows the problem. The final signrawtransaction step should succeed. [output]

Ugly workaround

I could use addmultisigaddress to teach the wallet about the p2sh-segwit address, and importprivkey to teach it 2-of-4 keys, then use signrawtransaction without providing any keys. This seems to work [output] but is very ugly because at the time of signing, I only have 2 of 4 keys, so in order to run addmultisigaddress I have to decode the redeemScript and parse its “asm” field to find the 4 pubkeys.

Somewhat related issue, never merged: issue #11693 and its associated PR #11708

sipa commented at 11:40 pm on February 12, 2018: member

I believe this issue is identical to #11708. In order for signrawtransaction to sign a P2SH-P2WSH-multisig output, it needs to know both the P2WSH script and the P2SH script.

bitcoinhodler commented at 11:53 pm on February 12, 2018: contributor

But it should be able to figure out the P2SH script, as explained by @NicolasDorier on Nov 20, 2017 (I can’t figure out how to make a link to his specific comment).

The current fix for #11708 would require that I provide an additional script to signrawtransaction, but where do I get that script? It’s not provided to me by addmultisigaddress. If I’m going to need it to sign transactions, then it should be. But I shouldn’t need it.

There are only two possibilities: either it is legacy, or it is p2sh-segwit. Looking at the redeemScript and the scriptPubKey, it can figure out which one it is without being told.

I still think my approach should work. I should not have to provide both scripts. I think I understand why it doesn’t work today, but that is a bug, not a feature.

sipa commented at 11:59 pm on February 12, 2018: member

@bitcoinhodler In theory, yes, but that’s not how the signing logic works at all. It sees a P2SH script, tries to find which redeemscript that hash corresponds to. If it finds that, it’ll see a P2WSH program, and try to find which witness script the 256-bit hash corresponds to. If it finds that, it’ll try to sign with it.

You’re certainly right that right now the number of possible combinations is low, and it could just try all of them. I’m not convinced (either way) that this is the right approach though. It may become more complicated when more witness versions exist.

In any case, there’s a clear shortcoming here and we should address it. addmultisigaddress should probbably return all redeemscripts involved, as just listunspent is not usable in your case (nor should it be needed).

bitcoinhodler commented at 0:10 am on February 13, 2018: contributor

What if there were a way to tell signrawtransaction that this redeemScript I’m providing is a p2sh-segwit type?

Another potential change that would make my ugly workaround less ugly: provide a JSON list of pubkeys from decodescript, in addition to the addresses provided today. Then I wouldn’t have to parse the asm field to recreate the multisig address in the wallet.

And another potential change that would help me: have decodescript show the p2sh-segwit address associated with the decoded script, in addition to today’s p2sh address. That way I could figure out for myself (since I know the address) which of the two types it is.

sipa commented at 0:29 am on February 13, 2018: member

What if there were a way to tell signrawtransaction that this redeemScript I’m providing is a p2sh-segwit type?

That sounds like a complexity the user shouldn’t need to deal with. There should be a way to encapsulate all the information needed to sign (apart from the private key) out of addmultisigaddress and co, and pass it as a black box to signrawtransaction.

I think either signrawtransaction should just infer it, or it comes in the form of a few more data fields to pass along.

Another potential change that would make my ugly workaround less ugly: provide a JSON list of pubkeys from decodescript, in addition to the addresses provided today. Then I wouldn’t have to parse the asm field to recreate the multisig address in the wallet.

I don’t think decodescript is the best place for this. Giving information about arbitrary scripts will likely become increasingly difficult as more private scripting techniques appear. The right place to analyse a particular script is by the person who created it. It can either come as a side effect out of addmultisigaddress. In 0.16, validateaddress on any address you created yourself will also include a full analysis, including redeemscripts and public keys used.

sipa commented at 0:47 am on February 13, 2018: member

Could you try if this fixes things for you? It should make signrawtransaction deal with multisig script where only the inner redeemscript is passed (untested).

 0--- a/src/rpc/rawtransaction.cpp
 1+++ b/src/rpc/rawtransaction.cpp
 2@@ -848,6 +848,8 @@ UniValue signrawtransaction(const JSONRPCRequest& request)
 3                     std::vector<unsigned char> rsData(ParseHexV(v, "redeemScript"));
 4                     CScript redeemScript(rsData.begin(), rsData.end());
 5                     tempKeystore.AddCScript(redeemScript);
 6+                    // Automatically also add the P2WSH wrapped version of the script (to deal with P2SH-P2WSH).
 7+                    tempKeystore.AddCScript(GetScriptForWitness(redeemScript));
 8                 }
 9             }
10         }

fanquake added the label RPC/REST/ZMQ on Feb 13, 2018

bitcoinhodler referenced this in commit 5c3d4a8f7e on Feb 13, 2018

bitcoinhodler referenced this in commit 624d7eaf19 on Feb 13, 2018

bitcoinhodler commented at 8:06 am on February 13, 2018: contributor

I went ahead and implemented the “ugly” workaround I originally suggested, and it’s not as ugly as I feared. I do have to parse the asm field of the decoded script, and then I run addmultisigaddress twice: once with type ’legacy’ and once with ‘p2sh-segwit’ (since I don’t know which type the user has provided me). After that, assuming I’ve used importprivkey with enough keys, signrawtransaction works without complaint for both types of scripts.

My immediate problem is solved, but it seems there’s a usability or documentation issue here, at the very least.

bitcoinhodler commented at 8:12 am on February 13, 2018: contributor

Just saw your suggested patch; I tried it and it does indeed solve my original issue.

bitcoinhodler referenced this in commit b8c236a5ad on Feb 15, 2018

bitcoinhodler referenced this in commit 1e0e927ff6 on Feb 15, 2018

bitcoinhodler referenced this in commit 2263262e65 on Feb 15, 2018

laanwj referenced this in commit 3fa556aee2 on Feb 15, 2018

bitcoinhodler referenced this in commit 12c709a5d2 on Feb 15, 2018

bitcoinhodler commented at 6:36 pm on February 28, 2018: contributor

I confirm this is now fixed in v0.16.0.

bitcoinhodler closed this on Feb 28, 2018

bitcoinhodler referenced this in commit 1d2951faf1 on Apr 6, 2018

bitcoinhodler referenced this in commit ac9708b138 on Dec 13, 2018

bitcoinhodler commented at 5:29 pm on December 22, 2018: contributor

@sipa do you think this same fix should be applied to importmulti? See my question on Stack Exchange which looks like a very similar problem. I can create a new issue if so.

DrahtBot locked this on Sep 8, 2021

signrawtransaction unable to sign p2sh-segwit multisig transaction #12418

Actual behavior

Expected behavior

To recreate

Ugly workaround

Related