/bitcoin

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

listreceivedbyaddress Filter Address #9503

Closed
wants to merge 2 commits into
from

Conversation

Reviewers

@kallewoof kallewoof

@TheBlueMatt TheBlueMatt

@jnewbery jnewbery

@luke-jr luke-jr

@NicolasDorier NicolasDorier

Assignees
No one assigned
Labels
Wallet
Projects
None yet
Milestone
No milestone
10 participants
@JeremyRubin @jnewbery @instagibbs @NicolasDorier @EthanHeilman @MarcoFalke @kallewoof @TheBlueMatt @luke-jr @fanquake
@JeremyRubin
Contributor

JeremyRubin commented Jan 10, 2017

This gives listreceivedbyaddress the ability to filter for a single address.

This functionality is useful for users such as TumbleBit who need to filter by address.
@jnewbery
Member

jnewbery commented Jan 10, 2017

A couple of general comments:

  • How large are the results from the listreceivedbyaddress rpc expected to be? If the rpc only ever returns a small number of addresses, it should be easy enough for the client to receive the full list of balances and then filter the list itself?
  • if there's a chance that this functionality needs to be extended further to filter on a list of addresses rather than a single address, it'd be better to include that now. Since #8811 , the arguments to the RPCs are part of the API, so changing them later becomes more troublesome. Perhaps change the only_address string to a filter_addresses array?

I've also added a few nits.
qa/rpc-tests/receivedby.py
+
+ #Not on addr
+ other_addr = self.nodes[0].getnewaddress() # note on node[0]! just a random addr
+ res = self.nodes[1].listreceivedbyaddress(5, True, True, other_addr)
@jnewbery

jnewbery Jan 10, 2017

Member

Why set minconf to 5 here? Can you just set it to 0 to match the call to listreceivedbyaddress() above?

@JeremyRubin

JeremyRubin Jan 13, 2017

Contributor

👍

src/wallet/rpcwallet.cpp
@@ -1133,6 +1133,9 @@ UniValue ListReceived(const UniValue& params, bool fByAccounts)
if (params.size() > 0)
nMinDepth = params[0].get_int();
+ bool fFilterAddress = !fByAccounts && params.size() > 3;
+ const CBitcoinAddress filterAddress = fFilterAddress ? CBitcoinAddress(params[3].get_str()) : CBitcoinAddress{};
@jnewbery

jnewbery Jan 10, 2017

Member

nit: I think it's clearer to move these two lines below the if(params.size() > 2) block, so the params are being tested in order. I also have a slight preference to change this to a if(params.size() > 3) block to match the other tests.

@JeremyRubin

JeremyRubin Jan 13, 2017

Contributor

I agree that it's important for the style to be consistent. I opted to convert the other argument parsing in ListReceived to match this style, at the expense of a larger diff, for the benefit of declaring the input parameters const.

@jnewbery

jnewbery Jan 13, 2017

Member

My personal preference is to keep the if statements rather than converting them to ternary conditionals, as I think that's clearer (and consistent with all the other RPCs). I'm not that concerned about having the parameters const.

src/wallet/rpcwallet.cpp
@@ -1178,10 +1185,22 @@ UniValue ListReceived(const UniValue& params, bool fByAccounts)
// Reply
UniValue ret(UniValue::VARR);
map<string, tallyitem> mapAccountTally;
- BOOST_FOREACH(const PAIRTYPE(CBitcoinAddress, CAddressBookData)& item, pwalletMain->mapAddressBook)
+
+
@jnewbery

jnewbery Jan 10, 2017

Member

nit: prefer not to have two blank lines in the middle of a function. Perhaps you can move one of them to between the if block and for block below.

@fanquake fanquake added the Wallet label Jan 10, 2017

@JeremyRubin
Contributor

JeremyRubin commented Jan 13, 2017

@jnewbery addressed nits.

  1. I think it could be quite large? And then you end up sending a bunch of extra crap over the network.
  2. I don't think it would need to be extended as such, but I'll let others chime in.

@JeremyRubin JeremyRubin closed this Jan 13, 2017

@JeremyRubin JeremyRubin reopened this Jan 13, 2017

@instagibbs
Member

instagibbs commented Jan 13, 2017

