listreceivedbyaddress Filter Address

NicolasDorier commented at 1:23 pm on March 14, 2017: contributor

Supersede #9503 created by @JeremyRubin , I will maintain it.

NicolasDorier force-pushed on Mar 14, 2017

NicolasDorier commented at 4:56 pm on March 14, 2017: contributor

Rebased and fixed conflicts
Addressed @kallewoof nits
Add the new parameter to client.cpp

kallewoof approved

kallewoof commented at 7:02 pm on March 14, 2017: member

utACK 8c373d5feedb428d234278d484e5d2a932010fae

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

fanquake added the label Wallet on Mar 14, 2017

NicolasDorier commented at 5:18 am on March 16, 2017: contributor

@luke-jr @jnewbery @JeremyRubin nit addressed, can you re-ACK?

in qa/rpc-tests/receivedby.py: in 8c373d5fee outdated

71+                           {"address":empty_addr, "account":"", "amount":0, "confirmations":0, "txids":[]})
72+
73+        #Test Address filtering
74+        #Only on addr
75+        expected = {"address":addr, "account":"", "amount":Decimal("0.1"), "confirmations":10, "txids":[txid,]}
76+        res = self.nodes[1].listreceivedbyaddress(0, True, True, addr)

jnewbery commented at 1:43 pm on March 16, 2017:

Nit: Consider using named arguments here, instead of positional arguments (since the meaning of the arguments is not clear without having to go back to the definition of listreceivedbyaddress).

NicolasDorier commented at 2:31 am on March 17, 2017:

Listreceivedbyaddress is not known at compile time, and thus has no definition in python. This is interpreted at runtime. So I can’t really use named argument. ~~Would inline comment be good ?~~

EDIT: No mid line comments in python

jnewbery commented at 3:13 am on March 17, 2017:

You can use named arguments in the RPC calls. See #9983 for example.

in qa/rpc-tests/receivedby.py: in 8c373d5fee outdated

68-                           {"address":addr},
69-                           {"address":addr, "account":"", "amount":0, "confirmations":0, "txids":[]})
70+                           {"address":empty_addr},
71+                           {"address":empty_addr, "account":"", "amount":0, "confirmations":0, "txids":[]})
72+
73+        #Test Address filtering

jnewbery commented at 1:52 pm on March 16, 2017:

I’d prefer this test if other_addr (currently declared on line 78) received some funds before you did your tests on listreceivedbyaddress() with a filter address. This isn’t currently testing the case where the wallet contains multiple addresses with funds and you want to see the transactions received by one of those addresses. Ideally the test case would be:

define addr and send funds to it (already done for you in lines 42-43)
define other_addr and send funds to it
call listreceivedbyaddress() filtering on addr. Assert only one transaction is returned.
call listreceivedbyaddress() filtering on other_addr. Assert only one transaction is returned.
call listreceivedbyaddress() with no filtering. Assert both transactions are returned.

in src/wallet/rpcwallet.cpp: in 8c373d5fee outdated

1352             "\nArguments:\n"
1353             "1. minconf           (numeric, optional, default=1) The minimum number of confirmations before payments are included.\n"
1354             "2. include_empty     (bool, optional, default=false) Whether to include addresses that haven't received any payments.\n"
1355             "3. include_watchonly (bool, optional, default=false) Whether to include watch-only addresses (see 'importaddress').\n"
1356-
1357+            "4. only_address   (string, optional) If present, only return information on this address. Otherwise, return all information.\n"

jnewbery commented at 1:53 pm on March 16, 2017:

minor nit: align parentheses with line above.

jnewbery commented at 2:30 pm on March 16, 2017: member

Looks good. A couple of nits and a suggestion on improving the test case.

JeremyRubin commented at 9:30 pm on March 16, 2017: contributor

utAck

NicolasDorier force-pushed on Mar 17, 2017

NicolasDorier commented at 3:10 am on March 17, 2017: contributor

