Skip to content

Commit cef87f7

Browse files
meshcollidermeshcollider
committed
Merge bitcoin#17290: Enable BnB coin selection for preset inputs and subtract fee from outputs
b007efd Allow BnB when subtract fee from outputs (Andrew Chow) db15e71 Use BnB when preset inputs are selected (Andrew Chow) Pull request description: Currently we explicitly disable BnB when there are preset inputs selected or when the subtract fee from outputs option is enabled. This PR enables BnB for both cases. Kind of an alternative to bitcoin#17246 (implements the subtract fee from outputs part of it) and borrows a test from there too. ACKs for top commit: instagibbs: reACK bitcoin@b007efd Sjors: re-ACK b007efd Tree-SHA512: 933276b09b2fa2ab43db7f0b98762f06f6f5fa8606195f96aca9fa1cb71ae4ee7156028dd482b1cada82ddd0996a9daf12ea5c152589fdf192cd96cbc51e99df
2 parents bb862d7 + b007efd commit cef87f7

File tree

3 files changed

+82
-33
lines changed

3 files changed

+82
-33
lines changed

‎src/wallet/test/coinselector_tests.cpp‎

Lines changed: 39 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -55,29 +55,39 @@ static void add_coin(const CAmount& nValue, int nInput, CoinSet& set)
5555
set.emplace(MakeTransactionRef(tx), nInput);
5656
}
5757

58-
static void add_coin(const CAmount& nValue, int nAge = 6*24, bool fIsFromMe = false, int nInput=0)
58+
static void add_coin(CWallet& wallet, const CAmount& nValue, int nAge = 6*24, bool fIsFromMe = false, int nInput=0, bool spendable = false)
5959
{
6060
balance += nValue;
6161
static int nextLockTime = 0;
6262
CMutableTransaction tx;
6363
tx.nLockTime = nextLockTime++; // so all transactions get different hashes
6464
tx.vout.resize(nInput + 1);
6565
tx.vout[nInput].nValue = nValue;
66+
if (spendable) {
67+
CTxDestination dest;
68+
std::string error;
69+
assert(wallet.GetNewDestination(OutputType::BECH32, "", dest, error));
70+
tx.vout[nInput].scriptPubKey = GetScriptForDestination(dest);
71+
}
6672
if (fIsFromMe) {
6773
// IsFromMe() returns (GetDebit() > 0), and GetDebit() is 0 if vin.empty(),
6874
// so stop vin being empty, and cache a non-zero Debit to fake out IsFromMe()
6975
tx.vin.resize(1);
7076
}
71-
std::unique_ptr<CWalletTx> wtx = MakeUnique<CWalletTx>(&testWallet, MakeTransactionRef(std::move(tx)));
77+
std::unique_ptr<CWalletTx> wtx = MakeUnique<CWalletTx>(&wallet, MakeTransactionRef(std::move(tx)));
7278
if (fIsFromMe)
7379
{
7480
wtx->m_amounts[CWalletTx::DEBIT].Set(ISMINE_SPENDABLE, 1);
7581
}
7682
COutput output(wtx.get(), nInput, nAge, true /* spendable */, true /* solvable */, true /* safe */);
7783
vCoins.push_back(output);
78-
testWallet.AddToWallet(*wtx.get());
84+
wallet.AddToWallet(*wtx.get());
7985
wtxn.emplace_back(std::move(wtx));
8086
}
87+
static void add_coin(const CAmount& nValue, int nAge = 6*24, bool fIsFromMe = false, int nInput=0, bool spendable = false)
88+
{
89+
add_coin(testWallet, nValue, nAge, fIsFromMe, nInput, spendable);
90+
}
8191

