wallet: Deprecate addwitnessaddress #12210

laanwj · 2018-01-17T12:22:04Z

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

luke-jr · 2018-01-17T12:23:26Z

src/wallet/rpcwallet.cpp

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


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

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

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

luke-jr · 2018-01-17T12:23:42Z

src/wallet/rpcwallet.cpp

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


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

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

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

meshcollider · 2018-01-17T12:32:05Z

utACK

sipa · 2018-01-17T12:35:00Z

utACK, thanks!

meshcollider · 2018-01-17T12:47:07Z

src/wallet/rpcwallet.cpp

@@ -1299,6 +1300,12 @@ UniValue addwitnessaddress(const JSONRPCRequest& request)
        throw std::runtime_error(msg);
    }

+    if (!IsDeprecatedRPCEnabled("addwitnessaddress")) {
+        throw JSONRPCError(RPC_METHOD_DEPRECATED, "addwitnessaddress is deprecated and will be fully removed in v0.17. "
+            "To use estimatefee in v0.16, restart bitcoind with -deprecatedrpc=estimatefee.\n"


This still mentions estimatefee :)

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)

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 · 2018-01-17T13:20:00Z

Concept ACK

laanwj · 2018-01-17T13:26:49Z

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.

maflcko · 2018-01-17T13:31:53Z

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

laanwj · 2018-01-17T13:34:17Z

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.

flack · 2018-01-17T14:57:30Z

src/wallet/rpcwallet.cpp

@@ -1299,6 +1300,12 @@ UniValue addwitnessaddress(const JSONRPCRequest& request)
        throw std::runtime_error(msg);
    }

+    if (!IsDeprecatedRPCEnabled("addwitnessaddress")) {
+        throw JSONRPCError(RPC_METHOD_DEPRECATED, "addwitnessaddress is deprecated and will be fully removed in v0.17. "
+            "To use addwitnessaddress in v0.16, restart bitcoind with -deprecatedrpc=estimatefee.\n"


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

achow101 · 2018-01-17T17:28:53Z

utACK modulo flack's comment.

ryanofsky

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

promag · 2018-01-17T23:58:33Z

utACK after comments.

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

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

laanwj · 2018-01-18T09:25:37Z

fixed copy/paste error and squashed e849ae03c12f63a4ec591018983f28968d4f3ded → cdf3e03

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.

cdf3e03 wallet: Deprecate addwitnessaddress (Wladimir J. van der Laan) Pull request description: Now that segwit is natively supported by the wallet, deprecate the hack `addwitnessaddress`. Tree-SHA512: f33b1c33d200fa8f1a0fba424b30e9c2a78147cde8bb0a3fd41194b77980454cddfb23da256cd6fe78726e87161deaa23357d0764e74c3eb83177cc518afa49c

f523c6b [qa] Use address type in addmultisigaddress to avoid addwitnessaddress (João Barbosa) 886a92f [rpc] Add address type option to addmultisigaddress (João Barbosa) Pull request description: Adds the option `address_type` to `addmultisigaddress` and `createmultisg` RPC. This also allows to avoid `addwitnessaddress` to obtain an `p2sh-segwit` or `bech32` multsig address. Related to #12210 as this reduces `addwitnessaddress` usage. Tree-SHA512: 8f8f85dfcff66bb6c7e1e9865e37c285dead1d6dadb9672a89b92fa209d03cc35817ca1d656588c6c2146b728daaf7540b851929b640294653c62836cbefe7ee

luke-jr reviewed Jan 17, 2018

View reviewed changes

laanwj added the Wallet label Jan 17, 2018

meshcollider reviewed Jan 17, 2018

View reviewed changes

maflcko added this to the 0.16.0 milestone Jan 17, 2018

flack reviewed Jan 17, 2018

View reviewed changes

ryanofsky reviewed Jan 17, 2018

View reviewed changes

promag mentioned this pull request Jan 17, 2018

Add address type option to addmultisigaddress #12213

Merged

wallet: Deprecate addwitnessaddress

cdf3e03

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

laanwj force-pushed the 2018_01_deprecate_addwitnessaddress branch from e849ae0 to cdf3e03 Compare January 18, 2018 09:24

laanwj merged commit cdf3e03 into bitcoin:master Jan 18, 2018

This was referenced Jan 18, 2018

External wallet files #11687

Merged

Rename account to label where appropriate #11536

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

wallet: Deprecate addwitnessaddress #12210

wallet: Deprecate addwitnessaddress #12210

Uh oh!

Conversation

laanwj commented Jan 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meshcollider commented Jan 17, 2018

Uh oh!

sipa commented Jan 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sjors commented Jan 17, 2018

Uh oh!

laanwj commented Jan 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Jan 17, 2018

Uh oh!

laanwj commented Jan 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

achow101 commented Jan 17, 2018

Uh oh!

ryanofsky left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag commented Jan 17, 2018

Uh oh!

laanwj commented Jan 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

laanwj commented Jan 17, 2018 •

edited

Loading

ryanofsky left a comment •

edited

Loading

laanwj commented Jan 18, 2018 •

edited

Loading