[RPC] Disallow using addresses in createmultisig

[achow101]

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]

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]

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]

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]

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]

Put inside a block?

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

[jnewbery]

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

[achow101]

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]

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;

[achow101]

Done

[jnewbery]

Suggestion: name this variable keys_or_addresses

[achow101]

Renamed to keys_or_addrs

[jnewbery]

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

[achow101]

Done.

[jnewbery]

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

[jnewbery]

Can you replace these errors with better JSON errors?

[promag]

Agree.

[achow101]

Done

[jnewbery]

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]

Done.

[jonasschnelli]

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

[promag]

Should have tests for these JSONRPCError.

[achow101]

Is that really necessary?

[promag]

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

[achow101]

Added a test in rawtransactions.py

[promag]

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

[achow101]

Why?

[ryanofsky]

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]

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.

[achow101]

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

[ryanofsky]

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

[ryanofsky]

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]

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]

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]

Rebased to fix travis.

[jnewbery]

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]

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]

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]

Done

[jnewbery]

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]

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

[achow101]

Done.

[TheBlueMatt]

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]

Done.

[TheBlueMatt]

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

[achow101]

Done

[TheBlueMatt]

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]

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

[TheBlueMatt]

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

[achow101]

Done.

[achow101]

@laanwj I think this is ready to be merged.

[achow101]

Merge please?

[achow101]

deprectaed

Grr. fixed.

[promag]

Some comments.

[promag]

Remove bitcoin addresses or ?

[achow101]

Done.

[promag]

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

[achow101]

Done.

[promag]

Nit, sort headers.

[achow101]

Done.

[promag]

No test for this error?

[achow101]

This error is untestable. It requires a corrupted wallet.

[promag]

No test for this error?

[achow101]

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

[achow101]

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.

[achow101]

Rebased again.

[ryanofsky]

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

[jnewbery]

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.