I think (2) isn't really needed since if you need more than a small number you can just do un-filtered or a few repeated calls. I'm no expert on this use-case though.
@JeremyRubin
Contributor

JeremyRubin commented Jan 13, 2017

@EthanHeilman thoughts?

Another optimization would be to allow for caching of this table on construction (maybe keep_cache/clear_cache parameters). This could reduce the O(n*m) complexity for making m repeated calls to O(n + m).
@NicolasDorier
Member

NicolasDorier commented Jan 13, 2017 •
Edited 1 time

@JeremyRubin The reason why I am interested into that is that here is the code I am using for querying the transactions of a scriptPubKey: Using listtransactions in tumblebit.

As far as I see this PR would be able to replace my listtransactions nicely. Will review.
src/wallet/rpcwallet.cpp
+ const isminefilter filter = ISMINE_SPENDABLE | (fIncludeWatchOnly ? ISMINE_WATCH_ONLY : 0);
+
+ const bool fFilterAddress = !fByAccounts && params.size() > 3;
+ const CBitcoinAddress filterAddress = fFilterAddress ? CBitcoinAddress(params[3].get_str()) : CBitcoinAddress{};
@NicolasDorier

NicolasDorier Jan 13, 2017 •
Edited 1 time

Member

CBitcoinAddress{} ? shouldn't it be CBitcoinAddress() ? (I guess not, as it build, but it is the first time I see that)

@JeremyRubin

JeremyRubin Jan 13, 2017

Contributor

should be equivalent; it's c++11 list initializer syntax

@EthanHeilman
Contributor

EthanHeilman commented Jan 13, 2017 •
Edited 1 time

@jnewbery

How large are the results from the listreceivedbyaddress rpc expected to be? If the rpc only ever returns a small number of addresses, it should be easy enough for the client to receive the full list of balances and then filter the list itself?

To perform a 800 user mix with TumbleBit we would need a watch list of 1600 addresses. However we there are times in which we only want to learn the status of a single address.

Sorting received transactions by address is a common enough usecase to have an RPC call. It seems likely that people are calling it and then writing filters to select the addresses they want (for instance this stackexchange question or this reddit post). It is an natural addition to the RPC API and one that would make our project and other projects more performant and cleaner.
@jnewbery
Member

jnewbery commented Jan 13, 2017

@EthanHeilman - thanks. Sounds like there's widespread demand for this functionality.

concept ACK
@luke-jr

luke-jr approved these changes Jan 20, 2017
View changes

utACK, with some nits.

src/wallet/rpcwallet.cpp
- int nMinDepth = 1;
- if (params.size() > 0)
- nMinDepth = params[0].get_int();
+ const int nMinDepth = params.size() == 0 ? 1 : params[0].get_int();
@luke-jr

luke-jr Jan 20, 2017

Member

Probably better to leave these alone. If params[0] is null, we really should silently use the default value... The longer version is also more readable/obvious.

src/wallet/rpcwallet.cpp
+ const bool fIncludeWatchOnly = params.size() > 2 && params[2].get_bool();
+ const isminefilter filter = ISMINE_SPENDABLE | (fIncludeWatchOnly ? ISMINE_WATCH_ONLY : 0);
+
+ const bool fFilterAddress = !fByAccounts && params.size() > 3;
@luke-jr

luke-jr Jan 20, 2017

Member

Should be false if params[3] is null.

