[RPC] Split part of validateaddress into getaddressinfo

achow101 commented at 10:17 pm on June 13, 2017: member

This PR makes a new RPC command called getaddressinfo which relies on the wallet. It contains all of validateaddress’s address info stuff. Those parts in validateaddress have been marked as deprecated. The tests have been updated to use getaddressinfo except the disablewallet test which is the only test that actually uses validateaddress to validate an address.

fanquake added the label RPC/REST/ZMQ on Jun 14, 2017

laanwj commented at 2:16 pm on June 14, 2017: member

Concept ACK. Makes a lot of sense to move the wallet-specific information to a separate RPC.

gmaxwell commented at 7:33 pm on June 15, 2017: contributor

Concept ACK.

sipa commented at 10:20 pm on June 16, 2017: member

Currently this duplicates a singificant amount of code. Would it be possible to abstract out the ‘info’ part of getaddressinfo, and then call that from validateaddress as well (temporarily, until removed)?

sipa added the label Needs release notes on Jun 17, 2017

achow101 force-pushed on Jun 19, 2017

achow101 commented at 11:33 pm on June 19, 2017: member

@sipa Apparently, no, this is not possible. My attempt at doing so has resulted in a linker failure. Apparently there is some makefile problems with that. https://github.com/achow101/bitcoin/tree/getaddressinfo-broken is the branch with the duplication removed, but there is a linker error with IsMine calls. The full build error is here: https://gist.github.com/achow101/0f0e2d9e75d9d77132ab83a7605f2377

achow101 commented at 0:04 am on June 20, 2017: member

On second thought, I think I fixed the problem.

achow101 force-pushed on Jun 20, 2017

sipa commented at 8:16 pm on June 20, 2017: member

I believe you can do:

0 bitcoind_LDADD = \
1   $(LIBBITCOIN_SERVER) \
2+  $(LIBBITCOIN_WALLET) \
3   $(LIBBITCOIN_COMMON) \
4   $(LIBUNIVALUE) \
5   $(LIBBITCOIN_UTIL) \
6-  $(LIBBITCOIN_WALLET) \
7   $(LIBBITCOIN_ZMQ) \

in Makefile.am.

Then the _ismine code can move from wallet to common (which I think belongs there, as signing code is already in common too).

achow101 commented at 0:01 am on June 21, 2017: member

@sipa I did that in my latest commit, but it seems that travis doesn’t like it.

achow101 force-pushed on Jun 21, 2017

laanwj commented at 2:21 pm on June 24, 2017: member

Then the _ismine code can move from wallet to common (which I think belongs there, as signing code is already in common too).

From what I remember it was already moved in the other direction at some point for the same reason. The circular dependency between _wallet and _server remains a problem. We’ll need to solve #7965 to get rid of it. Unfortunately, this is part of that…

jnewbery commented at 2:35 pm on August 15, 2017: member

needs rebase

achow101 force-pushed on Aug 15, 2017

achow101 commented at 9:27 pm on August 15, 2017: member

rebased

achow101 force-pushed on Aug 15, 2017

achow101 force-pushed on Aug 17, 2017

jnewbery commented at 4:07 pm on September 1, 2017: member

Concept ACK. This PR is currently difficult to review because there are broken intermediate commits, commits which reverse previous commits, random style changes in unconnected commits, etc. Are you able to tidy this up to make it easier to review? If not, I’m happy to take this PR and tidy it up.

