rpc: fix showing wrong amount when not all inputs are from me in gettransaction #16199

1. 
   web3shannon commented at 3:25 am on June 13, 2019: contributor

   Fix #14136.

   In the rpc gettransaction, there is an error in calculating nFee.

   0CAmount nFee = (wtx.IsFromMe(filter) ? wtx.tx->GetValueOut() - nDebit : 0);
   

   If not all the inputs are from me (like the case of CoinJoin), wtx.IsFromMe(filter) returns true, and nFee would get a wrong value from wtx.tx->GetValueOut() - nDebit. Here nDebit only stands for sum of my inputs rather than all inputs, so in this case, nFee would equal to all outputs minus my inputs.

   A feasible solution is to use IsAllFromMe instead of IsFromMe. By this method, nFee is set as 0 and amount can be correctly calculated from nCredit - nDebit, aka, all my outputs minus all my inputs.

   The right amount and fee in the case of #14136 is successfully got through this way, as illustrated below. Here we use importaddress, aka the watch-only mode, to simulate the case.

   

2. DrahtBot added the label RPC/REST/ZMQ on Jun 13, 2019
3. DrahtBot added the label Wallet on Jun 13, 2019
4. 
   promag commented at 12:03 pm on June 13, 2019: member

   Can you add a test to exercise this change?
5. 
   web3shannon commented at 1:38 pm on June 13, 2019: contributor

   Sure, one or two days later.
6. fanquake added the label Waiting for author on Jun 13, 2019
7. 
   web3shannon commented at 7:10 am on June 14, 2019: contributor

   @promag Do you mean adding a rpc_gettransaction.py to test/functional?
8. 
   promag commented at 7:16 am on June 14, 2019: member

   I think you can extend and existing test, there are some that use gettransaction.
9. web3shannon force-pushed on Jun 17, 2019
10. 
    web3shannon commented at 5:38 am on June 17, 2019: contributor

    The test is added.
11. fanquake removed the label Waiting for author on Jun 17, 2019
12. 
    DrahtBot commented at 6:15 am on June 17, 2019: member

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #16185 (gettransaction: add an argument to decode the transaction by darosior)

    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. in test/functional/wallet_basic.py:518 in 33c3d84d6d outdated

    513+        outputs = [{output_address_0: 49}, {output_address_1: 0.99}]
    514+        tx = self.nodes[0].createrawtransaction(inputs, outputs)
    515+        tx = self.nodes[0].signrawtransactionwithkey(tx, [input_privkey_0, input_privkey_1])['hex']
    516+        txid = self.nodes[0].sendrawtransaction(tx)
    517+        tx_obj = self.nodes[0].gettransaction(txid)
    518+        assert_equal(tx_obj['amount'], Decimal('24.00000000'))
    

    ---

    ---

    

    promag commented at 2:41 pm on June 17, 2019:

    Could also assert key trusted = false.

    ---

    

    web3shannon commented at 8:46 am on June 19, 2019:

    OK. Will add more assert_equals.
14. in test/functional/wallet_basic.py:509 in 33c3d84d6d outdated

    504+        input_address_1 = self.nodes[1].getnewaddress()
    505+        input_privkey_0 = self.nodes[0].dumpprivkey(input_address_0)
    506+        input_privkey_1 = self.nodes[1].dumpprivkey(input_address_1)
    507+        output_address_0 = self.nodes[0].getnewaddress()
    508+        output_address_1 = self.nodes[1].getnewaddress()
    509+        txid_0 = self.nodes[0].getblock(self.nodes[0].generatetoaddress(1, input_address_0)[0])['tx'][0]
    

    ---

    ---

    

    promag commented at 2:42 pm on June 17, 2019:

    At this point self.nodes[1] has some UTXO that can be used instead of generating more blocks.

    ---

    

    web3shannon commented at 8:38 am on June 19, 2019:

    Obtaining txids, amounts, addresses of those existing UTXOs would make the code much more complex. Is it really necessary?

    ---

    

    promag commented at 8:54 am on June 19, 2019:

    Just listunspent?
15. 
    promag commented at 2:43 pm on June 17, 2019: member

    This looks incomplete, please see details in gettransaction result - it shows fee 24.99.
16. 
    web3shannon commented at 8:39 am on June 19, 2019: contributor

    @promag Yes, I’ll fix the details part.
17. rpc: fix showing wrong amount when not all inputs are from me in gettransaction; see #14136 610fe574cd
18. web3shannon force-pushed on Jun 25, 2019
19. 
    web3shannon commented at 6:02 am on June 25, 2019: contributor

    The details is fixed and the test is modified. @promag
20. 
    instagibbs commented at 1:47 am on July 16, 2019: member

    Before gathering more review I think this will need some concept ACKs.

    For instance, I’m not sure returning a fee of 0 is the right thing here in cases of !IsAllFromMe? Clearly the previous behavior is odd/wrong, but we might want to re-think what information this is trying to convey first.

21. fanquake added the label Needs Conceptual Review on Jul 16, 2019
22. 
    luke-jr commented at 11:23 pm on August 19, 2019: member

    I’m pretty sure it’s impossible to do the right thing for CoinJoins here without extra metadata.

    We can’t just blindly add all the outputs as “sent”, because we only sent some of them - but we have no way to know which one(s).

23. DrahtBot added the label Needs rebase on Sep 2, 2019
24. 
    DrahtBot commented at 11:54 am on September 2, 2019: member
25. 
    adamjonas commented at 5:41 pm on June 5, 2020: member

    Hi @shannon1916 - It looks like this is still lacking concept ACKs. Is this something that you plan on continuing to pursue? #19002 and #18919 are also relevant and maybe you can piggyback off those approaches.
26. 
    shesek commented at 11:24 pm on March 5, 2021: contributor

    I’m not sure returning a fee of 0 is the right thing here

    I agree, I think it should be a null to differentiate from transactions that are actually paying no fee.

    It would be great to see this issue get fixed.

27. in src/wallet/wallet.cpp:1776 in 610fe574cd

    1772@@ -1772,7 +1773,7 @@ void CWalletTx::GetAmounts(std::list<COutputEntry>& listReceived,
    1773         COutputEntry output = {address, txout.nValue, (int)i};
    1774 
    1775         // If we are debited by the transaction, add the output as a "sent" entry
    1776-        if (nDebit > 0)
    1777+        if (fIsAllFromMe)
    

    ---

    ---

    

    shesek commented at 11:24 pm on March 5, 2021:

    Does this mean that a transaction where some inputs are wallet related and none of the outputs are won’t get registered as either ‘sent’ or ‘received’?

    Edit: Actually, it seems like these transactions will already get skipped sooner, in the continue above on line 1761.

28. 
    DrahtBot commented at 11:22 am on December 15, 2021: member

    • Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
    • Is it no longer relevant? ➡️ Please close.
    • Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.
29. 
    DrahtBot commented at 1:07 pm on March 21, 2022: member

    • Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
    • Is it no longer relevant? ➡️ Please close.
    • Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.
30. MarcoFalke closed this on Mar 21, 2022

    ---

31. 
    MarcoFalke commented at 1:16 pm on March 21, 2022: member

    Closing for now. See #14136 (comment)
32. DrahtBot locked this on Mar 21, 2023

Contributors
web3shannon promag DrahtBot instagibbs luke-jr adamjonas shesek MarcoFalke

Labels
Wallet RPC/REST/ZMQ Needs rebase Needs Conceptual Review