diff --git a/src/blind.h b/src/blind.h index d625978f29..508aa1afba 100644 --- a/src/blind.h +++ b/src/blind.h @@ -17,6 +17,10 @@ //! ELEMENTS: // 36-bit rangeproof size static const size_t DEFAULT_RANGEPROOF_SIZE = 2893; +// 64-bit rangeproof size +static const size_t MAX_RANGEPROOF_SIZE = 5126; +// 3-input ASP size +static const size_t DEFAULT_SURJECTIONPROOF_SIZE = 99; // 32 bytes of asset type, 32 bytes of asset blinding factor in sidechannel static const size_t SIDECHANNEL_MSG_SIZE = 64; diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp index 4a665ceeb9..fd6eab50cd 100644 --- a/src/wallet/test/coinselector_tests.cpp +++ b/src/wallet/test/coinselector_tests.cpp @@ -68,7 +68,7 @@ static void add_coin(const CAmount& nValue, int nInput, CoinSet& set) set.emplace(&wtx, nInput); } -static void add_coin(const CAmount& nValue, int nAge = 6*24, bool fIsFromMe = false, int nInput=0) +static void add_coin(CWallet& wallet, const CAmount& nValue, int nAge = 6*24, bool fIsFromMe = false, int nInput=0, bool spendable = false) { balance += nValue; static int nextLockTime = 0; @@ -76,12 +76,18 @@ static void add_coin(const CAmount& nValue, int nAge = 6*24, bool fIsFromMe = fa tx.nLockTime = nextLockTime++; // so all transactions get different hashes tx.vout.resize(nInput + 1); tx.vout[nInput].nValue = nValue; + if (spendable) { + CTxDestination dest; + std::string error; + assert(wallet.GetNewDestination(OutputType::BECH32, "", dest, error)); + tx.vout[nInput].scriptPubKey = GetScriptForDestination(dest); + } if (fIsFromMe) { // IsFromMe() returns (GetDebit() > 0), and GetDebit() is 0 if vin.empty(), // so stop vin being empty, and cache a non-zero Debit to fake out IsFromMe() tx.vin.resize(1); } - std::unique_ptr wtx = MakeUnique(&testWallet, MakeTransactionRef(std::move(tx))); + std::unique_ptr wtx = MakeUnique(&wallet, MakeTransactionRef(std::move(tx))); if (fIsFromMe) { CAmountMap map; @@ -90,9 +96,13 @@ static void add_coin(const CAmount& nValue, int nAge = 6*24, bool fIsFromMe = fa } COutput output(wtx.get(), nInput, nAge, true /* spendable */, true /* solvable */, true /* safe */); vCoins.push_back(output); - testWallet.AddToWallet(*wtx.get()); + wallet.AddToWallet(*wtx.get()); wtxn.emplace_back(std::move(wtx)); } +static void add_coin(const CAmount& nValue, int nAge = 6*24, bool fIsFromMe = false, int nInput=0, bool spendable = false) +{ + add_coin(testWallet, nValue, nAge, fIsFromMe, nInput, spendable); +} static void empty_wallet(void) { @@ -267,20 +277,36 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) 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 BOOST_CHECK(!SimpleSelectCoinsMinConf(testWallet, 1 * CENT, filter_standard, GroupCoins(vCoins), setCoinsRet, nValueRet, coin_selection_params_bnb, bnb_used)); - // Make sure that we aren't using BnB when there are preset inputs + // Test fees subtracted from output: empty_wallet(); - add_coin(5 * CENT); - add_coin(3 * CENT); - add_coin(2 * CENT); - CCoinControl coin_control; - coin_control.fAllowOtherInputs = true; - coin_control.Select(COutPoint(vCoins.at(0).tx->GetHash(), vCoins.at(0).i)); - CAmountMap mapTargetValue; - mapTargetValue[CAsset()] = 10 * CENT; - CAmountMap mapValueRet; - BOOST_CHECK(testWallet.SelectCoins(vCoins, mapTargetValue, setCoinsRet, mapValueRet, coin_control, coin_selection_params_bnb, bnb_used)); - BOOST_CHECK(!bnb_used); - BOOST_CHECK(!coin_selection_params_bnb.use_bnb); + add_coin(1 * CENT); + vCoins.at(0).nInputBytes = 40; + BOOST_CHECK(!SimpleSelectCoinsMinConf(testWallet, 1 * CENT, filter_standard, GroupCoins(vCoins), setCoinsRet, nValueRet, coin_selection_params_bnb, bnb_used)); + coin_selection_params_bnb.m_subtract_fee_outputs = true; + BOOST_CHECK(SimpleSelectCoinsMinConf(testWallet, 1 * CENT, filter_standard, GroupCoins(vCoins), setCoinsRet, nValueRet, coin_selection_params_bnb, bnb_used)); + BOOST_CHECK_EQUAL(nValueRet, 1 * CENT); + + // Make sure that can use BnB when there are preset inputs + empty_wallet(); + { + std::unique_ptr wallet = MakeUnique(m_chain.get(), WalletLocation(), WalletDatabase::CreateMock()); + bool firstRun; + wallet->LoadWallet(firstRun); + LOCK(wallet->cs_wallet); + add_coin(*wallet, 5 * CENT, 6 * 24, false, 0, true); + add_coin(*wallet, 3 * CENT, 6 * 24, false, 0, true); + add_coin(*wallet, 2 * CENT, 6 * 24, false, 0, true); + CCoinControl coin_control; + coin_control.fAllowOtherInputs = true; + coin_control.Select(COutPoint(vCoins.at(0).tx->GetHash(), vCoins.at(0).i)); + coin_selection_params_bnb.effective_fee = CFeeRate(0); + CAmountMap mapTargetValue; + mapTargetValue[CAsset()] = 10 * CENT; + CAmountMap mapValueRet; + BOOST_CHECK(wallet->SelectCoins(vCoins, mapTargetValue, setCoinsRet, mapValueRet, coin_control, coin_selection_params_bnb, bnb_used)); + BOOST_CHECK(bnb_used); + BOOST_CHECK(coin_selection_params_bnb.use_bnb); + } } BOOST_AUTO_TEST_CASE(knapsack_solver_test) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 88ec7111f0..7a0249fcaf 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2330,7 +2330,11 @@ bool CWallet::SelectCoinsMinConf(const CAmountMap& mapTargetValue, const CoinEli if (effective_value > 0) { group.fee += coin.m_input_bytes < 0 ? 0 : coin_selection_params.effective_fee.GetFee(coin.m_input_bytes); group.long_term_fee += coin.m_input_bytes < 0 ? 0 : long_term_feerate.GetFee(coin.m_input_bytes); - group.effective_value += effective_value; + if (coin_selection_params.m_subtract_fee_outputs) { + group.effective_value += coin.value; + } else { + group.effective_value += effective_value; + } ++it; } else { it = group.Discard(coin); @@ -2360,6 +2364,7 @@ bool CWallet::SelectCoins(const std::vector& vAvailableCoins, const CAm { AssertLockHeld(cs_wallet); // mapWallet std::vector vCoins(vAvailableCoins); + CAmountMap value_to_select = mapTargetValue; // coin control -> return all selected outputs (we want all selected to go into the transaction for sure) if (coin_control.HasSelected() && !coin_control.fAllowOtherInputs) @@ -2390,27 +2395,37 @@ bool CWallet::SelectCoins(const std::vector& vAvailableCoins, const CAm coin_control.ListSelected(vPresetInputs); for (const COutPoint& outpoint : vPresetInputs) { - // For now, don't use BnB if preset inputs are selected. TODO: Enable this later - bnb_used = false; - coin_selection_params.use_bnb = false; - std::map::const_iterator it = mapWallet.find(outpoint.hash); if (it != mapWallet.end()) { const CWalletTx& wtx = it->second; // Clearly invalid input, fail - if (wtx.tx->vout.size() <= outpoint.n) + if (wtx.tx->vout.size() <= outpoint.n) { + bnb_used = false; return false; - + } // Just to calculate the marginal byte size CAmount amt = wtx.GetOutputValueOut(outpoint.n); if (amt < 0) { continue; } + CInputCoin coin(&wtx, outpoint.n, wtx.GetSpendSize(outpoint.n, false)); mapValueFromPresetInputs[wtx.GetOutputAsset(outpoint.n)] += amt; - setPresetCoins.insert(CInputCoin(&wtx, outpoint.n)); - } else + if (coin.m_input_bytes <= 0) { + bnb_used = false; + return false; // Not solvable, can't estimate size for fee + } + coin.effective_value = coin.value - coin_selection_params.effective_fee.GetFee(coin.m_input_bytes); + if (coin_selection_params.use_bnb) { + value_to_select[coin.asset] -= coin.effective_value; + } else { + value_to_select[coin.asset] -= coin.value; + } + setPresetCoins.insert(coin); + } else { + bnb_used = false; return false; // TODO: Allow non-wallet inputs + } } // remove preset inputs from vCoins @@ -2450,25 +2465,25 @@ bool CWallet::SelectCoins(const std::vector& vAvailableCoins, const CAm bool fRejectLongChains = gArgs.GetBoolArg("-walletrejectlongchains", DEFAULT_WALLET_REJECT_LONG_CHAINS); // We will have to do coin selection on the difference between the target and the provided values. - // However, some inputs can be provided with assets that are not in the target, we need to make sure - // the map of the difference does not have negative values. - CAmountMap mapTargetMinusPreset = mapTargetValue - mapValueFromPresetInputs; - for (CAmountMap::const_iterator it = mapTargetMinusPreset.begin(); it != mapTargetMinusPreset.end();) { + // If value_to_select <= 0 for all asset types, we are done; but unlike in Bitcoin, this may be + // true for some assets whlie being false for others. So clear all the "completed" assets out + // of value_to_select before calling SelectCoinsMinConf. + for (CAmountMap::const_iterator it = value_to_select.begin(); it != value_to_select.end();) { if (it->second <= 0) { - it = mapTargetMinusPreset.erase(it); + it = value_to_select.erase(it); } else { ++it; } } - bool res = mapTargetValue <= mapValueFromPresetInputs || - SelectCoinsMinConf(mapTargetMinusPreset, CoinEligibilityFilter(1, 6, 0), groups, setCoinsRet, mapValueRet, coin_selection_params, bnb_used) || - SelectCoinsMinConf(mapTargetMinusPreset, CoinEligibilityFilter(1, 1, 0), groups, setCoinsRet, mapValueRet, coin_selection_params, bnb_used) || - (m_spend_zero_conf_change && SelectCoinsMinConf(mapTargetMinusPreset, CoinEligibilityFilter(0, 1, 2), groups, setCoinsRet, mapValueRet, coin_selection_params, bnb_used)) || - (m_spend_zero_conf_change && SelectCoinsMinConf(mapTargetMinusPreset, CoinEligibilityFilter(0, 1, std::min((size_t)4, max_ancestors/3), std::min((size_t)4, max_descendants/3)), groups, setCoinsRet, mapValueRet, coin_selection_params, bnb_used)) || - (m_spend_zero_conf_change && SelectCoinsMinConf(mapTargetMinusPreset, CoinEligibilityFilter(0, 1, max_ancestors/2, max_descendants/2), groups, setCoinsRet, mapValueRet, coin_selection_params, bnb_used)) || - (m_spend_zero_conf_change && SelectCoinsMinConf(mapTargetMinusPreset, CoinEligibilityFilter(0, 1, max_ancestors-1, max_descendants-1), groups, setCoinsRet, mapValueRet, coin_selection_params, bnb_used)) || - (m_spend_zero_conf_change && !fRejectLongChains && SelectCoinsMinConf(mapTargetMinusPreset, CoinEligibilityFilter(0, 1, std::numeric_limits::max()), groups, setCoinsRet, mapValueRet, coin_selection_params, bnb_used)); + bool res = value_to_select.empty() || + SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(1, 6, 0), groups, setCoinsRet, mapValueRet, coin_selection_params, bnb_used) || + SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(1, 1, 0), groups, setCoinsRet, mapValueRet, coin_selection_params, bnb_used) || + (m_spend_zero_conf_change && SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(0, 1, 2), groups, setCoinsRet, mapValueRet, coin_selection_params, bnb_used)) || + (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, mapValueRet, coin_selection_params, bnb_used)) || + (m_spend_zero_conf_change && SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(0, 1, max_ancestors/2, max_descendants/2), groups, setCoinsRet, mapValueRet, coin_selection_params, bnb_used)) || + (m_spend_zero_conf_change && SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(0, 1, max_ancestors-1, max_descendants-1), groups, setCoinsRet, mapValueRet, coin_selection_params, bnb_used)) || + (m_spend_zero_conf_change && !fRejectLongChains && SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(0, 1, std::numeric_limits::max()), groups, setCoinsRet, mapValueRet, coin_selection_params, bnb_used)); // because SelectCoinsMinConf clears the setCoinsRet, we now add the possible inputs to the coinset util::insert(setCoinsRet, setPresetCoins); @@ -2905,8 +2920,10 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std if (g_con_elementsmode) { // Assume blinded output for coin selection purposes. Over-paying is ok! change_prototype_txout.nAsset.vchCommitment.resize(33); + change_prototype_txout.nValue.vchCommitment.resize(33); + change_prototype_txout.nNonce.vchCommitment.resize(33); coin_selection_params.change_output_size = GetSerializeSize(change_prototype_txout); - coin_selection_params.change_output_size += DEFAULT_RANGEPROOF_SIZE/WITNESS_SCALE_FACTOR; + coin_selection_params.change_output_size += (MAX_RANGEPROOF_SIZE + DEFAULT_SURJECTIONPROOF_SIZE + WITNESS_SCALE_FACTOR - 1)/WITNESS_SCALE_FACTOR; } CFeeRate discard_rate = GetDiscardRate(*this); @@ -2922,7 +2939,10 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std // BnB selector is the only selector used when this is true. // That should only happen on the first pass through the loop. - coin_selection_params.use_bnb = nSubtractFeeFromAmount == 0; // If we are doing subtract fee from recipient, then don't use BnB + coin_selection_params.use_bnb = true; + coin_selection_params.m_subtract_fee_outputs = nSubtractFeeFromAmount != 0; // If we are doing subtract fee from recipient, don't use effective values + //ELEMENTS: stopgap solution to https://github.com/bitcoin/bitcoin/issues/20347 + bool one_more_try_20347 = false; // Start with no fee and loop until there is enough fee while (true) { @@ -2949,8 +2969,10 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std mapValueToSelect[::policyAsset] += nFeeRet; // vouts to the payees - 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) - for (const CRecipient& recipient : vecSend) + if (!coin_selection_params.m_subtract_fee_outputs) { + 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) + } + for (const auto& recipient : vecSend) { CTxOut txout(recipient.asset, recipient.nAmount, recipient.scriptPubKey); txout.nNonce.vchCommitment = std::vector(recipient.confidentiality_key.begin(), recipient.confidentiality_key.end()); @@ -2972,7 +2994,10 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std } } // Include the fee cost for outputs. Note this is only used for BnB right now - coin_selection_params.tx_noinputs_size += ::GetSerializeSize(txout, PROTOCOL_VERSION); + if (!coin_selection_params.m_subtract_fee_outputs) { + coin_selection_params.tx_noinputs_size += ::GetSerializeSize(txout, PROTOCOL_VERSION); + } + // ELEMENTS: Core's logic isn't great here. We should be computing // cost of making output + future spend. We're not as concerned // about dust anyways, so let's focus upstream. @@ -3299,7 +3324,7 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std } break; // Done, enough fee included. } - else if (!pick_new_inputs) { + else if (!pick_new_inputs && !one_more_try_20347) { // This shouldn't happen, we should have had enough excess // fee to pay for the new output and still meet nFeeNeeded // Or we should have just subtracted fee from recipients and @@ -3355,8 +3380,14 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std // If subtracting fee from recipients, we now know what fee we // need to subtract, we have no reason to reselect inputs + // In case we used branch-and-bound, this could result in our transaction + // since increasing since we force-elided change on this iteration for bnb. + // In this case we turn on `one_more_try` so that `!pick_new_inputs` doesn't + // cause the loop to fail. This is a stopgap. See Core #20347. + one_more_try_20347 = false; if (nSubtractFeeFromAmount > 0) { pick_new_inputs = false; + one_more_try_20347 = bnb_used; } // Include more fee and try again. diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 79b3f16614..ec642b7730 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -642,6 +642,8 @@ struct CoinSelectionParams size_t change_spend_size = 0; CFeeRate effective_fee = CFeeRate(0); size_t tx_noinputs_size = 0; + //! Indicate that we are subtracting the fee from outputs + bool m_subtract_fee_outputs = false; 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) {} CoinSelectionParams() {}