Update createmultisig RPC to support segwit #13072

pull ajtowns wants to merge 4 commits into bitcoin:master from ajtowns:signmultisig changing 10 files +271 −122

ajtowns commented at 10:46 AM on April 25, 2018: contributor

Adds an "address_type" parameter that accepts "legacy", "p2sh-segwit", and "bech32" to choose the type of address created. Defaults to "legacy" rather than the value of the -address-type option for backwards compatibility.

As part of implementing this, OutputType is moved from wallet into its own module, and AddAndGetDestinationForScript is changed to apply to a CKeyStore rather than a wallet, and to invoke keystore.AddCScript(script) itself rather than expecting the caller to have done that.

Fixes #12502
instagibbs commented at 12:42 PM on April 25, 2018: member

I think this would resolve #12502

Thanks for working on this!
ajtowns force-pushed on Apr 25, 2018
ajtowns force-pushed on Apr 25, 2018
jonasschnelli commented at 8:09 AM on April 26, 2018: contributor

Nice! Concept ACK.
ajtowns force-pushed on May 17, 2018
luke-jr commented at 8:26 PM on June 12, 2018: member

"legacy" doesn't support multisig... I think you need to add a "bip16" type for this.
sipa commented at 7:05 PM on June 13, 2018: member

@luke-jr The existing "legacy" address type (argument to -addresstype command line option) causes P2SH-multisig addresses for the multisig RPC. "p2sh-segwit" will cause P2SH-P2WSH-multisig addresses, and "bech32" will cause P2WSH-multisig.
DrahtBot cross-referenced this on Jun 19, 2018 from issue Improve handling of INVALID in IsMine by sipa
ajtowns commented at 10:19 AM on June 22, 2018: contributor

Ping @instagibbs @jonasschnelli ; Care to review code?
in src/outputtype.cpp:79 in fd130fa737 outdated
```
  74 | @@ -75,6 +75,8 @@ std::vector<CTxDestination> GetAllDestinationsForKey(const CPubKey& key)
  75 |  
  76 |  CTxDestination AddAndGetDestinationForScript(CKeyStore& keystore, const CScript& script, OutputType type)
  77 |  {
  78 | +    keystore.AddCScript(script);
```
instagibbs commented at 2:44 PM on June 22, 2018:

is this a bugfix?

ajtowns commented at 3:08 PM on June 22, 2018:

It moves the AddCScript() call from the function invoking AddAndGetDestinationForScript() into AAGDFS(); see corresponding change in src/wallet/rpcwallet.cpp
in src/rpc/misc.cpp:149 in fd130fa737 outdated
```
 147 |      // Construct using pay-to-script-hash:
 148 | -    CScript inner = CreateMultisigRedeemscript(required, pubkeys);
 149 | -    CScriptID innerID(inner);
 150 | +    const CScript inner = CreateMultisigRedeemscript(required, pubkeys);
 151 | +    CBasicKeyStore keystore;
 152 | +    const CTxDestination dest = AddAndGetDestinationForScript(keystore, inner, output_type);
```
instagibbs commented at 2:47 PM on June 22, 2018:

I didn't think this rpc call changed wallet state previously?

ajtowns commented at 3:09 PM on June 22, 2018:

This shouldn't change wallet state either -- keystore is a dummy variable that's just declared here and discarded. The functional tests don't work without wallet support compiled in, but I think I gave it a quick test by hand and it was okay.
laanwj added this to the "Blockers" column in a project

in src/rpc/misc.cpp:153 in 5f3d6689c9 outdated

 152 | +    const CTxDestination dest = AddAndGetDestinationForScript(keystore, inner, output_type);
 153 |  
 154 |      UniValue result(UniValue::VOBJ);
 155 | -    result.pushKV("address", EncodeDestination(innerID));
 156 | +    result.pushKV("address", EncodeDestination(dest));
 157 |      result.pushKV("redeemScript", HexStr(inner.begin(), inner.end()));

sipa commented at 3:45 AM on July 6, 2018:

This field becomes essentially useless for new address types. I'm not sure what to do about that.

achow101 commented at 9:41 PM on July 9, 2018:

It could be changed to script and have redeemScript behind a deprecation switch.

DrahtBot added the label Needs rebase on Jul 7, 2018
DrahtBot commented at 8:42 AM on July 7, 2018: contributor

Needs rebase
Add outputtype module
Moves OutputType into its own module
9a44db2e46

Move AddAndGetDestinationForScript from wallet to outputype module

Makes AddAndGetDestinationForScript use a generic CKeyStore rather than
the wallet, and makes it always add the script to the keystore, rather
than only adding related (redeem) scripts.

d58055d25f

segwit support for createmultisig RPC b9024fdda3
[tests] functional test for createmultisig RPC f40b3b82df
ajtowns force-pushed on Jul 9, 2018
laanwj added the label RPC/REST/ZMQ on Jul 9, 2018
laanwj removed the label Needs rebase on Jul 9, 2018
achow101 commented at 9:45 PM on July 9, 2018: member

utACK f40b3b82dfe873dd55ee24f4d6dec5d43756260a
laanwj commented at 6:42 PM on July 11, 2018: member

utACK f40b3b82dfe873dd55ee24f4d6dec5d43756260a
sipa commented at 6:09 PM on July 12, 2018: member

utACK f40b3b82dfe873dd55ee24f4d6dec5d43756260a.
sipa merged this on Jul 14, 2018
sipa closed this on Jul 14, 2018
sipa referenced this in commit b25a4c2284 on Jul 14, 2018
fanquake removed this from the "Blockers" column in a project

Empact cross-referenced this on Jul 14, 2018 from issue Add CMerkleTx::IsImmatureCoinBase method by Empact
Bushstar cross-referenced this on Jul 16, 2018 from issue commits from bitcoin/master by Bushstar
ryanofsky cross-referenced this on Aug 3, 2018 from issue Refactor: separate wallet from node by ryanofsky
kwvg cross-referenced this on Jun 26, 2021 from issue partial bitcoin#15638, #21966, #16889, merge #14555, #20499, #14074, #17073: util refactoring by kwvg
bitcoin locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#10973 Refactor: separate wallet from node #12502 [RPC] createmultisig does not respect `-addresstype`#13631 Add CMerkleTx::IsImmatureCoinBase method