[RPC] Split part of validateaddress into getaddressinfo #10583

achow101 · 2017-06-13T22:17:25Z

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.

laanwj · 2017-06-14T14:16:03Z

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

gmaxwell

Concept ACK.

sipa · 2017-06-16T22:20:19Z

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)?

achow101 · 2017-06-19T23:33:20Z

@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 · 2017-06-20T00:04:41Z

On second thought, I think I fixed the problem.

sipa · 2017-06-20T20:16:33Z

I believe you can do:

 bitcoind_LDADD = \
   $(LIBBITCOIN_SERVER) \
+  $(LIBBITCOIN_WALLET) \
   $(LIBBITCOIN_COMMON) \
   $(LIBUNIVALUE) \
   $(LIBBITCOIN_UTIL) \
-  $(LIBBITCOIN_WALLET) \
   $(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 · 2017-06-21T00:01:48Z

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

laanwj · 2017-06-24T14:21:36Z

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 · 2017-08-15T14:35:48Z

needs rebase

achow101 · 2017-08-15T21:27:43Z

rebased

jnewbery · 2017-09-01T16:07:45Z

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 · 2017-09-05T17:39:16Z

Rebased and tidied up

jnewbery · 2017-09-05T20:29:22Z

Travis is failing on invalid scripted diff check.

jnewbery

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

find ./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.

jnewbery · 2017-09-05T20:48:32Z

src/rpc/misc.cpp

-                }
+        try {
+            ret.pushKVs(getaddressinfo(request));
+        } catch (UniValue e) {


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

Can you add a comment explaining this?

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.

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

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

jnewbery · 2017-09-05T21:46:43Z

src/wallet/rpcwallet.cpp

+    if (!EnsureWalletIsAvailable(pwallet, request.fHelp))
+        return NullUniValue;
+
+    if (request.fHelp || request.params.size() != 1)


nit: braces for if block

jnewbery · 2017-09-05T21:48:05Z

src/wallet/rpcwallet.cpp

+            "1. \"address\"     (string, required) The bitcoin address to get the information of.\n"
+            "\nResult:\n"
+            "{\n"
+            "  \"address\" : \"address\", (string) The bitcoin address validated\n"


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

Forgot to say that this was addressed.

promag · 2018-02-15T22:37:37Z

src/rpc/util.cpp

+        return UniValue(UniValue::VOBJ);
+    }
+
+    UniValue operator()(const CKeyID &keyID) const {


Nit, const CKeyID& keyID. { on new line.

promag

Lightly tested ACK. Please consider the following comments.

promag · 2018-02-16T09:36:18Z

src/wallet/rpcwallet.cpp

+
+    UniValue operator()(const CNoDestination &dest) const { return UniValue(UniValue::VOBJ); }
+
+    UniValue operator()(const CKeyID &keyID) const {


& before space and { new line.

promag · 2018-02-16T09:36:27Z

src/wallet/rpcwallet.cpp

+        return obj;
+    }
+
+    UniValue operator()(const CScriptID &scriptID) const {


& before space and { new line.

promag · 2018-02-16T09:41:29Z

src/rpc/util.cpp

+public:
+    explicit DescribeAddressVisitor() {}
+
+    UniValue operator()(const CNoDestination &dest) const 


& before space.

travis linter is complaining about trailing whitespace.... I don't know protocol if it matters but just reporting where the complaint is coming from

promag · 2018-02-16T09:42:15Z

src/wallet/rpcwallet.cpp

+
+    explicit DescribeWalletAddressVisitor(CWallet *_pwallet) : pwallet(_pwallet) {}
+
+    UniValue operator()(const CNoDestination &dest) const { return UniValue(UniValue::VOBJ); }


& before space.

promag · 2018-02-16T09:46:17Z

src/wallet/rpcwallet.cpp

+    ret.pushKV("iswatchonly", bool(mine & ISMINE_WATCH_ONLY));
+    UniValue detail = DescribeWalletAddress(pwallet, dest);
+    ret.pushKVs(detail);
+    if (pwallet->mapAddressBook.count(dest)) {


Use find to avoid 2nd lookup below (unless you don't want to touch moved code).

promag · 2018-02-16T10:07:29Z

src/wallet/rpcwallet.cpp

+        CRIPEMD160 hasher;
+        uint160 hash;
+        hasher.Write(id.begin(), 32).Finalize(hash.begin());
+        if (pwallet && pwallet->GetCScript(CScriptID(hash), subscript)) {


In this context pwallet is valid so:

if (pwallet->GetCScript(CScriptID(hash), subscript)) {

promag · 2018-02-16T10:09:48Z

src/rpc/misc.cpp

-                    ret.pushKV("hdmasterkeyid", meta->hdMasterKeyID.GetHex());
-                }
-            }
+        if (!::vpwallets.empty() && IsDeprecatedRPCEnabled("validateaddress")) {


Why !::vpwallets.empty()? There is alwasy one wallet right?

No, you can run with -disablewallet which will disable wallets and thus vpwallets will be empty.

promag · 2018-02-16T10:10:15Z

src/rpc/util.cpp

+        return UniValue(UniValue::VOBJ);
+    }
+
+    UniValue operator()(const CKeyID &keyID) const


& before space.

promag · 2018-02-16T10:10:21Z

src/rpc/util.cpp

+        return obj;
+    }
+
+    UniValue operator()(const CScriptID &scriptID) const


& before space.

promag · 2018-02-16T10:11:10Z

src/wallet/rpcwallet.cpp

+    {
+        UniValue obj(UniValue::VOBJ);
+        CPubKey pubkey;
+        if (pwallet && pwallet->GetPubKey(CKeyID(id), pubkey)) {


In this context pwallet is valid so:

if (pwallet->GetPubKey(CKeyID(id), pubkey)) {

jnewbery · 2018-02-16T14:36:58Z

@promag - personally, I'd generally discourage nits after a PR has been open for many months and been rebased/re-reviewed many times already. Adding additional churn and workload to the contributor and reviewers at this point seems borderline sadistic 🙂

(just my opinion though)

Moves the parts of validateaddress which require the wallet into getaddressinfo which is part of the wallet RPCs. Mark those parts of validateaddress which require the wallet as deprecated. Validateaddress will call getaddressinfo for the data that both share for right now. Moves IsMine functions to libbitcoin_common and then links libbitcoin_wallet before libbitcoin_common in order to prevent linker errors since IsMine is no longer used in libbitcoin_server.

Change all instances of validateaddress to getaddressinfo since it seems that no test actually uses validateaddress for actually validating addresses. -BEGIN VERIFY SCRIPT- find ./test/functional -path '*py' -not -path ./test/functional/wallet_disable.py -not -path ./test/functional/rpc_deprecated.py -not -path ./test/functional/wallet_address_types.py -exec sed -i'' -e 's/validateaddress/getaddressinfo/g' {} \; -END VERIFY SCRIPT-

achow101 · 2018-02-16T17:10:55Z

Addressed some of @promag's nits. I don't really feel like fixing the other ones. It's also getting really tedious to do fix nits at this point, so I probably won't do so in the future for this PR.

jnewbery · 2018-02-16T18:48:59Z

Tested ACK b22cce0. Travis failure looks unrelated.

Let's get this merged!

sipa · 2018-02-16T19:12:37Z

utACK b22cce0

I have a few minor improvements, but I'll submit them as a separate PR after merge, nothing worth holding this up for.

promag · 2018-02-16T19:20:06Z

@jnewbery @achow101 sorry! didn't want to cause such pain :) I did report tested ACK.

jonasschnelli · 2018-02-17T11:27:13Z

Tested ACK b22cce0

b22cce0 scripted-diff: validateaddress to getaddressinfo in tests (Andrew Chow) b98bfc5 Create getaddressinfo RPC and deprecate parts of validateaddress (Andrew Chow) 1598f32 [rpc] Move DescribeAddressVisitor to rpc/util (John Newbery) 39633ec [rpc] split wallet and non-wallet parts of DescribeAddressVisitor (John Newbery) Pull request description: 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. Tree-SHA512: ce00ed0f2416200b8de1e0a75e8517c024be0b6153457d302c3879b3491cce28191e7c29aed08ec7d2eeeadc62918f5c43a7cb79cd2e4b6d9291bd83ec31c852

updates validateaddress to match the changes in bitcoind bitcoin/bitcoin#10583. removes the fields ismine and iswatchonly, those are things that a wallet would know, not a node. adds segwit related fields

updates validateaddress to match the changes in bitcoind bitcoin/bitcoin#10583. removes the fields ismine and iswatchonly, those are things that a wallet would know, not a node. adds segwit related fields, witness_version and witness_program. test changes to the node rpc method validateaddress with p2pkh, p2sh, p2wpkh and p2wsh addresses

This PR updates the node rpc validateaddress to better seperate the wallet and the node. The rpc was returning ismine and iswatch only. These values were moved to the wallet rpc method getaddressinfo. This corresponds to a change in bitcoind and bcoin. The updated validateaddress rpc returns the values: - isvalid - address - ismine - iswatchonly - isscript - isspendable - witness_version - witness_program See PR in bcoin: bcoin-org/bcoin#731 See PR in bitcoind: bitcoin/bitcoin#10583

updates validateaddress to match the changes in bitcoind bitcoin/bitcoin#10583. removes the fields ismine and iswatchonly, those are things that a wallet would know, not a node. adds segwit related fields, witness_version and witness_program. test changes to the node rpc method validateaddress with p2pkh, p2sh, p2wpkh and p2wsh addresses

…taddressinfo (#3880) * [rpc] split wallet and non-wallet parts of DescribeAddressVisitor * [rpc] Move DescribeAddressVisitor to rpc/util * Create getaddressinfo RPC and deprecate parts of validateaddress Moves the parts of validateaddress which require the wallet into getaddressinfo which is part of the wallet RPCs. Mark those parts of validateaddress which require the wallet as deprecated. Validateaddress will call getaddressinfo for the data that both share for right now. Moves IsMine functions to libbitcoin_common and then links libbitcoin_wallet before libbitcoin_common in order to prevent linker errors since IsMine is no longer used in libbitcoin_server. * scripted-diff: validateaddress to getaddressinfo in tests Change all instances of validateaddress to getaddressinfo since it seems that no test actually uses validateaddress for actually validating addresses. -BEGIN VERIFY SCRIPT- find ./test/functional -path '*py' -not -path ./test/functional/wallet_disable.py -not -path ./test/functional/rpc_deprecated.py -not -path ./test/functional/wallet_address_types.py -exec sed -i'' -e 's/validateaddress/getaddressinfo/g' {} \; -END VERIFY SCRIPT- * wallet: Add missing description of "hdchainid" * Update src/wallet/rpcwallet.cpp Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com> Co-authored-by: John Newbery <john@johnnewbery.com> Co-authored-by: Andrew Chow <achow101-github@achow101.com> Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>

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

gmaxwell reviewed Jun 15, 2017

View reviewed changes

sipa added the Needs release note label Jun 17, 2017

achow101 force-pushed the getaddressinfo branch from 6b40dca to 5ca08e0 Compare June 19, 2017 23:25

achow101 force-pushed the getaddressinfo branch 2 times, most recently from a64867b to 2d93b39 Compare June 20, 2017 18:43

achow101 force-pushed the getaddressinfo branch from 2d93b39 to 40a069f Compare June 21, 2017 00:27

This was referenced Jul 7, 2017

[wallet] Remove Wallet dependencies from init.cpp #10762

Merged

Multiwallet: add RPC endpoint support #10650

Closed

jnewbery mentioned this pull request Jul 21, 2017

Remaining instances of ENABLE_WALLET in libbitcoin_server.a #7965

Closed

14 tasks

achow101 force-pushed the getaddressinfo branch from 40a069f to 300808b Compare August 15, 2017 21:27

achow101 force-pushed the getaddressinfo branch 2 times, most recently from a3932cc to 47fd5fc Compare August 17, 2017 01:19

achow101 force-pushed the getaddressinfo branch 3 times, most recently from f76d026 to ad412dd Compare September 5, 2017 17:39

achow101 force-pushed the getaddressinfo branch from ad412dd to 7c3e7ac Compare September 5, 2017 20:34

jnewbery reviewed Sep 5, 2017

View reviewed changes

promag reviewed Feb 15, 2018

View reviewed changes

[rpc] split wallet and non-wallet parts of DescribeAddressVisitor

39633ec

achow101 force-pushed the getaddressinfo branch 2 times, most recently from 5f4e02f to baef775 Compare February 15, 2018 23:32

promag reviewed Feb 16, 2018

View reviewed changes

jnewbery and others added 3 commits February 16, 2018 12:09

[rpc] Move DescribeAddressVisitor to rpc/util

1598f32

achow101 force-pushed the getaddressinfo branch from baef775 to b22cce0 Compare February 16, 2018 17:09

jonasschnelli merged commit b22cce0 into bitcoin:master Feb 17, 2018

maflcko mentioned this pull request Feb 18, 2018

TODO for release notes 0.17.0 #12391

Closed

ryanofsky mentioned this pull request Mar 28, 2018

rpc: introduce 'label' API for wallet #7729

Closed

tynes mentioned this pull request Mar 16, 2019

validateaddress and getaddressinfo RPC Update bcoin-org/bcoin#731

Merged

fanquake removed the Needs release note label Mar 20, 2019

tynes mentioned this pull request Apr 2, 2019

rpc: update node rpc validateaddress handshake-org/hsd#123

Merged

jnewbery mentioned this pull request Apr 10, 2019

[rpc] Remove the addresses field from the getaddressinfo return object #15750

Merged

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021


		UniValue operator()(const CNoDestination &dest) const { return UniValue(UniValue::VOBJ); }

		UniValue operator()(const CKeyID &keyID) const {


		explicit DescribeWalletAddressVisitor(CWallet *_pwallet) : pwallet(_pwallet) {}

		UniValue operator()(const CNoDestination &dest) const { return UniValue(UniValue::VOBJ); }

[RPC] Split part of validateaddress into getaddressinfo #10583

[RPC] Split part of validateaddress into getaddressinfo #10583

Uh oh!

Conversation

achow101 commented Jun 13, 2017

Uh oh!

laanwj commented Jun 14, 2017

Uh oh!

gmaxwell left a comment

Choose a reason for hiding this comment

Uh oh!

sipa commented Jun 16, 2017

Uh oh!

achow101 commented Jun 19, 2017

Uh oh!

achow101 commented Jun 20, 2017

Uh oh!

sipa commented Jun 20, 2017

Uh oh!

achow101 commented Jun 21, 2017

Uh oh!

laanwj commented Jun 24, 2017

Uh oh!

jnewbery commented Aug 15, 2017

Uh oh!

achow101 commented Aug 15, 2017

Uh oh!

jnewbery commented Sep 1, 2017

Uh oh!

achow101 commented Sep 5, 2017

Uh oh!

jnewbery commented Sep 5, 2017

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

achow101 Sep 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

achow101 Sep 5, 2017 •

edited

Loading