8292
static void empty_wallet(void)
8393
{
@@ -252,17 +262,33 @@ BOOST_AUTO_TEST_CASE(bnb_search_test)
252262
vCoins.at(0).nInputBytes = 40; // Make sure that it has a negative effective value. The next check should assert if this somehow got through. Otherwise it will fail
253263
BOOST_CHECK(!testWallet.SelectCoinsMinConf( 1 * CENT, filter_standard, GroupCoins(vCoins), setCoinsRet, nValueRet, coin_selection_params_bnb, bnb_used));
254264

255-
// Make sure that we aren't using BnB when there are preset inputs
265+
// Test fees subtracted from output:
266+
empty_wallet();
267+
add_coin(1 * CENT);
268+
vCoins.at(0).nInputBytes = 40;
269+
BOOST_CHECK(!testWallet.SelectCoinsMinConf( 1 * CENT, filter_standard, GroupCoins(vCoins), setCoinsRet, nValueRet, coin_selection_params_bnb, bnb_used));
270+
coin_selection_params_bnb.m_subtract_fee_outputs = true;
271+
BOOST_CHECK(testWallet.SelectCoinsMinConf( 1 * CENT, filter_standard, GroupCoins(vCoins), setCoinsRet, nValueRet, coin_selection_params_bnb, bnb_used));
272+
BOOST_CHECK_EQUAL(nValueRet, 1 * CENT);
273+
274+
// Make sure that can use BnB when there are preset inputs
256275
empty_wallet();
257-
add_coin(5 * CENT);
258-
add_coin(3 * CENT);
259-
add_coin(2 * CENT);
260-
CCoinControl coin_control;
261-
coin_control.fAllowOtherInputs = true;
262-
coin_control.Select(COutPoint(vCoins.at(0).tx->GetHash(), vCoins.at(0).i));
263-
BOOST_CHECK(testWallet.SelectCoins(vCoins, 10 * CENT, setCoinsRet, nValueRet, coin_control, coin_selection_params_bnb, bnb_used));
264-
BOOST_CHECK(!bnb_used);
265-
BOOST_CHECK(!coin_selection_params_bnb.use_bnb);
276+
{
277+
std::unique_ptr<CWallet> wallet = MakeUnique<CWallet>(m_chain.get(), WalletLocation(), WalletDatabase::CreateMock());
278+
bool firstRun;
279+
wallet->LoadWallet(firstRun);
280+
LOCK(wallet->cs_wallet);
281+
add_coin(*wallet, 5 * CENT, 6 * 24, false, 0, true);
282+
add_coin(*wallet, 3 * CENT, 6 * 24, false, 0, true);
283+
add_coin(*wallet, 2 * CENT, 6 * 24, false, 0, true);
284+
CCoinControl coin_control;
285+
coin_control.fAllowOtherInputs = true;
286+
coin_control.Select(COutPoint(vCoins.at(0).tx->GetHash(), vCoins.at(0).i));
287+
coin_selection_params_bnb.effective_fee = CFeeRate(0);
288+
BOOST_CHECK(wallet->SelectCoins(vCoins, 10 * CENT, setCoinsRet, nValueRet, coin_control, coin_selection_params_bnb, bnb_used));
289+
BOOST_CHECK(bnb_used);
290+
BOOST_CHECK(coin_selection_params_bnb.use_bnb);
291+
}
266292
}
267293

268294
BOOST_AUTO_TEST_CASE(knapsack_solver_test)

‎src/wallet/wallet.cpp‎

