From 50ae2bac99c57831cbdd115f131f292abf5ae6ec Mon Sep 17 00:00:00 2001 From: Levon Petrosyan Date: Fri, 2 Aug 2019 06:02:39 +0400 Subject: [PATCH 1/5] Fixing wrong transaction size estimation --- src/qt/sigmacoincontroldialog.cpp | 72 ++++++++----------------------- src/sigma/coin.cpp | 10 +++++ src/sigma/coin.h | 1 + 3 files changed, 29 insertions(+), 54 deletions(-) diff --git a/src/qt/sigmacoincontroldialog.cpp b/src/qt/sigmacoincontroldialog.cpp index 0b0ad417fb..447b5e3084 100644 --- a/src/qt/sigmacoincontroldialog.cpp +++ b/src/qt/sigmacoincontroldialog.cpp @@ -509,54 +509,20 @@ void SigmaCoinControlDialog::updateLabels(WalletModel *model, QDialog* dialog) dPriorityInputs += (double)out.tx->vout[out.i].nValue * (out.nDepth+1); // Bytes - CTxDestination address; - int witnessversion = 0; - std::vector witnessprogram; - if (out.tx->vout[out.i].scriptPubKey.IsWitnessProgram(witnessversion, witnessprogram)) - { - nBytesInputs += (32 + 4 + 1 + (107 / WITNESS_SCALE_FACTOR) + 4); - fWitness = true; - } - else if(ExtractDestination(out.tx->vout[out.i].scriptPubKey, address)) - { - CPubKey pubkey; - CKeyID *keyid = boost::get(&address); - if (keyid && model->getPubKey(*keyid, pubkey)) - { - nBytesInputs += (pubkey.IsCompressed() ? 148 : 180); - if (!pubkey.IsCompressed()) - nQuantityUncompressed++; - } - else - nBytesInputs += 148; // in all error cases, simply assume 148 here - } - else nBytesInputs += 148; + nBytesInputs += 74; } // calculation if (nQuantity > 0) { - // Bytes - nBytes = nBytesInputs + ((SigmaCoinControlDialog::payAmounts.size() > 0 ? SigmaCoinControlDialog::payAmounts.size() + 1 : 2) * 34) + 10; // always assume +1 output for change here - if (fWitness) - { - // there is some fudging in these numbers related to the actual virtual transaction size calculation that will keep this estimate from being exact. - // usually, the result will be an overestimate within a couple of satoshis so that the confirmation dialog ends up displaying a slightly smaller fee. - // also, the witness stack size value value is a variable sized integer. usually, the number of stack items will be well under the single byte var int limit. - nBytes += 2; // account for the serialized marker and flag bytes - nBytes += nQuantity; // account for the witness byte that holds the number of stack items for each input. - } + // Bytes //1323 is script size for sigma spend + nBytes = nBytesInputs + vCoinControl.size() * 1323 + 10; // Priority double mempoolEstimatePriority = mempool.estimateSmartPriority(nTxConfirmTarget); dPriority = dPriorityInputs / (nBytes - nBytesInputs + (nQuantityUncompressed * 29)); // 29 = 180 - 151 (uncompressed public keys are over the limit. max 151 bytes of the input are ignored for priority) sPriorityLabel = SigmaCoinControlDialog::getPriorityLabel(dPriority, mempoolEstimatePriority); - // in the subtract fee from amount case, we can tell if zero change already and subtract the bytes, so that fee calculation afterwards is accurate - if (SigmaCoinControlDialog::fSubtractFeeFromAmount) - if (nAmount - nPayAmount == 0) - nBytes -= 34; - // Fee nPayFee = CWallet::GetMinimumFee(nBytes, nTxConfirmTarget, mempool); if (nPayFee > 0 && coinControl->nMinimumTotalFee > nPayFee) @@ -576,27 +542,25 @@ void SigmaCoinControlDialog::updateLabels(WalletModel *model, QDialog* dialog) nChange = nAmount - nPayAmount; if (!SigmaCoinControlDialog::fSubtractFeeFromAmount) nChange -= nPayFee; + } - // Never create dust outputs; if we would, just add the dust to the fee. - if (nChange > 0 && nChange < MIN_CHANGE) - { - CTxOut txout(nChange, (CScript)std::vector(24, 0)); - if (txout.IsDust(::minRelayTxFee)) - { - if (SigmaCoinControlDialog::fSubtractFeeFromAmount) // dust-change will be raised until no dust - nChange = txout.GetDustThreshold(::minRelayTxFee); - else - { - nPayFee += nChange; - nChange = 0; - } - } - } + //update bytes + CAmount remChange = nChange; - if (nChange == 0 && !SigmaCoinControlDialog::fSubtractFeeFromAmount) - nBytes -= 34; + std::vector allDenoms; + sigma::GetAllDenoms(allDenoms); + for(int64_t denom : allDenoms) { + while (remChange >= denom) { + nBytes += 44; + remChange -= denom; + } + if(!remChange) + break; } + //add remaining to fee + nPayFee += remChange; + // after fee nAfterFee = nAmount - nPayFee; if (nAfterFee < 0) diff --git a/src/sigma/coin.cpp b/src/sigma/coin.cpp index faebc74d6b..84ea79a985 100644 --- a/src/sigma/coin.cpp +++ b/src/sigma/coin.cpp @@ -157,6 +157,16 @@ void GetAllDenoms(std::vector& denominations_out) { denominations_out.push_back(CoinDenomination::SIGMA_DENOM_0_05); } +void GetAllDenoms(std::vector& denominations_out) { + denominations_out.push_back(100 * COIN); + denominations_out.push_back(25 * COIN); + denominations_out.push_back(10 * COIN); + denominations_out.push_back(1 * COIN); + denominations_out.push_back(50 * CENT); + denominations_out.push_back(10 * CENT); + denominations_out.push_back(5 * CENT); +} + //class PublicCoin PublicCoin::PublicCoin() : denomination(CoinDenomination::SIGMA_DENOM_1) diff --git a/src/sigma/coin.h b/src/sigma/coin.h index c2b92737ad..ee35246bf8 100644 --- a/src/sigma/coin.h +++ b/src/sigma/coin.h @@ -35,6 +35,7 @@ bool RealNumberToDenomination(const double& value, CoinDenomination& denom_out); /// \brief Returns a list of all possible denominations in descending order of value. void GetAllDenoms(std::vector& denominations_out); +void GetAllDenoms(std::vector& denominations_out); class PublicCoin { public: From df6e34acbe7b69f46ac0c7927045888de328542e Mon Sep 17 00:00:00 2001 From: Levon Petrosyan Date: Mon, 5 Aug 2019 05:21:03 +0400 Subject: [PATCH 2/5] Calculating sizes more accurately --- src/qt/sigmacoincontroldialog.cpp | 102 +++++++++++++++++++++++------- 1 file changed, 79 insertions(+), 23 deletions(-) diff --git a/src/qt/sigmacoincontroldialog.cpp b/src/qt/sigmacoincontroldialog.cpp index 447b5e3084..29dc26912c 100644 --- a/src/qt/sigmacoincontroldialog.cpp +++ b/src/qt/sigmacoincontroldialog.cpp @@ -507,22 +507,62 @@ void SigmaCoinControlDialog::updateLabels(WalletModel *model, QDialog* dialog) // Priority dPriorityInputs += (double)out.tx->vout[out.i].nValue * (out.nDepth+1); - - // Bytes - nBytesInputs += 74; + if(fMintTabSelected){ + // Bytes + CTxDestination address; + int witnessversion = 0; + std::vector witnessprogram; + if (out.tx->vout[out.i].scriptPubKey.IsWitnessProgram(witnessversion, witnessprogram)) + { + nBytesInputs += (32 + 4 + 1 + (107 / WITNESS_SCALE_FACTOR) + 4); + fWitness = true; + } + else if(ExtractDestination(out.tx->vout[out.i].scriptPubKey, address)) + { + CPubKey pubkey; + CKeyID *keyid = boost::get(&address); + if (keyid && model->getPubKey(*keyid, pubkey)) + { + nBytesInputs += (pubkey.IsCompressed() ? 148 : 180); + if (!pubkey.IsCompressed()) + nQuantityUncompressed++; + } + else + nBytesInputs += 148; // in all error cases, simply assume 148 here + } + else nBytesInputs += 148; + } } // calculation if (nQuantity > 0) { - // Bytes //1323 is script size for sigma spend - nBytes = nBytesInputs + vCoinControl.size() * 1323 + 10; + // Bytes + if(fMintTabSelected) { + nBytes = nBytesInputs + + ((SigmaCoinControlDialog::payAmounts.size() > 0 ? SigmaCoinControlDialog::payAmounts.size() + 1 : 2) * 34) + 10; // always assume +1 output for change here + if (fWitness) { + // there is some fudging in these numbers related to the actual virtual transaction size calculation that will keep this estimate from being exact. + // usually, the result will be an overestimate within a couple of satoshis so that the confirmation dialog ends up displaying a slightly smaller fee. + // also, the witness stack size value value is a variable sized integer. usually, the number of stack items will be well under the single byte var int limit. + nBytes += 2; // account for the serialized marker and flag bytes + nBytes += nQuantity; // account for the witness byte that holds the number of stack items for each input. + } + } else { + // 1363 is the size of each vin containing sigma proof, 34 is the size of each normal vout, 10 is the size of empty transaction, + nBytes = vCoinControl.size() * 1363 + SigmaCoinControlDialog::payAmounts.size() * 34 + 10; + } // Priority double mempoolEstimatePriority = mempool.estimateSmartPriority(nTxConfirmTarget); dPriority = dPriorityInputs / (nBytes - nBytesInputs + (nQuantityUncompressed * 29)); // 29 = 180 - 151 (uncompressed public keys are over the limit. max 151 bytes of the input are ignored for priority) sPriorityLabel = SigmaCoinControlDialog::getPriorityLabel(dPriority, mempoolEstimatePriority); - + if(fMintTabSelected) { + // in the subtract fee from amount case, we can tell if zero change already and subtract the bytes, so that fee calculation afterwards is accurate + if (SigmaCoinControlDialog::fSubtractFeeFromAmount) + if (nAmount - nPayAmount == 0) + nBytes -= 34; + } // Fee nPayFee = CWallet::GetMinimumFee(nBytes, nTxConfirmTarget, mempool); if (nPayFee > 0 && coinControl->nMinimumTotalFee > nPayFee) @@ -537,29 +577,45 @@ void SigmaCoinControlDialog::updateLabels(WalletModel *model, QDialog* dialog) if (fAllowFree && nBytes <= MAX_FREE_TRANSACTION_CREATE_SIZE) nPayFee = 0; - if (nPayAmount > 0) - { + if (nPayAmount > 0) { nChange = nAmount - nPayAmount; if (!SigmaCoinControlDialog::fSubtractFeeFromAmount) nChange -= nPayFee; - } - - //update bytes - CAmount remChange = nChange; - std::vector allDenoms; - sigma::GetAllDenoms(allDenoms); - for(int64_t denom : allDenoms) { - while (remChange >= denom) { - nBytes += 44; - remChange -= denom; + if (fMintTabSelected) { + // Never create dust outputs; if we would, just add the dust to the fee. + if (nChange > 0 && nChange < MIN_CHANGE) { + CTxOut txout(nChange, (CScript) std::vector(24, 0)); + if (txout.IsDust(::minRelayTxFee)) { + if (SigmaCoinControlDialog::fSubtractFeeFromAmount) // dust-change will be raised until no dust + nChange = txout.GetDustThreshold(::minRelayTxFee); + else { + nPayFee += nChange; + nChange = 0; + } + } + } + + if (nChange == 0 && !SigmaCoinControlDialog::fSubtractFeeFromAmount) + nBytes -= 34; + + } else { //adding sizes of automatic remints + CAmount remChange = nChange; + std::vector allDenoms; + sigma::GetAllDenoms(allDenoms); + for (int64_t denom : allDenoms) { + while (remChange >= denom) { + nBytes += 44; //is the siz of each vout containing sigma mint + remChange -= denom; + } + if (!remChange) + break; + } + //add remaining to fee + nPayFee += remChange; } - if(!remChange) - break; - } - //add remaining to fee - nPayFee += remChange; + } // after fee nAfterFee = nAmount - nPayFee; From 285653dc5a781bbbdf5f982ccb06fd582708fb38 Mon Sep 17 00:00:00 2001 From: Levon Petrosyan Date: Tue, 6 Aug 2019 02:42:43 +0400 Subject: [PATCH 3/5] Review comments applied --- src/qt/sigmacoincontroldialog.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/qt/sigmacoincontroldialog.cpp b/src/qt/sigmacoincontroldialog.cpp index 29dc26912c..14a48ac1b2 100644 --- a/src/qt/sigmacoincontroldialog.cpp +++ b/src/qt/sigmacoincontroldialog.cpp @@ -531,6 +531,8 @@ void SigmaCoinControlDialog::updateLabels(WalletModel *model, QDialog* dialog) nBytesInputs += 148; // in all error cases, simply assume 148 here } else nBytesInputs += 148; + } else { + nBytesInputs += 1363; //1363 is the size of each vin containing sigma proof } } @@ -549,8 +551,8 @@ void SigmaCoinControlDialog::updateLabels(WalletModel *model, QDialog* dialog) nBytes += nQuantity; // account for the witness byte that holds the number of stack items for each input. } } else { - // 1363 is the size of each vin containing sigma proof, 34 is the size of each normal vout, 10 is the size of empty transaction, - nBytes = vCoinControl.size() * 1363 + SigmaCoinControlDialog::payAmounts.size() * 34 + 10; + //34 is the size of each normal vout, 10 is the size of empty transaction, + nBytes = nBytesInputs + SigmaCoinControlDialog::payAmounts.size() * 34 + 10; } // Priority From 5dda8b8cc4462942d7418dc1387898611fc985fc Mon Sep 17 00:00:00 2001 From: Levon Petrosyan Date: Tue, 6 Aug 2019 15:33:22 +0400 Subject: [PATCH 4/5] Review comments applied. --- src/qt/sigmacoincontroldialog.cpp | 26 +++++++------------------- 1 file changed, 7 insertions(+), 19 deletions(-) diff --git a/src/qt/sigmacoincontroldialog.cpp b/src/qt/sigmacoincontroldialog.cpp index 14a48ac1b2..757c44f46e 100644 --- a/src/qt/sigmacoincontroldialog.cpp +++ b/src/qt/sigmacoincontroldialog.cpp @@ -559,12 +559,7 @@ void SigmaCoinControlDialog::updateLabels(WalletModel *model, QDialog* dialog) double mempoolEstimatePriority = mempool.estimateSmartPriority(nTxConfirmTarget); dPriority = dPriorityInputs / (nBytes - nBytesInputs + (nQuantityUncompressed * 29)); // 29 = 180 - 151 (uncompressed public keys are over the limit. max 151 bytes of the input are ignored for priority) sPriorityLabel = SigmaCoinControlDialog::getPriorityLabel(dPriority, mempoolEstimatePriority); - if(fMintTabSelected) { - // in the subtract fee from amount case, we can tell if zero change already and subtract the bytes, so that fee calculation afterwards is accurate - if (SigmaCoinControlDialog::fSubtractFeeFromAmount) - if (nAmount - nPayAmount == 0) - nBytes -= 34; - } + // Fee nPayFee = CWallet::GetMinimumFee(nBytes, nTxConfirmTarget, mempool); if (nPayFee > 0 && coinControl->nMinimumTotalFee > nPayFee) @@ -602,21 +597,14 @@ void SigmaCoinControlDialog::updateLabels(WalletModel *model, QDialog* dialog) nBytes -= 34; } else { //adding sizes of automatic remints - CAmount remChange = nChange; - std::vector allDenoms; - sigma::GetAllDenoms(allDenoms); - for (int64_t denom : allDenoms) { - while (remChange >= denom) { - nBytes += 44; //is the siz of each vout containing sigma mint - remChange -= denom; - } - if (!remChange) - break; - } + std::vector denominations; + sigma::GetAllDenoms(denominations); + std::vector coinsOut; + CAmount mintedChange = CWallet::SelectMintCoinsForAmount(nChange, denominations,coinsOut); + nBytes += 44 * coinsOut.size(); //44 is the siz of each vout containing sigma mint //add remaining to fee - nPayFee += remChange; + nPayFee += nChange - mintedChange; } - } // after fee From a25295d5dd988a10d4ecc0ddabb93c2789eec1b1 Mon Sep 17 00:00:00 2001 From: Levon Petrosyan Date: Tue, 6 Aug 2019 16:24:16 +0400 Subject: [PATCH 5/5] Removeing unused function --- src/sigma/coin.cpp | 10 ---------- src/sigma/coin.h | 1 - 2 files changed, 11 deletions(-) diff --git a/src/sigma/coin.cpp b/src/sigma/coin.cpp index 84ea79a985..faebc74d6b 100644 --- a/src/sigma/coin.cpp +++ b/src/sigma/coin.cpp @@ -157,16 +157,6 @@ void GetAllDenoms(std::vector& denominations_out) { denominations_out.push_back(CoinDenomination::SIGMA_DENOM_0_05); } -void GetAllDenoms(std::vector& denominations_out) { - denominations_out.push_back(100 * COIN); - denominations_out.push_back(25 * COIN); - denominations_out.push_back(10 * COIN); - denominations_out.push_back(1 * COIN); - denominations_out.push_back(50 * CENT); - denominations_out.push_back(10 * CENT); - denominations_out.push_back(5 * CENT); -} - //class PublicCoin PublicCoin::PublicCoin() : denomination(CoinDenomination::SIGMA_DENOM_1) diff --git a/src/sigma/coin.h b/src/sigma/coin.h index ee35246bf8..c2b92737ad 100644 --- a/src/sigma/coin.h +++ b/src/sigma/coin.h @@ -35,7 +35,6 @@ bool RealNumberToDenomination(const double& value, CoinDenomination& denom_out); /// \brief Returns a list of all possible denominations in descending order of value. void GetAllDenoms(std::vector& denominations_out); -void GetAllDenoms(std::vector& denominations_out); class PublicCoin { public: