Skip to content

Commit f7b03ad

Browse files
committed
Merge 1abbae6 into merged_master (Bitcoin PR bitcoin/bitcoin#24584)
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
2 parents 80c47aa + 1abbae6 commit f7b03ad

11 files changed

+659
-193
lines changed

src/Makefile.test.include

+1
Original file line numberDiff line numberDiff line change
@@ -173,6 +173,7 @@ BITCOIN_TESTS += \
173173
wallet/test/wallet_crypto_tests.cpp \
174174
wallet/test/wallet_transaction_tests.cpp \
175175
wallet/test/coinselector_tests.cpp \
176+
wallet/test/availablecoins_tests.cpp \
176177
wallet/test/init_tests.cpp \
177178
wallet/test/ismine_tests.cpp \
178179
wallet/test/scriptpubkeyman_tests.cpp

src/bench/coin_selection.cpp

+3-3
Original file line numberDiff line numberDiff line change
@@ -58,10 +58,10 @@ static void CoinSelection(benchmark::Bench& bench)
5858
addCoin(3 * COIN, wallet, wtxs);
5959

6060
// Create coins
61-
std::vector<COutput> coins;
61+
wallet::CoinsResult available_coins;
6262
for (const auto& wtx : wtxs) {
6363
const auto txout = wtx->tx->vout.at(0);
64-
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);
64+
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);
6565
}
6666