Lines changed: 41 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -2234,7 +2234,11 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const CoinEligibil
22342234
if (effective_value > 0) {
22352235
group.fee += coin.m_input_bytes < 0 ? 0 : coin_selection_params.effective_fee.GetFee(coin.m_input_bytes);
22362236
group.long_term_fee += coin.m_input_bytes < 0 ? 0 : long_term_feerate.GetFee(coin.m_input_bytes);
2237-
group.effective_value += effective_value;
2237+
if (coin_selection_params.m_subtract_fee_outputs) {
2238+
group.effective_value += coin.txout.nValue;
2239+
} else {
2240+
group.effective_value += effective_value;
2241+
}
22382242
++it;
22392243
} else {
22402244
it = group.Discard(coin);
@@ -2260,6 +2264,7 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const CoinEligibil
22602264
bool CWallet::SelectCoins(const std::vector<COutput>& vAvailableCoins, const CAmount& nTargetValue, std::set<CInputCoin>& setCoinsRet, CAmount& nValueRet, const CCoinControl& coin_control, CoinSelectionParams& coin_selection_params, bool& bnb_used) const
22612265
{
22622266
std::vector<COutput> vCoins(vAvailableCoins);
2267+
CAmount value_to_select = nTargetValue;
22632268

22642269
// coin control -> return all selected outputs (we want all selected to go into the transaction for sure)
22652270
if (coin_control.HasSelected() && !coin_control.fAllowOtherInputs)
@@ -2285,22 +2290,33 @@ bool CWallet::SelectCoins(const std::vector<COutput>& vAvailableCoins, const CAm
22852290
coin_control.ListSelected(vPresetInputs);
22862291
for (const COutPoint& outpoint : vPresetInputs)
22872292
{
2288-
// For now, don't use BnB if preset inputs are selected. TODO: Enable this later
2289-
bnb_used = false;
2290-
coin_selection_params.use_bnb = false;
2291-
22922293
std::map<uint256, CWalletTx>::const_iterator it = mapWallet.find(outpoint.hash);
22932294
if (it != mapWallet.end())
22942295
{
22952296
const CWalletTx& wtx = it->second;
22962297
// Clearly invalid input, fail
2297-
if (wtx.tx->vout.size() <= outpoint.n)
2298+
if (wtx.tx->vout.size() <= outpoint.n) {
2299+
bnb_used = false;
22982300
return false;
2301+
}
22992302
// Just to calculate the marginal byte size
2300-
nValueFromPresetInputs += wtx.tx->vout[outpoint.n].nValue;
2301-
setPresetCoins.insert(CInputCoin(wtx.tx, outpoint.n));
2302-
} else
2303+
CInputCoin coin(wtx.tx, outpoint.n, wtx.GetSpendSize(outpoint.n, false));
2304+
nValueFromPresetInputs += coin.txout.nValue;
2305+
if (coin.m_input_bytes <= 0) {
2306+
bnb_used = false;
2307+
return false; // Not solvable, can't estimate size for fee
2308+
}
2309+
coin.effective_value = coin.txout.nValue - coin_selection_params.effective_fee.GetFee(coin.m_input_bytes);
2310+
if (coin_selection_params.use_bnb) {
2311+
value_to_select -= coin.effective_value;
2312+
} else {
2313+
value_to_select -= coin.txout.nValue;
2314+
}
2315+
setPresetCoins.insert(coin);
2316+
} else {
2317+
bnb_used = false;
23032318
return false; // TODO: Allow non-wallet inputs
2319+
}
23042320
}
23052321

23062322
// remove preset inputs from vCoins
@@ -2329,14 +2345,14 @@ bool CWallet::SelectCoins(const std::vector<COutput>& vAvailableCoins, const CAm
23292345
size_t max_descendants = (size_t)std::max<int64_t>(1, limit_descendant_count);
23302346
bool fRejectLongChains = gArgs.GetBoolArg("-walletrejectlongchains", DEFAULT_WALLET_REJECT_LONG_CHAINS);
23312347

2332-
bool res = nTargetValue <= nValueFromPresetInputs ||
2333-
SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, CoinEligibilityFilter(1, 6, 0), groups, setCoinsRet, nValueRet, coin_selection_params, bnb_used) ||
2334-
SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, CoinEligibilityFilter(1, 1, 0), groups, setCoinsRet, nValueRet, coin_selection_params, bnb_used) ||
2335-
(m_spend_zero_conf_change && SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, CoinEligibilityFilter(0, 1, 2), groups, setCoinsRet, nValueRet, coin_selection_params, bnb_used)) ||
2336-
(m_spend_zero_conf_change && SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, CoinEligibilityFilter(0, 1, std::min((size_t)4, max_ancestors/3), std::min((size_t)4, max_descendants/3)), groups, setCoinsRet, nValueRet, coin_selection_params, bnb_used)) ||
2337-
(m_spend_zero_conf_change && SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, CoinEligibilityFilter(0, 1, max_ancestors/2, max_descendants/2), groups, setCoinsRet, nValueRet, coin_selection_params, bnb_used)) ||
2338-
(m_spend_zero_conf_change && SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, CoinEligibilityFilter(0, 1, max_ancestors-1, max_descendants-1), groups, setCoinsRet, nValueRet, coin_selection_params, bnb_used)) ||
2339-
(m_spend_zero_conf_change && !fRejectLongChains && SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, CoinEligibilityFilter(0, 1, std::numeric_limits<uint64_t>::max()), groups, setCoinsRet, nValueRet, coin_selection_params, bnb_used));
2348+
bool res = value_to_select <= 0 ||
2349+
SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(1, 6, 0), groups, setCoinsRet, nValueRet, coin_selection_params, bnb_used) ||
2350+
SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(1, 1, 0), groups, setCoinsRet, nValueRet, coin_selection_params, bnb_used) ||
2351+
(m_spend_zero_conf_change && SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(0, 1, 2), groups, setCoinsRet, nValueRet, coin_selection_params, bnb_used)) ||
2352+
(m_spend_zero_conf_change && SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(0, 1, std::min((size_t)4, max_ancestors/3), std::min((size_t)4, max_descendants/3)), groups, setCoinsRet, nValueRet, coin_selection_params, bnb_used)) ||
2353+
(m_spend_zero_conf_change && SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(0, 1, max_ancestors/2, max_descendants/2), groups, setCoinsRet, nValueRet, coin_selection_params, bnb_used)) ||
2354+
(m_spend_zero_conf_change && SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(0, 1, max_ancestors-1, max_descendants-1), groups, setCoinsRet, nValueRet, coin_selection_params, bnb_used)) ||
2355+
(m_spend_zero_conf_change && !fRejectLongChains && SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(0, 1, std::numeric_limits<uint64_t>::max()), groups, setCoinsRet, nValueRet, coin_selection_params, bnb_used));
23402356

