[RPC] Disallow using addresses in createmultisig

achow101 commented at 4:29 am on September 29, 2017: member

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.

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

achow101 force-pushed on Sep 30, 2017

in src/rpc/misc.cpp:294 in eb95b57eb9 outdated

296     }
297 
298+    int required = request.params[0].get_int();
299+
300+    // Get the public keys
301+    const UniValue& keys = request.params[1].get_array();

jnewbery commented at 8:18 pm on October 2, 2017:

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

jnewbery commented at 8:28 pm on October 2, 2017: member

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

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?

achow101 commented at 8:40 pm on October 2, 2017: member

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 commented at 8:53 pm on October 2, 2017: member

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 force-pushed on Oct 2, 2017

achow101 commented at 11:11 pm on October 2, 2017: member

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.

in src/rpc/misc.cpp:297 in a324a928f2 outdated

303+                _createmultisig_addr_to_pubkey(pwallet, pubkeys.at(i), keys[i].get_str());
304+            }
305+            _createmultisig_hex_to_pubkey(pubkeys.at(i), keys[i].get_str());
306+        } else
307+#endif
308+        _createmultisig_hex_to_pubkey(pubkeys.at(i), keys[i].get_str());

promag commented at 11:45 pm on October 2, 2017:

Put inside a block?

