Skip to content

Commit

Permalink
Merge 1abbae6 into merged_master (Bitcoin PR bitcoin/bitcoin#24584)
Browse files Browse the repository at this point in the history
This should be reviewed carefully. The new availablecoins_test was
showing an intermittent error [1]. The test variants were split out into
their own test cases to work around the issue, but requires further
investigation.

[1]: ElementsProject#1370
  • Loading branch information
delta1 committed Oct 24, 2024
2 parents 80c47aa + 1abbae6 commit f7b03ad
Show file tree
Hide file tree
Showing 11 changed files with 659 additions and 193 deletions.
1 change: 1 addition & 0 deletions src/Makefile.test.include
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,7 @@ BITCOIN_TESTS += \
wallet/test/wallet_crypto_tests.cpp \
wallet/test/wallet_transaction_tests.cpp \
wallet/test/coinselector_tests.cpp \
wallet/test/availablecoins_tests.cpp \
wallet/test/init_tests.cpp \
wallet/test/ismine_tests.cpp \
wallet/test/scriptpubkeyman_tests.cpp
Expand Down
6 changes: 3 additions & 3 deletions src/bench/coin_selection.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -58,10 +58,10 @@ static void CoinSelection(benchmark::Bench& bench)
addCoin(3 * COIN, wallet, wtxs);

// Create coins
std::vector<COutput> coins;
wallet::CoinsResult available_coins;
for (const auto& wtx : wtxs) {
const auto txout = wtx->tx->vout.at(0);
coins.emplace_back(COutPoint(wtx->GetHash(), 0), txout, /*depth=*/6 * 24, CalculateMaximumSignedInputSize(txout, &wallet, /*coin_control=*/nullptr), /*spendable=*/true, /*solvable=*/true, /*safe=*/true, wtx->GetTxTime(), /*from_me=*/true, /*fees=*/ 0);
available_coins.bech32.emplace_back(COutPoint(wtx->GetHash(), 0), txout, /*depth=*/6 * 24, CalculateMaximumSignedInputSize(txout, &wallet, /*coin_control=*/nullptr), /*spendable=*/true, /*solvable=*/true, /*safe=*/true, wtx->GetTxTime(), /*from_me=*/true, /*fees=*/ 0);
}

const CoinEligibilityFilter filter_standard(1, 6, 0);
Expand All @@ -80,7 +80,7 @@ static void CoinSelection(benchmark::Bench& bench)
bench.run([&] {
CAmountMap mapValue;
mapValue[::policyAsset] = 1003 * COIN;
auto result = AttemptSelection(wallet, mapValue, filter_standard, coins, coin_selection_params);
auto result = AttemptSelection(wallet, mapValue, filter_standard, available_coins, coin_selection_params, /*allow_mixed_output_types=*/true);
assert(result);
assert(result->GetSelectedValue() == mapValue);
assert(result->GetInputSet().size() == 2);
Expand Down
2 changes: 1 addition & 1 deletion src/wallet/rpc/coins.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -708,7 +708,7 @@ RPCHelpMan listunspent()
cctl.m_max_depth = nMaxDepth;
cctl.m_include_unsafe_inputs = include_unsafe;
LOCK(pwallet->cs_wallet);
vecOutputs = AvailableCoinsListUnspent(*pwallet, &cctl, nMinimumAmount, nMaximumAmount, nMinimumSumAmount, nMaximumCount, asset_filter.IsNull() ? nullptr : &asset_filter).coins;
vecOutputs = AvailableCoinsListUnspent(*pwallet, &cctl, nMinimumAmount, nMaximumAmount, nMinimumSumAmount, nMaximumCount, asset_filter.IsNull() ? nullptr : &asset_filter).all();
}

LOCK(pwallet->cs_wallet);
Expand Down
2 changes: 1 addition & 1 deletion src/wallet/rpc/spend.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1491,7 +1491,7 @@ RPCHelpMan sendall()
total_input_value += tx->tx->vout[input.prevout.n].nValue.GetAmount(); // ELEMENTS FIXME: is the unblinded value always available since it's in our wallet?
}
} else {
for (const COutput& output : AvailableCoins(*pwallet, &coin_control, fee_rate, /*nMinimumAmount=*/0).coins) {
for (const COutput& output : AvailableCoins(*pwallet, &coin_control, fee_rate, /*nMinimumAmount=*/0).all()) {
CHECK_NONFATAL(output.input_bytes > 0);
if (send_max && fee_rate.GetFee(output.input_bytes) > output.txout.nValue.GetAmount()) { // ELEMENTS FIXME: is the unblinded value always available since it's in our wallet?
continue;
Expand Down
180 changes: 141 additions & 39 deletions src/wallet/spend.cpp

Large diffs are not rendered by default.

71 changes: 56 additions & 15 deletions src/wallet/spend.h
Original file line number Diff line number Diff line change
Expand Up @@ -31,15 +31,39 @@ struct TxSize {
TxSize CalculateMaximumSignedTxSize(const CTransaction& tx, const CWallet* wallet, const std::vector<CTxOut>& txouts, const CCoinControl* coin_control = nullptr);
TxSize CalculateMaximumSignedTxSize(const CTransaction& tx, const CWallet* wallet, const CCoinControl* coin_control = nullptr);

/**
* COutputs available for spending, stored by OutputType.
* This struct is really just a wrapper around OutputType vectors with a convenient
* method for concatenating and returning all COutputs as one vector.
*
* clear(), size() methods are implemented so that one can interact with
* the CoinsResult struct as if it was a vector
*/
struct CoinsResult {
std::vector<COutput> coins;
// Sum of all the coins amounts
/** Vectors for each OutputType */
std::vector<COutput> legacy;
std::vector<COutput> P2SH_segwit;
std::vector<COutput> bech32;
std::vector<COutput> bech32m;

/** Other is a catch-all for anything that doesn't match the known OutputTypes */
std::vector<COutput> other;

/** Concatenate and return all COutputs as one vector */
std::vector<COutput> all() const;

/** The following methods are provided so that CoinsResult can mimic a vector,
* i.e., methods can work with individual OutputType vectors or on the entire object */
uint64_t size() const;
void clear();

/** Sum of all available coins */
// ELEMENTS: for each asset
CAmountMap total_amount;
};

/**
* Return vector of available COutputs.
* By default, returns only the spendable coins.
* Populate the CoinsResult struct with vectors of available COutputs, organized by OutputType.
*/
CoinsResult AvailableCoins(const CWallet& wallet,
const CCoinControl* coinControl = nullptr,
Expand Down Expand Up @@ -71,36 +95,53 @@ const CTxOut& FindNonChangeParentOutput(const CWallet& wallet, const COutPoint&
std::map<CTxDestination, std::vector<COutput>> ListCoins(const CWallet& wallet);

std::vector<OutputGroup> GroupOutputs(const CWallet& wallet, const std::vector<COutput>& outputs, const CoinSelectionParams& coin_sel_params, const CoinEligibilityFilter& filter, bool positive_only);
/**
* Attempt to find a valid input set that preserves privacy by not mixing OutputTypes.
* `ChooseSelectionResult()` will be called on each OutputType individually and the best
* the solution (according to the waste metric) will be chosen. If a valid input cannot be found from any
* single OutputType, fallback to running `ChooseSelectionResult()` over all available coins.
*
* param@[in] wallet The wallet which provides solving data for the coins
* param@[in] nTargetValue The target value
* param@[in] eligilibity_filter A filter containing rules for which coins are allowed to be included in this selection
* param@[in] available_coins The struct of coins, organized by OutputType, available for selection prior to filtering
* param@[in] coin_selection_params Parameters for the coin selection
* param@[in] allow_mixed_output_types Relax restriction that SelectionResults must be of the same OutputType
* returns If successful, a SelectionResult containing the input set
* If failed, a nullopt
*/
std::optional<SelectionResult> AttemptSelection(const CWallet& wallet, const CAmountMap& mapTargetValue, const CoinEligibilityFilter& eligibility_filter, const CoinsResult& available_coins,
const CoinSelectionParams& coin_selection_params, bool allow_mixed_output_types);

/**
* Attempt to find a valid input set that meets the provided eligibility filter and target.
* Multiple coin selection algorithms will be run and the input set that produces the least waste
* (according to the waste metric) will be chosen.
*
* param@[in] wallet The wallet which provides solving data for the coins
* param@[in] nTargetValue The target value
* param@[in] eligilibity_filter A filter containing rules for which coins are allowed to be included in this selection
* param@[in] coins The vector of coins available for selection prior to filtering
* param@[in] coin_selection_params Parameters for the coin selection
* returns If successful, a SelectionResult containing the input set
* If failed, a nullopt
* param@[in] wallet The wallet which provides solving data for the coins
* param@[in] nTargetValue The target value
* param@[in] eligilibity_filter A filter containing rules for which coins are allowed to be included in this selection
* param@[in] available_coins The struct of coins, organized by OutputType, available for selection prior to filtering
* param@[in] coin_selection_params Parameters for the coin selection
* returns If successful, a SelectionResult containing the input set
* If failed, a nullopt
*/
std::optional<SelectionResult> AttemptSelection(const CWallet& wallet, const CAmountMap& mapTargetValue, const CoinEligibilityFilter& eligibility_filter, std::vector<COutput> coins,
std::optional<SelectionResult> ChooseSelectionResult(const CWallet& wallet, const CAmountMap& mapTargetValue, const CoinEligibilityFilter& eligibility_filter, const std::vector<COutput>& available_coins,
const CoinSelectionParams& coin_selection_params);

/**
* Select a set of coins such that nTargetValue is met and at least
* all coins from coin_control are selected; never select unconfirmed coins if they are not ours
* param@[in] wallet The wallet which provides data necessary to spend the selected coins
* param@[in] vAvailableCoins The vector of coins available to be spent
* param@[in] available_coins The struct of coins, organized by OutputType, available for selection prior to filtering
* param@[in] nTargetValue The target value
* param@[in] coin_selection_params Parameters for this coin selection such as feerates, whether to avoid partial spends,
* and whether to subtract the fee from the outputs.
* returns If successful, a SelectionResult containing the selected coins
* If failed, a nullopt.
*/
std::optional<SelectionResult> SelectCoins(const CWallet& wallet, const std::vector<COutput>& vAvailableCoins, const CAmountMap& mapTargetValue, const CCoinControl& coin_control,
const CoinSelectionParams& coin_selection_params);
std::optional<SelectionResult> SelectCoins(const CWallet& wallet, CoinsResult& available_coins, const CAmountMap& mapTargetValue, const CCoinControl& coin_control,
const CoinSelectionParams& coin_selection_params) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);

struct CreatedTransactionResult
{
Expand Down
145 changes: 145 additions & 0 deletions src/wallet/test/availablecoins_tests.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,145 @@
// Copyright (c) 2022 The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or https://www.opensource.org/licenses/mit-license.php.

#include <validation.h>
#include <wallet/coincontrol.h>
#include <wallet/spend.h>
#include <wallet/test/util.h>
#include <wallet/test/wallet_test_fixture.h>

#include <boost/test/unit_test.hpp>

namespace wallet {
BOOST_FIXTURE_TEST_SUITE(availablecoins_tests, WalletTestingSetup)
class AvailableCoinsTestingSetup : public TestChain100Setup
{
public:
AvailableCoinsTestingSetup()
{
CreateAndProcessBlock({}, {});
wallet = CreateSyncedWallet(*m_node.chain, m_node.chainman->ActiveChain(), m_args, coinbaseKey);
}

~AvailableCoinsTestingSetup()
{
wallet.reset();
}
CWalletTx& AddTx(CRecipient recipient)
{
CTransactionRef tx;
CCoinControl dummy;
{
constexpr int RANDOM_CHANGE_POSITION = -1;
auto res = CreateTransaction(*wallet, {recipient}, RANDOM_CHANGE_POSITION, dummy);
BOOST_CHECK(res);
tx = res.GetObj().tx;
}
wallet->CommitTransaction(tx, {}, {});
CMutableTransaction blocktx;
{
LOCK(wallet->cs_wallet);
blocktx = CMutableTransaction(*wallet->mapWallet.at(tx->GetHash()).tx);
}
CreateAndProcessBlock({CMutableTransaction(blocktx)}, GetScriptForRawPubKey(coinbaseKey.GetPubKey()));

LOCK(wallet->cs_wallet);
wallet->SetLastBlockProcessed(wallet->GetLastBlockHeight() + 1, m_node.chainman->ActiveChain().Tip()->GetBlockHash());
auto it = wallet->mapWallet.find(tx->GetHash());
BOOST_CHECK(it != wallet->mapWallet.end());
it->second.m_state = TxStateConfirmed{m_node.chainman->ActiveChain().Tip()->GetBlockHash(), m_node.chainman->ActiveChain().Height(), /*index=*/1};
return it->second;
}

std::unique_ptr<CWallet> wallet;
};

// ELEMENTS FIXME: swapping these around so that bech32 is run first before bech32m
// fixes an intermittent issue with the transaction failing to validate its own signature,
// showing an "invalid schnorr signature" error. this requires further investigation.
// see https://github.com/ElementsProject/elements/issues/1370

BOOST_FIXTURE_TEST_CASE(BasicOutputTypesTestBech32m, AvailableCoinsTestingSetup)
{
CoinsResult available_coins;
BResult<CTxDestination> dest;
LOCK(wallet->cs_wallet);

// Verify our wallet has one usable coinbase UTXO before starting
// This UTXO is a P2PK, so it should show up in the Other bucket
available_coins = AvailableCoins(*wallet);
BOOST_CHECK_EQUAL(available_coins.size(), 1U);
BOOST_CHECK_EQUAL(available_coins.other.size(), 1U);

// We will create a self transfer for each of the OutputTypes and
// verify it is put in the correct bucket after running GetAvailablecoins
//
// For each OutputType, We expect 2 UTXOs in our wallet following the self transfer:
// 1. One UTXO as the recipient
// 2. One UTXO from the change, due to payment address matching logic


// Bech32m
dest = wallet->GetNewDestination(OutputType::BECH32M, "");
BOOST_ASSERT(dest.HasRes());
AddTx(CRecipient{{GetScriptForDestination(dest.GetObj())}, 1 * COIN, CAsset(), CPubKey(), /*fSubtractFeeFromAmount=*/true});
available_coins = AvailableCoins(*wallet);
BOOST_CHECK_EQUAL(available_coins.bech32m.size(), 2U);
}

BOOST_FIXTURE_TEST_CASE(BasicOutputTypesTestBech32, AvailableCoinsTestingSetup)
{
CoinsResult available_coins;
BResult<CTxDestination> dest;
LOCK(wallet->cs_wallet);

available_coins = AvailableCoins(*wallet);
BOOST_CHECK_EQUAL(available_coins.size(), 1U);
BOOST_CHECK_EQUAL(available_coins.other.size(), 1U);

// Bech32
dest = wallet->GetNewDestination(OutputType::BECH32, "");
BOOST_ASSERT(dest.HasRes());
AddTx(CRecipient{{GetScriptForDestination(dest.GetObj())}, 2 * COIN, CAsset(), CPubKey(), /*fSubtractFeeFromAmount=*/true});
available_coins = AvailableCoins(*wallet);
BOOST_CHECK_EQUAL(available_coins.bech32.size(), 2U);
}

BOOST_FIXTURE_TEST_CASE(BasicOutputTypesTestP2SHSegWit, AvailableCoinsTestingSetup)
{
CoinsResult available_coins;
BResult<CTxDestination> dest;
LOCK(wallet->cs_wallet);

available_coins = AvailableCoins(*wallet);
BOOST_CHECK_EQUAL(available_coins.size(), 1U);
BOOST_CHECK_EQUAL(available_coins.other.size(), 1U);

// P2SH-SEGWIT
dest = wallet->GetNewDestination(OutputType::P2SH_SEGWIT, "");
AddTx(CRecipient{{GetScriptForDestination(dest.GetObj())}, 3 * COIN, CAsset(), CPubKey(), /*fSubtractFeeFromAmount=*/true});
available_coins = AvailableCoins(*wallet);
BOOST_CHECK_EQUAL(available_coins.P2SH_segwit.size(), 2U);
}


BOOST_FIXTURE_TEST_CASE(BasicOutputTypesTestLegacy, AvailableCoinsTestingSetup)
{
CoinsResult available_coins;
BResult<CTxDestination> dest;
LOCK(wallet->cs_wallet);

available_coins = AvailableCoins(*wallet);
BOOST_CHECK_EQUAL(available_coins.size(), 1U);
BOOST_CHECK_EQUAL(available_coins.other.size(), 1U);

// Legacy (P2PKH)
dest = wallet->GetNewDestination(OutputType::LEGACY, "");
BOOST_ASSERT(dest.HasRes());
AddTx(CRecipient{{GetScriptForDestination(dest.GetObj())}, 4 * COIN, CAsset(), CPubKey(), /*fSubtractFeeFromAmount=*/true});
available_coins = AvailableCoins(*wallet);
BOOST_CHECK_EQUAL(available_coins.legacy.size(), 2U);
}

BOOST_AUTO_TEST_SUITE_END()
} // namespace wallet
Loading

0 comments on commit f7b03ad

Please sign in to comment.