Merge pull request #1277 from delta1/issues-1259

fix: assert failure on non-policy asset consolidation in CreateTransactionInternal

src/rpc/rawtransaction.cpp

@@ -972,7 +972,7 @@ static RPCHelpMan sendrawtransaction()
         //   will always be blinded and not explicit. In the former case, we
         //   error out because the transaction is not blinded properly.
         if (!out.nNonce.IsNull() && out.nValue.IsExplicit()) {
-            throw JSONRPCError(RPC_TRANSACTION_ERROR, "Transaction output has nonce, but is not blinded. Did you forget to call blindrawtranssaction, or rawblindrawtransaction?");
+            throw JSONRPCError(RPC_TRANSACTION_ERROR, "Transaction output has nonce, but is not blinded. Did you forget to call blindrawtransaction, or rawblindrawtransaction?");
         }
     }
 

src/wallet/coinselection.cpp

@@ -308,11 +308,18 @@ std::optional<SelectionResult> KnapsackSolver(std::vector<OutputGroup>& groups,
                 non_policy_effective_value += ic.effective_value;
             }
             result.AddInput(inner_result.value());
+        } else {
+            LogPrint(BCLog::SELECTCOINS, "Not enough funds to create target %d for asset %s\n", it->second, it->first.GetHex());
+            return std::nullopt;
         }
     }
 
     // Perform the standard Knapsack solver for the policy asset
