Move cs_wallet lock in CreateTransactionInternal to top of function

It isn't necessary to not lock parts of this function. Just lock the
whole thing and get rid of an indent.
This commit is contained in:
Andrew Chow 2021-05-20 14:51:50 -04:00
parent 55a156fca0
commit b2995963b5
2 changed files with 197 additions and 196 deletions

View file

@ -578,6 +578,8 @@ bool CWallet::CreateTransactionInternal(
FeeCalculation& fee_calc_out,
bool sign)
{
AssertLockHeld(cs_wallet);
CAmount nValue = 0;
const OutputType change_type = TransactionChangeType(coin_control.m_change_type ? *coin_control.m_change_type : m_default_change_type, vecSend);
ReserveDestination reservedest(this, change_type);
@ -606,224 +608,221 @@ bool CWallet::CreateTransactionInternal(
int nBytes;
{
std::set<CInputCoin> setCoins;
LOCK(cs_wallet);
txNew.nLockTime = GetLocktimeForNewTransaction(chain(), GetLastBlockHash(), GetLastBlockHeight());
std::vector<COutput> vAvailableCoins;
AvailableCoins(vAvailableCoins, &coin_control, 1, MAX_MONEY, MAX_MONEY, 0);
CoinSelectionParams coin_selection_params; // Parameters for coin selection, init with dummy
coin_selection_params.m_avoid_partial_spends = coin_control.m_avoid_partial_spends;
// Create change script that will be used if we need change
// TODO: pass in scriptChange instead of reservedest so
// change transaction isn't always pay-to-bitcoin-address
CScript scriptChange;
// coin control: send change to custom address
if (!std::get_if<CNoDestination>(&coin_control.destChange)) {
scriptChange = GetScriptForDestination(coin_control.destChange);
} else { // no coin control: send change to newly generated address
// Note: We use a new key here to keep it from being obvious which side is the change.
// The drawback is that by not reusing a previous key, the change may be lost if a
// backup is restored, if the backup doesn't have the new private key for the change.
// If we reused the old key, it would be possible to add code to look for and
// rediscover unknown transactions that were written with keys of ours to recover
// post-backup change.
// Reserve a new key pair from key pool. If it fails, provide a dummy
// destination in case we don't need change.
CTxDestination dest;
if (!reservedest.GetReservedDestination(dest, true)) {
error = _("Transaction needs a change address, but we can't generate it. Please call keypoolrefill first.");
}
scriptChange = GetScriptForDestination(dest);
// A valid destination implies a change script (and
// vice-versa). An empty change script will abort later, if the
// change keypool ran out, but change is required.
CHECK_NONFATAL(IsValidDestination(dest) != scriptChange.empty());
}
CTxOut change_prototype_txout(0, scriptChange);
coin_selection_params.change_output_size = GetSerializeSize(change_prototype_txout);
// Get size of spending the change output
int change_spend_size = CalculateMaximumSignedInputSize(change_prototype_txout, this);
// If the wallet doesn't know how to sign change output, assume p2sh-p2wpkh
// as lower-bound to allow BnB to do it's thing
if (change_spend_size == -1) {
coin_selection_params.change_spend_size = DUMMY_NESTED_P2WPKH_INPUT_SIZE;
} else {
coin_selection_params.change_spend_size = (size_t)change_spend_size;
}
// Set discard feerate
coin_selection_params.m_discard_feerate = GetDiscardRate(*this);
// Get the fee rate to use effective values in coin selection
coin_selection_params.m_effective_feerate = GetMinimumFeeRate(*this, coin_control, &feeCalc);
// Do not, ever, assume that it's fine to change the fee rate if the user has explicitly
// provided one
if (coin_control.m_feerate && coin_selection_params.m_effective_feerate > *coin_control.m_feerate) {
error = strprintf(_("Fee rate (%s) is lower than the minimum fee rate setting (%s)"), coin_control.m_feerate->ToString(FeeEstimateMode::SAT_VB), coin_selection_params.m_effective_feerate.ToString(FeeEstimateMode::SAT_VB));
return false;
}
if (feeCalc.reason == FeeReason::FALLBACK && !m_allow_fallback_fee) {
// eventually allow a fallback fee
error = _("Fee estimation failed. Fallbackfee is disabled. Wait a few blocks or enable -fallbackfee.");
return false;
}
// Get long term estimate
CCoinControl cc_temp;
cc_temp.m_confirm_target = chain().estimateMaxBlocks();
coin_selection_params.m_long_term_feerate = GetMinimumFeeRate(*this, cc_temp, nullptr);
// Calculate the cost of change
// Cost of change is the cost of creating the change output + cost of spending the change output in the future.
// For creating the change output now, we use the effective feerate.
// For spending the change output in the future, we use the discard feerate for now.
// So cost of change = (change output size * effective feerate) + (size of spending change output * discard feerate)
coin_selection_params.m_change_fee = coin_selection_params.m_effective_feerate.GetFee(coin_selection_params.change_output_size);
coin_selection_params.m_cost_of_change = coin_selection_params.m_discard_feerate.GetFee(coin_selection_params.change_spend_size) + coin_selection_params.m_change_fee;
coin_selection_params.m_subtract_fee_outputs = nSubtractFeeFromAmount != 0; // If we are doing subtract fee from recipient, don't use effective values
// vouts to the payees
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)
{
std::vector<COutput> vAvailableCoins;
AvailableCoins(vAvailableCoins, &coin_control, 1, MAX_MONEY, MAX_MONEY, 0);
CoinSelectionParams coin_selection_params; // Parameters for coin selection, init with dummy
coin_selection_params.m_avoid_partial_spends = coin_control.m_avoid_partial_spends;
CTxOut txout(recipient.nAmount, recipient.scriptPubKey);
// Create change script that will be used if we need change
// TODO: pass in scriptChange instead of reservedest so
// change transaction isn't always pay-to-bitcoin-address
CScript scriptChange;
// coin control: send change to custom address
if (!std::get_if<CNoDestination>(&coin_control.destChange)) {
scriptChange = GetScriptForDestination(coin_control.destChange);
} else { // no coin control: send change to newly generated address
// Note: We use a new key here to keep it from being obvious which side is the change.
// The drawback is that by not reusing a previous key, the change may be lost if a
// backup is restored, if the backup doesn't have the new private key for the change.
// If we reused the old key, it would be possible to add code to look for and
// rediscover unknown transactions that were written with keys of ours to recover
// post-backup change.
// Reserve a new key pair from key pool. If it fails, provide a dummy
// destination in case we don't need change.
CTxDestination dest;
if (!reservedest.GetReservedDestination(dest, true)) {
error = _("Transaction needs a change address, but we can't generate it. Please call keypoolrefill first.");
}
scriptChange = GetScriptForDestination(dest);
// A valid destination implies a change script (and
// vice-versa). An empty change script will abort later, if the
// change keypool ran out, but change is required.
CHECK_NONFATAL(IsValidDestination(dest) != scriptChange.empty());
}
CTxOut change_prototype_txout(0, scriptChange);
coin_selection_params.change_output_size = GetSerializeSize(change_prototype_txout);
// Get size of spending the change output
int change_spend_size = CalculateMaximumSignedInputSize(change_prototype_txout, this);
// If the wallet doesn't know how to sign change output, assume p2sh-p2wpkh
// as lower-bound to allow BnB to do it's thing
if (change_spend_size == -1) {
coin_selection_params.change_spend_size = DUMMY_NESTED_P2WPKH_INPUT_SIZE;
} else {
coin_selection_params.change_spend_size = (size_t)change_spend_size;
}
// Set discard feerate
coin_selection_params.m_discard_feerate = GetDiscardRate(*this);
// Get the fee rate to use effective values in coin selection
coin_selection_params.m_effective_feerate = GetMinimumFeeRate(*this, coin_control, &feeCalc);
// Do not, ever, assume that it's fine to change the fee rate if the user has explicitly
// provided one
if (coin_control.m_feerate && coin_selection_params.m_effective_feerate > *coin_control.m_feerate) {
error = strprintf(_("Fee rate (%s) is lower than the minimum fee rate setting (%s)"), coin_control.m_feerate->ToString(FeeEstimateMode::SAT_VB), coin_selection_params.m_effective_feerate.ToString(FeeEstimateMode::SAT_VB));
return false;
}
if (feeCalc.reason == FeeReason::FALLBACK && !m_allow_fallback_fee) {
// eventually allow a fallback fee
error = _("Fee estimation failed. Fallbackfee is disabled. Wait a few blocks or enable -fallbackfee.");
return false;
}
// Get long term estimate
CCoinControl cc_temp;
cc_temp.m_confirm_target = chain().estimateMaxBlocks();
coin_selection_params.m_long_term_feerate = GetMinimumFeeRate(*this, cc_temp, nullptr);
// Calculate the cost of change
// Cost of change is the cost of creating the change output + cost of spending the change output in the future.
// For creating the change output now, we use the effective feerate.
// For spending the change output in the future, we use the discard feerate for now.
// So cost of change = (change output size * effective feerate) + (size of spending change output * discard feerate)
coin_selection_params.m_change_fee = coin_selection_params.m_effective_feerate.GetFee(coin_selection_params.change_output_size);
coin_selection_params.m_cost_of_change = coin_selection_params.m_discard_feerate.GetFee(coin_selection_params.change_spend_size) + coin_selection_params.m_change_fee;
coin_selection_params.m_subtract_fee_outputs = nSubtractFeeFromAmount != 0; // If we are doing subtract fee from recipient, don't use effective values
// vouts to the payees
// Include the fee cost for outputs.
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.nAmount, recipient.scriptPubKey);
// Include the fee cost for outputs.
if (!coin_selection_params.m_subtract_fee_outputs) {
coin_selection_params.tx_noinputs_size += ::GetSerializeSize(txout, PROTOCOL_VERSION);
}
if (IsDust(txout, chain().relayDustFee()))
{
error = _("Transaction amount too small");
return false;
}
txNew.vout.push_back(txout);
coin_selection_params.tx_noinputs_size += ::GetSerializeSize(txout, PROTOCOL_VERSION);
}
// Include the fees for things that aren't inputs, excluding the change output
const CAmount not_input_fees = coin_selection_params.m_effective_feerate.GetFee(coin_selection_params.tx_noinputs_size);
CAmount nValueToSelect = nValue + not_input_fees;
// Choose coins to use
CAmount inputs_sum = 0;
setCoins.clear();
if (!SelectCoins(vAvailableCoins, /* nTargetValue */ nValueToSelect, setCoins, inputs_sum, coin_control, coin_selection_params))
if (IsDust(txout, chain().relayDustFee()))
{
error = _("Insufficient funds");
error = _("Transaction amount too small");
return false;
}
txNew.vout.push_back(txout);
}
// Always make a change output
// We will reduce the fee from this change output later, and remove the output if it is too small.
const CAmount change_and_fee = inputs_sum - nValue;
assert(change_and_fee >= 0);
CTxOut newTxOut(change_and_fee, scriptChange);
// Include the fees for things that aren't inputs, excluding the change output
const CAmount not_input_fees = coin_selection_params.m_effective_feerate.GetFee(coin_selection_params.tx_noinputs_size);
CAmount nValueToSelect = nValue + not_input_fees;
if (nChangePosInOut == -1)
{
// Insert change txn at random position:
nChangePosInOut = GetRandInt(txNew.vout.size()+1);
}
else if ((unsigned int)nChangePosInOut > txNew.vout.size())
{
error = _("Change index out of range");
return false;
}
// Choose coins to use
CAmount inputs_sum = 0;
setCoins.clear();
if (!SelectCoins(vAvailableCoins, /* nTargetValue */ nValueToSelect, setCoins, inputs_sum, coin_control, coin_selection_params))
{
error = _("Insufficient funds");
return false;
}
assert(nChangePosInOut != -1);
auto change_position = txNew.vout.insert(txNew.vout.begin() + nChangePosInOut, newTxOut);
// Always make a change output
// We will reduce the fee from this change output later, and remove the output if it is too small.
const CAmount change_and_fee = inputs_sum - nValue;
assert(change_and_fee >= 0);
CTxOut newTxOut(change_and_fee, scriptChange);
// Dummy fill vin for maximum size estimation
//
for (const auto& coin : setCoins) {
txNew.vin.push_back(CTxIn(coin.outpoint,CScript()));
}
if (nChangePosInOut == -1)
{
// Insert change txn at random position:
nChangePosInOut = GetRandInt(txNew.vout.size()+1);
}
else if ((unsigned int)nChangePosInOut > txNew.vout.size())
{
error = _("Change index out of range");
return false;
}
// Calculate the transaction fee
assert(nChangePosInOut != -1);
auto change_position = txNew.vout.insert(txNew.vout.begin() + nChangePosInOut, newTxOut);
// Dummy fill vin for maximum size estimation
//
for (const auto& coin : setCoins) {
txNew.vin.push_back(CTxIn(coin.outpoint,CScript()));
}
// Calculate the transaction fee
tx_sizes = CalculateMaximumSignedTxSize(CTransaction(txNew), this, coin_control.fAllowWatchOnly);
nBytes = tx_sizes.vsize;
if (nBytes < 0) {
error = _("Signing transaction failed");
return false;
}
nFeeRet = coin_selection_params.m_effective_feerate.GetFee(nBytes);
// Subtract fee from the change output if not subtrating it from recipient outputs
CAmount fee_needed = nFeeRet;
if (nSubtractFeeFromAmount == 0) {
change_position->nValue -= fee_needed;
}
// We want to drop the change to fees if:
// 1. The change output would be dust
// 2. The change is within the (almost) exact match window, i.e. it is less than or equal to the cost of the change output (cost_of_change)
CAmount change_amount = change_position->nValue;
if (IsDust(*change_position, coin_selection_params.m_discard_feerate) || change_amount <= coin_selection_params.m_cost_of_change)
{
nChangePosInOut = -1;
change_amount = 0;
txNew.vout.erase(change_position);
// Because we have dropped this change, the tx size and required fee will be different, so let's recalculate those
tx_sizes = CalculateMaximumSignedTxSize(CTransaction(txNew), this, coin_control.fAllowWatchOnly);
nBytes = tx_sizes.vsize;
if (nBytes < 0) {
error = _("Signing transaction failed");
return false;
}
nFeeRet = coin_selection_params.m_effective_feerate.GetFee(nBytes);
fee_needed = coin_selection_params.m_effective_feerate.GetFee(nBytes);
}
// Subtract fee from the change output if not subtrating it from recipient outputs
CAmount fee_needed = nFeeRet;
if (nSubtractFeeFromAmount == 0) {
change_position->nValue -= fee_needed;
}
// Update nFeeRet in case fee_needed changed due to dropping the change output
if (fee_needed <= change_and_fee - change_amount) {
nFeeRet = change_and_fee - change_amount;
}
// We want to drop the change to fees if:
// 1. The change output would be dust
// 2. The change is within the (almost) exact match window, i.e. it is less than or equal to the cost of the change output (cost_of_change)
CAmount change_amount = change_position->nValue;
if (IsDust(*change_position, coin_selection_params.m_discard_feerate) || change_amount <= coin_selection_params.m_cost_of_change)
// Reduce output values for subtractFeeFromAmount
if (nSubtractFeeFromAmount != 0) {
CAmount to_reduce = fee_needed + change_amount - change_and_fee;
int i = 0;
bool fFirst = true;
for (const auto& recipient : vecSend)
{
nChangePosInOut = -1;
change_amount = 0;
txNew.vout.erase(change_position);
// Because we have dropped this change, the tx size and required fee will be different, so let's recalculate those
tx_sizes = CalculateMaximumSignedTxSize(CTransaction(txNew), this, coin_control.fAllowWatchOnly);
nBytes = tx_sizes.vsize;
fee_needed = coin_selection_params.m_effective_feerate.GetFee(nBytes);
}
// Update nFeeRet in case fee_needed changed due to dropping the change output
if (fee_needed <= change_and_fee - change_amount) {
nFeeRet = change_and_fee - change_amount;
}
// Reduce output values for subtractFeeFromAmount
if (nSubtractFeeFromAmount != 0) {
CAmount to_reduce = fee_needed + change_amount - change_and_fee;
int i = 0;
bool fFirst = true;
for (const auto& recipient : vecSend)
{
if (i == nChangePosInOut) {
++i;
}
CTxOut& txout = txNew.vout[i];
if (recipient.fSubtractFeeFromAmount)
{
txout.nValue -= to_reduce / nSubtractFeeFromAmount; // Subtract fee equally from each selected recipient
if (fFirst) // first receiver pays the remainder not divisible by output count
{
fFirst = false;
txout.nValue -= to_reduce % nSubtractFeeFromAmount;
}
// Error if this output is reduced to be below dust
if (IsDust(txout, chain().relayDustFee())) {
if (txout.nValue < 0) {
error = _("The transaction amount is too small to pay the fee");
} else {
error = _("The transaction amount is too small to send after the fee has been deducted");
}
return false;
}
}
if (i == nChangePosInOut) {
++i;
}
nFeeRet = fee_needed;
}
CTxOut& txout = txNew.vout[i];
// Give up if change keypool ran out and change is required
if (scriptChange.empty() && nChangePosInOut != -1) {
return false;
if (recipient.fSubtractFeeFromAmount)
{
txout.nValue -= to_reduce / nSubtractFeeFromAmount; // Subtract fee equally from each selected recipient
if (fFirst) // first receiver pays the remainder not divisible by output count
{
fFirst = false;
txout.nValue -= to_reduce % nSubtractFeeFromAmount;
}
// Error if this output is reduced to be below dust
if (IsDust(txout, chain().relayDustFee())) {
if (txout.nValue < 0) {
error = _("The transaction amount is too small to pay the fee");
} else {
error = _("The transaction amount is too small to send after the fee has been deducted");
}
return false;
}
}
++i;
}
nFeeRet = fee_needed;
}
// Give up if change keypool ran out and change is required
if (scriptChange.empty() && nChangePosInOut != -1) {
return false;
}
// Shuffle selected coins and fill in final vin
@ -900,6 +899,8 @@ bool CWallet::CreateTransaction(
FeeCalculation& fee_calc_out,
bool sign)
{
LOCK(cs_wallet);
int nChangePosIn = nChangePosInOut;
Assert(!tx); // tx is an out-param. TODO change the return type from bool to tx (or nullptr)
bool res = CreateTransactionInternal(vecSend, tx, nFeeRet, nChangePosInOut, error, coin_control, fee_calc_out, sign);

View file

@ -326,7 +326,7 @@ private:
// ScriptPubKeyMan::GetID. In many cases it will be the hash of an internal structure
std::map<uint256, std::unique_ptr<ScriptPubKeyMan>> m_spk_managers;
bool CreateTransactionInternal(const std::vector<CRecipient>& vecSend, CTransactionRef& tx, CAmount& nFeeRet, int& nChangePosInOut, bilingual_str& error, const CCoinControl& coin_control, FeeCalculation& fee_calc_out, bool sign);
bool CreateTransactionInternal(const std::vector<CRecipient>& vecSend, CTransactionRef& tx, CAmount& nFeeRet, int& nChangePosInOut, bilingual_str& error, const CCoinControl& coin_control, FeeCalculation& fee_calc_out, bool sign) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
/**
* Catch wallet up to current chain, scanning new blocks, updating the best