Update createmultisig RPC to support segwit #13072

1. 
   ajtowns commented at 10:46 am on April 25, 2018: member

   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

2. 
   instagibbs commented at 12:42 pm on April 25, 2018: member

   I think this would resolve #12502

   Thanks for working on this!

3. ajtowns force-pushed on Apr 25, 2018
4. ajtowns force-pushed on Apr 25, 2018
5. 
   jonasschnelli commented at 8:09 am on April 26, 2018: contributor

   Nice! Concept ACK.
6. ajtowns force-pushed on May 17, 2018
7. 
   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.
8. 
   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.
9. 
   ajtowns commented at 10:19 am on June 22, 2018: member

   Ping @instagibbs @jonasschnelli ; Care to review code?
10. 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
11. 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.
12. laanwj added this to the "Blockers" column in a project

    ---

13. 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.
14. DrahtBot added the label Needs rebase on Jul 7, 2018
15. 
    DrahtBot commented at 8:42 am on July 7, 2018: member
16. Add outputtype module

    Moves OutputType into its own module

    9a44db2e46
17. 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
18. segwit support for createmultisig RPC b9024fdda3
19. [tests] functional test for createmultisig RPC f40b3b82df
20. ajtowns force-pushed on Jul 9, 2018
21. laanwj added the label RPC/REST/ZMQ on Jul 9, 2018
22. laanwj removed the label Needs rebase on Jul 9, 2018
23. 
    achow101 commented at 9:45 pm on July 9, 2018: member

    utACK f40b3b82dfe873dd55ee24f4d6dec5d43756260a
24. 
    laanwj commented at 6:42 pm on July 11, 2018: member

    utACK f40b3b82dfe873dd55ee24f4d6dec5d43756260a
25. 
    sipa commented at 6:09 pm on July 12, 2018: member

    utACK f40b3b82dfe873dd55ee24f4d6dec5d43756260a.
26. sipa merged this on Jul 14, 2018
27. sipa closed this on Jul 14, 2018

    ---

28. sipa referenced this in commit b25a4c2284 on Jul 14, 2018
29. fanquake removed this from the "Blockers" column in a project

    ---

30. DrahtBot locked this on Sep 8, 2021

Contributors
ajtowns instagibbs jonasschnelli luke-jr sipa achow101 DrahtBot laanwj

Labels
RPC/REST/ZMQ