listreceivedbyaddress Filter Address #9503

JeremyRubin commented at 6:26 PM on January 10, 2017: contributor

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.

in qa/rpc-tests/receivedby.py:None in ea19f790d2 outdated

  81 | +        if len(res) != 1:
  82 | +            raise AssertionError("listreceivedbyaddress expected only 1 result")
  83 | +
  84 | +        #Not on addr
  85 | +        other_addr = self.nodes[0].getnewaddress() # note on node[0]! just a random addr
  86 | +        res = self.nodes[1].listreceivedbyaddress(5, True, True, other_addr)

jnewbery commented at 10:12 PM on January 10, 2017:

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

JeremyRubin commented at 3:17 PM on January 13, 2017:

👍

in src/wallet/rpcwallet.cpp:None in ea19f790d2 outdated

1132 | @@ -1133,6 +1133,9 @@ UniValue ListReceived(const UniValue& params, bool fByAccounts)
1133 |      if (params.size() > 0)
1134 |          nMinDepth = params[0].get_int();
1135 |  
1136 | +    bool fFilterAddress = !fByAccounts && params.size() > 3;
1137 | +    const CBitcoinAddress filterAddress = fFilterAddress ?  CBitcoinAddress(params[3].get_str()) : CBitcoinAddress{};

jnewbery commented at 10:14 PM on January 10, 2017:

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 commented at 3:44 PM on January 13, 2017:

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 commented at 9:24 PM on January 13, 2017:

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.

in src/wallet/rpcwallet.cpp:None in ea19f790d2 outdated

1184 | @@ -1178,10 +1185,22 @@ UniValue ListReceived(const UniValue& params, bool fByAccounts)
1185 |      // Reply
1186 |      UniValue ret(UniValue::VARR);
1187 |      map<string, tallyitem> mapAccountTally;
1188 | -    BOOST_FOREACH(const PAIRTYPE(CBitcoinAddress, CAddressBookData)& item, pwalletMain->mapAddressBook)
1189 | +
1190 | +

jnewbery commented at 10:14 PM on January 10, 2017:

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.

jnewbery commented at 10:22 PM on January 10, 2017: member

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.

fanquake added the label Wallet on Jan 10, 2017

JeremyRubin force-pushed on Jan 13, 2017

JeremyRubin commented at 4:38 PM on January 13, 2017: contributor

@jnewbery addressed nits.

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

JeremyRubin closed this on Jan 13, 2017

JeremyRubin reopened this on Jan 13, 2017

instagibbs commented at 6:03 PM on January 13, 2017: member

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 commented at 7:18 PM on January 13, 2017: contributor

@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 commented at 7:41 PM on January 13, 2017: contributor

@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.

in src/wallet/rpcwallet.cpp:None in 5bb78f976d outdated

1146 | -            filter = filter | ISMINE_WATCH_ONLY;
1147 | +    const bool fIncludeWatchOnly = params.size() > 2 && params[2].get_bool();
1148 | +    const isminefilter filter =  ISMINE_SPENDABLE | (fIncludeWatchOnly ? ISMINE_WATCH_ONLY : 0);
1149 | +
1150 | +    const bool fFilterAddress = !fByAccounts && params.size() > 3;
1151 | +    const CBitcoinAddress filterAddress = fFilterAddress ?  CBitcoinAddress(params[3].get_str()) : CBitcoinAddress{};

NicolasDorier commented at 7:52 PM on January 13, 2017:

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

JeremyRubin commented at 8:26 PM on January 13, 2017:

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

EthanHeilman commented at 8:09 PM on January 13, 2017: contributor

@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 commented at 9:23 PM on January 13, 2017: member

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

concept ACK

in src/wallet/rpcwallet.cpp:None in 5bb78f976d outdated

