wallet: Deprecate addwitnessaddress

laanwj commented at 12:22 pm on January 17, 2018: member

Now that segwit is natively supported by the wallet, deprecate the hack addwitnessaddress.

in src/wallet/rpcwallet.cpp:1288 in 092e43e14d outdated

1284@@ -1285,7 +1285,8 @@ UniValue addwitnessaddress(const JSONRPCRequest& request)
1285     if (request.fHelp || request.params.size() < 1 || request.params.size() > 2)
1286     {
1287         std::string msg = "addwitnessaddress \"address\" ( p2sh )\n"
1288-            "\nAdd a witness address for a script (with pubkey or redeemscript known). Requires a new wallet backup.\n"
1289+            "\nDEPRECATED: use option -addresstype=[bech32|p2sh-segwit] instead.\n"

luke-jr commented at 12:23 pm on January 17, 2018:

Wouldn’t it be better to suggest using the RPC parameters, rather than changing the config file and restarting?

meshcollider commented at 12:26 pm on January 17, 2018:

Yeah getnewaddress with the address_type parameter more closely resembles what this RPC was used for IMO

laanwj commented at 12:26 pm on January 17, 2018:

best to document both, I’ll mention that option as well.

in src/wallet/rpcwallet.cpp:1289 in 092e43e14d outdated

1284@@ -1285,7 +1285,8 @@ UniValue addwitnessaddress(const JSONRPCRequest& request)
1285     if (request.fHelp || request.params.size() < 1 || request.params.size() > 2)
1286     {
1287         std::string msg = "addwitnessaddress \"address\" ( p2sh )\n"
1288-            "\nAdd a witness address for a script (with pubkey or redeemscript known). Requires a new wallet backup.\n"
1289+            "\nDEPRECATED: use option -addresstype=[bech32|p2sh-segwit] instead.\n"
1290+            "Add a witness address for a script (with pubkey or redeemscript known). Requires a new wallet backup.\n"

luke-jr commented at 12:23 pm on January 17, 2018:

Doesn’t require a new wallet backup anymore, IIRC.

laanwj commented at 12:28 pm on January 17, 2018:

I don’t think it makes sense to update the documentation of a deprecated call, besides adding a DEPRECATED message and hiding it.

meshcollider commented at 12:31 pm on January 17, 2018:

Agreed, its not like backing up would actually be a bad thing anyway

meshcollider commented at 12:32 pm on January 17, 2018: contributor

utACK

sipa commented at 12:35 pm on January 17, 2018: member

utACK, thanks!

laanwj added the label Wallet on Jan 17, 2018

in src/wallet/rpcwallet.cpp:1305 in d0440d482c outdated

1299@@ -1299,6 +1300,12 @@ UniValue addwitnessaddress(const JSONRPCRequest& request)
1300         throw std::runtime_error(msg);
1301     }
1302 
1303+    if (!IsDeprecatedRPCEnabled("addwitnessaddress")) {
1304+        throw JSONRPCError(RPC_METHOD_DEPRECATED, "addwitnessaddress is deprecated and will be fully removed in v0.17. "
1305+            "To use estimatefee in v0.16, restart bitcoind with -deprecatedrpc=estimatefee.\n"

meshcollider commented at 12:47 pm on January 17, 2018:

This still mentions estimatefee :)

meshcollider commented at 12:47 pm on January 17, 2018:

Also, if you deprecate it like this you’ll have to add -deprecatedrpc=addwitnessaddress to the tests which use it (easier than reworking the tests at the same time)

laanwj commented at 1:06 pm on January 17, 2018:

Gah, will update. Nothing is ever easy, is it? I assumed I was just updating the documentation hence I didn’t bother running any tests.

Sjors commented at 1:20 pm on January 17, 2018: member

Concept ACK

laanwj commented at 1:26 pm on January 17, 2018: member

The following tests are failing due to to the deprecation:

segwit.py
p2p-compactblocks.py
wallet-dump.py
nulldummy.py
bumpfee.py

I’m starting to think this was a kind of bad idea, if we still rely so much on that call.

MarcoFalke commented at 1:31 pm on January 17, 2018: member

We can fix the test, but lets move the deprecation to 0.17 to not block 0.16?

laanwj commented at 1:34 pm on January 17, 2018: member

I don’t think anything needs to be blocked on that, just adding -deprecatedrpc=addwitnessaddress will do fine, I was just surprised there are so many.

MarcoFalke added this to the milestone 0.16.0 on Jan 17, 2018

in src/wallet/rpcwallet.cpp:1305 in 117ee54aa6 outdated

1299@@ -1299,6 +1300,12 @@ UniValue addwitnessaddress(const JSONRPCRequest& request)
1300         throw std::runtime_error(msg);
1301     }
1302 
1303+    if (!IsDeprecatedRPCEnabled("addwitnessaddress")) {
1304+        throw JSONRPCError(RPC_METHOD_DEPRECATED, "addwitnessaddress is deprecated and will be fully removed in v0.17. "
1305+            "To use addwitnessaddress in v0.16, restart bitcoind with -deprecatedrpc=estimatefee.\n"

flack commented at 2:57 pm on January 17, 2018:

should say -deprecatedrpc=addwitnessaddress instead of -deprecatedrpc=estimatefee

achow101 commented at 5:28 pm on January 17, 2018: member

utACK modulo flack’s comment.

ryanofsky commented at 9:35 pm on January 17, 2018: member

utACK 117ee54aa62ddb43f039797d5496e014553f28ec with estimatefee fix and squash commits squashed (obviously)

promag commented at 11:58 pm on January 17, 2018: member

utACK after comments.

We can slowly remove addwitnessaddress from the test suite right? For instance, #12213.

wallet: Deprecate addwitnessaddress

Now that segwit is natively supported by the wallet, deprecate the hack `addwitnessaddress`.

cdf3e03a72

laanwj force-pushed on Jan 18, 2018

laanwj commented at 9:25 am on January 18, 2018: member

fixed copy/paste error and squashed e849ae03c12f63a4ec591018983f28968d4f3ded → cdf3e03a723b1a0242199672878caf1543ba8124

We can slowly remove addwitnessaddress from the test suite right? For instance, #12213.

Agreed. This deprecation made that an obligation before 0.17.0, not for 0.16.0, so could be done slowly but surely.

laanwj merged this on Jan 18, 2018

laanwj closed this on Jan 18, 2018

laanwj referenced this in commit 17180fa608 on Jan 18, 2018

jonasschnelli referenced this in commit eadb2dacc3 on Jan 24, 2018

DrahtBot locked this on Sep 8, 2021

wallet: Deprecate addwitnessaddress #12210