23412357
// because SelectCoinsMinConf clears the setCoinsRet, we now add the possible inputs to the coinset
23422358
util::insert(setCoinsRet, setPresetCoins);
@@ -2602,7 +2618,8 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std
26022618

26032619
// BnB selector is the only selector used when this is true.
26042620
// That should only happen on the first pass through the loop.
2605-
coin_selection_params.use_bnb = nSubtractFeeFromAmount == 0; // If we are doing subtract fee from recipient, then don't use BnB
2621+
coin_selection_params.use_bnb = true;
2622+
coin_selection_params.m_subtract_fee_outputs = nSubtractFeeFromAmount != 0; // If we are doing subtract fee from recipient, don't use effective values
26062623
// Start with no fee and loop until there is enough fee
26072624
while (true)
26082625
{
@@ -2616,7 +2633,9 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std
26162633
nValueToSelect += nFeeRet;
26172634

26182635
// vouts to the payees
2619-
coin_selection_params.tx_noinputs_size = 11; // Static vsize overhead + outputs vsize. 4 nVersion, 4 nLocktime, 1 input count, 1 output count, 1 witness overhead (dummy, flag, stack size)
2636+
if (!coin_selection_params.m_subtract_fee_outputs) {
2637+
coin_selection_params.tx_noinputs_size = 11; // Static vsize overhead + outputs vsize. 4 nVersion, 4 nLocktime, 1 input count, 1 output count, 1 witness overhead (dummy, flag, stack size)
2638+
}
26202639
for (const auto& recipient : vecSend)
26212640
{
26222641
CTxOut txout(recipient.nAmount, recipient.scriptPubKey);
@@ -2633,7 +2652,9 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std
26332652
}
26342653
}
26352654
// Include the fee cost for outputs. Note this is only used for BnB right now
2636-
coin_selection_params.tx_noinputs_size += ::GetSerializeSize(txout, PROTOCOL_VERSION);
2655+
if (!coin_selection_params.m_subtract_fee_outputs) {
2656+
coin_selection_params.tx_noinputs_size += ::GetSerializeSize(txout, PROTOCOL_VERSION);
2657+
}
26372658

26382659
if (IsDust(txout, chain().relayDustFee()))
26392660
{

‎src/wallet/wallet.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -584,6 +584,8 @@ struct CoinSelectionParams
584584
size_t change_spend_size = 0;
585585
CFeeRate effective_fee = CFeeRate(0);
586586
size_t tx_noinputs_size = 0;
587+
//! Indicate that we are subtracting the fee from outputs
588+
bool m_subtract_fee_outputs = false;
587589

588590
CoinSelectionParams(bool use_bnb, size_t change_output_size, size_t change_spend_size, CFeeRate effective_fee, size_t tx_noinputs_size) : use_bnb(use_bnb), change_output_size(change_output_size), change_spend_size(change_spend_size), effective_fee(effective_fee), tx_noinputs_size(tx_noinputs_size) {}
589591
CoinSelectionParams() {}

0 commit comments

Comments
 (0)