My high-level feedback is that we should hide the deprecated fields in validateaddress behind a command-line argument (similar to #11031). The ismine move to libbitcoin_common seems fine to me.

achow101 force-pushed on Sep 5, 2017

achow101 commented at 5:39 pm on September 5, 2017: member

Rebased and tidied up

jnewbery commented at 8:29 pm on September 5, 2017: member

Travis is failing on invalid scripted diff check.

achow101 force-pushed on Sep 5, 2017

in src/rpc/misc.cpp:172 in 7c3e7ac6cd outdated

195-                    ret.push_back(Pair("hdkeypath", it->second.hdKeypath));
196-                    ret.push_back(Pair("hdmasterkeyid", it->second.hdMasterKeyID.GetHex()));
197-                }
198+        try {
199+            ret.pushKVs(getaddressinfo(request));
200+        } catch (UniValue e) {

jnewbery commented at 8:48 pm on September 5, 2017:

I don’t understand this. Why do we swallow the RPC_METHOD_NOT_FOUND error?

Can you add a comment explaining this?

achow101 commented at 11:21 pm on September 5, 2017:

That is swallowed in the event that -disablewallet is used; getaddressinfo will return RPC_METHOD_NOT_FOUND but validateaddress should still work and validate addresses.

I will add a comment.

jnewbery commented at 11:45 pm on September 5, 2017:

In that case, I think it’s preferable to test for wallet existence before calling getaddressinfo. The following should do it:

0        if (!::vpwallets.empty()) {
1            ret.pushKVs(getaddressinfo(request));
2        }

achow101 commented at 0:16 am on September 6, 2017:

Done

in src/wallet/rpcwallet.cpp:3219 in 7c3e7ac6cd outdated

3214+{
3215+    CWallet * const pwallet = GetWalletForJSONRPCRequest(request);
3216+    if (!EnsureWalletIsAvailable(pwallet, request.fHelp))
3217+        return NullUniValue;
3218+
3219+    if (request.fHelp || request.params.size() != 1)

jnewbery commented at 9:46 pm on September 5, 2017:

nit: braces for if block

in src/wallet/rpcwallet.cpp:3242 in 7c3e7ac6cd outdated

3223+            "to be in the wallet.\n"
3224+            "\nArguments:\n"
3225+            "1. \"address\"     (string, required) The bitcoin address to get the information of.\n"
3226+            "\nResult:\n"
3227+            "{\n"
3228+            "  \"address\" : \"address\", (string) The bitcoin address validated\n"

jnewbery commented at 9:48 pm on September 5, 2017:

Help text seems to be missing result fields, including at least script, hex, addresses, sigsrequired.

achow101 commented at 2:04 am on September 19, 2017:

Forgot to say that this was addressed.

jnewbery commented at 11:05 pm on September 5, 2017: member

A few nits inline. I think you could drop the third commit by making your scripted diff more focussed:

0find ./test/functional -path '*py' -not -path ./test/functional/disablewallet.py -exec sed -i'' -e 's/validateaddress/getaddressinfo/g' {} \;

note that sed syntax differs between BSD and GNU. You’ll need to use sed -i '' (space between i and empty string) on BSD.

(this also has the benefit of not breaking git bisect)

I still think it’s a good idea to hide the deprecated functionality behind a command-line argument to make removing it in the next version easier.

in src/wallet/rpcwallet.cpp:3178 in 7c3e7ac6cd outdated

3169@@ -3169,6 +3170,114 @@ UniValue generate(const JSONRPCRequest& request)
3170     return generateBlocks(coinbase_script, num_generate, max_tries, true);
3171 }
3172 
3173+class DescribeAddressVisitor : public boost::static_visitor<UniValue>
3174+{
3175+public:
3176+    CWallet * const pwallet;
3177+
3178+    DescribeAddressVisitor(CWallet *_pwallet) : pwallet(_pwallet) {}

promag commented at 11:20 pm on September 5, 2017:

Nit, * and & next to type. In this case also const: const CWallet* wallet.

achow101 commented at 2:06 am on September 19, 2017:

Done

in src/wallet/rpcwallet.cpp:3569 in 7c3e7ac6cd outdated

3175+public:
3176+    CWallet * const pwallet;
3177+
3178+    DescribeAddressVisitor(CWallet *_pwallet) : pwallet(_pwallet) {}
3179+
3180+    UniValue operator()(const CNoDestination &dest) const { return UniValue(UniValue::VOBJ); }

promag commented at 11:21 pm on September 5, 2017:

Nit, expand to multiline?

achow101 commented at 2:06 am on September 19, 2017:

I don’t think that is necessary

achow101 force-pushed on Sep 5, 2017

achow101 force-pushed on Sep 6, 2017

promag commented at 0:52 am on September 6, 2017: member

New code (and probably moved code) should take into account developer notes.

achow101 force-pushed on Sep 19, 2017

achow101 commented at 2:58 pm on September 19, 2017: member

Rebased

in src/rpc/misc.cpp:76 in 098cebb709 outdated

82+#ifdef ENABLE_WALLET
83+        if (!::vpwallets.empty()) {
84+            ret.pushKVs(getaddressinfo(request));
85+        }
86+#endif
87+        if (ret["address"].isNull()) {

jnewbery commented at 3:06 pm on September 19, 2017:

nit: indentation for this if block

in src/wallet/rpcwallet.cpp:3230 in 098cebb709 outdated

3225+};
3226+
3227+UniValue getaddressinfo(const JSONRPCRequest& request)
3228+{
3229+    CWallet * const pwallet = GetWalletForJSONRPCRequest(request);
3230+    if (!EnsureWalletIsAvailable(pwallet, request.fHelp))

jnewbery commented at 3:07 pm on September 19, 2017:

nit: braces

in src/wallet/rpcwallet.cpp:3277 in 098cebb709 outdated

3272+    ret.push_back(Pair("address", currentAddress));
3273+
3274+    CScript scriptPubKey = GetScriptForDestination(dest);
3275+    ret.push_back(Pair("scriptPubKey", HexStr(scriptPubKey.begin(), scriptPubKey.end())));;
3276+    
3277+    ret.push_back(Pair("isscript", scriptPubKey.IsPayToScriptHash()));

jnewbery commented at 3:10 pm on September 19, 2017:

micronit: not in same ordering as help text (either for getaddressinfo or validateaddress. Suggest you move this line below ret.push_back(Pair("iswatchonly", ...

in src/wallet/rpcwallet.cpp:3239 in 098cebb709 outdated

3234+        throw std::runtime_error(
3235+            "getaddressinfo \"address\"\n"
3236+            "\nReturn information about the given bitcoin address. Some information requires the address\n"
3237+            "to be in the wallet.\n"
3238+            "\nArguments:\n"
3239+            "1. \"address\"     (string, required) The bitcoin address to get the information of.\n"