-    CAmount policy_target = non_policy_effective_value + mapTargetValue.at(::policyAsset);
+    /*
+    NOTE:
+    CInputCoin::effective_value is negative for non-policy assets, so the sum non_policy_effective_value is negative. Therefore, it is subtracted in order to increase policy_target by the fees required.
+    */
+    CAmount policy_target = mapTargetValue.at(::policyAsset) - non_policy_effective_value;
     if (policy_target > 0) {
         inner_groups.clear();
 
@@ -346,6 +353,9 @@ std::optional<SelectionResult> KnapsackSolver(std::vector<OutputGroup>& groups,
 
         if (auto inner_result = KnapsackSolver(inner_groups, policy_target, ::policyAsset)) {
             result.AddInput(*inner_result);
+        } else {
+            LogPrint(BCLog::SELECTCOINS, "Not enough funds to create target %d for policy asset %s\n", policy_target, ::policyAsset.GetHex());
+            return std::nullopt;
         }
     }
 

src/wallet/receive.cpp

@@ -52,16 +52,35 @@ bool AllInputsMine(const CWallet& wallet, const CTransaction& tx, const isminefi
 }
 
 CAmountMap OutputGetCredit(const CWallet& wallet, const CTransaction& tx, const size_t out_index, const isminefilter& filter) {
-    CAmountMap nCredit;
-    if (wallet.IsMine(tx.vout[out_index]) & filter) {
-        CWalletTx wtx(MakeTransactionRef(std::move(tx)), TxStateInactive{});
-        CAmount credit = std::max<CAmount>(0, wtx.GetOutputValueOut(wallet, out_index));
-        if (!MoneyRange(credit))
-            throw std::runtime_error(std::string(__func__) + ": value out of range");
+    std::map<uint256, CWalletTx>::const_iterator mi = wallet.mapWallet.find(tx.GetHash());
+    if (mi != wallet.mapWallet.end())
+    {
+        const CWalletTx& wtx = (*mi).second;
+        if (out_index < wtx.tx->vout.size() && wallet.IsMine(wtx.tx->vout[out_index]) & filter) {
+            CAmountMap amounts;
+            amounts[wtx.GetOutputAsset(wallet, out_index)] = std::max<CAmount>(0, wtx.GetOutputValueOut(wallet, out_index));
+            return amounts;
+        }
+    }
+    return CAmountMap();
+}
 
-        nCredit[wtx.GetOutputAsset(wallet, out_index)] += credit;
-        if (!MoneyRange(nCredit))
-            throw std::runtime_error(std::string(__func__) + ": value out of range");
+CAmountMap TxGetCredit(const CWallet& wallet, const CWalletTx& wtx, const isminefilter& filter) {
+    CAmountMap nCredit;
+    {
+        LOCK(wallet.cs_wallet);
+
+        for (unsigned int i = 0; i < wtx.tx->vout.size(); ++i) {
+            if (wallet.IsMine(wtx.tx->vout[i]) & filter) {
+                CAmount credit = std::max<CAmount>(0, wtx.GetOutputValueOut(wallet, i));
+                if (!MoneyRange(credit))
+                    throw std::runtime_error(std::string(__func__) + ": value out of range");
+
+                nCredit[wtx.GetOutputAsset(wallet, i)] += credit;
+                if (!MoneyRange(nCredit))
+                    throw std::runtime_error(std::string(__func__) + ": value out of range");
+            }
+        }
     }
     return nCredit;
 }
@@ -126,7 +145,7 @@ static CAmountMap GetCachableAmount(const CWallet& wallet, const CWalletTx& wtx,
 {
     auto& amount = wtx.m_amounts[type];
     if (recalculate || !amount.m_cached[filter]) {
-        amount.Set(filter, type == CWalletTx::DEBIT ? wallet.GetDebit(*wtx.tx, filter) : TxGetCredit(wallet, *wtx.tx, filter));
+        amount.Set(filter, type == CWalletTx::DEBIT ? wallet.GetDebit(*wtx.tx, filter) : TxGetCredit(wallet, wtx, filter));
         wtx.m_is_cache_empty = false;
     }
     return amount.m_value[filter];
@@ -149,26 +168,6 @@ CAmountMap CachedTxGetCredit(const CWallet& wallet, const CWalletTx& wtx, const
     return credit;
 }
 
-CAmountMap TxGetCredit(const CWallet& wallet, const CTransaction& tx, const isminefilter& filter)
-{
-    {
-        LOCK(wallet.cs_wallet);
-        std::map<uint256, CWalletTx>::const_iterator mi = wallet.mapWallet.find(tx.GetHash());
-        if (mi != wallet.mapWallet.end())
-        {
-            const CWalletTx& wtx = (*mi).second;
-            for (size_t i = 0; i < wtx.tx->vout.size(); ++i) {
-                if (wallet.IsMine(wtx.tx->vout[i]) & filter) {
-                    CAmountMap amounts;
-                    amounts[wtx.GetOutputAsset(wallet, i)] = std::max<CAmount>(0, wtx.GetOutputValueOut(wallet, i));
-                    return amounts;
-                }
-            }
-        }
-    }
-    return CAmountMap();
-}
-
 CAmountMap CachedTxGetDebit(const CWallet& wallet, const CWalletTx& wtx, const isminefilter& filter)
     {
     if (wtx.tx->vin.empty())

src/wallet/spend.cpp

@@ -1529,7 +1529,12 @@ static bool CreateTransactionInternal(
 
     // The only time that fee_needed should be less than the amount available for fees (in change_and_fee - change_amount) is when
     // we are subtracting the fee from the outputs. If this occurs at any other time, it is a bug.
-    assert(coin_selection_params.m_subtract_fee_outputs || fee_needed <= map_change_and_fee.at(policyAsset) - change_amount);
+    if (!coin_selection_params.m_subtract_fee_outputs && fee_needed > map_change_and_fee.at(policyAsset) - change_amount) {
+        wallet.WalletLogPrintf("ERROR: not enough coins to cover for fee (needed: %d, total: %d, change: %d)\n",
+            fee_needed, map_change_and_fee.at(policyAsset), change_amount);
+        error = _("Could not cover fee");
+        return false;
+    }
 
     // Update nFeeRet in case fee_needed changed due to dropping the change output
     if (fee_needed <= map_change_and_fee.at(policyAsset) - change_amount) {

src/wallet/test/wallet_tests.cpp

@@ -359,8 +359,7 @@ BOOST_FIXTURE_TEST_CASE(coin_mark_dirty_immature_credit, TestChain100Setup)
     // credit amount is calculated.
     wtx.MarkDirty(wallet);
     AddKey(wallet, coinbaseKey);
-    // ELEMENTS: FIXME failing
-    // BOOST_CHECK_EQUAL(CachedTxGetImmatureCredit(wallet, wtx)[CAsset()], 50*COIN);
+    BOOST_CHECK_EQUAL(CachedTxGetImmatureCredit(wallet, wtx)[CAsset()], 50*COIN);
 }
 
 static int64_t AddTx(ChainstateManager& chainman, CWallet& wallet, uint32_t lockTime, int64_t mockTime, int64_t blockTime)

test/functional/example_elements_code_tutorial.py

@@ -0,0 +1,73 @@
+#!/usr/bin/env python3
+# Copyright (c) 2017-2020 The Bitcoin Core developers
+# Distributed under the MIT software license, see the accompanying
+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
+"""Tests reissuance functionality from the elements code tutorial
+See: https://elementsproject.org/elements-code-tutorial/reissuing-assets
+
+TODO: add functionality from contrib/assets_tutorial/assets_tutorial.py into here
+"""
+from test_framework.blocktools import COINBASE_MATURITY
+from test_framework.test_framework import BitcoinTestFramework
+
+class WalletTest(BitcoinTestFramework):
+    def set_test_params(self):
+        self.setup_clean_chain = True
+        self.num_nodes = 2
+        self.extra_args = [[
+            "-blindedaddresses=1",
+            "-initialfreecoins=2100000000000000",
+            "-con_blocksubsidy=0",
+            "-con_connect_genesis_outputs=1",
+            "-txindex=1",
+        ]] * self.num_nodes
+        self.extra_args[0].append("-anyonecanspendaremine=1")
+
+    def skip_test_if_missing_module(self):
+        self.skip_if_no_wallet()
+
+    def run_test(self):
+        self.generate(self.nodes[0], COINBASE_MATURITY + 1)
+        self.sync_all()
+
+        assert self.nodes[0].dumpassetlabels() == {'bitcoin': 'b2e15d0d7a0c94e4e2ce0fe6e8691b9e451377f6e46e8045a86f7c4b5d4f0f23'}
+
+        issuance = self.nodes[0].issueasset(100, 1)
+        asset = issuance['asset']
+        #token = issuance['token']
+        issuance_txid = issuance['txid']
+        issuance_vin = issuance['vin']
+
+        assert len(self.nodes[0].listissuances()) == 2  # asset & reisuance token
+
+        self.nodes[0].generatetoaddress(1, self.nodes[0].getnewaddress(), invalid_call=False)  # confirm the tx
+
+        issuance_addr = self.nodes[0].gettransaction(issuance_txid)['details'][0]['address']
+        self.nodes[1].importaddress(issuance_addr)
+
+        issuance_key = self.nodes[0].dumpissuanceblindingkey(issuance_txid, issuance_vin)
+        self.nodes[1].importissuanceblindingkey(issuance_txid, issuance_vin, issuance_key)
+        issuances = self.nodes[1].listissuances()
+        assert (issuances[0]['tokenamount'] == 1 and issuances[0]['assetamount'] == 100) \
+            or (issuances[1]['tokenamount'] == 1 and issuances[1]['assetamount'] == 100)
+
+        self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), 1)
+        self.generate(self.nodes[0], 10)
+
+        reissuance_tx = self.nodes[0].reissueasset(asset, 99)
+        reissuance_txid = reissuance_tx['txid']
+        issuances = self.nodes[0].listissuances(asset)
+        assert len(issuances) == 2
+        assert issuances[0]['isreissuance'] or issuances[1]['isreissuance']
+
+        self.generate(self.nodes[0], 1)
+
+        expected_amt = {
+            'bitcoin': 0,
+            '8f1560e209f6bcac318569a935a0b2513c54f326ee4820ccd5b8c1b1b4632373': 0,
+            '4fa41f2929d4bf6975a55967d9da5b650b6b9bfddeae4d7b54b04394be328f7f': 99
+        }
+        assert self.nodes[0].gettransaction(reissuance_txid)['amount'] == expected_amt
+
+if __name__ == '__main__':
+    WalletTest().main()

test/functional/test_runner.py

@@ -88,6 +88,7 @@ EXTENDED_SCRIPTS = [
 BASE_SCRIPTS = [
     # Scripts that are run by default.
     # vv First elements tests vv
+    'example_elements_code_tutorial.py',
     'feature_fedpeg.py --legacy-wallet',
     'feature_fedpeg.py --pre_transition --legacy-wallet',
     'feature_fedpeg.py --post_transition --legacy-wallet',
@@ -110,6 +111,7 @@ BASE_SCRIPTS = [
     'feature_progress.py',
     'rpc_getnewblockhex.py',
     'wallet_elements_regression_1172.py --legacy-wallet',
+    'wallet_elements_regression_1259.py --legacy-wallet',
     # Longest test should go first, to favor running tests in parallel
     'wallet_hd.py --legacy-wallet',
     'wallet_hd.py --descriptors',

test/functional/wallet_elements_regression_1259.py

@@ -0,0 +1,91 @@
+#!/usr/bin/env python3
+# Copyright (c) 2017-2020 The Bitcoin Core developers
+# Distributed under the MIT software license, see the accompanying
+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
+"""Tests that fundrawtransaction correctly handles the case of sending many
+inputs of an issued asset, with no policy asset recipient.
+
+See: https://github.com/ElementsProject/elements/issues/1259
+
+This test issues an asset and creates many utxos, which are then used as inputs in
+a consolidation transaction created with the various raw transaction calls.
+"""
+from decimal import Decimal
+
+from test_framework.blocktools import COINBASE_MATURITY
+from test_framework.test_framework import BitcoinTestFramework
+from test_framework.util import (
+    assert_equal,
+)
+
+class WalletTest(BitcoinTestFramework):
+    def set_test_params(self):
+        self.setup_clean_chain = True
+        self.num_nodes = 3
+        self.extra_args = [[
+            "-blindedaddresses=1",
+            "-initialfreecoins=2100000000000000",
+            "-con_blocksubsidy=0",
+            "-con_connect_genesis_outputs=1",
+            "-txindex=1",
+        ]] * self.num_nodes
+        self.extra_args[0].append("-anyonecanspendaremine=1")
+
+    def skip_test_if_missing_module(self):
+        self.skip_if_no_wallet()
+
+    def run_test(self):
+        self.generate(self.nodes[0], COINBASE_MATURITY + 1)
+        self.sync_all()
+
+        self.log.info(f"Send some policy asset to node 1")
+        self.nodes[0].sendtoaddress(self.nodes[1].getnewaddress(), 10)
+        self.generate(self.nodes[0], 1)
+
+        self.log.info(f"Issuing an asset from node 0")
+        issuance = self.nodes[0].issueasset(1000, 1, True)
+        self.generate(self.nodes[0], 1)
+        asset = issuance["asset"]
+        self.log.info(f"Asset ID is {asset}")
+
+        # create many outputs of the new asset on node 1
+        num_utxos = 45
+        value = 10
+        fee_rate = 10
+        self.log.info(f"Sending {num_utxos} utxos of asset to node 1")
+        for i in range(num_utxos):
+            self.nodes[0].sendtoaddress(self.nodes[1].getnewaddress(), value, "", "", False, False, None, None, None, asset, False, fee_rate, True)
+            self.generate(self.nodes[0], 1)
+
+        # create a raw tx on node 1 consolidating the asset utxos
+        self.log.info(f"Create the raw consolidation transaction")
+        hex = self.nodes[1].createrawtransaction([], [{ 'asset': asset, self.nodes[2].getnewaddress(): num_utxos * value }])
+
+        # fund the raw tx
+        self.log.info(f"Fund the raw transaction")
+        raw_tx = self.nodes[1].fundrawtransaction(hex, True)
+
+        # blind and sign the tx
+        self.log.info(f"Blind and sign the raw transaction")
+        hex = self.nodes[1].blindrawtransaction(raw_tx['hex'])
+        signed_tx = self.nodes[1].signrawtransactionwithwallet(hex)
+        assert_equal(signed_tx['complete'], True)
+
+        # decode tx
+        tx = self.nodes[1].decoderawtransaction(signed_tx['hex'])
+
+        assert_equal(len(tx['vin']), num_utxos + 1)
+        assert_equal(len(tx['vout']), 3)
+        assert_equal(tx['fee'], {'b2e15d0d7a0c94e4e2ce0fe6e8691b9e451377f6e46e8045a86f7c4b5d4f0f23': Decimal('0.00112380')}) # fee output
+
+        # send and mine the tx
+        self.log.info(f"Send the raw transaction")
+        self.nodes[1].sendrawtransaction(signed_tx['hex'])
+        self.generate(self.nodes[1], 1)
+        self.sync_all()
+        balance = self.nodes[2].getbalance()
+        assert_equal(balance[asset], Decimal(num_utxos * value))
+
+
+if __name__ == '__main__':
+    WalletTest().main()