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

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

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.

DrahtBot added the label RPC/REST/ZMQ on Jun 13, 2019

DrahtBot added the label Wallet on Jun 13, 2019

promag commented at 12:03 pm on June 13, 2019: member

Can you add a test to exercise this change?

web3shannon commented at 1:38 pm on June 13, 2019: contributor

Sure, one or two days later.

fanquake added the label Waiting for author on Jun 13, 2019

web3shannon commented at 7:10 am on June 14, 2019: contributor

@promag Do you mean adding a rpc_gettransaction.py to test/functional?

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.

web3shannon force-pushed on Jun 17, 2019

web3shannon commented at 5:38 am on June 17, 2019: contributor

The test is added.

fanquake removed the label Waiting for author on Jun 17, 2019

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.

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.

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?

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.

web3shannon commented at 8:39 am on June 19, 2019: contributor

@promag Yes, I’ll fix the details part.

rpc: fix showing wrong amount when not all inputs are from me in gettransaction; see #14136 610fe574cd

web3shannon force-pushed on Jun 25, 2019

web3shannon commented at 6:02 am on June 25, 2019: contributor

The details is fixed and the test is modified. @promag

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.

fanquake added the label Needs Conceptual Review on Jul 16, 2019

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

DrahtBot added the label Needs rebase on Sep 2, 2019

DrahtBot commented at 11:54 am on September 2, 2019: member

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.

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.

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.

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.

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.

MarcoFalke closed this on Mar 21, 2022

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

Closing for now. See #14136 (comment)

DrahtBot locked this on Mar 21, 2023

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

Conflicts