Skip to content

wallet: Fix bug in CreateTransaction causing insufficient fees #2029

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Feb 28, 2021
Merged

wallet: Fix bug in CreateTransaction causing insufficient fees #2029

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ba23d4f
Add logging to CreateTransaction and AcceptToMemPool
jamescowens Feb 28, 2021
97bdc01
Fix bug in CreateTransaction
jamescowens Feb 28, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -992,9 +992,9 @@ bool AcceptToMemoryPool(CTxMemPool& pool, CTransaction &tx, bool* pfMissingInput
// Don't accept it if it can't get into a block
int64_t txMinFee = tx.GetMinFee(1000, GMF_RELAY, nSize);
if (nFees < txMinFee)
return error("AcceptToMemoryPool : not enough fees %s, %" PRId64 " < %" PRId64,
return error("AcceptToMemoryPool : not enough fees %s, %" PRId64 " < %" PRId64 ", nSize %" PRId64,
hash.ToString().c_str(),
nFees, txMinFee);
nFees, txMinFee, nSize);

// Continuously rate-limit free transactions
// This mitigates 'penny-flooding' -- sending thousands of free transactions just to
Expand Down
150 changes: 130 additions & 20 deletions src/wallet/wallet.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1673,6 +1673,8 @@ bool CWallet::SelectCoins(int64_t nTargetValue, unsigned int nSpendTime, set<pai
}

if (contract) {
LogPrint(BCLog::LogFlags::ESTIMATEFEE, "INFO %s: Contract is included so SelectSmallestCoins will be used.", __func__);

return (SelectSmallestCoins(nTargetValue, nSpendTime, 1, 10, vCoins, setCoinsRet, nValueRet) ||
SelectSmallestCoins(nTargetValue, nSpendTime, 1, 1, vCoins, setCoinsRet, nValueRet) ||
SelectSmallestCoins(nTargetValue, nSpendTime, 0, 1, vCoins, setCoinsRet, nValueRet));
Expand Down Expand Up @@ -1795,12 +1797,15 @@ bool CWallet::SelectCoinsForStaking(unsigned int nSpendTime, std::vector<pair<co
return true;
}

bool CWallet::CreateTransaction(const vector<pair<CScript, int64_t> >& vecSend, set<pair<const CWalletTx*,unsigned int>>& setCoins,
bool CWallet::CreateTransaction(const vector<pair<CScript, int64_t> >& vecSend, set<pair<const CWalletTx*,unsigned int>>& setCoins_in,
CWalletTx& wtxNew, CReserveKey& reservekey, int64_t& nFeeRet, const CCoinControl* coinControl)
{

int64_t nValueOut = 0;
int64_t message_fee = 0;
set<pair<const CWalletTx*,unsigned int>> setCoins_out;

bool provided_coin_set = !setCoins_in.empty();

for (auto const& s : vecSend)
{
Expand Down Expand Up @@ -1833,10 +1838,13 @@ bool CWallet::CreateTransaction(const vector<pair<CScript, int64_t> >& vecSend,
{
wtxNew.vin.clear();
wtxNew.vout.clear();
setCoins_out.clear();
wtxNew.fFromMe = true;

int64_t nTotalValue = nValueOut + nFeeRet;
double dPriority = 0;
// dPriority is not currently used. Commented out.
//double dPriority = 0;

// vouts to the payees
for (auto const& s : vecSend)
wtxNew.vout.emplace_back(s.second, s.first);
Expand All @@ -1850,7 +1858,7 @@ bool CWallet::CreateTransaction(const vector<pair<CScript, int64_t> >& vecSend,
int64_t nValueIn = 0;

// If provided coin set is empty, choose coins to use.
if (!setCoins.size())
if (!provided_coin_set)
{
// If the transaction contains a contract, we want to select the
// smallest UTXOs available:
Expand All @@ -1862,26 +1870,78 @@ bool CWallet::CreateTransaction(const vector<pair<CScript, int64_t> >& vecSend,
&& !wtxNew.vContracts.empty()
&& wtxNew.vContracts[0].m_type != GRC::ContractType::MESSAGE;

if (!SelectCoins(nTotalValue, wtxNew.nTime, setCoins, nValueIn, coinControl, contract)) {
// Notice that setCoins_out is that set PRODUCED by SelectCoins. Tying this to the input
// parameter of CreateTransaction was a major bug here before. It is now separated.
if (!SelectCoins(nTotalValue, wtxNew.nTime, setCoins_out, nValueIn, coinControl, contract)) {
return error("%s: Failed to select coins", __func__);
}

if (LogInstance().WillLogCategory(BCLog::LogFlags::ESTIMATEFEE))
{
CAmount setcoins_total = 0;

for (const auto& output: setCoins_out)
{
setcoins_total += output.first->vout[output.second].nValue;
}

LogPrintf("INFO %s: Just after SelectCoins: "
"nTotalValue = %s, nValueIn = %s, nValueOut = %s, setCoins total = %s.",
__func__,
FormatMoney(nTotalValue),
FormatMoney(nValueIn),
FormatMoney(nValueOut),
FormatMoney(setcoins_total));
}

// Compute dPriority on automatically selected coins.
/*
for (auto const& input : setCoins_out)
{
int64_t nCredit = input.first->vout[input.second].nValue;
dPriority += (double) nCredit * input.first->GetDepthInMainChain();
}
*/
}
else
{
// Add up input value for the provided set of coins.
for (auto const& input : setCoins)
// Add up input value for the provided set of coins, and also compute dPriority. (dPriority is
// commented out because it is not actually used right now.
for (auto const& input : setCoins_in)
{
nValueIn += input.first->vout[input.second].nValue;
int64_t nCredit = input.first->vout[input.second].nValue;
//dPriority += (double) nCredit * input.first->GetDepthInMainChain();
nValueIn += nCredit;
}
}

for (auto const& pcoin : setCoins)
int64_t nChange = nValueIn - nValueOut - nFeeRet;

// Note: In the case where CreateTransaction is called with a provided input set of coins,
// if the nValueIn of those coins is sufficient to cover the minimum nTransactionFee that starts
// the while loop, it will pass the first iteration. If the size of the transaction causes the nFeeRet
// to elevate and a second pass shows that the nValueOut + required fee is greater than that available
// i.e. negative change, then the loop is exited with an error. The reasoning for this is that
// in the case of no provided coin set, SelectTransaction above will be given the chance to modify its
// selection to cover the increased fees, hopefully converging on an appropriate solution. In the case
// of a provided set of inputs, that set is immutable for this transaction, so no point in continuing.
if (provided_coin_set && nChange < 0)
{
int64_t nCredit = pcoin.first->vout[pcoin.second].nValue;
dPriority += (double)nCredit * pcoin.first->GetDepthInMainChain();
return error("%s: Total value of inputs, %s, cannot cover the transaction fees of %s. "
"CreateTransaction aborted.",
__func__,
FormatMoney(nValueIn),
FormatMoney(nFeeRet));
}

int64_t nChange = nValueIn - nValueOut - nFeeRet;
LogPrint(BCLog::LogFlags::ESTIMATEFEE, "INFO %s: Before CENT test: nValueIn = %s, nValueOut = %s, "
"nChange = %s, nFeeRet = %s.",
__func__,
FormatMoney(nValueIn),
FormatMoney(nValueOut),
FormatMoney(nChange),
FormatMoney(nFeeRet));

// if sub-cent change is required, the fee must be raised to at least GetBaseFee
// or until nChange becomes zero
// NOTE: this depends on the exact behaviour of GetMinFee
Expand All @@ -1890,6 +1950,12 @@ bool CWallet::CreateTransaction(const vector<pair<CScript, int64_t> >& vecSend,
int64_t nMoveToFee = min(nChange, wtxNew.GetBaseFee() - nFeeRet);
nChange -= nMoveToFee;
nFeeRet += nMoveToFee;

LogPrint(BCLog::LogFlags::ESTIMATEFEE, "INFO %s: After CENT limit adjustment: nChange = %s, "
"nFeeRet = %s",
__func__,
FormatMoney(nChange),
FormatMoney(nFeeRet));
}

if (nChange > 0)
Expand Down Expand Up @@ -1925,37 +1991,81 @@ bool CWallet::CreateTransaction(const vector<pair<CScript, int64_t> >& vecSend,
wtxNew.vout.insert(position, CTxOut(nChange, scriptChange));
}
else
{
reservekey.ReturnKey();
}

// Fill vin
for (auto const& coin : setCoins)
wtxNew.vin.push_back(CTxIn(coin.first->GetHash(),coin.second));
if (setCoins_in.size())
{
// Fill vin from provided inputs
for (auto const& coin : setCoins_in)
{
wtxNew.vin.push_back(CTxIn(coin.first->GetHash(),coin.second));
}

// Sign
int nIn = 0;
for (auto const& coin : setCoins)
if (!SignSignature(*this, *coin.first, wtxNew, nIn++)) {
return error("%s: Failed to sign tx", __func__);
// Sign
int nIn = 0;
for (auto const& coin : setCoins_in)
if (!SignSignature(*this, *coin.first, wtxNew, nIn++)) {
return error("%s: Failed to sign tx", __func__);
}
}
else // use setCoins_out from SelectCoins as the inputs
{
// Fill vin from provided inputs
for (auto const& coin : setCoins_out)
{
wtxNew.vin.push_back(CTxIn(coin.first->GetHash(),coin.second));
}

// Sign
int nIn = 0;
for (auto const& coin : setCoins_out)
if (!SignSignature(*this, *coin.first, wtxNew, nIn++))
{
return error("%s: Failed to sign tx", __func__);
}
}

// Limit size
unsigned int nBytes = ::GetSerializeSize(*(CTransaction*)&wtxNew, SER_NETWORK, PROTOCOL_VERSION);
if (nBytes >= MAX_STANDARD_TX_SIZE) {
return error("%s: tx size %d greater than standard %d", __func__, nBytes, MAX_STANDARD_TX_SIZE);
}

dPriority /= nBytes;
// dPriority is not currently used.
//dPriority /= nBytes;

// Check that enough fee is included
int64_t nPayFee = nTransactionFee * (1 + (int64_t)nBytes / 1000);
int64_t nMinFee = wtxNew.GetMinFee(1000, GMF_SEND, nBytes);

LogPrint(BCLog::LogFlags::ESTIMATEFEE, "INFO %s: nTransactionFee = %s, nBytes = %" PRId64 ", nPayFee = %s"
", nMinFee = %s, nFeeRet = %s.",
__func__,
FormatMoney(nTransactionFee),
nBytes,
FormatMoney(nPayFee),
FormatMoney(nMinFee),
FormatMoney(nFeeRet));

if (nFeeRet < max(nPayFee, nMinFee))
{
nFeeRet = max(nPayFee, nMinFee);
continue;
}

LogPrint(BCLog::LogFlags::ESTIMATEFEE, "INFO %s: FINAL nValueIn = %s, nChange = %s, nTransactionFee = %s,"
" nBytes = %" PRId64 ", nPayFee = %s, nMinFee = %s, nFeeRet = %s.",
__func__,
FormatMoney(nValueIn),
FormatMoney(nChange),
FormatMoney(nTransactionFee),
nBytes,
FormatMoney(nPayFee),
FormatMoney(nMinFee),
FormatMoney(nFeeRet));

// Fill vtxPrev by copying from previous transactions vtxPrev
wtxNew.AddSupportingTransactions(txdb);
wtxNew.fTimeReceivedIsTxTime = true;
Expand Down