From 6b0e622c25ddba656149de0f9f8b7f2a0aa0deba Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Sun, 13 Sep 2015 23:23:59 +0200 Subject: [PATCH 1/3] [wallet] Refactor to use new MIN_CHANGE * Introduce new constant MIN_CHANGE and use it instead of the hardcoded "CENT" * Add test case for MIN_CHANGE * Introduce new constant for -mintxfee default: DEFAULT_TRANSACTION_MINFEE = 1000 --- src/qt/coincontroldialog.cpp | 2 +- src/wallet/test/wallet_tests.cpp | 111 ++++++++++++++++++------------- src/wallet/wallet.cpp | 15 ++--- src/wallet/wallet.h | 11 +++ 4 files changed, 84 insertions(+), 55 deletions(-) diff --git a/src/qt/coincontroldialog.cpp b/src/qt/coincontroldialog.cpp index 51008ad2d..74bc8dbec 100644 --- a/src/qt/coincontroldialog.cpp +++ b/src/qt/coincontroldialog.cpp @@ -567,7 +567,7 @@ void CoinControlDialog::updateLabels(WalletModel *model, QDialog* dialog) nChange -= nPayFee; // Never create dust outputs; if we would, just add the dust to the fee. - if (nChange > 0 && nChange < CENT) + if (nChange > 0 && nChange < MIN_CHANGE) { CTxOut txout(nChange, (CScript)std::vector(24, 0)); if (txout.IsDust(::minRelayTxFee)) diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index a5bc52b8d..8b9292bd1 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -117,7 +117,7 @@ BOOST_AUTO_TEST_CASE(coin_selection_tests) // try making 34 cents from 1,2,5,10,20 - we can't do it exactly BOOST_CHECK( wallet.SelectCoinsMinConf(34 * CENT, 1, 1, vCoins, setCoinsRet, nValueRet)); - BOOST_CHECK_GT(nValueRet, 34 * CENT); // but should get more than 34 cents + BOOST_CHECK_EQUAL(nValueRet, 35 * CENT); // but 35 cents is closest BOOST_CHECK_EQUAL(setCoinsRet.size(), 3U); // the best should be 20+10+5. it's incredibly unlikely the 1 or 2 got included (but possible) // when we try making 7 cents, the smaller coins (1,2,5) are enough. We should see just 2+5 @@ -185,33 +185,34 @@ BOOST_AUTO_TEST_CASE(coin_selection_tests) BOOST_CHECK_EQUAL(nValueRet, 2 * COIN); // we should get 2 BTC in 1 coin BOOST_CHECK_EQUAL(setCoinsRet.size(), 1U); - // empty the wallet and start again, now with fractions of a cent, to test sub-cent change avoidance + // empty the wallet and start again, now with fractions of a cent, to test small change avoidance + empty_wallet(); - add_coin(0.1*CENT); - add_coin(0.2*CENT); - add_coin(0.3*CENT); - add_coin(0.4*CENT); - add_coin(0.5*CENT); + add_coin(0.1*MIN_CHANGE); + add_coin(0.2*MIN_CHANGE); + add_coin(0.3*MIN_CHANGE); + add_coin(0.4*MIN_CHANGE); + add_coin(0.5*MIN_CHANGE); - // try making 1 cent from 0.1 + 0.2 + 0.3 + 0.4 + 0.5 = 1.5 cents - // we'll get sub-cent change whatever happens, so can expect 1.0 exactly - BOOST_CHECK( wallet.SelectCoinsMinConf(1 * CENT, 1, 1, vCoins, setCoinsRet, nValueRet)); - BOOST_CHECK_EQUAL(nValueRet, 1 * CENT); + // try making 1 * MIN_CHANGE from the 1.5 * MIN_CHANGE + // we'll get change smaller than MIN_CHANGE whatever happens, so can expect MIN_CHANGE exactly + BOOST_CHECK( wallet.SelectCoinsMinConf(MIN_CHANGE, 1, 1, vCoins, setCoinsRet, nValueRet)); + BOOST_CHECK_EQUAL(nValueRet, MIN_CHANGE); - // but if we add a bigger coin, making it possible to avoid sub-cent change, things change: - add_coin(1111*CENT); + // but if we add a bigger coin, small change is avoided + add_coin(1111*MIN_CHANGE); - // try making 1 cent from 0.1 + 0.2 + 0.3 + 0.4 + 0.5 + 1111 = 1112.5 cents - BOOST_CHECK( wallet.SelectCoinsMinConf(1 * CENT, 1, 1, vCoins, setCoinsRet, nValueRet)); - BOOST_CHECK_EQUAL(nValueRet, 1 * CENT); // we should get the exact amount + // try making 1 from 0.1 + 0.2 + 0.3 + 0.4 + 0.5 + 1111 = 1112.5 + BOOST_CHECK( wallet.SelectCoinsMinConf(1 * MIN_CHANGE, 1, 1, vCoins, setCoinsRet, nValueRet)); + BOOST_CHECK_EQUAL(nValueRet, 1 * MIN_CHANGE); // we should get the exact amount - // if we add more sub-cent coins: - add_coin(0.6*CENT); - add_coin(0.7*CENT); + // if we add more small coins: + add_coin(0.6*MIN_CHANGE); + add_coin(0.7*MIN_CHANGE); - // and try again to make 1.0 cents, we can still make 1.0 cents - BOOST_CHECK( wallet.SelectCoinsMinConf(1 * CENT, 1, 1, vCoins, setCoinsRet, nValueRet)); - BOOST_CHECK_EQUAL(nValueRet, 1 * CENT); // we should get the exact amount + // and try again to make 1.0 * MIN_CHANGE + BOOST_CHECK( wallet.SelectCoinsMinConf(1 * MIN_CHANGE, 1, 1, vCoins, setCoinsRet, nValueRet)); + BOOST_CHECK_EQUAL(nValueRet, 1 * MIN_CHANGE); // we should get the exact amount // run the 'mtgox' test (see http://blockexplorer.com/tx/29a3efd3ef04f9153d47a990bd7b048a4b2d213daaa5fb8ed670fb85f13bdbcf) // they tried to consolidate 10 50k coins into one 500k coin, and ended up with 50k in change @@ -223,45 +224,65 @@ BOOST_AUTO_TEST_CASE(coin_selection_tests) BOOST_CHECK_EQUAL(nValueRet, 500000 * COIN); // we should get the exact amount BOOST_CHECK_EQUAL(setCoinsRet.size(), 10U); // in ten coins - // if there's not enough in the smaller coins to make at least 1 cent change (0.5+0.6+0.7 < 1.0+1.0), + // if there's not enough in the smaller coins to make at least 1 * MIN_CHANGE change (0.5+0.6+0.7 < 1.0+1.0), // we need to try finding an exact subset anyway // sometimes it will fail, and so we use the next biggest coin: empty_wallet(); - add_coin(0.5 * CENT); - add_coin(0.6 * CENT); - add_coin(0.7 * CENT); - add_coin(1111 * CENT); - BOOST_CHECK( wallet.SelectCoinsMinConf(1 * CENT, 1, 1, vCoins, setCoinsRet, nValueRet)); - BOOST_CHECK_EQUAL(nValueRet, 1111 * CENT); // we get the bigger coin + add_coin(0.5 * MIN_CHANGE); + add_coin(0.6 * MIN_CHANGE); + add_coin(0.7 * MIN_CHANGE); + add_coin(1111 * MIN_CHANGE); + BOOST_CHECK( wallet.SelectCoinsMinConf(1 * MIN_CHANGE, 1, 1, vCoins, setCoinsRet, nValueRet)); + BOOST_CHECK_EQUAL(nValueRet, 1111 * MIN_CHANGE); // we get the bigger coin BOOST_CHECK_EQUAL(setCoinsRet.size(), 1U); // but sometimes it's possible, and we use an exact subset (0.4 + 0.6 = 1.0) empty_wallet(); - add_coin(0.4 * CENT); - add_coin(0.6 * CENT); - add_coin(0.8 * CENT); - add_coin(1111 * CENT); - BOOST_CHECK( wallet.SelectCoinsMinConf(1 * CENT, 1, 1, vCoins, setCoinsRet, nValueRet)); - BOOST_CHECK_EQUAL(nValueRet, 1 * CENT); // we should get the exact amount + add_coin(0.4 * MIN_CHANGE); + add_coin(0.6 * MIN_CHANGE); + add_coin(0.8 * MIN_CHANGE); + add_coin(1111 * MIN_CHANGE); + BOOST_CHECK( wallet.SelectCoinsMinConf(MIN_CHANGE, 1, 1, vCoins, setCoinsRet, nValueRet)); + BOOST_CHECK_EQUAL(nValueRet, MIN_CHANGE); // we should get the exact amount BOOST_CHECK_EQUAL(setCoinsRet.size(), 2U); // in two coins 0.4+0.6 - // test avoiding sub-cent change + // test avoiding small change empty_wallet(); - add_coin(0.0005 * COIN); - add_coin(0.01 * COIN); - add_coin(1 * COIN); + add_coin(0.05 * MIN_CHANGE); + add_coin(1 * MIN_CHANGE); + add_coin(100 * MIN_CHANGE); - // trying to make 1.0001 from these three coins - BOOST_CHECK( wallet.SelectCoinsMinConf(1.0001 * COIN, 1, 1, vCoins, setCoinsRet, nValueRet)); - BOOST_CHECK_EQUAL(nValueRet, 1.0105 * COIN); // we should get all coins + // trying to make 100.01 from these three coins + BOOST_CHECK( wallet.SelectCoinsMinConf(100.01 * MIN_CHANGE, 1, 1, vCoins, setCoinsRet, nValueRet)); + BOOST_CHECK_EQUAL(nValueRet, 101.05 * MIN_CHANGE); // we should get all coins BOOST_CHECK_EQUAL(setCoinsRet.size(), 3U); - // but if we try to make 0.999, we should take the bigger of the two small coins to avoid sub-cent change - BOOST_CHECK( wallet.SelectCoinsMinConf(0.999 * COIN, 1, 1, vCoins, setCoinsRet, nValueRet)); - BOOST_CHECK_EQUAL(nValueRet, 1.01 * COIN); // we should get 1 + 0.01 + // but if we try to make 99.9, we should take the bigger of the two small coins to avoid small change + BOOST_CHECK( wallet.SelectCoinsMinConf(99.9 * MIN_CHANGE, 1, 1, vCoins, setCoinsRet, nValueRet)); + BOOST_CHECK_EQUAL(nValueRet, 101 * MIN_CHANGE); BOOST_CHECK_EQUAL(setCoinsRet.size(), 2U); + // test with many inputs + for (CAmount amt=1500; amt < COIN; amt*=10) { + empty_wallet(); + // Create 676 inputs (= MAX_STANDARD_TX_SIZE / 148 bytes per input) + for (uint16_t j = 0; j < 676; j++) + add_coin(amt); + BOOST_CHECK(wallet.SelectCoinsMinConf(2000, 1, 1, vCoins, setCoinsRet, nValueRet)); + if (amt - 2000 < MIN_CHANGE) { + // needs more than one input: + uint16_t returnSize = std::ceil((2000.0 + MIN_CHANGE)/amt); + CAmount returnValue = amt * returnSize; + BOOST_CHECK_EQUAL(nValueRet, returnValue); + BOOST_CHECK_EQUAL(setCoinsRet.size(), returnSize); + } else { + // one input is sufficient: + BOOST_CHECK_EQUAL(nValueRet, amt); + BOOST_CHECK_EQUAL(setCoinsRet.size(), 1U); + } + } + // test randomness { empty_wallet(); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index c3b117220..2414532f7 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -47,7 +47,7 @@ bool fPayAtLeastCustomFee = true; * Fees smaller than this (in satoshi) are considered zero fee (for transaction creation) * Override with -mintxfee */ -CFeeRate CWallet::minTxFee = CFeeRate(1000); +CFeeRate CWallet::minTxFee = CFeeRate(DEFAULT_TRANSACTION_MINFEE); /** @defgroup mapWallet * @@ -1498,9 +1498,6 @@ CAmount CWallet::GetImmatureWatchOnlyBalance() const return nTotal; } -/** - * populate vCoins with vector of available COutputs. - */ void CWallet::AvailableCoins(vector& vCoins, bool fOnlyConfirmed, const CCoinControl *coinControl, bool fIncludeZeroValue) const { vCoins.clear(); @@ -1620,7 +1617,7 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, int nConfMine, int nValueRet += coin.first; return true; } - else if (n < nTargetValue + CENT) + else if (n < nTargetValue + MIN_CHANGE) { vValue.push_back(coin); nTotalLower += n; @@ -1655,14 +1652,14 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, int nConfMine, int vector vfBest; CAmount nBest; - ApproximateBestSubset(vValue, nTotalLower, nTargetValue, vfBest, nBest, 1000); - if (nBest != nTargetValue && nTotalLower >= nTargetValue + CENT) - ApproximateBestSubset(vValue, nTotalLower, nTargetValue + CENT, vfBest, nBest, 1000); + ApproximateBestSubset(vValue, nTotalLower, nTargetValue, vfBest, nBest); + if (nBest != nTargetValue && nTotalLower >= nTargetValue + MIN_CHANGE) + ApproximateBestSubset(vValue, nTotalLower, nTargetValue + MIN_CHANGE, vfBest, nBest); // If we have a bigger coin and (either the stochastic approximation didn't find a good solution, // or the next bigger coin is closer), return the bigger coin if (coinLowestLarger.second.first && - ((nBest != nTargetValue && nBest < nTargetValue + CENT) || coinLowestLarger.first <= nBest)) + ((nBest != nTargetValue && nBest < nTargetValue + MIN_CHANGE) || coinLowestLarger.first <= nBest)) { setCoinsRet.insert(coinLowestLarger.second); nValueRet += coinLowestLarger.first; diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index bd30b67b0..143ca22bd 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -41,8 +41,12 @@ extern bool fPayAtLeastCustomFee; static const CAmount DEFAULT_TRANSACTION_FEE = 0; //! -paytxfee will warn if called with a higher fee than this amount (in satoshis) per KB static const CAmount nHighTransactionFeeWarning = 0.01 * COIN; +//! -mintxfee default +static const CAmount DEFAULT_TRANSACTION_MINFEE = 1000; //! -maxtxfee default static const CAmount DEFAULT_TRANSACTION_MAXFEE = 0.1 * COIN; +//! minimum change amount +static const CAmount MIN_CHANGE = CENT; //! -txconfirmtarget default static const unsigned int DEFAULT_TX_CONFIRM_TARGET = 2; //! -maxtxfee will warn if called with a higher fee than this amount (in satoshis) @@ -544,7 +548,14 @@ public: //! check whether we are allowed to upgrade (or already support) to the named feature bool CanSupportFeature(enum WalletFeature wf) { AssertLockHeld(cs_wallet); return nWalletMaxVersion >= wf; } + /** + * populate vCoins with vector of available COutputs. + */ void AvailableCoins(std::vector& vCoins, bool fOnlyConfirmed=true, const CCoinControl *coinControl = NULL, bool fIncludeZeroValue=false) const; + /** + * Select coins until nTargetValue is reached. Return the actual value + * and the corresponding coin set. + */ bool SelectCoinsMinConf(const CAmount& nTargetValue, int nConfMine, int nConfTheirs, std::vector vCoins, std::set >& setCoinsRet, CAmount& nValueRet) const; bool IsSpent(const uint256& hash, unsigned int n) const; From a9c73a130ebf2369381241d0053c8fa0a301e0fc Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Mon, 14 Sep 2015 14:49:59 +0200 Subject: [PATCH 2/3] [wallet] Add comments for doxygen --- src/wallet/wallet.cpp | 1 + src/wallet/wallet.h | 22 ++++++++++++++++++++-- 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 2414532f7..4e05f519c 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1842,6 +1842,7 @@ bool CWallet::CreateTransaction(const vector& vecSend, CWalletTx& wt LOCK2(cs_main, cs_wallet); { nFeeRet = 0; + // Start with no fee and loop until there is enough fee while (true) { txNew.vin.clear(); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 143ca22bd..cb2c09b89 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -451,6 +451,11 @@ public: class CWallet : public CCryptoKeyStore, public CValidationInterface { private: + /** + * Select a set of coins such that nValueRet >= nTargetValue and at least + * all coins from coinControl are selected; Never select unconfirmed coins + * if they are not ours + */ bool SelectCoins(const CAmount& nTargetValue, std::set >& setCoinsRet, CAmount& nValueRet, const CCoinControl *coinControl = NULL) const; CWalletDB *pwalletdbEncryption; @@ -552,9 +557,12 @@ public: * populate vCoins with vector of available COutputs. */ void AvailableCoins(std::vector& vCoins, bool fOnlyConfirmed=true, const CCoinControl *coinControl = NULL, bool fIncludeZeroValue=false) const; + /** - * Select coins until nTargetValue is reached. Return the actual value - * and the corresponding coin set. + * Shuffle and select coins until nTargetValue is reached while avoiding + * small change; This method is stochastic for some inputs and upon + * completion the coin set and corresponding actual target value is + * assembled */ bool SelectCoinsMinConf(const CAmount& nTargetValue, int nConfMine, int nConfTheirs, std::vector vCoins, std::set >& setCoinsRet, CAmount& nValueRet) const; @@ -638,7 +646,17 @@ public: CAmount GetWatchOnlyBalance() const; CAmount GetUnconfirmedWatchOnlyBalance() const; CAmount GetImmatureWatchOnlyBalance() const; + + /** + * Insert additional inputs into the transaction by + * calling CreateTransaction(); + */ bool FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, int& nChangePosRet, std::string& strFailReason, bool includeWatching); + + /** + * Create a new transaction paying the recipients with a set of coins + * selected by SelectCoins(); Also create the change output, when needed + */ bool CreateTransaction(const std::vector& vecSend, CWalletTx& wtxNew, CReserveKey& reservekey, CAmount& nFeeRet, int& nChangePosRet, std::string& strFailReason, const CCoinControl *coinControl = NULL, bool sign = true); bool CommitTransaction(CWalletTx& wtxNew, CReserveKey& reservekey); From 6342a4889bc4add42c270e1e0aee246a7f0692dc Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Thu, 29 Oct 2015 16:52:40 +0100 Subject: [PATCH 3/3] Init: Use DEFAULT_TRANSACTION_MINFEE in help message --- src/init.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/init.cpp b/src/init.cpp index f03388120..48680edc2 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -374,7 +374,7 @@ std::string HelpMessage(HelpMessageMode mode) strUsage += HelpMessageOpt("-keypool=", strprintf(_("Set key pool size to (default: %u)"), 100)); if (showDebug) strUsage += HelpMessageOpt("-mintxfee=", strprintf("Fees (in %s/kB) smaller than this are considered zero fee for transaction creation (default: %s)", - CURRENCY_UNIT, FormatMoney(CWallet::minTxFee.GetFeePerK()))); + CURRENCY_UNIT, FormatMoney(DEFAULT_TRANSACTION_MINFEE))); strUsage += HelpMessageOpt("-paytxfee=", strprintf(_("Fee (in %s/kB) to add to transactions you send (default: %s)"), CURRENCY_UNIT, FormatMoney(payTxFee.GetFeePerK()))); strUsage += HelpMessageOpt("-rescan", _("Rescan the block chain for missing wallet transactions") + " " + _("on startup"));