Merge pull request #2029 from jamescowens/fee_fix

wallet: Fix bug in CreateTransaction causing insufficient fees

Author: jamescowens

src/main.cpp

@@ -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

src/wallet/wallet.cpp

@@ -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));
@@ -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)
     {
@@ -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);
@@ -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:
@@ -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
@@ -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)
@@ -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;