Remove createwitnessaddress RPC command #8699

1. 
   jl2012 commented at 6:10 pm on September 11, 2016: contributor

   For 0.13.1

   createwitnessaddress does not examine script validity in any way and may return unspendable addresses. It should be disabled on mainnet.

   In longer term (maybe 0.14), we should disable all the sensitive commands (e.g. dumpprivkey, importprivkey) by default, and only be enabled with a command-line argument

2. MarcoFalke added the label Needs backport on Sep 11, 2016
3. MarcoFalke added this to the milestone 0.13.1 on Sep 11, 2016
4. in src/rpc/misc.cpp: in 3339721652 outdated

   329         string msg = "createwitnessaddress \"script\"\n"
   330-            "\nCreates a witness address for a particular script.\n"
   331+            "\nFOR TESTNET ONLY: Creates a witness address for a particular script.\n"
   332             "It returns a json object with the address and witness script.\n"
   333+            "WARNING: It does NOT examine validity of the script in any way. Resulting address may be unspendable\n"
   334+            "WARNING: Sementics and network rules of witness and non-witness scripts may not be identical.\n"
   

   ---

   ---

   

   rodasmith commented at 6:45 pm on September 11, 2016:

   (typo) “Semantics”

   ---

   

   jl2012 commented at 7:01 pm on September 11, 2016:

   Fixed, thanks
5. jl2012 force-pushed on Sep 11, 2016
6. 
   btcdrak commented at 12:04 pm on September 12, 2016: contributor

   utACK 656c2ad
7. Remove createwitnessaddress

   This RPC command is unsafe as it will return an address even if the script is invalid.

   86c3f8db0b
8. in src/rpc/misc.cpp: in 656c2adb7b outdated

   322@@ -323,11 +323,13 @@ UniValue createmultisig(const UniValue& params, bool fHelp)
   323 
   324 UniValue createwitnessaddress(const UniValue& params, bool fHelp)
   325 {
   326-    if (fHelp || params.size() < 1 || params.size() > 1)
   327+    if (fHelp || params.size() < 1 || params.size() > 1 || Params().NetworkIDString() == "main")
   

   ---

   ---

   

   sipa commented at 3:51 pm on September 12, 2016:

   Please don’t do a string test on the chain params. If you need a specific condition, add a ChainParams::AllowCreateWitness method or so.

   ---

   

   sipa commented at 4:17 pm on September 12, 2016:

   Also, is there any need to not just remove the RPC entirely?

   ---

   

   jl2012 commented at 6:54 pm on September 12, 2016:

   Just not sure if that’s useful for testing. Should we keep it for test only or remove it entirely? I don’t have strong opinion.
9. jl2012 force-pushed on Sep 13, 2016
10. jl2012 renamed this:
    Disable createwitnessaddress in mainnet
    Remove createwitnessaddress RPC command
    on Sep 13, 2016
11. 
    jl2012 commented at 3:44 am on September 13, 2016: contributor

    Revised to simply remove the command
12. 
    laanwj commented at 9:19 am on September 13, 2016: member

    I’m a bit confused by the description. Is this only for 0.13.1? Should this be merged into master/0.14 at all?

    It should be disabled on mainnet

    Does it have value on testnet/regtest?

13. 
    MarcoFalke commented at 9:32 am on September 13, 2016: member

    @laanwj If this is merged, it is meant for all branches.
14. 
    laanwj commented at 9:33 am on September 13, 2016: member

    So we want that RPC call to disappear forever, not just disable it on mainnet pending segwit rollout?
15. 
    jl2012 commented at 10:12 am on September 13, 2016: contributor

    I think it should never be allowed on mainnet. In principle we should not generate any address that may likely be unspendable.

    Also, I don’t think it’s very useful for testing. It is not used by any RPC test, and a witness address could be generated trivially with python.

16. 
    laanwj commented at 2:59 pm on September 13, 2016: member

    If it is never useful, sorry to ask, but why did this command make it into a release in the first place?

    Edit: obviously ACK on removing it

17. 
    sipa commented at 3:01 pm on September 13, 2016: member

    It was part of the segwit pull request, and just added to mimick createmultisig/addmultisigaddress. However, we only now realized that it’s pretty dangerous as it can’t guarantee the returned address is valid. Maybe it’s still useful, but it at least needs a big warning.
18. laanwj merged this on Sep 13, 2016
19. laanwj closed this on Sep 13, 2016

    ---

20. laanwj referenced this in commit c9914c2094 on Sep 13, 2016
21. laanwj referenced this in commit a5ec248323 on Sep 26, 2016
22. laanwj removed the label Needs backport on Sep 26, 2016
23. laanwj added the label RPC/REST/ZMQ on Sep 26, 2016
24. 
    laanwj commented at 2:58 pm on September 26, 2016: member

    This is backported in #8815, removing tag
25. MarcoFalke locked this on Sep 8, 2021

Contributors
jl2012 rodasmith btcdrak sipa laanwj MarcoFalke

Labels
RPC/REST/ZMQ

Milestone
0.13.1