src/wallet/rpcwallet.cpp
@@ -1162,6 +1159,10 @@ UniValue ListReceived(const UniValue& params, bool fByAccounts)
if (!ExtractDestination(txout.scriptPubKey, address))
continue;
+ if (fFilterAddress && !(filterAddress.Get() == address)) {
@luke-jr

luke-jr Jan 20, 2017

Member

Maybe we should be storing filterAddress.Get() above rather than a CBitcoinAddress?

src/wallet/rpcwallet.cpp
throw runtime_error(
- "listreceivedbyaddress ( minconf include_empty include_watchonly)\n"
+ "listreceivedbyaddress ( minconf include_empty include_watchonly only_address)\n"
@luke-jr

luke-jr Jan 20, 2017

Member

Prefer replacing all the params with an options Object, but perhaps that is better done in a separate PR.

@JeremyRubin

JeremyRubin Jan 20, 2017

Contributor

yeah. Separate PR...

@JeremyRubin
Contributor

JeremyRubin commented Jan 20, 2017

Addressed feedback, and squashed.

@luke-jr it now errors if the passed in address was not a valid address.
src/wallet/rpcwallet.cpp
throw runtime_error(
- "listreceivedbyaddress ( minconf include_empty include_watchonly)\n"
+ "listreceivedbyaddress ( minconf include_empty include_watchonly only_address)\n"
@kallewoof

kallewoof Jan 30, 2017

Member

Nit: space before end paren at 'only_address )' to match space after start paren at '( minconf'

@JeremyRubin

JeremyRubin Jan 31, 2017 •
Edited 1 time

Contributor

Not sure if I should fix that -- that's how it was before my PR.

@TheBlueMatt

TheBlueMatt Feb 7, 2017

Contributor

Yea, best to fix, I think.

@luke-jr

luke-jr approved these changes Feb 2, 2017
View changes

utACK

src/wallet/rpcwallet.cpp
@@ -1145,6 +1145,17 @@ UniValue ListReceived(const UniValue& params, bool fByAccounts)
if(params[2].get_bool())
filter = filter | ISMINE_WATCH_ONLY;
+ bool fFilterAddress = false;
+ CTxDestination filterAddress = CBitcoinAddress{}.Get();
@NicolasDorier

NicolasDorier Feb 3, 2017 •
Edited 1 time

Member

can you use CNoDestination(); here ? just to be coherent with nulladdress. (and the fact that I never saw this syntax before)

@TheBlueMatt

TheBlueMatt Feb 7, 2017

Contributor

Yes, please.

NicolasDorier added a commit to NTumbleBit/NTumbleBit that referenced this pull request Feb 6, 2017

@NicolasDorier
Support NTumbleBit with Rubin bitcoin/bitcoin#9503/ ListReceivedByAdd… 
…ress
cd7c2f4
@NicolasDorier
Member

NicolasDorier commented Feb 6, 2017 •
Edited 1 time

tested and integrated in NTumbleBit (NTumbleBit/NTumbleBit@cd7c2f4). This replace listtransaction * as I needed.

Outside of my nit, ACK a96fbed
@TheBlueMatt

TheBlueMatt reviewed Feb 7, 2017
View changes

utACK a96fbed aside from the style nits.

src/wallet/rpcwallet.cpp
@@ -1145,6 +1145,17 @@ UniValue ListReceived(const UniValue& params, bool fByAccounts)
if(params[2].get_bool())
filter = filter | ISMINE_WATCH_ONLY;
+ bool fFilterAddress = false;
+ CTxDestination filterAddress = CBitcoinAddress{}.Get();
@TheBlueMatt

TheBlueMatt Feb 7, 2017

Contributor

Yes, please.

src/wallet/rpcwallet.cpp
throw runtime_error(
- "listreceivedbyaddress ( minconf include_empty include_watchonly)\n"
+ "listreceivedbyaddress ( minconf include_empty include_watchonly only_address)\n"
@TheBlueMatt

TheBlueMatt Feb 7, 2017

Contributor

Yea, best to fix, I think.

@JeremyRubin
Contributor

JeremyRubin commented Feb 16, 2017

Nits addressed and squashed. Preserved the pre-squash here https://github.com/JeremyRubin/bitcoin/tree/listreceivedbyaddress-filtered-a96fbed73ed24591dd42845088639da0afaa436a
@NicolasDorier
Member

NicolasDorier commented Feb 16, 2017

ACK e6f053a
qa/rpc-tests/receivedby.py
+ res = self.nodes[1].listreceivedbyaddress(0, True, True, addr)
+ assert_array_result(res, {"address":addr}, expected)
+ if len(res) != 1:
+ raise AssertionError("listreceivedbyaddress expected only 1 result")
@kallewoof

kallewoof Mar 1, 2017

Member

Couldn't you do

assert_equal(len(res), 1)

instead, here? That way the resulting len(res) would be visible in the output.

qa/rpc-tests/receivedby.py
+ other_addr = self.nodes[0].getnewaddress() # note on node[0]! just a random addr
+ res = self.nodes[1].listreceivedbyaddress(0, True, True, other_addr)
+ if res != []:
+ raise AssertionError("Should not have listed any transactions, got\n%s"%res)
@kallewoof

kallewoof Mar 1, 2017

Member

I believe you can use assert_equal here too.

src/wallet/rpcwallet.cpp
@@ -1164,6 +1175,10 @@ UniValue ListReceived(const UniValue& params, bool fByAccounts)
if (!ExtractDestination(txout.scriptPubKey, address))
continue;
+ if (fFilterAddress && !(filterAddress == address)) {
@kallewoof

kallewoof Mar 1, 2017

Member

Why not fFilterAddress && filterAddress != address?

@JeremyRubin

JeremyRubin Mar 1, 2017

Contributor

(a != b) is not the same as !(a == b). I don't think != is defined here iirc.

@kallewoof

kallewoof Mar 1, 2017 •
Edited 1 time

Member

Not the same only in the sense that the != operator may not be defined, right? Semantically they're identical. And yeah, I see now. CNoDestination has no operator!=.

I wonder if it would be worth adding the one liner

friend bool operator!=(const CNoDestination &a, const CNoDestination &b) { return false; }

to make this line look less confusing.

@JeremyRubin

JeremyRubin Mar 1, 2017

Contributor

Except it's not a one liner, it needs to be added for all sorts of classes.

@luke-jr

luke-jr Mar 1, 2017

Member

It's not a CNoDestination, it's a CTxDestination, which is a boost variant. Some supported versions of boost do not support operator!= for variants.

@kallewoof

kallewoof Mar 1, 2017 •
Edited 1 time

Member

I thought if all variants accepted the operator the variant would accept it, in which case adding it to CNoDestination would be enough, but maybe I'm off on that one.
Edit: re-read your response; okay, didn't know that. Gotcha.

src/wallet/rpcwallet.cpp
throw runtime_error(
- "listreceivedbyaddress ( minconf include_empty include_watchonly)\n"
+ "listreceivedbyaddress (minconf include_empty include_watchonly only_address)\n"
@kallewoof

kallewoof Mar 1, 2017

Member

Nit: standard elsewhere is to show optionals as "( opt1 opt2 ... )", not "(opt1 opt2 ...)" (i.e. instead of removing starting space, add ending space)

src/wallet/rpcwallet.cpp
@@ -2940,7 +2969,7 @@ static const CRPCCommand commands[] =
{ "wallet", "listaddressgroupings", &listaddressgroupings, false, {} },
{ "wallet", "listlockunspent", &listlockunspent, false, {} },
{ "wallet", "listreceivedbyaccount", &listreceivedbyaccount, false, {"minconf","include_empty","include_watchonly"} },
- { "wallet", "listreceivedbyaddress", &listreceivedbyaddress, false, {"minconf","include_empty","include_watchonly"} },
+ { "wallet", "listreceivedbyaddress", &listreceivedbyaddress, false, {"minconf","include_empty","include_watchonly", "only_address"} },
@kallewoof

kallewoof Mar 1, 2017

Member

Nit: abide by surrounding style of no space after comma

@NicolasDorier
Member

NicolasDorier commented Mar 13, 2017

@JeremyRubin you might need to add a line in client.cpp (#9982)
@NicolasDorier
Member

NicolasDorier commented Mar 14, 2017

@JeremyRubin Let me know if you are a bit busy and prefer I take care of this PR.
@JeremyRubin
Contributor

JeremyRubin commented Mar 14, 2017

@NicolasDorier Go for it, sorry for the hold up!

@NicolasDorier NicolasDorier referenced this pull request Mar 14, 2017

Open

listreceivedbyaddress Filter Address #9991

@MarcoFalke
Member

MarcoFalke commented Mar 14, 2017

Discussion continues at #9991

@MarcoFalke MarcoFalke closed this Mar 14, 2017

DanGould added a commit to DanGould/NTumbleBit that referenced this pull request Mar 29, 2017

@NicolasDorier @DanGould
Support NTumbleBit with Rubin bitcoin/bitcoin#9503/ ListReceivedByAdd… 
…ress
6c7a475
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment