Fix coinbase transaction amount returned by filtertransactions

Currently, a transaction amount and fee are calculated by the
following algorithm:

* The 'fee' is the difference between a transaction's sum of outputs and
its sum of inputs.

* The 'amount' is the difference between the sum of its outputs and the
sum of those outputs that are sent to the node itself.

This makes sense for a regular transaction, however, for a coinbase Tx,
the amount will be zero, because all of its outputs are sent to the node
itself.

This commit special-cases coinbase Txs and sets the amount to the fee
itself, which is what most people expect to see.

Signed-off-by: Mihai Ciumeica <mihai@thirdhash.com>

src/Makefile.test.include

@@ -161,6 +161,7 @@ UNITE_TESTS += \
   test/validation_block_tests.cpp \
   test/waiter_tests.cpp \
   wallet/test/rpcvalidator_tests.cpp \
+  wallet/test/rpcwalletext_tests.cpp \
   wallet/test/accounting_tests.cpp \
   wallet/test/wallet_tests.cpp \
   wallet/test/crypto_tests.cpp

src/wallet/rpcwalletext.cpp

@@ -484,14 +484,13 @@ static void TxWithOutputsToJSON(const CWalletTx &wtx, CWallet *const pwallet,
   UniValue entry(UniValue::VOBJ);
 
   // GetAmounts variables
-  std::list<COutputEntry> listReceived;
-  std::list<COutputEntry> listSent;
-  std::list<COutputEntry> listStaked;
+  std::list<COutputEntry> list_received;
+  std::list<COutputEntry> list_sent;
   CAmount fee;
   CAmount amount = 0;
-  std::string strSentAccount;
+  std::string sent_account;
 
-  wtx.GetAmounts(listReceived, listSent, fee, strSentAccount, ISMINE_ALL);
+  wtx.GetAmounts(list_received, list_sent, fee, sent_account, ISMINE_ALL);
 
   if (wtx.IsFromMe(ISMINE_WATCH_ONLY) && !(watchonly & ISMINE_WATCH_ONLY)) {
     return;
@@ -502,87 +501,67 @@ static void TxWithOutputsToJSON(const CWalletTx &wtx, CWallet *const pwallet,
 
   UniValue outputs(UniValue::VARR);
   // common to every type of transaction
-  if (!strSentAccount.empty()) {
-    entry.pushKV("account", strSentAccount);
+  if (!sent_account.empty()) {
+    entry.pushKV("account", sent_account);
   }
   WalletTxToJSON(wtx, entry);
 
-  if (!listStaked.empty() || !listSent.empty()) {
+  if (!list_sent.empty()) {
     entry.pushKV("abandoned", wtx.isAbandoned());
   }
 
-  // UNIT-E: TODO: get staked transactions
-  if (!listStaked.empty()) {
-    if (!wtx.IsInMainChain()) {
-      entry.pushKV("category", "orphaned_stake");
-    } else {
-      entry.pushKV("category", "stake");
-    }
-    for (const auto &s : listStaked) {
+  std::set<int> receive_outputs;
+  for (const auto &r : list_received) {
+    receive_outputs.insert(r.vout);
+  }
+
+  // sent
+  if (!list_sent.empty()) {
+    entry.pushKV("fee", ValueFromAmount(-fee));
+    for (const auto &s : list_sent) {
       UniValue output(UniValue::VOBJ);
       if (!OutputToJSON(output, s, pwallet, wtx, watchonly)) {
         return;
       }
-
-      output.pushKV("amount", ValueFromAmount(s.amount));
-      outputs.push_back(output);
-    }
-
-    amount += -fee;
-  } else {
-    std::set<int> receivedOutputs;
-    for (const auto &r : listReceived) {
-      receivedOutputs.insert(r.vout);
-    }
-
-    // sent
-    if (!listSent.empty()) {
-      entry.pushKV("fee", ValueFromAmount(-fee));
-      for (const auto &s : listSent) {
-        UniValue output(UniValue::VOBJ);
-        if (!OutputToJSON(output, s, pwallet, wtx, watchonly)) {
-          return;
-        }
-        amount -= s.amount;
-        if (receivedOutputs.count(s.vout) == 0) {
-          output.pushKV("amount", ValueFromAmount(-s.amount));
-          outputs.push_back(output);
-        }
+      amount -= s.amount;
+      if (receive_outputs.count(s.vout) == 0) {
+        output.pushKV("amount", ValueFromAmount(-s.amount));
+        outputs.push_back(output);
       }
     }
+  }
 
-    // received
-    for (const auto &r : listReceived) {
-      UniValue output(UniValue::VOBJ);
-      if (!OutputToJSON(output, r, pwallet, wtx, watchonly)) {
-        return;
-      }
+  // received
+  for (const auto &r : list_received) {
+    UniValue output(UniValue::VOBJ);
+    if (!OutputToJSON(output, r, pwallet, wtx, watchonly)) {
+      return;
+    }
 
-      output.pushKV("amount", ValueFromAmount(r.amount));
-      amount += r.amount;
+    output.pushKV("amount", ValueFromAmount(r.amount));
+    amount += r.amount;
 
-      outputs.push_back(output);
-    }
+    outputs.push_back(output);
+  }
 
-    if (wtx.IsCoinBase()) {
-      if (!wtx.IsInMainChain()) {
-        entry.pushKV("category", "orphan");
-      } else if (wtx.GetBlocksToRewardMaturity() > 0) {
-        entry.pushKV("category", "immature");
-      } else {
-        entry.pushKV("category", "coinbase");
-      }
-    } else if (!fee) {
-      entry.pushKV("category", "receive");
-    } else if (amount == 0) {
-      if (listSent.empty()) {
-        entry.pushKV("fee", ValueFromAmount(-fee));
-      }
-      entry.pushKV("category", "internal_transfer");
+  if (wtx.IsCoinBase()) {
+    if (!wtx.IsInMainChain()) {
+      entry.pushKV("category", "orphan");
+    } else if (wtx.GetBlocksToRewardMaturity() > 0) {
+      entry.pushKV("category", "immature");
     } else {
-      entry.pushKV("category", "send");
+      entry.pushKV("category", "coinbase");
     }
-  };
+  } else if (!fee) {
+    entry.pushKV("category", "receive");
+  } else if (amount == 0) {
+    if (list_sent.empty()) {
+      entry.pushKV("fee", ValueFromAmount(-fee));
+    }
+    entry.pushKV("category", "internal_transfer");
+  } else {
+    entry.pushKV("category", "send");
+  }
 
   entry.pushKV("outputs", outputs);
   entry.pushKV("amount", ValueFromAmount(amount));

src/wallet/test/rpcwalletext_tests.cpp

@@ -0,0 +1,49 @@
+// Copyright (c) 2019 The Unit-e developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+
+#include <rpc/server.h>
+#include <script/standard.h>
+#include <test/rpc_test_utils.h>
+#include <validation.h>
+#include <wallet/rpcwalletext.h>
+#include <wallet/test/wallet_test_fixture.h>
+
+#include <boost/test/unit_test.hpp>
+
+
+// Retrieve details about a transaction with specified number of confirmations
+static UniValue FindByConfirmations(int confirmations) {
+  const UniValue transactions = CallRPC("filtertransactions {\"count\":0}");
+
+  const auto &values = transactions.getValues();
+  const auto it = std::find_if(values.begin(), values.end(), [&confirmations](const UniValue &x) {
+    return find_value(x, "confirmations").get_int64() == confirmations;
+  });
+
+  return *it;
+}
+
+
+BOOST_AUTO_TEST_SUITE(rpcwalletext_tests)
+
+BOOST_FIXTURE_TEST_CASE(genesis_block_coinbase, TestChain100Setup) {
+  const UniValue genesis_coinbase = FindByConfirmations(COINBASE_MATURITY + 1);
+
+  BOOST_CHECK_EQUAL(find_value(genesis_coinbase, "category").get_str(), "coinbase");
+
+  // The returned amount should equal the amount credited to us
+  BOOST_CHECK_EQUAL(find_value(genesis_coinbase, "amount").get_real(), 10000);
+}
+
+BOOST_FIXTURE_TEST_CASE(regular_coinbase, TestChain100Setup) {
+  const UniValue regular_coinbase = FindByConfirmations(1);
+
+  BOOST_CHECK_EQUAL(find_value(regular_coinbase, "category").get_str(), "immature");
+
+  // The amount should equal the block reward
+  BOOST_CHECK_EQUAL(find_value(regular_coinbase, "amount").get_real(), 3.75);
+}
+
+BOOST_AUTO_TEST_SUITE_END()

src/wallet/test/wallet_test_fixture.cpp

@@ -9,6 +9,7 @@
 #include <validation.h>
 #include <wallet/db.h>
 #include <wallet/rpcvalidator.h>
+#include <wallet/rpcwalletext.h>
 #include <consensus/merkle.h>
 
 WalletTestingSetup::WalletTestingSetup(const std::string& chainName)
@@ -34,6 +35,7 @@ WalletTestingSetup::WalletTestingSetup(std::function<void(Settings&)> f, const s
 
     RegisterWalletRPCCommands(tableRPC);
     RegisterValidatorRPCCommands(tableRPC);
+    RegisterWalletextRPCCommands(tableRPC);
 }
 
 WalletTestingSetup::~WalletTestingSetup()

src/wallet/wallet.cpp

@@ -1685,7 +1685,9 @@ void CWalletTx::GetAmounts(std::list<COutputEntry>& listReceived,
 
     // Compute fee:
     CAmount nDebit = GetDebit(filter);
-    if (nDebit > 0) { // debit>0 means we signed/sent this transaction
+    if (nDebit > 0 && !IsCoinBase()) {
+        // debit>0 means we signed/sent this transaction
+        // Coinbase transactions cannot have fees
         CAmount nValueOut = tx->GetValueOut();
         nFee = nDebit - nValueOut;
     }
@@ -1699,7 +1701,7 @@ void CWalletTx::GetAmounts(std::list<COutputEntry>& listReceived,
         //   2) the output is to us (received)
         if (nDebit > 0) {
             // Don't report 'change' txouts
-            if (pwallet->IsChange(txout)) {
+            if (!IsCoinBase() && pwallet->IsChange(txout)) {
                 continue;
             }
         } else if (!(fIsMine & filter)) {
@@ -1717,6 +1719,20 @@ void CWalletTx::GetAmounts(std::list<COutputEntry>& listReceived,
 
         COutputEntry output = {address, txout.nValue, (int)i};
 
+        if (IsCoinBase()) {
+            // TODO UNIT-E: implement a better way to distinguish reward outputs
+            // For example, current approach does not work in case of stake splitting
+            if (i == tx->vout.size() - 1) {
+                // We consider the last output (and only it) to be a non-reward output
+                if (nDebit > 0 && !(fIsMine & filter)) {
+                    listSent.push_back(output);
+                }
+            } else if (fIsMine & filter) {
+                listReceived.push_back(output);
+            }
+            continue;
+        }
+
         // If we are debited by the transaction, add the output as a "sent" entry
         if (nDebit > 0) {
             listSent.push_back(output);

test/functional/rpc_filtertransactions.py

@@ -3,7 +3,10 @@
 # Distributed under the MIT software license, see the accompanying
 # file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
-from test_framework.test_framework import UnitETestFramework
+from test_framework.test_framework import (
+    UnitETestFramework,
+    PROPOSER_REWARD,
+)
 from test_framework.util import *
 from test_framework.address import *
 
@@ -95,6 +98,16 @@ def test_output_format(self):
         )
         assert result_tx is None
 
+        # coinbase
+        sync_mempools(self.nodes)
+        self.nodes[0].generatetoaddress(1, self.nodes[0].getnewaddress())  # Clear node1's mempool
+        sync_blocks(self.nodes)
+
+        address = self.nodes[1].getnewaddress()
+        self.nodes[1].generatetoaddress(1, address)
+        tx = self.nodes[1].filtertransactions({"search": address})[0]
+        assert_equal(tx['amount'], PROPOSER_REWARD)
+
     def test_count_option(self):
         # count: -1 => JSONRPCException
         assert_raises_rpc_error(-8, "Invalid count", self.nodes[0].filtertransactions, {"count": -1})