From 2e7125c3c8d4ea3758f23b9257b2b3f7c715f136 Mon Sep 17 00:00:00 2001 From: goatpig <moothecowlord@gmail.com> Date: Wed, 16 Aug 2023 21:15:05 +0200 Subject: [PATCH 1/6] fix: assert failure on non-policy asset consolidation in CreateTransactionInternal Squash of 2 commits from https://github.com/ElementsProject/elements/pull/1258 - correct application of non_policy_effective_value to policy output in KnapsackSolver - replace bad fee amount assert with error log and graceful failure in CWallet::CreateTransactionInternal (cherry picked from commit 0f92a38254bf4049ad6466d188b99c68a5e68906) Update src/wallet/coinselection.cpp Co-authored-by: Byron Hambly <byron@hambly.dev> (cherry picked from commit cf0f56107be24c0272df08c449444d47b92a9c15) --- src/wallet/coinselection.cpp | 6 +++++- src/wallet/spend.cpp | 7 ++++++- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/src/wallet/coinselection.cpp b/src/wallet/coinselection.cpp index 3449bd4262..41ed9acb46 100644 --- a/src/wallet/coinselection.cpp +++ b/src/wallet/coinselection.cpp @@ -312,7 +312,11 @@ std::optional<SelectionResult> KnapsackSolver(std::vector<OutputGroup>& groups, } // 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(); diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index ff508ca364..65a7a6f228 100644 --- a/src/wallet/spend.cpp +++ b/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) { From d3ffd2a380f036a8ee3f98203e16edefc1a3afc7 Mon Sep 17 00:00:00 2001 From: Byron Hambly <bhambly@blockstream.com> Date: Fri, 18 Aug 2023 15:47:10 +0200 Subject: [PATCH 2/6] test: add functional test for issue #1259 Adds a functional test to cover the issue uncovered in #1259, where calling fundrawtransaction with many non-policy inputs and no policy recipients results in an assertion failure and a crash. Fixed in #1258. (cherry picked from commit a8b0ed6f96226556ed9e397b204e1adcd4f65765) --- src/rpc/rawtransaction.cpp | 2 +- test/functional/test_runner.py | 1 + .../wallet_elements_regression_1259.py | 91 +++++++++++++++++++ 3 files changed, 93 insertions(+), 1 deletion(-) create mode 100755 test/functional/wallet_elements_regression_1259.py diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index 2bd8914716..1881bb12f7 100644 --- a/src/rpc/rawtransaction.cpp +++ b/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?"); } } diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index dd0fc8fbd0..d282c81b4f 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -110,6 +110,7 @@ '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', diff --git a/test/functional/wallet_elements_regression_1259.py b/test/functional/wallet_elements_regression_1259.py new file mode 100755 index 0000000000..88e9c3bb8c --- /dev/null +++ b/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 correcly 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() From 5b72ae23afcb9a2d21a3ce7e1cfc40e104e9607b Mon Sep 17 00:00:00 2001 From: James Dorfman <dorfmanjames@gmail.com> Date: Mon, 23 Oct 2023 14:31:37 +0000 Subject: [PATCH 3/6] coinselection: fail if each non-policy asset doesn't have a solution --- src/wallet/coinselection.cpp | 3 +++ test/functional/wallet_elements_regression_1259.py | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/src/wallet/coinselection.cpp b/src/wallet/coinselection.cpp index 41ed9acb46..9d8ad24be0 100644 --- a/src/wallet/coinselection.cpp +++ b/src/wallet/coinselection.cpp @@ -308,6 +308,9 @@ 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; } } diff --git a/test/functional/wallet_elements_regression_1259.py b/test/functional/wallet_elements_regression_1259.py index 88e9c3bb8c..e6499d443e 100755 --- a/test/functional/wallet_elements_regression_1259.py +++ b/test/functional/wallet_elements_regression_1259.py @@ -2,7 +2,7 @@ # 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 correcly handles the case of sending many +"""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 From 04cba29faf6383d99a47d373c58cc52eacd3a4ad Mon Sep 17 00:00:00 2001 From: James Dorfman <dorfmanjames@gmail.com> Date: Mon, 23 Oct 2023 15:48:33 +0000 Subject: [PATCH 4/6] coinselection: fail KnapsackSolver if the policy asset doesn't have a solution --- src/wallet/coinselection.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/wallet/coinselection.cpp b/src/wallet/coinselection.cpp index 9d8ad24be0..7f337bfdb4 100644 --- a/src/wallet/coinselection.cpp +++ b/src/wallet/coinselection.cpp @@ -353,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; } } From 39be0f092554399a8a89505ecd8f04383d59ff86 Mon Sep 17 00:00:00 2001 From: James Dorfman <dorfmanjames@gmail.com> Date: Wed, 25 Oct 2023 16:50:12 +0000 Subject: [PATCH 5/6] wallet: fix tx credit calculation issues introduced in merge of bitcoin/bitcoin#22100 in 3e939cca and 103069c4 --- src/wallet/receive.cpp | 59 ++++++++++++++++---------------- src/wallet/test/wallet_tests.cpp | 3 +- 2 files changed, 30 insertions(+), 32 deletions(-) diff --git a/src/wallet/receive.cpp b/src/wallet/receive.cpp index 8e43f3f537..20a8c61376 100644 --- a/src/wallet/receive.cpp +++ b/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) { + 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(); +} + +CAmountMap TxGetCredit(const CWallet& wallet, const CWalletTx& wtx, 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"); - - nCredit[wtx.GetOutputAsset(wallet, out_index)] += credit; - if (!MoneyRange(nCredit)) - throw std::runtime_error(std::string(__func__) + ": value out of range"); + { + 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()) diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index 982cc95fbe..27291eb987 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/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) From e02024e53fa1a7501222f868a7524a1259186071 Mon Sep 17 00:00:00 2001 From: James Dorfman <dorfmanjames@gmail.com> Date: Wed, 25 Oct 2023 17:02:35 +0000 Subject: [PATCH 6/6] test: add new elements_code_tutorial functional test --- .../example_elements_code_tutorial.py | 73 +++++++++++++++++++ test/functional/test_runner.py | 1 + 2 files changed, 74 insertions(+) create mode 100755 test/functional/example_elements_code_tutorial.py diff --git a/test/functional/example_elements_code_tutorial.py b/test/functional/example_elements_code_tutorial.py new file mode 100755 index 0000000000..feb645f1f1 --- /dev/null +++ b/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() diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index d282c81b4f..028beb661a 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -88,6 +88,7 @@ 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',