[RPC] Disallow using addresses in createmultisig #11415

achow101 · 2017-09-29T04:29:43Z

This PR should be the last part of #7965.

This PR makes createmultisig only accept public keys and marks the old functionality of accepting addresses as deprecated.

It also splits _createmultisig_redeemscript into two functions, _createmultisig_getpubkeys and _createmultisig_getaddr_pubkeys. _createmultisig_getpubkeys retrieves public keys from the RPC parameters and _createmultisig_getaddr_pubkeys retrieves addresses' public keys from the wallet. _createmultisig_getaddr_pubkeys requires the wallet and is only used by addwitnessaddress (except when createmultisig is used in deprecated mode).

addwitnessaddress's API is also changed. Instead of returning just an address, it now returns the same thing as createmultisig: a JSON object with two fields, address and redeemscript.

jnewbery

Definite concept ACK. Thanks for tackling the last item in #7965! 🎉

A few high-level thoughts:

I don't love the only_pubkeys argument in _createmultisig_getpubkeys(). I'm also not a huge fan of passing the address/keys in as a UniValue reference. Is it possible to change _createmultisig_getpubkeys() and _createmultisig_getaddr_pubkeys() to both take a single string and return a single CPubKey? You could then do the iteration over the UniValue keys_in outside of the function.
I don't understand why createmultisig in deprecated mode calls _createmultisig_getpubkeys() then _createmultisig_getaddrpubkeys(), but addmultisigaddress calls them the other way round. Is there a reason for that?
I think you could take the opportunity to tidy up the error handling. Instead of calling throw std::runtime_error, call throw JSONRPCError() so the correct error codes are returned.
it'd be great to add some tests that call both these RPCs with a mixture of keys and addresses to verify that they can handle that.
it seems a little gratuitous to change the return type of addmultisigaddress (especially without hiding it behind a -deprecatedrpc argument). What was the reasoning for that? To not break the tests?

jnewbery · 2017-10-02T20:18:36Z

src/rpc/misc.cpp

+    int required = request.params[0].get_int();
+
+    // Get the public keys
+    const UniValue& keys = request.params[1].get_array();


Can we rename this variable keys_or_addresses (and the same in addmultisigaddress) so it's clear that these could be keys or addresses.

achow101 · 2017-10-02T20:40:19Z

I don't love the only_pubkeys argument in _createmultisig_getpubkeys().

I don't really either. I was trying to avoid as much code duplication as possible, but I guess that can't really be avoided.

I'm also not a huge fan of passing the address/keys in as a UniValue reference. Is it possible to change _createmultisig_getpubkeys() and _createmultisig_getaddr_pubkeys() to both take a single string and return a single CPubKey? You could then do the iteration over the UniValue keys_in outside of the function.

I'll try doing that.

but addmultisigaddress calls them the other way round. Is there a reason for that?

Nope.

it seems a little gratuitous to change the return type of addmultisigaddress (especially without hiding it behind a -deprecatedrpc argument). What was the reasoning for that? To not break the tests?

That was mostly to not break tests and also because I felt that addmultisigaddress should be the wallet version mirror of createmultisig so they both should have the same output. I suppose the original behavior could be hidden behind a -deprecatedrpc argument but I'm not sure how useful that is. The same information (and more) is being returned.

jnewbery · 2017-10-02T20:53:58Z

the original behavior could be hidden behind a -deprecatedrpc argument but I'm not sure how useful that is.

The only reason would be to not break clients that are expecting a string and now receive a JSON object when they upgrade to v0.16. Hiding behind the -deprecatedrpc argument give them breathing space to upgrade the client.

I don't know if this would really be a problem for users, but worth considering.

achow101 · 2017-10-02T23:11:02Z

The latest commit changes the loop to happen in the RPC call body and changes the two helper functions to take a single pubkey and string instead of vectors and UniValue. I have also put the old addmultisigaddress behavior behind -deprecatedrpc.

promag · 2017-10-02T23:45:41Z

