github
/
bitcoin
mirror of https://github.com/bitcoin/bitcoin
1
Fork 0
Code Releases Activity

wallet: use optional for change position as an optional in CreateTransaction 

Instead of making -1 a magic number meaning no change or random change
position, use an optional to have that meaning.
This commit is contained in:
Andrew Chow 2022-06-01 14:32:26 -04:00 committed by Andrew Chow
parent 2d39db7aa1
commit 758501b713
7 changed files with 28 additions and 36 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
src/wallet/feebumper.cpp
View File

@ -316,8 +316,7 @@ Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCo
// We cannot source new unconfirmed inputs(bip125 rule 2)
new_coin_control.m_min_depth = 1;
constexpr int RANDOM_CHANGE_POSITION = -1;
auto res = CreateTransaction(wallet, recipients, RANDOM_CHANGE_POSITION, new_coin_control, false);
auto res = CreateTransaction(wallet, recipients, std::nullopt, new_coin_control, false);
if (!res) {
errors.push_back(Untranslated("Unable to create transaction.") + Untranslated(" ") + util::ErrorString(res));
return Result::WALLET_ERROR;

4
src/wallet/interfaces.cpp
View File

@ -281,12 +281,12 @@ public:
CAmount& fee) override
{
LOCK(m_wallet->cs_wallet);
auto res = CreateTransaction(*m_wallet, recipients, change_pos,
auto res = CreateTransaction(*m_wallet, recipients, change_pos == -1 ? std::nullopt : std::make_optional(change_pos),
coin_control, sign);
if (!res) return util::Error{util::ErrorString(res)};
const auto& txr = *res;
fee = txr.fee;
change_pos = txr.change_pos;
change_pos = txr.change_pos ? *txr.change_pos : -1;
return txr.tx;
}

3
src/wallet/rpc/spend.cpp
View File

@ -155,8 +155,7 @@ UniValue SendMoney(CWallet& wallet, const CCoinControl &coin_control, std::vecto
std::shuffle(recipients.begin(), recipients.end(), FastRandomContext());
// Send
constexpr int RANDOM_CHANGE_POSITION = -1;
auto res = CreateTransaction(wallet, recipients, RANDOM_CHANGE_POSITION, coin_control, true);
auto res = CreateTransaction(wallet, recipients, std::nullopt, coin_control, true);
if (!res) {
throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, util::ErrorString(res).original);
}

42
src/wallet/spend.cpp
View File

@ -965,15 +965,12 @@ static void DiscourageFeeSniping(CMutableTransaction& tx, FastRandomContext& rng
static util::Result<CreatedTransactionResult> CreateTransactionInternal(
CWallet& wallet,
const std::vector<CRecipient>& vecSend,
int change_pos,
std::optional<unsigned int> change_pos,
const CCoinControl& coin_control,
bool sign) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet)
{
AssertLockHeld(wallet.cs_wallet);
// out variables, to be packed into returned result structure
int nChangePosInOut = change_pos;
FastRandomContext rng_fast;
CMutableTransaction txNew; // The resulting transaction that we make
@ -1132,15 +1129,15 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(
const CAmount change_amount = result.GetChange(coin_selection_params.min_viable_change, coin_selection_params.m_change_fee);
if (change_amount > 0) {
CTxOut newTxOut(change_amount, scriptChange);
if (nChangePosInOut == -1) {
if (!change_pos) {
// Insert change txn at random position:
nChangePosInOut = rng_fast.randrange(txNew.vout.size() + 1);
} else if ((unsigned int)nChangePosInOut > txNew.vout.size()) {
change_pos = rng_fast.randrange(txNew.vout.size() + 1);
} else if ((unsigned int)*change_pos > txNew.vout.size()) {
return util::Error{_("Transaction change output index out of range")};
}
txNew.vout.insert(txNew.vout.begin() + nChangePosInOut, newTxOut);
txNew.vout.insert(txNew.vout.begin() + *change_pos, newTxOut);
} else {
nChangePosInOut = -1;
change_pos = std::nullopt;
}
// Shuffle selected coins and fill in final vin
@ -1217,8 +1214,8 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(
}
// If there is a change output and we overpay the fees then increase the change to match the fee needed
if (nChangePosInOut != -1 && fee_needed < current_fee) {
auto& change = txNew.vout.at(nChangePosInOut);
if (change_pos && fee_needed < current_fee) {
auto& change = txNew.vout.at(*change_pos);
change.nValue += current_fee - fee_needed;
current_fee = result.GetSelectedValue() - CalculateOutputValue(txNew);
if (fee_needed != current_fee) {
@ -1229,11 +1226,11 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(
// Reduce output values for subtractFeeFromAmount
if (coin_selection_params.m_subtract_fee_outputs) {
CAmount to_reduce = fee_needed - current_fee;
int i = 0;
unsigned int i = 0;
bool fFirst = true;
for (const auto& recipient : vecSend)
{
if (i == nChangePosInOut) {
if (change_pos && i == *change_pos) {
++i;
}
CTxOut& txout = txNew.vout[i];
@ -1272,7 +1269,7 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(
}
// Give up if change keypool ran out and change is required
if (scriptChange.empty() && nChangePosInOut != -1) {
if (scriptChange.empty() && change_pos) {
return util::Error{error};
}
@ -1313,13 +1310,13 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(
feeCalc.est.fail.start, feeCalc.est.fail.end,
(feeCalc.est.fail.totalConfirmed + feeCalc.est.fail.inMempool + feeCalc.est.fail.leftMempool) > 0.0 ? 100 * feeCalc.est.fail.withinTarget / (feeCalc.est.fail.totalConfirmed + feeCalc.est.fail.inMempool + feeCalc.est.fail.leftMempool) : 0.0,
feeCalc.est.fail.withinTarget, feeCalc.est.fail.totalConfirmed, feeCalc.est.fail.inMempool, feeCalc.est.fail.leftMempool);
return CreatedTransactionResult(tx, current_fee, nChangePosInOut, feeCalc);
return CreatedTransactionResult(tx, current_fee, change_pos, feeCalc);
}
util::Result<CreatedTransactionResult> CreateTransaction(
CWallet& wallet,
const std::vector<CRecipient>& vecSend,
int change_pos,
std::optional<unsigned int> change_pos,
const CCoinControl& coin_control,
bool sign)
{
@ -1335,7 +1332,7 @@ util::Result<CreatedTransactionResult> CreateTransaction(
auto res = CreateTransactionInternal(wallet, vecSend, change_pos, coin_control, sign);
TRACE4(coin_selection, normal_create_tx_internal, wallet.GetName().c_str(), bool(res),
res ? res->fee : 0, res ? res->change_pos : 0);
res ? res->fee : 0, res && res->change_pos.has_value() ? *res->change_pos : 0);
if (!res) return res;
const auto& txr_ungrouped = *res;
// try with avoidpartialspends unless it's enabled already
@ -1345,16 +1342,15 @@ util::Result<CreatedTransactionResult> CreateTransaction(
tmp_cc.m_avoid_partial_spends = true;
// Reuse the change destination from the first creation attempt to avoid skipping BIP44 indexes
const int ungrouped_change_pos = txr_ungrouped.change_pos;
if (ungrouped_change_pos != -1) {
ExtractDestination(txr_ungrouped.tx->vout[ungrouped_change_pos].scriptPubKey, tmp_cc.destChange);
if (txr_ungrouped.change_pos) {
ExtractDestination(txr_ungrouped.tx->vout[*txr_ungrouped.change_pos].scriptPubKey, tmp_cc.destChange);
}
auto txr_grouped = CreateTransactionInternal(wallet, vecSend, change_pos, tmp_cc, sign);
// if fee of this alternative one is within the range of the max fee, we use this one
const bool use_aps{txr_grouped.has_value() ? (txr_grouped->fee <= txr_ungrouped.fee + wallet.m_max_aps_fee) : false};
TRACE5(coin_selection, aps_create_tx_internal, wallet.GetName().c_str(), use_aps, txr_grouped.has_value(),
txr_grouped.has_value() ? txr_grouped->fee : 0, txr_grouped.has_value() ? txr_grouped->change_pos : 0);
txr_grouped.has_value() ? txr_grouped->fee : 0, txr_grouped.has_value() && txr_grouped->change_pos.has_value() ? *txr_grouped->change_pos : 0);
if (txr_grouped) {
wallet.WalletLogPrintf("Fee non-grouped = %lld, grouped = %lld, using %s\n",
txr_ungrouped.fee, txr_grouped->fee, use_aps ? "grouped" : "non-grouped");
@ -1412,7 +1408,7 @@ bool FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& nFeeRet,
preset_txin.SetScriptWitness(txin.scriptWitness);
}
auto res = CreateTransaction(wallet, vecSend, nChangePosInOut, coinControl, false);
auto res = CreateTransaction(wallet, vecSend, nChangePosInOut == -1 ? std::nullopt : std::optional<unsigned int>((unsigned int)nChangePosInOut), coinControl, false);
if (!res) {
error = util::ErrorString(res);
return false;
@ -1420,7 +1416,7 @@ bool FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& nFeeRet,
const auto& txr = *res;
CTransactionRef tx_new = txr.tx;
nFeeRet = txr.fee;
nChangePosInOut = txr.change_pos;
nChangePosInOut = txr.change_pos ? *txr.change_pos : -1;
if (nChangePosInOut != -1) {
tx.vout.insert(tx.vout.begin() + nChangePosInOut, tx_new->vout[nChangePosInOut]);

6
src/wallet/spend.h
View File

@ -207,9 +207,9 @@ struct CreatedTransactionResult
CTransactionRef tx;
CAmount fee;
FeeCalculation fee_calc;
int change_pos;
std::optional<unsigned int> change_pos;
CreatedTransactionResult(CTransactionRef _tx, CAmount _fee, int _change_pos, const FeeCalculation& _fee_calc)
CreatedTransactionResult(CTransactionRef _tx, CAmount _fee, std::optional<unsigned int> _change_pos, const FeeCalculation& _fee_calc)
: tx(_tx), fee(_fee), fee_calc(_fee_calc), change_pos(_change_pos) {}
};
@ -218,7 +218,7 @@ struct CreatedTransactionResult
* selected by SelectCoins(); Also create the change output, when needed
* @note passing change_pos as -1 will result in setting a random position
*/
util::Result<CreatedTransactionResult> CreateTransaction(CWallet& wallet, const std::vector<CRecipient>& vecSend, int change_pos, const CCoinControl& coin_control, bool sign = true);
util::Result<CreatedTransactionResult> CreateTransaction(CWallet& wallet, const std::vector<CRecipient>& vecSend, std::optional<unsigned int> change_pos, const CCoinControl& coin_control, bool sign = true);
/**
* Insert additional inputs into the transaction by

3
src/wallet/test/spend_tests.cpp
View File

@ -28,13 +28,12 @@ BOOST_FIXTURE_TEST_CASE(SubtractFee, TestChain100Setup)
// instead of the miner.
auto check_tx = [&wallet](CAmount leftover_input_amount) {
CRecipient recipient{PubKeyDestination({}), 50 * COIN - leftover_input_amount, /*subtract_fee=*/true};
constexpr int RANDOM_CHANGE_POSITION = -1;
CCoinControl coin_control;
coin_control.m_feerate.emplace(10000);
coin_control.fOverrideFeeRate = true;
// We need to use a change type with high cost of change so that the leftover amount will be dropped to fee instead of added as a change output
coin_control.m_change_type = OutputType::LEGACY;
auto res = CreateTransaction(*wallet, {recipient}, RANDOM_CHANGE_POSITION, coin_control);
auto res = CreateTransaction(*wallet, {recipient}, std::nullopt, coin_control);
BOOST_CHECK(res);
const auto& txr = *res;
BOOST_CHECK_EQUAL(txr.tx->vout.size(), 1);

3
src/wallet/test/wallet_tests.cpp
View File

@ -558,8 +558,7 @@ public:
CTransactionRef tx;
CCoinControl dummy;
{
constexpr int RANDOM_CHANGE_POSITION = -1;
auto res = CreateTransaction(*wallet, {recipient}, RANDOM_CHANGE_POSITION, dummy);
auto res = CreateTransaction(*wallet, {recipient}, std::nullopt, dummy);
BOOST_CHECK(res);
tx = res->tx;
}