rpcwallet: listsentbyaddress RPC #14498

1. 
   rodentrabies commented at 10:17 pm on October 16, 2018: contributor

   This implements a proposal in #10599.

   Complementary method to listreceivedbyaddress that can be useful for statistics gathering and various layer 2 protocols like TumbleBit (ping @NicolasDorier) and authentication protocols I’m currently working on.

   Since this replaces ListReceived with ListByAddress parameterized with received/sent flag, other methods like listsentbylabel (complementary to listreceivedbylabel) and getsentbyaddress (complementary to getreceivedbyaddress) can be implemented as well.

2. rpcwallet: implement 'listsentbyaddress' RPC afd2f777bd
3. rodentrabies force-pushed on Oct 16, 2018
4. fanquake added the label Wallet on Oct 17, 2018
5. fanquake added the label RPC/REST/ZMQ on Oct 17, 2018
6. 
   NicolasDorier commented at 4:47 am on October 17, 2018: contributor

   Concept ACK I needed that long ago.
7. 
   sipa commented at 7:03 am on October 17, 2018: member

   NACK, transactions don’t have from addresses in the model our wallet exposes.

   It may make sense to have a separate simpler transaction watcher, which is similar to the wallet, but doesn’t have all the accounting logic. However, for our current wallet, I believe this is feature creep and confusing.

8. 
   rodentrabies commented at 8:13 am on October 17, 2018: contributor

   Fair point on feature creep:) I opened this more as a continuation of discussion in #10599 than as a new feature to merge, so please close the PR if there are no strong arguments for this.

   By a separate simpler transaction watcher you mean a part of core or an external tool?

9. in src/wallet/rpcwallet.cpp:1193 in afd2f777bd

   1194+        else
   1195+        {
   1196+            for (const CTxIn& txin : wtx.tx->vin)
   1197+            {
   1198+                const COutPoint& prevout = txin.prevout;
   1199+                const CTxOut& txout = pwallet->GetWalletTx(prevout.hash)->tx->vout[prevout.n];
   

   ---

   ---

   

   promag commented at 3:28 pm on October 17, 2018:

   Are you sure about this?

   ---

   

   rodentrabies commented at 5:24 pm on October 17, 2018:

   Well, after @sipa’s comment I’m not sure about this PR in general, but I adapted some tests from rpc_listreceivedby.py and it seems to be working. I may be missing something deep here though.

   ---

   

   promag commented at 0:26 am on October 18, 2018:

   There are no tests here. Anyway CWallet::GetWalletTx only returns wallet transactions and so this would fail when wtx in an incoming transaction.

   ---

   

   rodentrabies commented at 11:38 am on October 18, 2018:

   I assumed there’s no need for tests since this got a NACK.

   I was going to check it with tests, but if I undersand it correctly, since we’re interested only in wallet transactions, checking if result of GetWalletTx is non-nullptrwould be enough?

   0                const CWalletTx* iwtx = pwallet->GetWalletTx(prevout.hash);
   1                if (!iwtx) continue;
   2                const CTxOut& txout = iwtx->tx->vout[prevout.n];
   

   Or am I missing something again?

   ---

   

   promag commented at 11:48 am on October 18, 2018:

   I think that is correct.
10. in src/wallet/rpcwallet.cpp:1202 in afd2f777bd

    1206+                }
    1207 
    1208-            isminefilter mine = IsMine(*pwallet, address);
    1209-            if(!(mine & filter))
    1210-                continue;
    1211+                isminefilter mine = IsMine(*pwallet, address);
    

    ---

    ---

    

    promag commented at 11:55 am on October 18, 2018:

    This can be avoided when has_filtered_address:

    above before the loop:

    0const isminefilter is_mine_filtered_address = has_filtered_address ? IsMine(*pwallet, filtered_address) : ISMINE_NO;
    

    and here:

    0isminefilter mine = has_filtered_address ? is_mine_filtered_address : IsMine(*pwallet, address);
    

11. 
    promag commented at 12:04 pm on October 18, 2018: member

    I agree with the NACK. Like @sipa said, this doesn’t fit the wallet model.

    Also note that this doesn’t perform well especially for big wallets.

12. 
    DrahtBot commented at 9:54 am on October 20, 2018: member

    • #14437 (Refactor: Start to separate wallet from node by ryanofsky)
    • #11634 (wallet: Add missing cs_wallet/cs_KeyStore locks to wallet by practicalswift)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

13. DrahtBot added the label Needs rebase on Oct 24, 2018
14. 
    DrahtBot commented at 9:40 am on October 24, 2018: member
15. 
    promag commented at 6:18 pm on November 4, 2018: member

    @mrwhythat agree in closing?
16. 
    rodentrabies commented at 6:44 pm on November 4, 2018: contributor

    @promag, yup, closing.
17. rodentrabies closed this on Nov 4, 2018

    ---

18. laanwj removed the label Needs rebase on Oct 24, 2019
19. rodentrabies deleted the branch on Mar 15, 2021
20. DrahtBot locked this on Aug 18, 2022

Contributors
rodentrabies NicolasDorier sipa promag DrahtBot

Labels
Wallet RPC/REST/ZMQ