0    } else {
1#else
2    {
3#endif
4        _createmultisig_hex_to_pubkey(pubkeys.at(i), keys[i].get_str());
5    }

jnewbery commented at 8:42 pm on October 3, 2017:

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

achow101 commented at 4:36 pm on October 5, 2017:

Removed the else branch.

achow101 force-pushed on Oct 3, 2017

in src/wallet/rpcwallet.cpp:1234 in a169f17434 outdated

1189 
1190     pwallet->SetAddressBook(innerID, strAccount, "send");
1191-    return EncodeDestination(innerID);
1192+
1193+    UniValue result(UniValue::VOBJ);
1194+    if (IsDeprecatedRPCEnabled("addmultisigaddress")) {

jnewbery commented at 8:33 pm on October 3, 2017:

This can be made simpler:

0    if (IsDeprecatedRPCEnabled("addmultisigaddress")) {
1        // Old-style interface: just return the address
2        return EncodeDestination(innerID);
3    }
4
5    // return an object containing the address and redeem script
6    UniValue result(UniValue::VOBJ);
7    result.pushKV("address", EncodeDestination(innerID));
8    result.pushKV("redeemScript", HexStr(inner.begin(), inner.end()));
9    return result;

achow101 commented at 4:36 pm on October 5, 2017:

Done

in src/wallet/rpcwallet.cpp:1173 in a169f17434 outdated

1166@@ -1141,13 +1167,35 @@ UniValue addmultisigaddress(const JSONRPCRequest& request)
1167     if (!request.params[2].isNull())
1168         strAccount = AccountFromValue(request.params[2]);
1169 
1170+    int required = request.params[0].get_int();
1171+
1172+    // Get the public keys
1173+    const UniValue& keys = request.params[1].get_array();

jnewbery commented at 8:34 pm on October 3, 2017:

Suggestion: name this variable keys_or_addresses

achow101 commented at 4:36 pm on October 5, 2017:

Renamed to keys_or_addrs

in src/rpc/misc.cpp:296 in a169f17434 outdated

294+    pubkeys.resize(keys.size());
295+    for (unsigned int i = 0; i < keys.size(); ++i) {
296+#ifdef ENABLE_WALLET
297+        if (IsDeprecatedRPCEnabled("createmultisig")) {
298+            CWallet* const pwallet = GetWalletForJSONRPCRequest(request);
299+            if (!EnsureWalletIsAvailable(pwallet, request.fHelp)) {

jnewbery commented at 8:42 pm on October 3, 2017:

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:

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

achow101 commented at 4:36 pm on October 5, 2017:

Done.

in src/rpc/misc.cpp:235 in a169f17434 outdated

276-        }
277+CScript _createmultisig_redeemScript(const int required, const std::vector<CPubKey>& pubkeys)
278+{
279+    // Gather public keys
280+    if (required < 1) {
281+        throw std::runtime_error("a multisignature address must require at least one key to redeem");

jnewbery commented at 8:43 pm on October 3, 2017:

Can you replace these errors with better JSON errors?

promag commented at 7:00 am on October 4, 2017:

Agree.

achow101 commented at 4:36 pm on October 5, 2017:

Done

in src/rpc/misc.cpp:307 in a169f17434 outdated

305+            _createmultisig_hex_to_pubkey(pubkeys.at(i), keys[i].get_str());
306+        } else
307+#endif
308+        _createmultisig_hex_to_pubkey(pubkeys.at(i), keys[i].get_str());
309+        if (!pubkeys.at(i).IsFullyValid()) {
310+            throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid public key: " + keys[i].get_str());

jnewbery commented at 8:45 pm on October 3, 2017:

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.

achow101 commented at 4:36 pm on October 5, 2017:

Done.

jnewbery commented at 8:47 pm on October 3, 2017: member

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.

jonasschnelli added the label Needs release notes on Oct 4, 2017

jonasschnelli commented at 5:32 am on October 4, 2017: contributor

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

in src/rpc/misc.cpp:223 in a169f17434 outdated

252-            if (!vchPubKey.IsFullyValid())
253-                throw std::runtime_error(" Invalid public key: "+ks);
254-            pubkeys[i] = vchPubKey;
255+        CPubKey vchPubKey(ParseHex(hex_in));
256+        if (!vchPubKey.IsFullyValid()) {
257+            throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid public key: " + hex_in);

promag commented at 7:02 am on October 4, 2017:

Should have tests for these JSONRPCError.

achow101 commented at 4:37 pm on October 5, 2017:

Is that really necessary?

promag commented at 8:09 am on October 7, 2017:

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

achow101 commented at 6:41 pm on October 12, 2017:

Added a test in rawtransactions.py

achow101 force-pushed on Oct 5, 2017

in src/rpc/misc.cpp:292 in 1e7a0ce655 outdated

292+    const UniValue& keys = request.params[1].get_array();
293+    std::vector<CPubKey> pubkeys;
294+    pubkeys.resize(keys.size());
295+    for (unsigned int i = 0; i < keys.size(); ++i) {
296+#ifdef ENABLE_WALLET
297+        CWallet* const pwallet = GetWalletForJSONRPCRequest(request);

promag commented at 8:18 am on October 7, 2017:

Split in two loops:

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

achow101 commented at 6:24 pm on October 12, 2017:

Why?

ryanofsky commented at 3:48 pm on October 13, 2017:

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

achow101 force-pushed on Oct 12, 2017

in src/rpc/misc.cpp:215 in a0e0c4b674 outdated

211@@ -212,85 +212,59 @@ class CWallet;
212 /**
213  * Used by addmultisigaddress / createmultisig:
214  */
215-CScript _createmultisig_redeemScript(CWallet * const pwallet, const UniValue& params)
216-{
217-    int nRequired = params[0].get_int();
218-    const UniValue& keys = params[1].get_array();
219+#ifdef ENABLE_WALLET

ryanofsky commented at 3:28 pm on October 13, 2017:

No need for ENABLE_WALLET macro here.

Following would be equivalent:

0void _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.

achow101 commented at 6:21 pm on October 13, 2017:

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

in src/wallet/rpcwallet.cpp:1104 in a0e0c4b674 outdated

1097@@ -1098,7 +1098,27 @@ UniValue sendmany(const JSONRPCRequest& request)
1098 }
1099 
1100 // Defined in rpc/misc.cpp
1101-extern CScript _createmultisig_redeemScript(CWallet * const pwallet, const UniValue& params);
1102+extern CScript _createmultisig_redeemScript(const int required, const std::vector<CPubKey>& pubkeys);
1103+extern void _createmultisig_hex_to_pubkey(CPubKey& pubkey_out, const std::string& hex_in);
1104+
1105+void _createmultisig_addr_to_pubkey(CWallet* const pwallet, CPubKey& pubkey_out, const std::string& addr_in)

ryanofsky commented at 3:59 pm on October 13, 2017:

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.

achow101 commented at 6:19 pm on October 13, 2017:

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?

ryanofsky commented at 6:32 pm on October 13, 2017:

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.

ryanofsky commented at 4:06 pm on October 13, 2017: member

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.

achow101 force-pushed on Oct 13, 2017

achow101 commented at 6:21 pm on October 13, 2017: member

Rebased to fix travis.

jnewbery commented at 7:06 pm on October 13, 2017: member

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 force-pushed on Oct 13, 2017

achow101 commented at 9:29 pm on October 13, 2017: member

Actually fixed travis this time, I think.

in test/functional/rawtransactions.py:65 in 897b5c0cfb outdated

59@@ -60,7 +60,12 @@ def run_test(self):
60         addr1Obj = self.nodes[2].validateaddress(addr1)
61         addr2Obj = self.nodes[2].validateaddress(addr2)
62 
63-        mSigObj = self.nodes[2].addmultisigaddress(2, [addr1Obj['pubkey'], addr2Obj['pubkey']])
64+        # Tests for createmultisig and addmultisigaddress
65+        assert_raises_rpc_error(-5, "Invalid public key", self.nodes[0].createmultisig, 1, ["01020304"])
66+        self.nodes[0].createmultisig(2, [addr1Obj['pubkey'], addr2Obj['pubkey']])

jnewbery commented at 10:03 pm on October 13, 2017:

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)

achow101 commented at 10:14 pm on October 13, 2017:

Done

jnewbery commented at 10:04 pm on October 13, 2017: member

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.

achow101 force-pushed on Oct 13, 2017

jnewbery commented at 10:18 pm on October 13, 2017: member

Tested ACK ac33d788e2d7dbfdf760f3a620c86eeaf7cff3fd

ryanofsky commented at 3:02 pm on October 16, 2017: member

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

in src/rpc/misc.cpp:303 in ac33d788e2 outdated

297+            _createmultisig_addr_to_pubkey(pwallet, pubkeys.at(i), keys[i].get_str());
298+        }
299+#endif
300+        _createmultisig_hex_to_pubkey(pubkeys.at(i), keys[i].get_str());
301+        if (!pubkeys.at(i).IsFullyValid()) {
302+            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()));

TheBlueMatt commented at 5:59 pm on October 17, 2017:

Dangling “)” at the end of the err string.

achow101 commented at 11:32 pm on October 18, 2017:

Done.

in src/rpc/misc.cpp:219 in ac33d788e2 outdated

230-    std::vector<CPubKey> pubkeys;
231-    pubkeys.resize(keys.size());
232-    for (unsigned int i = 0; i < keys.size(); i++)
233+void _createmultisig_hex_to_pubkey(CPubKey& pubkey_out, const std::string& hex_in)
234+{
235+    if (IsHex(hex_in))

TheBlueMatt commented at 6:05 pm on October 17, 2017:

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.

achow101 commented at 11:30 pm on October 18, 2017:

Done.

in src/rpc/misc.cpp:293 in ac33d788e2 outdated

291+    std::vector<CPubKey> pubkeys;
292+    pubkeys.resize(keys.size());
293+    for (unsigned int i = 0; i < keys.size(); ++i) {
294+#ifdef ENABLE_WALLET
295+        CWallet* const pwallet = GetWalletForJSONRPCRequest(request);
296+        if (IsDeprecatedRPCEnabled("createmultisig") && EnsureWalletIsAvailable(pwallet, request.fHelp) && pwallet) {

TheBlueMatt commented at 6:11 pm on October 17, 2017:

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

achow101 commented at 11:30 pm on October 18, 2017:

Done

in src/wallet/rpcwallet.cpp:1110 in ac33d788e2 outdated

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

TheBlueMatt commented at 8:00 pm on October 17, 2017:

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.

achow101 commented at 11:58 pm on October 18, 2017:

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

in src/wallet/rpcwallet.cpp:1117 in ac33d788e2 outdated

1113+        CPubKey vchPubKey;
1114+        if (!pwallet->GetPubKey(*keyID, vchPubKey)) {
1115+            throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("no full public key for address %s", addr_in));
1116+        }
1117+        if (!vchPubKey.IsFullyValid()) {
1118+            throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid public key: " + addr_in);

TheBlueMatt commented at 8:02 pm on October 17, 2017:

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

achow101 commented at 11:32 pm on October 18, 2017:

Done.

TheBlueMatt commented at 8:03 pm on October 17, 2017: member

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

achow101 force-pushed on Oct 18, 2017

ryanofsky commented at 7:27 pm on October 31, 2017: member

utACK 257d4fd6fb0ff0347284980f61c014c179f579d5. Just has suggested changes from matt, mainly pulling IsHex check out of _createmultisig_hex_to_pubkey

in src/rpc/misc.cpp:300 in 257d4fd6fb outdated

298+            CWallet* const pwallet = GetWalletForJSONRPCRequest(request);
299+            if (IsDeprecatedRPCEnabled("createmultisig") && EnsureWalletIsAvailable(pwallet, request.fHelp)) {
300+                _createmultisig_addr_to_pubkey(pwallet, pubkeys.at(i), keys[i].get_str());
301+            } else
302+#endif
303+            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()));

TheBlueMatt commented at 5:04 pm on November 7, 2017:

nit: can we get some line breaks here? 400+ char-long line is long….

achow101 commented at 7:06 pm on November 7, 2017:

Done

in src/rpc/misc.cpp:296 in 257d4fd6fb outdated

294+        if (IsHex(keys[i].get_str())) {
295+            _createmultisig_hex_to_pubkey(pubkeys.at(i), keys[i].get_str());
296+        } else {
297+#ifdef ENABLE_WALLET
298+            CWallet* const pwallet = GetWalletForJSONRPCRequest(request);
299+            if (IsDeprecatedRPCEnabled("createmultisig") && EnsureWalletIsAvailable(pwallet, request.fHelp)) {

TheBlueMatt commented at 5:55 pm on November 7, 2017:

I think you want false for EnsureWalletIsAvailable’s second argument (avoidException), which makes EnsureWalletIsAvailable just return true if pwallet is non-null, so you could replace it with that anyway.

achow101 commented at 7:06 pm on November 7, 2017:

Done

in src/rpc/misc.cpp:302 in 257d4fd6fb outdated

300+                _createmultisig_addr_to_pubkey(pwallet, pubkeys.at(i), keys[i].get_str());
301+            } else
302+#endif
303+            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()));
304+        }
305+        if (!pubkeys.at(i).IsFullyValid()) {

TheBlueMatt commented at 6:02 pm on November 7, 2017:

I guess this is now an internal error, given that both _createmultisig_addr_to_pubkey and _createmultisig_hex_to_pubkey throw if !IsFullyValid?

achow101 commented at 7:07 pm on November 7, 2017:

This check is extraneous now since both _createmultisig_addr_to_pubkey and _createmultisig_hex_to_pubkey do the same check, so I removed it.

in src/wallet/rpcwallet.cpp:1195 in 257d4fd6fb outdated

1152-            "\"address\"         (string) A bitcoin address associated with the keys.\n"
1153+            "{\n"
1154+            "  \"address\":\"multisigaddress\",    (string) The value of the new multisig address.\n"
1155+            "  \"redeemScript\":\"script\"         (string) The string value of the hex-encoded redemption script.\n"
1156+            "}\n"
1157+            "\nResult (DEPRECATED. To see this result in v0.16 instead, please start bitcoind with -deprecatedrpc=addmultisigaddress).\n"

TheBlueMatt commented at 6:14 pm on November 7, 2017:

I dont think you need to bother deprecating the return value - the old value is still there, just in the address field - I’d say just add this to the release notes and skip this part.

achow101 commented at 7:00 pm on November 7, 2017:

That’s what I was originally going to do but I think we should be consistent on when and how things are deprecated. So this follows what we have been doing with deprecation of other things.

achow101 force-pushed on Nov 7, 2017

in src/wallet/rpcwallet.cpp:1183 in 36b9e675b9 outdated

1178+        if (IsHex(keys_or_addrs[i].get_str())) {
1179+            _createmultisig_hex_to_pubkey(pubkeys.at(i), keys_or_addrs[i].get_str());
1180+        } else {
1181+            _createmultisig_addr_to_pubkey(pwallet, pubkeys.at(i), keys_or_addrs[i].get_str());
1182+        }
1183+        if (!pubkeys.at(i).IsFullyValid()) {

TheBlueMatt commented at 10:09 pm on November 7, 2017:

Same as in the non-wallet call - this is superfluous now.

achow101 commented at 4:26 am on November 8, 2017:

Done.

in src/rpc/misc.cpp:215 in 36b9e675b9 outdated

211@@ -212,85 +212,57 @@ class CWallet;
212 /**
213  * Used by addmultisigaddress / createmultisig:
214  */
215-CScript _createmultisig_redeemScript(CWallet * const pwallet, const UniValue& params)
216+extern void _createmultisig_addr_to_pubkey(CWallet* const pwallet, CPubKey& pubkey_out, const std::string& addr_in);

TheBlueMatt commented at 10:11 pm on November 7, 2017:

nit: for functions which are not put in headers but used expernally, can you put a comment where the function is defined noting that it is duplicated elsewhere so people wishing to change its API are aware.

achow101 commented at 4:25 am on November 8, 2017:

Renamed to AddrToPubKey and the extern declaration was moved to rpc/util.h. Also added a comment with where it is defined.

in src/wallet/rpcwallet.cpp:1101 in 36b9e675b9 outdated

1097@@ -1098,7 +1098,28 @@ UniValue sendmany(const JSONRPCRequest& request)
1098 }
1099 
1100 // Defined in rpc/misc.cpp
1101-extern CScript _createmultisig_redeemScript(CWallet * const pwallet, const UniValue& params);
1102+extern CScript _createmultisig_redeemScript(const int required, const std::vector<CPubKey>& pubkeys);

TheBlueMatt commented at 10:12 pm on November 7, 2017:

nit: really sucks that these are not put in headers - we’re not just waiting to remove them in 0.17 when the deprecation goes away, but we’re stuck with them. Can we find some place to put them?

ryanofsky commented at 10:52 pm on November 7, 2017:

nit: really sucks that these are not put in headers - we’re not just waiting to remove them in 0.17 when the deprecation goes away, but we’re stuck with them. Can we find some place to put them?

Agree these are really ugly. I would put them in rpc/util.h and rpc/util.cpp files, where other rpc helper functions like ParseConfirmTarget could live as well. As mentioned above, I would also change these to normal CamelCase, drop mention of multisig in _createmultisig_hex_to_pubkey and _createmultisig_addr_to_pubkey since they are not multisig-specific, and change the wallet argument to a keystore pointer in the latter since it is not wallet-specific.

achow101 commented at 4:25 am on November 8, 2017:

I created rpc/util.cpp and rpc/util.h and moved the functions there. I also renamed them to be HexToPubKey and AddrToPubKey.

TheBlueMatt commented at 10:13 pm on November 7, 2017: member

Modulo a few remaining minor issues, utACK 36b9e675b93415bac9fe0bf8e9cd2243a6bee5aa

achow101 force-pushed on Nov 8, 2017

jnewbery commented at 1:32 pm on November 8, 2017: member

Build fails locally and on Travis. Will review once build is fixed.

achow101 commented at 4:46 pm on November 8, 2017: member

Build fails locally and on Travis. Will review once build is fixed.

Ehh.. It works fine for me locally :/ (even after git clean -fdx)

ryanofsky commented at 5:26 pm on November 8, 2017: member

Ehh.. It works fine for me locally :/ (even after git clean -fdx)

I get the errors from travis locally. Following fix worked for me:

 0--- a/src/rpc/server.cpp
 1+++ b/src/rpc/server.cpp
 2@@ -11,7 +11,7 @@
 3 #include "random.h"
 4 #include "sync.h"
 5 #include "ui_interface.h"
 6-#include "util.h"
 7+#include <util.h>
 8 #include "utilstrencodings.h"
 9 
10 #include <univalue.h>

(There’s a pending PR to update the include style in #11053)

in src/rpc/util.h:12 in a5df27e2c6 outdated

 7+
 8+class CWallet;
 9+class CPubKey;
10+class CScript;
11+
12+// Defined in wallet/rpcwallet.cpp. Used to get a public key for an address if that address is in the wallet.

ryanofsky commented at 5:51 pm on November 8, 2017:

Could define it in rpc/util.cpp, since this function doesn’t do anything wallet-specific (just s/CWallet/CKeyStore/)

achow101 commented at 7:29 pm on November 8, 2017:

Done

in src/rpc/util.h:13 in a5df27e2c6 outdated

 8+class CWallet;
 9+class CPubKey;
10+class CScript;
11+
12+// Defined in wallet/rpcwallet.cpp. Used to get a public key for an address if that address is in the wallet.
13+extern void AddrToPubKey(CWallet* const pwallet, CPubKey& pubkey_out, const std::string& addr_in);

ryanofsky commented at 5:52 pm on November 8, 2017:

Could return CPubKey instead of using an output parameter. This would simplify both the function and the calling code (could get rid of vector resize).

achow101 commented at 7:29 pm on November 8, 2017:

Done

in src/rpc/util.h:15 in a5df27e2c6 outdated

10+class CScript;
11+
12+// Defined in wallet/rpcwallet.cpp. Used to get a public key for an address if that address is in the wallet.
13+extern void AddrToPubKey(CWallet* const pwallet, CPubKey& pubkey_out, const std::string& addr_in);
14+
15+void HexToPubKey(CPubKey& pubkey_out, const std::string& hex_in);

ryanofsky commented at 5:53 pm on November 8, 2017:

Could return CPubKey instead of using an output parameter.

achow101 commented at 7:29 pm on November 8, 2017:

Done

ryanofsky commented at 6:06 pm on November 8, 2017: member

Conditional utACK a5df27e2c692bb928535bc0c98b91bce63ba9826 if a fix for the travis errors is added.

Nice cleanup! I left a few minor review comments, but feel free to ignore them if you don’t want to incorporate them.

achow101 force-pushed on Nov 8, 2017

achow101 commented at 7:29 pm on November 8, 2017: member

Addressed @ryanofsky’s comments and am trying his fix.

in src/rpc/util.h:8 in 7af17a1613 outdated

0@@ -0,0 +1,17 @@
1+// Copyright (c) 2017 The Bitcoin Core developers
2+// Distributed under the MIT software license, see the accompanying
3+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
4+
5+#ifndef BITCOIN_RPC_UTIL_H
6+#define BITCOIN_RPC_UTIL_H
7+
8+#include "keystore.h"

ryanofsky commented at 7:36 pm on November 8, 2017:

Could drop this include and just forward-declare CKeyStore. Might be good to add includes for <string> and <vector>.

achow101 commented at 7:45 pm on November 8, 2017:

I tried forward declaring CKeytStore and it didn’t work.

ryanofsky commented at 7:51 pm on November 8, 2017:

Following change should work:

 0diff --git a/src/rpc/util.h b/src/rpc/util.h
 1index f933b3da92..18f38e8cd3 100644
 2--- a/src/rpc/util.h
 3+++ b/src/rpc/util.h
 4@@ -5,8 +5,7 @@
 5 #ifndef BITCOIN_RPC_UTIL_H
 6 #define BITCOIN_RPC_UTIL_H
 7 
 8-#include "keystore.h"
 9-
10+class CKeyStore;
11 class CPubKey;
12 class CScript;
13 
14diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp
15index 2ebec303cb..a288fe75f5 100644
16--- a/src/rpc/util.cpp
17+++ b/src/rpc/util.cpp
18@@ -3,6 +3,7 @@
19 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
20 
21 #include "base58.h"
22+#include "keystore.h"
23 #include "pubkey.h"
24 #include "tinyformat.h"
25 #include "rpc/protocol.h"

achow101 commented at 8:25 pm on November 8, 2017:

Done

ryanofsky commented at 7:40 pm on November 8, 2017: member

utACK 7af17a16134cd3485f1dce7fc9351705eec9cdb7. PR description is a little out of date though, and might be worth updating since it will be included in the merge commit.

achow101 force-pushed on Nov 8, 2017

in src/wallet/rpcwallet.cpp:1195 in 84fbc440c3 outdated

1129-            "\"address\"         (string) A bitcoin address associated with the keys.\n"
1130+            "{\n"
1131+            "  \"address\":\"multisigaddress\",    (string) The value of the new multisig address.\n"
1132+            "  \"redeemScript\":\"script\"         (string) The string value of the hex-encoded redemption script.\n"
1133+            "}\n"
1134+            "\nResult (DEPRECATED. To see this result in v0.16 instead, please start bitcoind with -deprecatedrpc=addmultisigaddress).\n"

luke-jr commented at 2:04 pm on November 10, 2017:

I don’t think we should change return value types based on deprecatedrpc… What if someone wants to use two different pieces of software that expects the old vs new interface? (For example, a developer running the current version of their software in production, but wishing to adapt to and use the new API in development of the next version.)

jnewbery commented at 5:43 pm on November 10, 2017:

@luke-jr - what you’re describing is API versioning. Bitcoind doesn’t currently support that, but I could certainly see it being useful if done in the right way.

For now, I think the approach of using deprecatedrpc is the best option we have.

achow101 force-pushed on Nov 10, 2017

achow101 commented at 6:42 pm on November 10, 2017: member

Rebased

in src/rpc/misc.cpp:252 in 43198606bd outdated

250+
251+    // Get the public keys
252+    const UniValue& keys = request.params[1].get_array();
253+    std::vector<CPubKey> pubkeys;
254+    for (unsigned int i = 0; i < keys.size(); ++i) {
255+        if (IsHex(keys[i].get_str())) {

jnewbery commented at 8:47 pm on November 14, 2017:

Is there a possibility for an address to be misparsed as a hex string? I suppose it’s vanishingly unlikely, but can we add an extra check here that the length of the string is 66 or 130 (ie 33 or 65 bytes). Same comment goes for addmultisigaddress.

achow101 commented at 9:31 pm on November 14, 2017:

Done

in src/rpc/misc.cpp:261 in 43198606bd outdated

259+            CWallet* const pwallet = GetWalletForJSONRPCRequest(request);
260+            if (IsDeprecatedRPCEnabled("createmultisig") && EnsureWalletIsAvailable(pwallet, false)) {
261+                pubkeys.push_back(AddrToPubKey(pwallet, keys[i].get_str()));
262+            } else
263+#endif
264+            throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("Invalid public key: %s Note that from v0.16, createmultisig no longer accepts addresses."

jnewbery commented at 9:10 pm on November 14, 2017:

nit: can you add a newline between %s and Note. At the moment, the error message runs together into one line:

0Invalid public key: mmP95gb3sXa9sK4bq8c9KX1RNWnDaz8hbf 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

achow101 commented at 9:31 pm on November 14, 2017:

Done

jnewbery commented at 9:11 pm on November 14, 2017: member

I’ve tested 43198606bdbb408c262e5dcb726022c9249f72b6. It looks good, and I really like the refactor tidy-ups. A couple of comments inline: one functional, one nit.

achow101 force-pushed on Nov 14, 2017

jnewbery commented at 9:40 pm on November 14, 2017: member

Latest changes look good. Tested ACK bac7e31a84d47081e4bc441780ec2a25892d3447

achow101 force-pushed on Nov 30, 2017

achow101 commented at 4:48 pm on November 30, 2017: member

Rebased

jnewbery commented at 7:53 pm on December 1, 2017: member

utACK 18eb69cb3987def797c74110c2fc2d27a8d0a767.

I’ve eyeballed the rebase. Changes are limited to fixing up the #include conflicts.

ryanofsky commented at 8:52 pm on December 14, 2017: member

utACK 18eb69cb3987def797c74110c2fc2d27a8d0a767. Only change since last review is rebase and adding address/key length checks.

This seems to have had pretty thorough review, so maybe it is ready for merge.

achow101 commented at 7:30 pm on December 15, 2017: member

@laanwj I think this is ready to be merged.

achow101 commented at 11:08 pm on December 22, 2017: member

Merge please?

achow101 force-pushed on Dec 22, 2017

achow101 commented at 11:16 pm on December 22, 2017: member

deprectaed

Grr. fixed.

in src/rpc/misc.cpp:225 in d1916aa0c2 outdated

292+            "to the wallet before upgrading to v0.17. To use the deprecated functionality, start bitcoind with -deprecatedrpc=createmultisig\n"
293             "\nArguments:\n"
294-            "1. nrequired      (numeric, required) The number of required signatures out of the n keys or addresses.\n"
295-            "2. \"keys\"       (string, required) A json array of keys which are bitcoin addresses or hex-encoded public keys\n"
296+            "1. nrequired                    (numeric, required) The number of required signatures out of the n keys or addresses.\n"
297+            "2. \"keys\"                       (string, required) A json array of keys which are bitcoin addresses or hex-encoded public keys\n"

promag commented at 0:26 am on December 23, 2017:

Remove bitcoin addresses or ?

achow101 commented at 1:01 am on December 23, 2017:

Done.

in src/rpc/misc.cpp:227 in d1916aa0c2 outdated

295-            "2. \"keys\"       (string, required) A json array of keys which are bitcoin addresses or hex-encoded public keys\n"
296+            "1. nrequired                    (numeric, required) The number of required signatures out of the n keys or addresses.\n"
297+            "2. \"keys\"                       (string, required) A json array of keys which are bitcoin addresses or hex-encoded public keys\n"
298             "     [\n"
299-            "       \"key\"    (string) bitcoin address or hex-encoded public key\n"
300+            "       \"key\"                    (string) hex-encoded public key\n"

promag commented at 0:26 am on December 23, 2017:

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

achow101 commented at 1:01 am on December 23, 2017:

Done.

in src/rpc/util.cpp:6 in d1916aa0c2 outdated

0@@ -0,0 +1,68 @@
1+// Copyright (c) 2017 The Bitcoin Core developers
2+// Distributed under the MIT software license, see the accompanying
3+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
4+
5+#include "base58.h"
6+#include "pubkey.h"

promag commented at 0:30 am on December 23, 2017:

Nit, sort headers.

achow101 commented at 1:01 am on December 23, 2017:

Done.

in src/rpc/util.cpp:42 in d1916aa0c2 outdated

37+    CPubKey vchPubKey;
38+    if (!keystore->GetPubKey(*keyID, vchPubKey)) {
39+        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("no full public key for address %s", addr_in));
40+    }
41+    if (!vchPubKey.IsFullyValid()) {
42+       throw JSONRPCError(RPC_INTERNAL_ERROR, "Wallet contains an invalid public key");

promag commented at 0:33 am on December 23, 2017:

No test for this error?

achow101 commented at 0:57 am on December 23, 2017:

This error is untestable. It requires a corrupted wallet.

in src/rpc/util.cpp:39 in d1916aa0c2 outdated

34+    if (!keyID) {
35+        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("%s does not refer to a key", addr_in));
36+    }
37+    CPubKey vchPubKey;
38+    if (!keystore->GetPubKey(*keyID, vchPubKey)) {
39+        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("no full public key for address %s", addr_in));

promag commented at 0:33 am on December 23, 2017:

No test for this error?

achow101 commented at 1:03 am on December 23, 2017:

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

promag commented at 0:34 am on December 23, 2017: member

Some comments.

achow101 force-pushed on Dec 23, 2017

achow101 commented at 2:03 am on December 23, 2017: member

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

ryanofsky commented at 8:01 pm on January 8, 2018: member

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.

Disallow using addresses in createmultisig

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.

1df206f854

achow101 force-pushed on Jan 11, 2018

achow101 commented at 7:13 pm on January 11, 2018: member

Rebased again.

ryanofsky commented at 7:37 pm on January 17, 2018: member

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

jnewbery commented at 9:00 pm on January 23, 2018: member

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.

laanwj merged this on Jan 24, 2018

laanwj closed this on Jan 24, 2018

laanwj referenced this in commit 69ec021969 on Jan 24, 2018

laanwj referenced this in commit 3843780fd8 on Feb 8, 2018

sipa removed the label Needs release notes on Aug 14, 2018

PastaPastaPasta referenced this in commit 13b34dde3e on May 14, 2020

UdjinM6 referenced this in commit 0b3c3e8406 on May 15, 2020

PastaPastaPasta referenced this in commit 168e93162f on Jun 9, 2020

PastaPastaPasta referenced this in commit 7e04a9a3b1 on Jun 9, 2020

PastaPastaPasta referenced this in commit 8254d30d21 on Jun 10, 2020

PastaPastaPasta referenced this in commit 55cc7d8f89 on Jun 10, 2020

PastaPastaPasta referenced this in commit da0042972b on Jun 11, 2020

PastaPastaPasta referenced this in commit 1b7e6b45af on Jun 11, 2020

ckti referenced this in commit 8558a96dc7 on Mar 28, 2021

MarcoFalke locked this on Sep 8, 2021

[RPC] Disallow using addresses in createmultisig #11415