6767
const CoinEligibilityFilter filter_standard(1, 6, 0);
@@ -80,7 +80,7 @@ static void CoinSelection(benchmark::Bench& bench)
8080
bench.run([&] {
8181
CAmountMap mapValue;
8282
mapValue[::policyAsset] = 1003 * COIN;
83-
auto result = AttemptSelection(wallet, mapValue, filter_standard, coins, coin_selection_params);
83+
auto result = AttemptSelection(wallet, mapValue, filter_standard, available_coins, coin_selection_params, /*allow_mixed_output_types=*/true);
8484
assert(result);
8585
assert(result->GetSelectedValue() == mapValue);
8686
assert(result->GetInputSet().size() == 2);

src/wallet/rpc/coins.cpp

+1-1
Original file line numberDiff line numberDiff line change
@@ -708,7 +708,7 @@ RPCHelpMan listunspent()
708708
cctl.m_max_depth = nMaxDepth;
709709
cctl.m_include_unsafe_inputs = include_unsafe;
710710
LOCK(pwallet->cs_wallet);
711-
vecOutputs = AvailableCoinsListUnspent(*pwallet, &cctl, nMinimumAmount, nMaximumAmount, nMinimumSumAmount, nMaximumCount, asset_filter.IsNull() ? nullptr : &asset_filter).coins;
711+
vecOutputs = AvailableCoinsListUnspent(*pwallet, &cctl, nMinimumAmount, nMaximumAmount, nMinimumSumAmount, nMaximumCount, asset_filter.IsNull() ? nullptr : &asset_filter).all();
712712
}
713713

714714
LOCK(pwallet->cs_wallet);

src/wallet/rpc/spend.cpp

+1-1
Original file line numberDiff line numberDiff line change
@@ -1491,7 +1491,7 @@ RPCHelpMan sendall()
14911491
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?
14921492
}
14931493
} else {
1494-
for (const COutput& output : AvailableCoins(*pwallet, &coin_control, fee_rate, /*nMinimumAmount=*/0).coins) {
1494+
for (const COutput& output : AvailableCoins(*pwallet, &coin_control, fee_rate, /*nMinimumAmount=*/0).all()) {
14951495
CHECK_NONFATAL(output.input_bytes > 0);
14961496
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?
14971497
continue;

src/wallet/spend.cpp

+141-39
Large diffs are not rendered by default.

src/wallet/spend.h

+56-15
Original file line numberDiff line numberDiff line change
@@ -31,15 +31,39 @@ struct TxSize {
3131
TxSize CalculateMaximumSignedTxSize(const CTransaction& tx, const CWallet* wallet, const std::vector<CTxOut>& txouts, const CCoinControl* coin_control = nullptr);
3232
TxSize CalculateMaximumSignedTxSize(const CTransaction& tx, const CWallet* wallet, const CCoinControl* coin_control = nullptr);
3333

34+
/**
35+
* COutputs available for spending, stored by OutputType.
36+
* This struct is really just a wrapper around OutputType vectors with a convenient
37+
* method for concatenating and returning all COutputs as one vector.
38+
*
39+
* clear(), size() methods are implemented so that one can interact with
40+
* the CoinsResult struct as if it was a vector
41+
*/
3442
struct CoinsResult {
35-
std::vector<COutput> coins;
36-
// Sum of all the coins amounts
43+
/** Vectors for each OutputType */
44+
std::vector<COutput> legacy;
45+
std::vector<COutput> P2SH_segwit;
46+
std::vector<COutput> bech32;
47+
std::vector<COutput> bech32m;
48+
49+
/** Other is a catch-all for anything that doesn't match the known OutputTypes */
50+
std::vector<COutput> other;
51+
52+
/** Concatenate and return all COutputs as one vector */
53+
std::vector<COutput> all() const;
54+
55+
/** The following methods are provided so that CoinsResult can mimic a vector,
56+
* i.e., methods can work with individual OutputType vectors or on the entire object */
57+
uint64_t size() const;
58+
void clear();
59+
60+
/** Sum of all available coins */
3761
// ELEMENTS: for each asset
3862
CAmountMap total_amount;
3963
};
64+
4065
/**
41-
* Return vector of available COutputs.
42-
* By default, returns only the spendable coins.
66+
* Populate the CoinsResult struct with vectors of available COutputs, organized by OutputType.
4367
*/
4468
CoinsResult AvailableCoins(const CWallet& wallet,
4569
const CCoinControl* coinControl = nullptr,
@@ -71,36 +95,53 @@ const CTxOut& FindNonChangeParentOutput(const CWallet& wallet, const COutPoint&
7195
std::map<CTxDestination, std::vector<COutput>> ListCoins(const CWallet& wallet);
7296

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

75116
/**
76117
* Attempt to find a valid input set that meets the provided eligibility filter and target.
77118
* Multiple coin selection algorithms will be run and the input set that produces the least waste
78119
* (according to the waste metric) will be chosen.
79120
*
80-
* param@[in] wallet The wallet which provides solving data for the coins
81-
* param@[in] nTargetValue The target value
82-
* param@[in] eligilibity_filter A filter containing rules for which coins are allowed to be included in this selection
83-
* param@[in] coins The vector of coins available for selection prior to filtering
84-
* param@[in] coin_selection_params Parameters for the coin selection
85-
* returns If successful, a SelectionResult containing the input set
86-
* If failed, a nullopt
121+
* param@[in] wallet The wallet which provides solving data for the coins
122+
* param@[in] nTargetValue The target value
123+
* param@[in] eligilibity_filter A filter containing rules for which coins are allowed to be included in this selection
124+
* param@[in] available_coins The struct of coins, organized by OutputType, available for selection prior to filtering
125+
* param@[in] coin_selection_params Parameters for the coin selection
126+
* returns If successful, a SelectionResult containing the input set
127+
* If failed, a nullopt
87128
*/
88-
std::optional<SelectionResult> AttemptSelection(const CWallet& wallet, const CAmountMap& mapTargetValue, const CoinEligibilityFilter& eligibility_filter, std::vector<COutput> coins,
129+
std::optional<SelectionResult> ChooseSelectionResult(const CWallet& wallet, const CAmountMap& mapTargetValue, const CoinEligibilityFilter& eligibility_filter, const std::vector<COutput>& available_coins,
89130
const CoinSelectionParams& coin_selection_params);
90131

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

105146
struct CreatedTransactionResult
106147
{
+145
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,145 @@
1+
// Copyright (c) 2022 The Bitcoin Core developers
2+
// Distributed under the MIT software license, see the accompanying
3+
// file COPYING or https://www.opensource.org/licenses/mit-license.php.
4+
5+
#include <validation.h>
6+
#include <wallet/coincontrol.h>
7+
#include <wallet/spend.h>
8+
#include <wallet/test/util.h>
9+
#include <wallet/test/wallet_test_fixture.h>
10+
11+
#include <boost/test/unit_test.hpp>
12+
13+
namespace wallet {
14+
BOOST_FIXTURE_TEST_SUITE(availablecoins_tests, WalletTestingSetup)
15+
class AvailableCoinsTestingSetup : public TestChain100Setup
16+
{
17+
public:
18+
AvailableCoinsTestingSetup()
19+
{
20+
CreateAndProcessBlock({}, {});
21+
wallet = CreateSyncedWallet(*m_node.chain, m_node.chainman->ActiveChain(), m_args, coinbaseKey);
22+
}
23+
24+
~AvailableCoinsTestingSetup()
25+
{
26+
wallet.reset();
27+
}
28+
CWalletTx& AddTx(CRecipient recipient)
29+
{
30+
CTransactionRef tx;
31+
CCoinControl dummy;
32+
{
33+
constexpr int RANDOM_CHANGE_POSITION = -1;
34+
auto res = CreateTransaction(*wallet, {recipient}, RANDOM_CHANGE_POSITION, dummy);
35+
BOOST_CHECK(res);
36+
tx = res.GetObj().tx;
37+
}
38+
wallet->CommitTransaction(tx, {}, {});
39+
CMutableTransaction blocktx;
40+
{
41+
LOCK(wallet->cs_wallet);
42+
blocktx = CMutableTransaction(*wallet->mapWallet.at(tx->GetHash()).tx);
43+
}
44+
CreateAndProcessBlock({CMutableTransaction(blocktx)}, GetScriptForRawPubKey(coinbaseKey.GetPubKey()));
45+
46+
LOCK(wallet->cs_wallet);
47+
wallet->SetLastBlockProcessed(wallet->GetLastBlockHeight() + 1, m_node.chainman->ActiveChain().Tip()->GetBlockHash());
48+
auto it = wallet->mapWallet.find(tx->GetHash());
49+
BOOST_CHECK(it != wallet->mapWallet.end());
50+
it->second.m_state = TxStateConfirmed{m_node.chainman->ActiveChain().Tip()->GetBlockHash(), m_node.chainman->ActiveChain().Height(), /*index=*/1};
51+
return it->second;
52+
}
53+
54+
std::unique_ptr<CWallet> wallet;
55+
};
56+
57+
// ELEMENTS FIXME: swapping these around so that bech32 is run first before bech32m
58+
// fixes an intermittent issue with the transaction failing to validate its own signature,
59+
// showing an "invalid schnorr signature" error. this requires further investigation.
60+
// see https://github.com/ElementsProject/elements/issues/1370
61+
62+
BOOST_FIXTURE_TEST_CASE(BasicOutputTypesTestBech32m, AvailableCoinsTestingSetup)
63+
{
64+
CoinsResult available_coins;
65+
BResult<CTxDestination> dest;
66+
LOCK(wallet->cs_wallet);
67+
68+
// Verify our wallet has one usable coinbase UTXO before starting
69+
// This UTXO is a P2PK, so it should show up in the Other bucket
70+
available_coins = AvailableCoins(*wallet);
71+
BOOST_CHECK_EQUAL(available_coins.size(), 1U);
72+
BOOST_CHECK_EQUAL(available_coins.other.size(), 1U);
73+
74+
// We will create a self transfer for each of the OutputTypes and
75+
// verify it is put in the correct bucket after running GetAvailablecoins
76+
//
77+
// For each OutputType, We expect 2 UTXOs in our wallet following the self transfer:
78+
// 1. One UTXO as the recipient
79+
// 2. One UTXO from the change, due to payment address matching logic
80+
81+
82+
// Bech32m
83+
dest = wallet->GetNewDestination(OutputType::BECH32M, "");
84+
BOOST_ASSERT(dest.HasRes());
85+
AddTx(CRecipient{{GetScriptForDestination(dest.GetObj())}, 1 * COIN, CAsset(), CPubKey(), /*fSubtractFeeFromAmount=*/true});
86+
available_coins = AvailableCoins(*wallet);
87+
BOOST_CHECK_EQUAL(available_coins.bech32m.size(), 2U);
88+
}
89+
90+
BOOST_FIXTURE_TEST_CASE(BasicOutputTypesTestBech32, AvailableCoinsTestingSetup)
91+
{
92+
CoinsResult available_coins;
93+
BResult<CTxDestination> dest;
94+
LOCK(wallet->cs_wallet);
95+
96+
available_coins = AvailableCoins(*wallet);
97+
BOOST_CHECK_EQUAL(available_coins.size(), 1U);
98+
BOOST_CHECK_EQUAL(available_coins.other.size(), 1U);
99+
100+
// Bech32
101+
dest = wallet->GetNewDestination(OutputType::BECH32, "");
102+
BOOST_ASSERT(dest.HasRes());
103+
AddTx(CRecipient{{GetScriptForDestination(dest.GetObj())}, 2 * COIN, CAsset(), CPubKey(), /*fSubtractFeeFromAmount=*/true});
104+
available_coins = AvailableCoins(*wallet);
105+
BOOST_CHECK_EQUAL(available_coins.bech32.size(), 2U);
106+
}
107+
108+
BOOST_FIXTURE_TEST_CASE(BasicOutputTypesTestP2SHSegWit, AvailableCoinsTestingSetup)
109+
{
110+
CoinsResult available_coins;
111+
BResult<CTxDestination> dest;
112+
LOCK(wallet->cs_wallet);
113+
114+
available_coins = AvailableCoins(*wallet);
115+
BOOST_CHECK_EQUAL(available_coins.size(), 1U);
116+
BOOST_CHECK_EQUAL(available_coins.other.size(), 1U);
117+
118+
// P2SH-SEGWIT
119+
dest = wallet->GetNewDestination(OutputType::P2SH_SEGWIT, "");
120+
AddTx(CRecipient{{GetScriptForDestination(dest.GetObj())}, 3 * COIN, CAsset(), CPubKey(), /*fSubtractFeeFromAmount=*/true});
121+
available_coins = AvailableCoins(*wallet);
122+
BOOST_CHECK_EQUAL(available_coins.P2SH_segwit.size(), 2U);
123+
}
124+
125+
126+
BOOST_FIXTURE_TEST_CASE(BasicOutputTypesTestLegacy, AvailableCoinsTestingSetup)
127+
{
128+
CoinsResult available_coins;
129+
BResult<CTxDestination> dest;
130+
LOCK(wallet->cs_wallet);
131+
132+
available_coins = AvailableCoins(*wallet);
133+
BOOST_CHECK_EQUAL(available_coins.size(), 1U);
134+
BOOST_CHECK_EQUAL(available_coins.other.size(), 1U);
135+
136+
// Legacy (P2PKH)
137+
dest = wallet->GetNewDestination(OutputType::LEGACY, "");
138+
BOOST_ASSERT(dest.HasRes());
139+
AddTx(CRecipient{{GetScriptForDestination(dest.GetObj())}, 4 * COIN, CAsset(), CPubKey(), /*fSubtractFeeFromAmount=*/true});
140+
available_coins = AvailableCoins(*wallet);
141+
BOOST_CHECK_EQUAL(available_coins.legacy.size(), 2U);
142+
}
143+
144+
BOOST_AUTO_TEST_SUITE_END()
145+
} // namespace wallet

0 commit comments

Comments
 (0)