@jnewbery I improved the tests, and fixed the alignement. I can’t use named parameter in the test though.

NicolasDorier force-pushed on Mar 17, 2017

NicolasDorier commented at 2:47 pm on March 17, 2017: contributor

added named parameters to one call to listreceivedbyaddress in the tests for better readability.

jnewbery commented at 3:23 pm on March 17, 2017: member

line 73 minConf needs to be minconf.

with that change, tested ACK. Good stuff @NicolasDorier !

NicolasDorier force-pushed on Mar 18, 2017

NicolasDorier commented at 2:39 pm on March 18, 2017: contributor

@jnewbery thanks done, @JeremyRubin did the hard work :)

NicolasDorier force-pushed on Mar 21, 2017

EthanHeilman commented at 8:07 pm on March 21, 2017: contributor

utACK - the code looks good and this is very useful functionality.

NicolasDorier commented at 0:43 am on March 22, 2017: contributor

@TheBlueMatt I fixed nits and added tests, can you re-ACK ?

nopara73 commented at 6:18 am on May 25, 2017: none

ACK - works fine.
This could be (not sure if should be) combined with fixing this issue for correct GUI experience: #9192

NicolasDorier commented at 7:10 am on June 14, 2017: contributor

Closing. I am using a workaround now, I am caching the whole listtransaction, at every block. This call would have been super useful though.

NicolasDorier closed this on Jun 14, 2017

jnewbery commented at 2:27 pm on June 14, 2017: member

:disappointed: I’m happy to reACK if you decide to reopen this!

NicolasDorier reopened this on Jun 15, 2017

NicolasDorier commented at 1:37 pm on June 15, 2017: contributor

@jnewbery, if you have interest, I am reopening, I guess it does not hurt to keep it opens, it is not high maintenance PR. I was thinking to allow multiple addresses to be passed in the filter, instead of only one.

For NTumbleBit, I changed the design, I have a method called GetTransactions(address) which should send back the inputs AND outputs transactions of this address. listreceivedbyaddress would only show input transactions.

The way I ended up implemeting my need is by caching listtransaction then for each transaction in it, caching gettransaction in a internal database. I now have good performance, and I am not sure this RPC method would suit my case now.

EDIT: Documenting why I need also all transaction spent from this address: (not only transactions received)

In TumbleBit, there is a chain of transaction.

0[Escrow] -> [Offer] -> [Fulfill]

Escrow is confirmed. The user knows one of the ScriptPubKey inside Offer, but not the transaction ID. His goal is to fetch Fulfill on the blockchain. (When the ScriptPubKey of Offer get spent, then important preimages are revealed)

So the way I am doing it now is adding Offer’s ScriptPubKey as WatchOnly. Then fetching all the transactions from the wallet, caching them in internal database, and going through all those transactions to see if there is one input which is spent by the ScriptPubKey of Offer. If yes, then we have [Fulfill].

Watching all transactions related to one address is very useful, not only the received transaction of the address.

In Core, however it is not possible, because knowing if a transaction is “from an address” is considered bad practice. At the root, this is the same reason as to why #10007 could not make it..

sipa commented at 11:26 pm on June 16, 2017: member

@NicolasDorier Do you actually need the wallet functionality (balance tracking, UTXO management, coin selection, …), or just a means to be notified of certain transactions?

nopara73 commented at 4:48 am on June 17, 2017: none

@sipa Need the wallet functionality for NTumbleBit.

sipa commented at 5:09 am on June 17, 2017: member

Can you clarify what functionality it relies on?

nopara73 commented at 5:46 am on June 17, 2017: none

A lot, but I am not sure which are relevant to this discussion with the workaround. @NicolasDorier will have a better insight. https://github.com/NTumbleBit/NTumbleBit/tree/master/NTumbleBit.TumblerServer/Services/RPCServices

in src/wallet/rpcwallet.cpp:1408 in c262be5f99 outdated