1128 | @@ -1129,19 +1129,16 @@ struct tallyitem
1129 |  UniValue ListReceived(const UniValue& params, bool fByAccounts)
1130 |  {
1131 |      // Minimum confirmations
1132 | -    int nMinDepth = 1;
1133 | -    if (params.size() > 0)
1134 | -        nMinDepth = params[0].get_int();
1135 | +    const int nMinDepth = params.size() == 0 ? 1 : params[0].get_int();

luke-jr commented at 8:32 PM on January 20, 2017:

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.

in src/wallet/rpcwallet.cpp:None in 5bb78f976d outdated

1145 | -        if(params[2].get_bool())
1146 | -            filter = filter | ISMINE_WATCH_ONLY;
1147 | +    const bool fIncludeWatchOnly = params.size() > 2 && params[2].get_bool();
1148 | +    const isminefilter filter =  ISMINE_SPENDABLE | (fIncludeWatchOnly ? ISMINE_WATCH_ONLY : 0);
1149 | +
1150 | +    const bool fFilterAddress = !fByAccounts && params.size() > 3;

luke-jr commented at 8:35 PM on January 20, 2017:

Should be false if params[3] is null.

in src/wallet/rpcwallet.cpp:None in 5bb78f976d outdated

1158 | @@ -1162,6 +1159,10 @@ UniValue ListReceived(const UniValue& params, bool fByAccounts)
1159 |              if (!ExtractDestination(txout.scriptPubKey, address))
1160 |                  continue;
1161 |  
1162 | +            if (fFilterAddress && !(filterAddress.Get() == address)) {

luke-jr commented at 8:37 PM on January 20, 2017:

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

in src/wallet/rpcwallet.cpp:None in 5bb78f976d outdated

1264 | @@ -1251,14 +1265,15 @@ UniValue listreceivedbyaddress(const JSONRPCRequest& request)
1265 |      if (!EnsureWalletIsAvailable(request.fHelp))
1266 |          return NullUniValue;
1267 |  
1268 | -    if (request.fHelp || request.params.size() > 3)
1269 | +    if (request.fHelp || request.params.size() > 4)
1270 |          throw runtime_error(
1271 | -            "listreceivedbyaddress ( minconf include_empty include_watchonly)\n"
1272 | +            "listreceivedbyaddress ( minconf include_empty include_watchonly only_address)\n"

luke-jr commented at 8:40 PM on January 20, 2017:

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

JeremyRubin commented at 11:19 PM on January 20, 2017:

yeah. Separate PR...

luke-jr approved

luke-jr commented at 8:40 PM on January 20, 2017: member

utACK, with some nits.

JeremyRubin force-pushed on Jan 20, 2017

JeremyRubin commented at 11:12 PM on January 20, 2017: contributor

Addressed feedback, and squashed. @luke-jr it now errors if the passed in address was not a valid address.

in src/wallet/rpcwallet.cpp:None in 1f41da943c outdated

1280 | @@ -1253,15 +1281,15 @@ UniValue listreceivedbyaddress(const JSONRPCRequest& request)
1281 |      if (!EnsureWalletIsAvailable(request.fHelp))
1282 |          return NullUniValue;
1283 |  
1284 | -    if (request.fHelp || request.params.size() > 3)
1285 | +    if (request.fHelp || request.params.size() > 4)
1286 |          throw runtime_error(
1287 | -            "listreceivedbyaddress ( minconf include_empty include_watchonly)\n"
1288 | +            "listreceivedbyaddress ( minconf include_empty include_watchonly only_address)\n"

kallewoof commented at 11:11 AM on January 30, 2017:

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

JeremyRubin commented at 9:07 PM on January 31, 2017:

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

TheBlueMatt commented at 11:11 PM on February 7, 2017:

Yea, best to fix, I think.

luke-jr approved

luke-jr commented at 9:19 PM on February 2, 2017: member

utACK

in src/wallet/rpcwallet.cpp:None in a96fbed73e outdated

1144 | @@ -1145,6 +1145,17 @@ UniValue ListReceived(const UniValue& params, bool fByAccounts)
1145 |          if(params[2].get_bool())
1146 |              filter = filter | ISMINE_WATCH_ONLY;
1147 |  
1148 | +    bool fFilterAddress = false;
1149 | +    CTxDestination filterAddress =  CBitcoinAddress{}.Get();

NicolasDorier commented at 4:32 AM on February 3, 2017:

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

TheBlueMatt commented at 11:11 PM on February 7, 2017:

Yes, please.

NicolasDorier referenced this in commit cd7c2f42eb on Feb 6, 2017

NicolasDorier commented at 7:03 AM on February 6, 2017: contributor

tested and integrated in NTumbleBit (https://github.com/NTumbleBit/NTumbleBit/commit/cd7c2f42eb7ba1ff071519dd0c2798a1f2fc43d7). This replace listtransaction * as I needed.

Outside of my nit, ACK a96fbed

TheBlueMatt commented at 11:12 PM on February 7, 2017: member

utACK a96fbed73ed24591dd42845088639da0afaa436a aside from the style nits.

Add address filtering to listreceivedbyaddress c60de5d3b9

Add tests of listreceivedbyaddress address filtering e6f053a66f

JeremyRubin force-pushed on Feb 16, 2017

JeremyRubin commented at 6:55 AM on February 16, 2017: contributor

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

NicolasDorier commented at 7:30 AM on February 16, 2017: contributor

ACK e6f053a66f9196e2d75d15af2f89facd693402ff

in qa/rpc-tests/receivedby.py:None in e6f053a66f

  77 | +        #Only on addr
  78 | +        expected = {"address":addr, "account":"", "amount":Decimal("0.1"), "confirmations":10, "txids":[txid,]}
  79 | +        res = self.nodes[1].listreceivedbyaddress(0, True, True, addr)
  80 | +        assert_array_result(res, {"address":addr}, expected)
  81 | +        if len(res) != 1:
  82 | +            raise AssertionError("listreceivedbyaddress expected only 1 result")

kallewoof commented at 10:20 PM on February 28, 2017:

Couldn't you do

assert_equal(len(res), 1)

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

in qa/rpc-tests/receivedby.py:None in e6f053a66f

  83 | +
  84 | +        #Not on addr
  85 | +        other_addr = self.nodes[0].getnewaddress() # note on node[0]! just a random addr
  86 | +        res = self.nodes[1].listreceivedbyaddress(0, True, True, other_addr)
  87 | +        if res != []:
  88 | +            raise AssertionError("Should not have listed any transactions, got\n%s"%res)

kallewoof commented at 11:12 PM on February 28, 2017:

I believe you can use assert_equal here too.

in src/wallet/rpcwallet.cpp:None in e6f053a66f

1174 | @@ -1164,6 +1175,10 @@ UniValue ListReceived(const UniValue& params, bool fByAccounts)
1175 |              if (!ExtractDestination(txout.scriptPubKey, address))
1176 |                  continue;
1177 |  
1178 | +            if (fFilterAddress && !(filterAddress == address)) {

kallewoof commented at 11:55 PM on February 28, 2017:

Why not fFilterAddress && filterAddress != address?

JeremyRubin commented at 12:30 AM on March 1, 2017:

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

kallewoof commented at 12:45 AM on March 1, 2017:

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 commented at 4:52 AM on March 1, 2017:

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

luke-jr commented at 6:01 AM on March 1, 2017:

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 commented at 7:05 AM on March 1, 2017:

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.

in src/wallet/rpcwallet.cpp:None in e6f053a66f

1280 | @@ -1253,15 +1281,15 @@ UniValue listreceivedbyaddress(const JSONRPCRequest& request)
1281 |      if (!EnsureWalletIsAvailable(request.fHelp))
1282 |          return NullUniValue;
1283 |  
1284 | -    if (request.fHelp || request.params.size() > 3)
1285 | +    if (request.fHelp || request.params.size() > 4)
1286 |          throw runtime_error(
1287 | -            "listreceivedbyaddress ( minconf include_empty include_watchonly)\n"
1288 | +            "listreceivedbyaddress (minconf include_empty include_watchonly only_address)\n"

kallewoof commented at 11:58 PM on February 28, 2017:

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

in src/wallet/rpcwallet.cpp:None in e6f053a66f

2968 | @@ -2940,7 +2969,7 @@ static const CRPCCommand commands[] =
2969 |      { "wallet",             "listaddressgroupings",     &listaddressgroupings,     false,  {} },
2970 |      { "wallet",             "listlockunspent",          &listlockunspent,          false,  {} },
2971 |      { "wallet",             "listreceivedbyaccount",    &listreceivedbyaccount,    false,  {"minconf","include_empty","include_watchonly"} },
2972 | -    { "wallet",             "listreceivedbyaddress",    &listreceivedbyaddress,    false,  {"minconf","include_empty","include_watchonly"} },
2973 | +    { "wallet",             "listreceivedbyaddress",    &listreceivedbyaddress,    false,  {"minconf","include_empty","include_watchonly", "only_address"} },

kallewoof commented at 11:59 PM on February 28, 2017:

Nit: abide by surrounding style of no space after comma

kallewoof commented at 12:01 AM on March 1, 2017: member

utACK e6f053a

NicolasDorier commented at 6:17 AM on March 13, 2017: contributor

@JeremyRubin you might need to add a line in client.cpp (https://github.com/bitcoin/bitcoin/issues/9982)

NicolasDorier commented at 5:05 AM on March 14, 2017: contributor

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

JeremyRubin commented at 9:03 AM on March 14, 2017: contributor

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

MarcoFalke commented at 2:11 PM on March 14, 2017: member

Discussion continues at #9991

MarcoFalke closed this on Mar 14, 2017

DanGould referenced this in commit 6c7a4756a0 on Mar 29, 2017

laanwj referenced this in commit 4ca7c1e4ac on Mar 7, 2018

PastaPastaPasta referenced this in commit 37a54bf50f on Jun 17, 2020

PastaPastaPasta referenced this in commit 0c9932fd64 on Jun 17, 2020

PastaPastaPasta referenced this in commit fd97aecfa8 on Jun 17, 2020

PastaPastaPasta referenced this in commit e653fb567c on Jun 27, 2020

MarcoFalke locked this on Sep 8, 2021