listreceivedbyaddress Filter Address #9991

NicolasDorier · Mar 14, 2017

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

NicolasDorier · Mar 14, 2017

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

kallewoof · Mar 14, 2017

kallewoof approved these changes Mar 14, 2017

View changes

utACK 8c373d5

NicolasDorier · Mar 16, 2017

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

jnewbery · Mar 16, 2017

jnewbery reviewed Mar 16, 2017

View changes

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

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

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

jnewbery · Mar 16, 2017

jnewbery reviewed Mar 16, 2017

View changes

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.

jnewbery · Mar 16, 2017

jnewbery reviewed Mar 16, 2017

View changes

minor nit: align parentheses with line above.

jnewbery · Mar 16, 2017

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

JeremyRubin · Mar 16, 2017

utAck

NicolasDorier · Mar 17, 2017

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

NicolasDorier · Mar 17, 2017

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

jnewbery · Mar 17, 2017

line 73 minConf needs to be minconf.

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

NicolasDorier · Mar 18, 2017

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

EthanHeilman · Mar 21, 2017

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

NicolasDorier · Mar 22, 2017

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

nopara73 · May 25, 2017

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

NicolasDorier · Jun 14, 2017

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

jnewbery · Jun 14, 2017

😞 I'm happy to reACK if you decide to reopen this!

NicolasDorier · Jun 15, 2017

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

[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 · Jun 16, 2017

@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 · Jun 17, 2017

@sipa Need the wallet functionality for NTumbleBit.

sipa · Jun 17, 2017

Can you clarify what functionality it relies on?

nopara73 · Jun 17, 2017

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

jonasschnelli · Aug 15, 2017

jonasschnelli reviewed Aug 15, 2017

View changes

Use a simpler check:

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

jonasschnelli · Aug 15, 2017

jonasschnelli reviewed Aug 15, 2017

View changes

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 · Aug 15, 2017

Needs rebase.

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

NicolasDorier · Aug 17, 2017

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

MarcoFalke referenced this pull request Mar 14, 2017
Closed
listreceivedbyaddress Filter Address #9503

fanquake added RPC/REST/ZMQ Wallet labels Mar 14, 2017

JeremyRubin added some commits Jan 9, 2017

Add address filtering to listreceivedbyaddress

e5060ab

Add tests of listreceivedbyaddress address filtering

c262be5

nopara73 referenced this pull request in NTumbleBit/NTumbleBit May 25, 2017
Closed
Tumbling is very slow, lot's of Client's redeem broadcasted #34

NicolasDorier closed this Jun 14, 2017

NicolasDorier reopened this Jun 15, 2017

jnewbery referenced this pull request Jun 16, 2017
Open
RPC: Add parameter to addmultisigaddress / createmultisig to sort public keys #8751

This was referenced Sep 6, 2017

Merged

Prepare for non-Base58 addresses #11117

Closed

Add != to Base58 class #11253

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

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

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

bitcoin/bitcoin

Join GitHub today

listreceivedbyaddress Filter Address #9991

Conversation

NicolasDorier commented Mar 14, 2017 • edited Edited 1 time NicolasDorier Mar 14, 2017

MarcoFalke referenced this pull request Mar 14, 2017

NicolasDorier commented Mar 14, 2017

kallewoof approved these changes Mar 14, 2017 View changes

fanquake added RPC/REST/ZMQ Wallet labels Mar 14, 2017

NicolasDorier commented Mar 16, 2017 • edited Edited 1 time NicolasDorier Mar 16, 2017

jnewbery reviewed Mar 16, 2017 View changes

NicolasDorier Mar 17, 2017 • edited Edited 1 time NicolasDorier Mar 17, 2017

jnewbery reviewed Mar 16, 2017 View changes

jnewbery reviewed Mar 16, 2017 View changes

jnewbery commented Mar 16, 2017

JeremyRubin commented Mar 16, 2017

NicolasDorier commented Mar 17, 2017

NicolasDorier commented Mar 17, 2017

jnewbery commented Mar 17, 2017 • edited Edited 1 time jnewbery Mar 17, 2017

NicolasDorier commented Mar 18, 2017 • edited Edited 1 time NicolasDorier Mar 18, 2017

JeremyRubin added some commits Jan 9, 2017

EthanHeilman commented Mar 21, 2017

NicolasDorier commented Mar 22, 2017

nopara73 commented May 25, 2017

nopara73 referenced this pull request in NTumbleBit/NTumbleBit May 25, 2017

NicolasDorier commented Jun 14, 2017 • edited Edited 1 time NicolasDorier Jun 14, 2017

NicolasDorier closed this Jun 14, 2017

jnewbery commented Jun 14, 2017

NicolasDorier reopened this Jun 15, 2017

NicolasDorier commented Jun 15, 2017 • edited Edited 1 time NicolasDorier Jun 15, 2017

jnewbery referenced this pull request Jun 16, 2017

sipa commented Jun 16, 2017

nopara73 commented Jun 17, 2017

sipa commented Jun 17, 2017

nopara73 commented Jun 17, 2017

jonasschnelli reviewed Aug 15, 2017 View changes

jonasschnelli reviewed Aug 15, 2017 View changes

jonasschnelli commented Aug 15, 2017

NicolasDorier commented Aug 17, 2017 • edited Edited 1 time NicolasDorier Aug 17, 2017

This was referenced Sep 6, 2017

NicolasDorier commented Mar 14, 2017 •

Edited 1 time

NicolasDorier
Mar 14, 2017

kallewoof approved these changes Mar 14, 2017

View changes

NicolasDorier commented Mar 16, 2017 •

Edited 1 time

NicolasDorier
Mar 16, 2017

jnewbery reviewed Mar 16, 2017

View changes

NicolasDorier Mar 17, 2017 •

Edited 1 time

NicolasDorier
Mar 17, 2017

jnewbery reviewed Mar 16, 2017

View changes

jnewbery reviewed Mar 16, 2017

View changes

jnewbery commented Mar 17, 2017 •

Edited 1 time

jnewbery
Mar 17, 2017

NicolasDorier commented Mar 18, 2017 •

Edited 1 time

NicolasDorier
Mar 18, 2017

NicolasDorier commented Jun 14, 2017 •

Edited 1 time

NicolasDorier
Jun 14, 2017

NicolasDorier commented Jun 15, 2017 •

Edited 1 time

NicolasDorier
Jun 15, 2017

jonasschnelli reviewed Aug 15, 2017

View changes

jonasschnelli reviewed Aug 15, 2017

View changes

NicolasDorier commented Aug 17, 2017 •

Edited 1 time

NicolasDorier
Aug 17, 2017