1205@@ -1206,6 +1206,17 @@ UniValue ListReceived(CWallet * const pwallet, const UniValue& params, bool fByA
1206         if(params[2].get_bool())
1207             filter = filter | ISMINE_WATCH_ONLY;
1208 
1209+    bool fFilterAddress = false;
1210+    CTxDestination filterAddress =  CNoDestination();
1211+    if (!fByAccounts && params.size() > 3) {

jonasschnelli commented at 7:49 pm on August 15, 2017:

Use a simpler check:

0CBitcoinAddress address(request.params[3].get_str());
1    if (!address.IsValid())

NicolasDorier commented at 11:13 pm on March 6, 2018:

This does not apply anymore.

in src/wallet/rpcwallet.cpp:1457 in c262be5f99 outdated

1260-        const std::string& strAccount = item.second.name;
1261-        std::map<CBitcoinAddress, tallyitem>::iterator it = mapTally.find(address);
1262+
1263+    // Create mapAddressBook iterator
1264+    // If we aren't filtering, go from begin() to end()
1265+    auto start = pwallet->mapAddressBook.begin();

jonasschnelli commented at 7:54 pm on August 15, 2017:

This loop-or-find logic seems to be a bit wired. I probably would have factored out the JSON object packing and either called the new function from within the range based loop or after the find().

jonasschnelli commented at 7:56 pm on August 15, 2017: contributor

Needs rebase.

Concept ACK. I think the by address filter is in general useful.

NicolasDorier commented at 4:19 am on August 17, 2017: contributor

@sipa just saw now your previous question. As far as TB is concerned, I am using balance tracking, UTXO management, and coin selection management of Bitcoin Core for funding the escrow transactions.

I am also using Core as a block explorer: There is some well known address on which I would like to be notified is money is sent to it, or sent from. This PR allows me to know when something is sent to the address. However, I still need a new rpc method for knowing if something is sent from an address.

Until I have the two features, my only way is to poll periodically listtransaction then fetch all transaction with gettransaction, and see if the relevant addresses are inside. This work well but is very cumbersome. It also does not scale well, if I do not cache the result of gettransaction.

We kind of talked about that in Tokyo, and you seemed to think that the best is that I develop my own wallet, unrelated to bitcoin RPC. I don’t like it but I think this is the way I will go for future projects.

in test/functional/receivedby.py:78 in c262be5f99 outdated

76+        res = self.nodes[1].listreceivedbyaddress(minconf=0, include_empty=True, include_watchonly=True, only_address=addr)
77+        assert_array_result(res, {"address":addr}, expected)
78+        assert_equal(len(res), 1)
79+        #Another address receive money
80+        res = self.nodes[1].listreceivedbyaddress(0, True, True)
81+        assert_equal(len(res), 3) #Right now 3 entries

luke-jr commented at 3:21 am on March 5, 2018:

Note, this needs to be 2->3 (instead of 3->4) due to removal of the “default address”.

luke-jr commented at 4:13 am on March 5, 2018: member

Rebased here: https://github.com/luke-jr/bitcoin/tree/listreceivedbyaddress-filtered

in src/wallet/rpcwallet.cpp:1411 in c262be5f99 outdated

1205@@ -1206,6 +1206,17 @@ UniValue ListReceived(CWallet * const pwallet, const UniValue& params, bool fByA
1206         if(params[2].get_bool())
1207             filter = filter | ISMINE_WATCH_ONLY;
1208 
1209+    bool fFilterAddress = false;
1210+    CTxDestination filterAddress =  CNoDestination();
1211+    if (!fByAccounts && params.size() > 3) {
1212+        filterAddress =  CBitcoinAddress(params[3].get_str()).Get();
1213+        CTxDestination nulladdress = CNoDestination();
1214+        if (filterAddress == nulladdress) {

promag commented at 5:52 pm on March 5, 2018:

Just if (filterAddress == CNoDestination())?

in src/wallet/rpcwallet.cpp:1409 in c262be5f99 outdated

1205@@ -1206,6 +1206,17 @@ UniValue ListReceived(CWallet * const pwallet, const UniValue& params, bool fByA
1206         if(params[2].get_bool())
1207             filter = filter | ISMINE_WATCH_ONLY;
1208 
1209+    bool fFilterAddress = false;
1210+    CTxDestination filterAddress =  CNoDestination();
1211+    if (!fByAccounts && params.size() > 3) {
1212+        filterAddress =  CBitcoinAddress(params[3].get_str()).Get();

promag commented at 5:52 pm on March 5, 2018:

Remove extra space.

in src/wallet/rpcwallet.cpp:1407 in c262be5f99 outdated

1205@@ -1206,6 +1206,17 @@ UniValue ListReceived(CWallet * const pwallet, const UniValue& params, bool fByA
1206         if(params[2].get_bool())
1207             filter = filter | ISMINE_WATCH_ONLY;
1208 
1209+    bool fFilterAddress = false;
1210+    CTxDestination filterAddress =  CNoDestination();

promag commented at 5:53 pm on March 5, 2018:

Remove extra space.

in src/rpc/client.cpp:46 in c262be5f99 outdated

43@@ -44,6 +44,7 @@ static const CRPCConvertParam vRPCConvertParams[] =
44     { "listreceivedbyaddress", 0, "minconf" },
45     { "listreceivedbyaddress", 1, "include_empty" },
46     { "listreceivedbyaddress", 2, "include_watchonly" },
47+    { "listreceivedbyaddress", 3, "only_address" },

promag commented at 5:54 pm on March 5, 2018:

Not sure what others think, but I kind of prefer filter_address.

in src/wallet/rpcwallet.cpp:1411 in c262be5f99 outdated

1210+    CTxDestination filterAddress =  CNoDestination();
1211+    if (!fByAccounts && params.size() > 3) {
1212+        filterAddress =  CBitcoinAddress(params[3].get_str()).Get();
1213+        CTxDestination nulladdress = CNoDestination();
1214+        if (filterAddress == nulladdress) {
1215+            throw JSONRPCError(RPC_WALLET_ERROR, "only_address parameter was invalid");

promag commented at 5:57 pm on March 5, 2018:

Missing test assert_raises_rpc_error.

in src/wallet/rpcwallet.cpp:1551 in c262be5f99 outdated

1352             "\nArguments:\n"
1353             "1. minconf           (numeric, optional, default=1) The minimum number of confirmations before payments are included.\n"
1354             "2. include_empty     (bool, optional, default=false) Whether to include addresses that haven't received any payments.\n"
1355             "3. include_watchonly (bool, optional, default=false) Whether to include watch-only addresses (see 'importaddress').\n"
1356-
1357+            "4. only_address      (string, optional) If present, only return information on this address. Otherwise, return all information.\n"

promag commented at 5:58 pm on March 5, 2018:

Drop Otherwise, return all information?

in src/wallet/rpcwallet.cpp:1468 in c262be5f99 outdated

1270+        if (start != end) {
1271+            end = std::next(start);
1272+        }
1273+    }
1274+
1275+    for(auto item_it = start; item_it != end; ++item_it)

promag commented at 5:59 pm on March 5, 2018:

Nit, space after for, { on this line.

promag commented at 6:02 pm on March 5, 2018: member

Some minor comments, new code could use current convention, for instance, s/filterAddress/filter_address.

NicolasDorier commented at 5:10 pm on March 6, 2018: contributor

Closing this I workedaround the need of this

NicolasDorier closed this on Mar 6, 2018

JeremyRubin commented at 5:51 pm on March 6, 2018: contributor

@NicolasDorier I think this should reopen, AFAIK LN people would like this too… @cfromknecht @Roasbeef

NicolasDorier reopened this on Mar 6, 2018

NicolasDorier commented at 6:13 pm on March 6, 2018: contributor

ok rebasing

NicolasDorier force-pushed on Mar 6, 2018

NicolasDorier commented at 11:10 pm on March 6, 2018: contributor

Rebased, fixed the nits of @luke-jr and @promag .

I renamed only_address to address_filter so that if later we decide it is also possible to pass an array of address instead of a single address, we don’t need to rename anything. It is more generic.

NicolasDorier force-pushed on Mar 6, 2018

in src/wallet/rpcwallet.cpp:1406 in 522c1196a9 outdated

1402@@ -1403,6 +1403,16 @@ UniValue ListReceived(CWallet * const pwallet, const UniValue& params, bool fByA
1403         if(params[2].get_bool())
1404             filter = filter | ISMINE_WATCH_ONLY;
1405 
1406+    bool fFilterAddress = false;

kallewoof commented at 4:45 am on March 7, 2018:

The code convention has changed a bit recently. Now, we don’t put the type as prefix, and we try to use snake case. I would rename this has_filter_address and the below to filter_address. You can leave existing vars as is, if you don’t want the diff to grow for no reason though (like fByAccounts below).

in src/wallet/rpcwallet.cpp:3869 in 522c1196a9 outdated

3865@@ -3837,7 +3866,7 @@ static const CRPCCommand commands[] =
3866     { "wallet",             "listaddressgroupings",             &listaddressgroupings,          {} },
3867     { "wallet",             "listlockunspent",                  &listlockunspent,               {} },
3868     { "wallet",             "listreceivedbyaccount",            &listreceivedbyaccount,         {"minconf","include_empty","include_watchonly"} },
3869-    { "wallet",             "listreceivedbyaddress",            &listreceivedbyaddress,         {"minconf","include_empty","include_watchonly"} },
3870+    { "wallet",             "listreceivedbyaddress",            &listreceivedbyaddress,         {"minconf","include_empty","include_watchonly","address_filter" } },

kallewoof commented at 4:50 am on March 7, 2018:

No space after " (end of line) – compare to other lines.

kallewoof approved

kallewoof commented at 4:50 am on March 7, 2018: member

utACK

NicolasDorier force-pushed on Mar 7, 2018

Add address filtering to listreceivedbyaddress 8ee08120de

Add tests of listreceivedbyaddress address filtering f087613719

NicolasDorier force-pushed on Mar 7, 2018

NicolasDorier commented at 1:31 pm on March 7, 2018: contributor

Addressed @kallewoof nits

kallewoof approved

kallewoof commented at 2:07 pm on March 7, 2018: member

utACK f087613719026bcd5cba95ec64c19361fcc71ecf

laanwj merged this on Mar 7, 2018

laanwj closed this on Mar 7, 2018

laanwj commented at 3:09 pm on March 7, 2018: member

utACK f08761371

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

in test/functional/wallet_listreceivedby.py:53 in 522c1196a9 outdated

51-                            {"address": addr},
52-                            {"address": addr, "account": "", "amount": 0, "confirmations": 0, "txids": []})
53+                            {"address": empty_addr},
54+                            {"address": empty_addr, "account": "", "amount": 0, "confirmations": 0, "txids": []})
55+
56+        #Test Address filtering

luke-jr commented at 3:16 pm on March 7, 2018:

Space after # is expected now it seems.

in test/functional/wallet_listreceivedby.py:55 in 522c1196a9 outdated

53+                            {"address": empty_addr},
54+                            {"address": empty_addr, "account": "", "amount": 0, "confirmations": 0, "txids": []})
55+
56+        #Test Address filtering
57+        #Only on addr
58+        expected = {"address":addr, "account":"", "amount":Decimal("0.1"), "confirmations":10, "txids":[txid,]}

luke-jr commented at 3:16 pm on March 7, 2018:

Space after : is expected now it seems.

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

listreceivedbyaddress Filter Address #9991