src/rpc/misc.cpp

+            _createmultisig_hex_to_pubkey(pubkeys.at(i), keys[i].get_str());
+        } else
+#endif
+        _createmultisig_hex_to_pubkey(pubkeys.at(i), keys[i].get_str());


Put inside a block?

} else { #else { #endif _createmultisig_hex_to_pubkey(pubkeys.at(i), keys[i].get_str()); }

I think the else branch is unnecessary here. See comment above.

Removed the else branch.

jnewbery

I like this better, and I think that once the deprecated functionality is removed it'll be a cleaner interface.

I still think we should have cases that test both these RPCs with a mixture of addresses and keys (in different permutations). I'm happy to write those if you want.

jnewbery · 2017-10-03T20:33:25Z

src/wallet/rpcwallet.cpp

-    return EncodeDestination(innerID);
+
+    UniValue result(UniValue::VOBJ);
+    if (IsDeprecatedRPCEnabled("addmultisigaddress")) {


This can be made simpler:

if (IsDeprecatedRPCEnabled("addmultisigaddress")) { // Old-style interface: just return the address return EncodeDestination(innerID); } // return an object containing the address and redeem script UniValue result(UniValue::VOBJ); result.pushKV("address", EncodeDestination(innerID)); result.pushKV("redeemScript", HexStr(inner.begin(), inner.end())); return result;

jnewbery · 2017-10-03T20:34:44Z

src/wallet/rpcwallet.cpp

+    int required = request.params[0].get_int();
+
+    // Get the public keys
+    const UniValue& keys = request.params[1].get_array();


Suggestion: name this variable keys_or_addresses

Renamed to keys_or_addrs

jnewbery · 2017-10-03T20:42:40Z

src/rpc/misc.cpp

+#ifdef ENABLE_WALLET
+        if (IsDeprecatedRPCEnabled("createmultisig")) {
+            CWallet* const pwallet = GetWalletForJSONRPCRequest(request);
+            if (!EnsureWalletIsAvailable(pwallet, request.fHelp)) {


There's an implicit assumption here that if ENABLE_WALLET is true then there will necessarily be a wallet available. That may be broken by dynamic wallet load/unload. If we allow the wallet to be unloaded then this rpc will stop working.

The else statement is also unnecessary (once we reach line 302, then both branches converge to calling _createmultisig_hex_to_pubkey()).

How about:

#ifdef ENABLE_WALLET CWallet* const pwallet = GetWalletForJSONRPCRequest(request); if (IsDeprecatedRPCEnabled("createmultisig") && EnsureWalletIsAvailable(pwallet, request.fHelp) && pwallet) { _createmultisig_addr_to_pubkey(pwallet, pubkeys.at(i), keys[i].get_str()); } #endif

jnewbery · 2017-10-03T20:42:56Z

src/rpc/misc.cpp

+            _createmultisig_hex_to_pubkey(pubkeys.at(i), keys[i].get_str());
+        } else
+#endif
+        _createmultisig_hex_to_pubkey(pubkeys.at(i), keys[i].get_str());


I think the else branch is unnecessary here. See comment above.

jnewbery · 2017-10-03T20:43:22Z

src/rpc/misc.cpp

+{
+    // Gather public keys
+    if (required < 1) {
+        throw std::runtime_error("a multisignature address must require at least one key to redeem");


Can you replace these errors with better JSON errors?

jnewbery · 2017-10-03T20:45:47Z

src/rpc/misc.cpp

+#endif
+        _createmultisig_hex_to_pubkey(pubkeys.at(i), keys[i].get_str());
+        if (!pubkeys.at(i).IsFullyValid()) {
+            throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid public key: " + keys[i].get_str());


Perhaps add a note into the error text here: "Note that from V0.16, createmultisig no longer accepts addresses. Clients must transition to using addmultisigaddress to create multisig addresses with addresses known to the wallet before upgrading to v0.17. To use the deprecated functionality, start bitcoind with -deprecatedrpc=createmultisig"

This is the error that clients will hit if they're relying on old behaviour, so the error message should be delivered here.

jonasschnelli · 2017-10-04T05:32:54Z

Concept ACK. API change, added the "Needs release notes" tag.

promag · 2017-10-04T07:02:30Z

src/rpc/misc.cpp

-            pubkeys[i] = vchPubKey;
+        CPubKey vchPubKey(ParseHex(hex_in));
+        if (!vchPubKey.IsFullyValid()) {
+            throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid public key: " + hex_in);


Should have tests for these JSONRPCError.

Is that really necessary?

IMO yes, should always test success and failure cases as this is an interface others use.

Added a test in rawtransactions.py

promag · 2017-10-07T08:18:21Z

src/rpc/misc.cpp

+    pubkeys.resize(keys.size());
+    for (unsigned int i = 0; i < keys.size(); ++i) {
+#ifdef ENABLE_WALLET
+        CWallet* const pwallet = GetWalletForJSONRPCRequest(request);


Split in two loops:

#ifdef ENABLE_WALLET CWallet* const pwallet = GetWalletForJSONRPCRequest(request); if (IsDeprecatedRPCEnabled("createmultisig") && EnsureWalletIsAvailable(pwallet, request.fHelp) && pwallet) { // call _createmultisig_addr_to_pubkey for each key } #endif // ENABLE_WALLET // call _createmultisig_hex_to_pubkey for each key

Guessing it was suggested for efficiency, but I think the code is better the way it is (one loop), since it avoids duplication.

ryanofsky

utACK a0e0c4b674b1cde4e754ab0071b81eb8ba3df40a. Needs rebase to fix travis failures due to assert_raises_jsonrpc obnoxiousness.

Maybe consider renaming the _createmultisig_getpubkeys and _createmultisig_getaddr_pubkeys functions to use current naming convention and to drop mention of multisig, since there is really nothing nothing multisig specific about them. All they are doing is turning strings into pubkeys.

ryanofsky · 2017-10-13T15:28:06Z

src/rpc/misc.cpp

-{
-    int nRequired = params[0].get_int();
-    const UniValue& keys = params[1].get_array();
+#ifdef ENABLE_WALLET


No need for ENABLE_WALLET macro here.

Following would be equivalent:

void _createmultisig_addr_to_pubkey(class CWallet* const pwallet, CPubKey& pubkey_out, const std::string& addr_in);

extern keyword also doesn't actually do anything, but maybe it is useful to make the declaration stand out more.

Removed the ifdef here. I'm leaving the extern for clarity

ryanofsky · 2017-10-13T15:48:31Z

src/rpc/misc.cpp

+    pubkeys.resize(keys.size());
+    for (unsigned int i = 0; i < keys.size(); ++i) {
+#ifdef ENABLE_WALLET
+        CWallet* const pwallet = GetWalletForJSONRPCRequest(request);


Guessing it was suggested for efficiency, but I think the code is better the way it is (one loop), since it avoids duplication.

ryanofsky · 2017-10-13T15:59:47Z

src/wallet/rpcwallet.cpp

+extern CScript _createmultisig_redeemScript(const int required, const std::vector<CPubKey>& pubkeys);
+extern void _createmultisig_hex_to_pubkey(CPubKey& pubkey_out, const std::string& hex_in);
+
+void _createmultisig_addr_to_pubkey(CWallet* const pwallet, CPubKey& pubkey_out, const std::string& addr_in)


There is really no reason for this function to live in the wallet. The CWallet* argument could be changed to a CKeyStore argument and this could just be a generic jsonrpc util function without all the weird ifdef and naming and extern declaration stuff.

Sure, that could be done, but is it necessary? Where else would you need to use such a function with a CKeyStore instead of CWallet?

Just a suggestion to let you define the two functions together in one place. Please ignore if you don't think this is an improvement, or don't think it is worth the effort.

achow101 · 2017-10-13T18:21:39Z

Rebased to fix travis.

jnewbery · 2017-10-13T19:06:20Z

sorry @achow101 , you'll also need to change https://github.com/bitcoin/bitcoin/pull/11415/files#diff-8f3a598a6e52799596008ae7c19d1333R23 to use assert_raises_rpc_error()

achow101 · 2017-10-13T21:29:56Z

Actually fixed travis this time, I think.

jnewbery

This is great stuff @achow101!

utACK 897b5c0cfbdbe90c032a980e8567ddca7b499281

Changes from a169f17434b9d96de478a6641e6a087be97631e8 are as expected: removing the unnecessary #ifdef, using JSONRPCErrors, simplifying the logic in createmultisig(), updating the variable name to keys_or_addrs, simplifying the logic around returning the legacy object, adding tests for calling createmultisig() and addmultisigaddress()` with a mixture of keys and addrs, and fixing the functional tests after rebase on master.

One very minor request for an additional comment.

jnewbery · 2017-10-13T22:03:26Z

test/functional/rawtransactions.py

-        mSigObj = self.nodes[2].addmultisigaddress(2, [addr1Obj['pubkey'], addr2Obj['pubkey']])
+        # Tests for createmultisig and addmultisigaddress
+        assert_raises_rpc_error(-5, "Invalid public key", self.nodes[0].createmultisig, 1, ["01020304"])
+        self.nodes[0].createmultisig(2, [addr1Obj['pubkey'], addr2Obj['pubkey']])


minor nit: Future readers of this test may find it clearer if you add a comment saying that createmultisig can only take keys as inputs, but addmultisigaddress can take a mixture of keys and addrs (as long as the wallet owns those addresses)

jnewbery · 2017-10-13T22:18:14Z

Tested ACK ac33d788e2d7dbfdf760f3a620c86eeaf7cff3fd

ryanofsky

utACK ac33d788e2d7dbfdf760f3a620c86eeaf7cff3fd. Only changes are assert_raises fix and test comment.

TheBlueMatt

Looks good. Just some internal-API-nits and wishing it worked with segwit addresses.

TheBlueMatt · 2017-10-17T17:59:37Z

src/rpc/misc.cpp

+#endif
+        _createmultisig_hex_to_pubkey(pubkeys.at(i), keys[i].get_str());
+        if (!pubkeys.at(i).IsFullyValid()) {
+            throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("Invalid public key: %s Note that from v0.16, createmultisig no longer accepts addresses. Clients must transition to using addmultisigaddress to create multisig addresses with addresses known to the wallet before upgrading to v0.17. To use the deprecated functionality, start bitcoind with -deprecatedrpc=createmultisig)", keys[i].get_str()));


Dangling ")" at the end of the err string.

TheBlueMatt · 2017-10-17T18:05:47Z

src/rpc/misc.cpp

-    for (unsigned int i = 0; i < keys.size(); i++)
+void _createmultisig_hex_to_pubkey(CPubKey& pubkey_out, const std::string& hex_in)
+{
+    if (IsHex(hex_in))


I find this API super weird - its confusing as-is that we will throw if the hex represents an invalid pubkey but not if the string represents an empty string or something non-hex, expecting two functions which might each optioanlly populate pubkey_out to be called back-to-back. Can you add the IsHex() check in createmultisig/addmultisigaddress itself before the call so that you switch which function you call there instead? Then you could just throw a JSONRPCError here if !IsHex.

TheBlueMatt · 2017-10-17T18:11:05Z

src/rpc/misc.cpp

+    for (unsigned int i = 0; i < keys.size(); ++i) {
+#ifdef ENABLE_WALLET
+        CWallet* const pwallet = GetWalletForJSONRPCRequest(request);
+        if (IsDeprecatedRPCEnabled("createmultisig") && EnsureWalletIsAvailable(pwallet, request.fHelp) && pwallet) {


Note that pretty much all EnsureWalletIsAvailable does is a null-check on pwallet, so the && pwallet part is pretty redundant here.

TheBlueMatt · 2017-10-17T20:00:58Z

src/wallet/rpcwallet.cpp

+    if (IsValidDestination(dest)) {
+        const CKeyID *keyID = boost::get<CKeyID>(&dest);
+        if (!keyID) {
+            throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("%s does not refer to a key", addr_in));


This error message is somewhat confusing (maybe "%s is not an address"), but, also, it'd be nice if this also worked for WitnessV0KeyHash as well.

I'm not sure what even causes this error to happen.

TheBlueMatt · 2017-10-17T20:02:05Z

src/wallet/rpcwallet.cpp

+            throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("no full public key for address %s", addr_in));
+        }
+        if (!vchPubKey.IsFullyValid()) {
+            throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid public key: " + addr_in);


I believe we could INTERNAL_ERROR here as wallet should never contain invalid pubkeys (and call the error "Wallet contains invalid pubkey" instead).

achow101 · 2017-12-15T19:30:23Z

@laanwj I think this is ready to be merged.

achow101 · 2017-12-22T23:08:34Z

Merge please?

achow101 · 2017-12-22T23:16:42Z

deprectaed

Grr. fixed.

promag

Some comments.

promag · 2017-12-23T00:26:20Z

src/rpc/misc.cpp

-            "1. nrequired      (numeric, required) The number of required signatures out of the n keys or addresses.\n"
-            "2. \"keys\"       (string, required) A json array of keys which are bitcoin addresses or hex-encoded public keys\n"
+            "1. nrequired                    (numeric, required) The number of required signatures out of the n keys or addresses.\n"
+            "2. \"keys\"                       (string, required) A json array of keys which are bitcoin addresses or hex-encoded public keys\n"


Remove bitcoin addresses or ?

promag · 2017-12-23T00:26:45Z

src/rpc/misc.cpp

            "     [\n"
-            "       \"key\"    (string) bitcoin address or hex-encoded public key\n"
+            "       \"key\"                    (string) hex-encoded public key\n"


Nit, use sentence case (string) The hex-encoded ....

promag · 2017-12-23T00:30:40Z

src/rpc/util.cpp

+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#include "base58.h"
+#include "pubkey.h"


Nit, sort headers.

promag · 2017-12-23T00:33:18Z

src/rpc/util.cpp

+        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("no full public key for address %s", addr_in));
+    }
+    if (!vchPubKey.IsFullyValid()) {
+       throw JSONRPCError(RPC_INTERNAL_ERROR, "Wallet contains an invalid public key");


No test for this error?

This error is untestable. It requires a corrupted wallet.

promag · 2017-12-23T00:33:46Z

src/rpc/util.cpp

+    }
+    CPubKey vchPubKey;
+    if (!keystore->GetPubKey(*keyID, vchPubKey)) {
+        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("no full public key for address %s", addr_in));


No test for this error?

AFAIK, this error is untestable and requires a corrupted wallet.

achow101 · 2017-12-23T02:03:39Z

Had to rebase and fix wallet-dump.py to fix the travis failure.

ryanofsky

utACK 9bda49eb30b154f927f8de79f54c29ab950e15a5. Main change since last review is rebase and wallet-dump test fix, and there are also minor include order and rpc documentation changes.

Again, this seems like it might be ready for merge.

Make createmultisig only accept public keys with the old functionality marked as deprecated. Splits _createmultisig_redeemscript into two functions, one for getting public keys from UniValue and one for getting addresses from UniValue and then their respective public keys. The one for retrieving address's public keys is located in rpcwallet.cpp Changes addwitnessaddress's output to be a JSON object with two fields, address and redeemscript. Adds a test to deprecated_rpc.py for testing the deprecation. Update the tests to use addwitnessaddress or give only public keys to createmultisig. Anything that used addwitnessaddress was also updated to reflect the new API.

achow101 · 2018-01-11T19:13:56Z

Rebased again.

ryanofsky

utACK 1df206f. Only changes since last review are obvious ones needed for segwit wallet #11403 rebase.

jnewbery · 2018-01-23T21:00:10Z

Sadly this is going to need another substantial rebase if #12213 gets merged.

This has been rebased 5 times and has received several rounds of review:

ACK from me (x4)
utACK from @ryanofsky (x8)
utACK from @TheBlueMatt (x1)

@achow101 and I have asked for merge a couple of times here and in IRC.

Maintainers - is there anything we can do to get this unblocked? Are you waiting for additional reviewers?

It'd be a shame to have this go through another round of rebase/review.

1df206f Disallow using addresses in createmultisig (Andrew Chow) Pull request description: This PR should be the last part of #7965. This PR makes createmultisig only accept public keys and marks the old functionality of accepting addresses as deprecated. It also splits `_createmultisig_redeemscript` into two functions, `_createmultisig_getpubkeys` and `_createmultisig_getaddr_pubkeys`. `_createmultisig_getpubkeys` retrieves public keys from the RPC parameters and `_createmultisig_getaddr_pubkeys` retrieves addresses' public keys from the wallet. `_createmultisig_getaddr_pubkeys` requires the wallet and is only used by `addwitnessaddress` (except when `createmultisig` is used in deprecated mode). `addwitnessaddress`'s API is also changed. Instead of returning just an address, it now returns the same thing as `createmultisig`: a JSON object with two fields, address and redeemscript. Tree-SHA512: a5796e41935ad5e47d8165ff996a8b20d5112b5fc1a06a6d3c7f5513c13e7628a4fd37ec30fde05d8b15abfed51bc250710140f6834b13f64d0a0e47a3817969

db1cbcc [RPC] Remove deprecated addmultisigaddress return format (John Newbery) cb28a0b [RPC] Remove deprecated createmultisig object (John Newbery) ed45c82 [tests] Remove test for deprecated createmultsig option (John Newbery) d066a1c [rpc] Remove deprecated getmininginfo RPC option (John Newbery) c6f09c2 [rpc] remove deprecated estimatefee RPC (John Newbery) a8e437a [tests] Remove estimatefee from rpc_deprecated.py test (John Newbery) a5623b1 [tests] Remove tests for deprecated estimatefee RPC (John Newbery) d119f2e [tests] Fix style warnings in feature_fee_estimation.py (John Newbery) Pull request description: There were some RPC/RPC options deprecated in v0.16. Those can now be removed from master since v0.16 has been branched. - `estimatefee` RPC has been removed. The `feature_fee_estimation.py` test has been updated to remove the RPC, but doesn't yet have good coverage of the replacement RPC `estimatesmartfee`. Improving the test coverage should be done in a new PR. (#11031) - the `errors` field returned by `getmininginfo` has been deprecated and replaced by a `warning` field. (#10858) - providing addresses as inputs to `createmultisig` has been deprecated. Users should use `addmultisigaddress` instead (#11415) - The return format from `addmultisigaddress` has changed (#11415) `getwitnessaddress` was also deprecated in v0.16 and can be removed, but many tests are using that RPC, so it's a larger job to remove. It should be removed in a separate PR (possibly after #11739 and #11398 have been merged and the segwit test code tidied up) Tree-SHA512: 8ffaa5f6094131339b9e9e468e8b141de4b144697d2271efa2992b80b12eb97849ade3da8df5c1c9400ed4c04e6a029926550a3e5846d2029b644f9e84ac7124

1df206f Disallow using addresses in createmultisig (Andrew Chow) Pull request description: This PR should be the last part of bitcoin#7965. This PR makes createmultisig only accept public keys and marks the old functionality of accepting addresses as deprecated. It also splits `_createmultisig_redeemscript` into two functions, `_createmultisig_getpubkeys` and `_createmultisig_getaddr_pubkeys`. `_createmultisig_getpubkeys` retrieves public keys from the RPC parameters and `_createmultisig_getaddr_pubkeys` retrieves addresses' public keys from the wallet. `_createmultisig_getaddr_pubkeys` requires the wallet and is only used by `addwitnessaddress` (except when `createmultisig` is used in deprecated mode). `addwitnessaddress`'s API is also changed. Instead of returning just an address, it now returns the same thing as `createmultisig`: a JSON object with two fields, address and redeemscript. Tree-SHA512: a5796e41935ad5e47d8165ff996a8b20d5112b5fc1a06a6d3c7f5513c13e7628a4fd37ec30fde05d8b15abfed51bc250710140f6834b13f64d0a0e47a3817969

…#3482) * Merge bitcoin#11415: [RPC] Disallow using addresses in createmultisig 1df206f Disallow using addresses in createmultisig (Andrew Chow) Pull request description: This PR should be the last part of bitcoin#7965. This PR makes createmultisig only accept public keys and marks the old functionality of accepting addresses as deprecated. It also splits `_createmultisig_redeemscript` into two functions, `_createmultisig_getpubkeys` and `_createmultisig_getaddr_pubkeys`. `_createmultisig_getpubkeys` retrieves public keys from the RPC parameters and `_createmultisig_getaddr_pubkeys` retrieves addresses' public keys from the wallet. `_createmultisig_getaddr_pubkeys` requires the wallet and is only used by `addwitnessaddress` (except when `createmultisig` is used in deprecated mode). `addwitnessaddress`'s API is also changed. Instead of returning just an address, it now returns the same thing as `createmultisig`: a JSON object with two fields, address and redeemscript. Tree-SHA512: a5796e41935ad5e47d8165ff996a8b20d5112b5fc1a06a6d3c7f5513c13e7628a4fd37ec30fde05d8b15abfed51bc250710140f6834b13f64d0a0e47a3817969 * fix backport Signed-off-by: pasta <pasta@dashboost.org> * fix backport Signed-off-by: pasta <pasta@dashboost.org> * fix backport Signed-off-by: pasta <pasta@dashboost.org> * Dashify Co-authored-by: dustinface <35775977+xdustinface@users.noreply.github.com> Co-authored-by: Wladimir J. van der Laan <laanwj@gmail.com> Co-authored-by: dustinface <35775977+xdustinface@users.noreply.github.com>

db1cbcc [RPC] Remove deprecated addmultisigaddress return format (John Newbery) cb28a0b [RPC] Remove deprecated createmultisig object (John Newbery) ed45c82 [tests] Remove test for deprecated createmultsig option (John Newbery) d066a1c [rpc] Remove deprecated getmininginfo RPC option (John Newbery) c6f09c2 [rpc] remove deprecated estimatefee RPC (John Newbery) a8e437a [tests] Remove estimatefee from rpc_deprecated.py test (John Newbery) a5623b1 [tests] Remove tests for deprecated estimatefee RPC (John Newbery) d119f2e [tests] Fix style warnings in feature_fee_estimation.py (John Newbery) Pull request description: There were some RPC/RPC options deprecated in v0.16. Those can now be removed from master since v0.16 has been branched. - `estimatefee` RPC has been removed. The `feature_fee_estimation.py` test has been updated to remove the RPC, but doesn't yet have good coverage of the replacement RPC `estimatesmartfee`. Improving the test coverage should be done in a new PR. (bitcoin#11031) - the `errors` field returned by `getmininginfo` has been deprecated and replaced by a `warning` field. (bitcoin#10858) - providing addresses as inputs to `createmultisig` has been deprecated. Users should use `addmultisigaddress` instead (bitcoin#11415) - The return format from `addmultisigaddress` has changed (bitcoin#11415) `getwitnessaddress` was also deprecated in v0.16 and can be removed, but many tests are using that RPC, so it's a larger job to remove. It should be removed in a separate PR (possibly after bitcoin#11739 and bitcoin#11398 have been merged and the segwit test code tidied up) Tree-SHA512: 8ffaa5f6094131339b9e9e468e8b141de4b144697d2271efa2992b80b12eb97849ade3da8df5c1c9400ed4c04e6a029926550a3e5846d2029b644f9e84ac7124

achow101 mentioned this pull request Sep 29, 2017

Remaining instances of ENABLE_WALLET in libbitcoin_server.a #7965

Closed

14 tasks

fanquake added the RPC/REST/ZMQ label Sep 29, 2017

achow101 force-pushed the createmultisig-no-addr branch from 64d027c to eb95b57 Compare September 30, 2017 03:38

jnewbery reviewed Oct 2, 2017

View reviewed changes

achow101 force-pushed the createmultisig-no-addr branch from eb95b57 to a324a92 Compare October 2, 2017 23:09

promag reviewed Oct 2, 2017

View reviewed changes

achow101 force-pushed the createmultisig-no-addr branch from a324a92 to a169f17 Compare October 3, 2017 01:24

jnewbery reviewed Oct 3, 2017

View reviewed changes

jonasschnelli added the Needs release note label Oct 4, 2017

promag reviewed Oct 4, 2017

View reviewed changes

achow101 force-pushed the createmultisig-no-addr branch from a169f17 to 1e7a0ce Compare October 5, 2017 16:35

promag reviewed Oct 7, 2017

View reviewed changes

achow101 force-pushed the createmultisig-no-addr branch from 1e7a0ce to a0e0c4b Compare October 12, 2017 18:41

ryanofsky reviewed Oct 13, 2017

View reviewed changes

achow101 force-pushed the createmultisig-no-addr branch from a0e0c4b to 2a7b80c Compare October 13, 2017 18:21

achow101 force-pushed the createmultisig-no-addr branch from 2a7b80c to 897b5c0 Compare October 13, 2017 21:29

jnewbery reviewed Oct 13, 2017

View reviewed changes

achow101 force-pushed the createmultisig-no-addr branch from 897b5c0 to ac33d78 Compare October 13, 2017 22:14

ryanofsky reviewed Oct 16, 2017

View reviewed changes

TheBlueMatt reviewed Oct 17, 2017

View reviewed changes

achow101 force-pushed the createmultisig-no-addr branch 2 times, most recently from c4342d2 to 257d4fd Compare October 18, 2017 23:32

achow101 force-pushed the createmultisig-no-addr branch from 18eb69c to d1916aa Compare December 22, 2017 23:16

promag reviewed Dec 23, 2017

View reviewed changes

achow101 force-pushed the createmultisig-no-addr branch 2 times, most recently from 9e69c4f to 9bda49e Compare December 23, 2017 02:03

ryanofsky reviewed Jan 8, 2018

View reviewed changes

achow101 force-pushed the createmultisig-no-addr branch from 9bda49e to 1df206f Compare January 11, 2018 19:13

ryanofsky reviewed Jan 17, 2018

View reviewed changes

jnewbery mentioned this pull request Jan 23, 2018

Add address type option to addmultisigaddress #12213

Merged

laanwj merged commit 1df206f into bitcoin:master Jan 24, 2018

achow101 mentioned this pull request Jan 24, 2018

TODO for release notes 0.16.0 #11054

Closed

jnewbery mentioned this pull request Feb 2, 2018

Remove deprecated rpc options #12336

Merged

sipa removed the Needs release note label Aug 14, 2018

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

[RPC] Disallow using addresses in createmultisig #11415

[RPC] Disallow using addresses in createmultisig #11415

Conversation

achow101 commented Sep 29, 2017

jnewbery left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

achow101 commented Oct 2, 2017

jnewbery commented Oct 2, 2017

achow101 commented Oct 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnewbery left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnewbery Oct 3, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonasschnelli commented Oct 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanofsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

achow101 commented Oct 13, 2017

jnewbery commented Oct 13, 2017

achow101 commented Oct 13, 2017

jnewbery left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnewbery commented Oct 13, 2017

ryanofsky left a comment

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

achow101 commented Dec 15, 2017

achow101 commented Dec 22, 2017

achow101 commented Dec 22, 2017

promag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnewbery Oct 3, 2017 •

edited