From b3d7b1cbe7afdc6a63bbcbe938e8639deedb04a1 Mon Sep 17 00:00:00 2001 From: Gregory Maxwell Date: Wed, 4 Jan 2017 08:51:14 +0000 Subject: [PATCH] Wallet: Do not perform ECDSA in the fee calculation inner loop. Performing signing in the inner loop has terrible performance when many passes through are needed to complete the selection. Signing before the algorithm is complete also gets in the way of correctly setting the fee (e.g. preventing over-payment when the fee required goes down on the final selection.) Use of the dummy might overpay on the signatures by a couple bytes in uncommon cases where the signatures' DER encoding is smaller than the dummy: Who cares? --- src/wallet/wallet.cpp | 79 +++++++++++++++++++++++++------------------ 1 file changed, 47 insertions(+), 32 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index c25e8be13c..185c4590cd 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2245,7 +2245,7 @@ bool CWallet::CreateTransaction(const vector& vecSend, CWalletTx& wt CAmount nValue = 0; int nChangePosRequest = nChangePosInOut; unsigned int nSubtractFeeFromAmount = 0; - BOOST_FOREACH (const CRecipient& recipient, vecSend) + for (const auto& recipient : vecSend) { if (nValue < 0 || recipient.nAmount < 0) { @@ -2300,6 +2300,7 @@ bool CWallet::CreateTransaction(const vector& vecSend, CWalletTx& wt assert(txNew.nLockTime < LOCKTIME_THRESHOLD); { + set > setCoins; LOCK2(cs_main, cs_wallet); { std::vector vAvailableCoins; @@ -2320,7 +2321,7 @@ bool CWallet::CreateTransaction(const vector& vecSend, CWalletTx& wt nValueToSelect += nFeeRet; double dPriority = 0; // vouts to the payees - BOOST_FOREACH (const CRecipient& recipient, vecSend) + for (const auto& recipient : vecSend) { CTxOut txout(recipient.nAmount, recipient.scriptPubKey); @@ -2352,14 +2353,14 @@ bool CWallet::CreateTransaction(const vector& vecSend, CWalletTx& wt } // Choose coins to use - set > setCoins; CAmount nValueIn = 0; + setCoins.clear(); if (!SelectCoins(vAvailableCoins, nValueToSelect, setCoins, nValueIn, coinControl)) { strFailReason = _("Insufficient funds"); return false; } - BOOST_FOREACH(PAIRTYPE(const CWalletTx*, unsigned int) pcoin, setCoins) + for (const auto& pcoin : setCoins) { CAmount nCredit = pcoin.first->tx->vout[pcoin.second].nValue; //The coin age after the next block (depth+1) is used instead of the current, @@ -2470,24 +2471,18 @@ bool CWallet::CreateTransaction(const vector& vecSend, CWalletTx& wt // to avoid conflicting with other possible uses of nSequence, // and in the spirit of "smallest posible change from prior // behavior." - BOOST_FOREACH(const PAIRTYPE(const CWalletTx*,unsigned int)& coin, setCoins) + for (const auto& coin : setCoins) txNew.vin.push_back(CTxIn(coin.first->GetHash(),coin.second,CScript(), std::numeric_limits::max() - (fWalletRbf ? 2 : 1))); - // Sign + // Fill in dummy signatures for fee calculation. int nIn = 0; - CTransaction txNewConst(txNew); - BOOST_FOREACH(const PAIRTYPE(const CWalletTx*,unsigned int)& coin, setCoins) + for (const auto& coin : setCoins) { - bool signSuccess; const CScript& scriptPubKey = coin.first->tx->vout[coin.second].scriptPubKey; SignatureData sigdata; - if (sign) - signSuccess = ProduceSignature(TransactionSignatureCreator(this, &txNewConst, nIn, coin.first->tx->vout[coin.second].nValue, SIGHASH_ALL), scriptPubKey, sigdata); - else - signSuccess = ProduceSignature(DummySignatureCreator(this), scriptPubKey, sigdata); - if (!signSuccess) + if (!ProduceSignature(DummySignatureCreator(this), scriptPubKey, sigdata)) { strFailReason = _("Signing transaction failed"); return false; @@ -2500,26 +2495,15 @@ bool CWallet::CreateTransaction(const vector& vecSend, CWalletTx& wt unsigned int nBytes = GetVirtualTransactionSize(txNew); - // Remove scriptSigs if we used dummy signatures for fee calculation - if (!sign) { - BOOST_FOREACH (CTxIn& vin, txNew.vin) { - vin.scriptSig = CScript(); - vin.scriptWitness.SetNull(); - } + CTransaction txNewConst(txNew); + dPriority = txNewConst.ComputePriority(dPriority, nBytes); + + // Remove scriptSigs to eliminate the fee calculation dummy signatures + for (auto& vin : txNew.vin) { + vin.scriptSig = CScript(); + vin.scriptWitness.SetNull(); } - // Embed the constructed transaction data in wtxNew. - wtxNew.SetTx(MakeTransactionRef(std::move(txNew))); - - // Limit size - if (GetTransactionWeight(wtxNew) >= MAX_STANDARD_TX_WEIGHT) - { - strFailReason = _("Transaction too large"); - return false; - } - - dPriority = wtxNew.tx->ComputePriority(dPriority, nBytes); - // Allow to override the default confirmation target over the CoinControl instance int currentConfirmationTarget = nTxConfirmTarget; if (coinControl && coinControl->nConfirmTarget > 0) @@ -2558,6 +2542,37 @@ bool CWallet::CreateTransaction(const vector& vecSend, CWalletTx& wt continue; } } + + if (sign) + { + CTransaction txNewConst(txNew); + int nIn = 0; + for (const auto& coin : setCoins) + { + const CScript& scriptPubKey = coin.first->tx->vout[coin.second].scriptPubKey; + SignatureData sigdata; + + if (!ProduceSignature(TransactionSignatureCreator(this, &txNewConst, nIn, coin.first->tx->vout[coin.second].nValue, SIGHASH_ALL), scriptPubKey, sigdata)) + { + strFailReason = _("Signing transaction failed"); + return false; + } else { + UpdateTransaction(txNew, nIn, sigdata); + } + + nIn++; + } + } + + // Embed the constructed transaction data in wtxNew. + wtxNew.SetTx(MakeTransactionRef(std::move(txNew))); + + // Limit size + if (GetTransactionWeight(wtxNew) >= MAX_STANDARD_TX_WEIGHT) + { + strFailReason = _("Transaction too large"); + return false; + } } if (GetBoolArg("-walletrejectlongchains", DEFAULT_WALLET_REJECT_LONG_CHAINS)) {