From 00c42d39d4da986de884e8db30a19f9954f6ef9e Mon Sep 17 00:00:00 2001 From: Cave Spectre Date: Sat, 20 Apr 2019 13:30:05 -0400 Subject: [PATCH 1/4] Redo AutoCombineRewards fixes and improvements based on Fuzzbawls Review. --- src/rpc/client.cpp | 1 + src/rpc/server.cpp | 1 + src/rpc/server.h | 1 + src/wallet/rpcwallet.cpp | 92 ++++++++++++++++++++++++++++++++++------ src/wallet/wallet.cpp | 22 +++++++--- src/wallet/wallet.h | 2 + src/wallet/walletdb.cpp | 14 +++--- src/wallet/walletdb.h | 2 +- 8 files changed, 110 insertions(+), 25 deletions(-) diff --git a/src/rpc/client.cpp b/src/rpc/client.cpp index a9ddea0e2ee4..f8ff15fc2279 100644 --- a/src/rpc/client.cpp +++ b/src/rpc/client.cpp @@ -128,6 +128,7 @@ static const CRPCConvertParam vRPCConvertParams[] = {"setstakesplitthreshold", 0}, {"autocombinerewards", 0}, {"autocombinerewards", 1}, + {"autocombinerewards", 2}, {"getzerocoinbalance", 0}, {"listmintedzerocoins", 0}, {"listmintedzerocoins", 1}, diff --git a/src/rpc/server.cpp b/src/rpc/server.cpp index 6539294df5aa..31788a0af756 100644 --- a/src/rpc/server.cpp +++ b/src/rpc/server.cpp @@ -418,6 +418,7 @@ static const CRPCCommand vRPCCommands[] = {"wallet", "getreceivedbyaddress", &getreceivedbyaddress, false, false, true}, {"wallet", "getstakingstatus", &getstakingstatus, false, false, true}, {"wallet", "getstakesplitthreshold", &getstakesplitthreshold, false, false, true}, + {"wallet", "getautocombineinfo", &getautocombineinfo, false, false, true}, {"wallet", "gettransaction", &gettransaction, false, false, true}, {"wallet", "getunconfirmedbalance", &getunconfirmedbalance, false, false, true}, {"wallet", "getwalletinfo", &getwalletinfo, false, false, true}, diff --git a/src/rpc/server.h b/src/rpc/server.h index 88897e82f113..217eba5944cb 100644 --- a/src/rpc/server.h +++ b/src/rpc/server.h @@ -249,6 +249,7 @@ extern UniValue reservebalance(const UniValue& params, bool fHelp); extern UniValue setstakesplitthreshold(const UniValue& params, bool fHelp); extern UniValue getstakesplitthreshold(const UniValue& params, bool fHelp); extern UniValue multisend(const UniValue& params, bool fHelp); +extern UniValue getautocombineinfo(const UniValue& params, bool fHelp); extern UniValue autocombinerewards(const UniValue& params, bool fHelp); extern UniValue getzerocoinbalance(const UniValue& params, bool fHelp); extern UniValue listmintedzerocoins(const UniValue& params, bool fHelp); diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 35cae7b3ed9c..713ce92db371 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -2262,38 +2262,106 @@ UniValue getstakesplitthreshold(const UniValue& params, bool fHelp) return int(pwalletMain->nStakeSplitThreshold); } +UniValue getautocombineinfo(const UniValue& params, bool fHelp) +{ + if (fHelp || params.size() != 0) + throw runtime_error( + "getautocombineinfo\n" + "Returns the autocombinerewards settings\n" + "\nResult:\n" + "1. enabled (string) The feature turned \"on\" or \"off\".\n" + "2. threshold (numeric) If enabled on, returns autocombine threshold.\n" + "3. frequency (variable) If enabled, returns frequency in blocks, or \"nextblock\" if one time. " + "If one time already run, \"startup\" is returned\n"); + + UniValue obj(UniValue::VOBJ); + obj.push_back(Pair("enabled", pwalletMain->fCombineDust ? "on" : "off")); + if (pwalletMain->fCombineDust) { + obj.push_back(Pair("threshold", + int(pwalletMain->nAutoCombineThreshold))); + if (0 == pwalletMain->nAutoCombineBlockFrequency) { + obj.push_back(Pair("frequency", "nextblock")); + } else { + obj.push_back(Pair("frequency", + int(pwalletMain->nAutoCombineBlockFrequency))); + } + } + else { + if (0 == pwalletMain->nAutoCombineBlockFrequency) { + obj.push_back(Pair("threshold", int(pwalletMain->nAutoCombineThreshold))); + obj.push_back(Pair("frequency", "startup")); + } + } + + return obj; +} + UniValue autocombinerewards(const UniValue& params, bool fHelp) { - bool fEnable; - if (params.size() >= 1) + string strCommand; + bool fEnable = false; + + if (params.size() >= 1) { fEnable = params[0].get_bool(); + } - if (fHelp || params.size() < 1 || (fEnable && params.size() != 2) || params.size() > 2) + if (fHelp || params.size() < 1 || (fEnable && params.size() < 2) || params.size() > 3) throw runtime_error( - "autocombinerewards enable ( threshold )\n" - "\nWallet will automatically monitor for any coins with value below the threshold amount, and combine them if they reside with the same PIVX address\n" - "When autocombinerewards runs it will create a transaction, and therefore will be subject to transaction fees.\n" + "autocombinerewards enable ( threshold ) ( frequency )\n" + "\nWallet will automatically monitor for UTXOs with values below the threshold amount, " + "and combine them into transactions sized to the threshold amount, if they reside with " + "the same UCC address.\n" + "\nA frequency value of \"0\" will run the combine once, on the next available block, " + "and once again on each wallet startup.\n" + "\nWhen autocombinerewards runs it will create a transaction, and therefore will be subject " + "to transaction fees. Transactions will be limited to a full combine of the threshold " + "amount unless the transaction fees are zero.\n" "\nArguments:\n" - "1. enable (boolean, required) Enable auto combine (true) or disable (false)\n" - "2. threshold (numeric, optional) Threshold amount (default: 0)\n" + "1. enable (boolean, required) Enable auto combine (true) or disable (false).\n" + "2. threshold (numeric, optional) (required for enable) target total PIV to combine into one UTXO.\n" + "3. frequency (numeric, optional) Frequency (in blocks) for autocombine to run (default: 15)\n" + + "\nResult:\n" + "1. enabled (string) The feature turned \"on\" or \"off\".\n" + "2. threshold (numeric) If enabled on, returns autocombine threshold.\n" + "3. frequency (variable) If enabled, returns frequency in blocks, or \"nextblock\" if one time.\n" "\nExamples:\n" + - HelpExampleCli("autocombinerewards", "true 500") + HelpExampleRpc("autocombinerewards", "true 500")); + HelpExampleCli("autocombinerewards", "true 500 15") + HelpExampleRpc("autocombinerewards", "true 500 15")); CWalletDB walletdb(pwalletMain->strWalletFile); CAmount nThreshold = 0; + int nBlockFrequency = 15; - if (fEnable) + if (fEnable) { nThreshold = params[1].get_int(); + if (params.size() > 2) { + nBlockFrequency = params[2].get_int(); + if (nBlockFrequency < 0) + nBlockFrequency = 1; + } + } pwalletMain->fCombineDust = fEnable; pwalletMain->nAutoCombineThreshold = nThreshold; + pwalletMain->nAutoCombineBlockFrequency = nBlockFrequency; - if (!walletdb.WriteAutoCombineSettings(fEnable, nThreshold)) + if (!walletdb.WriteAutoCombineSettings(fEnable, nThreshold, nBlockFrequency)) throw runtime_error("Changed settings in wallet but failed to save to database\n"); - return NullUniValue; + UniValue obj(UniValue::VOBJ); + obj.push_back(Pair("enabled", pwalletMain->fCombineDust ? "on" : "off")); + if (pwalletMain->fCombineDust) { + obj.push_back(Pair("threshold", int(pwalletMain->nAutoCombineThreshold))); + if (0 == pwalletMain->nAutoCombineBlockFrequency) { + obj.push_back(Pair("frequency", "nextblock")); + } else { + obj.push_back(Pair("frequency", int(pwalletMain->nAutoCombineBlockFrequency))); + } + } + + return obj; } UniValue printMultiSend() diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 0eb5b659f8f5..55a510702fdf 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4190,6 +4190,18 @@ void CWallet::AutoCombineDust() return; } + if (0 != nAutoCombineBlockFrequency) { + // If the block height hasn't exceeded our frequency; or is not a multiple of our frequency. + if ((nAutoCombineBlockFrequency > chainActive.Tip()->nHeight) || + (chainActive.Tip()->nHeight % nAutoCombineBlockFrequency)) { + return; + } + } else { + // If nAutoCombineBlockFrequency is 0, it's the special onetime case + // so let it rip but turn it off so it doesn't rip again. + fCombineDust = 0; + } + map > mapCoinsByAddress = AvailableCoinsByAddress(true, nAutoCombineThreshold * COIN); //coins are sectioned by address. This combination code only wants to combine inputs that belong to the same address @@ -4216,9 +4228,8 @@ void CWallet::AutoCombineDust() coinControl->Select(outpt); vRewardCoins.push_back(out); nTotalRewardsValue += out.Value(); - - // Combine to the threshold and not way above - if (nTotalRewardsValue > nAutoCombineThreshold * COIN) + // Combine until our total is enough above the threshold to remain above after adjustments + if ((nTotalRewardsValue - nTotalRewardsValue / 10) > nAutoCombineThreshold * COIN) break; // Around 180 bytes per input. We use 190 to be certain @@ -4264,7 +4275,7 @@ void CWallet::AutoCombineDust() } //we don't combine below the threshold unless the fees are 0 to avoid paying fees over fees over fees - if (!maxSize && nTotalRewardsValue < nAutoCombineThreshold * COIN && nFeeRet > 0) + if (!maxSize && vecSend[0].second < nAutoCombineThreshold * COIN && nFeeRet > 0) continue; if (!CommitTransaction(wtx, keyChange)) { @@ -4272,7 +4283,8 @@ void CWallet::AutoCombineDust() continue; } - LogPrintf("AutoCombineDust sent transaction\n"); + LogPrintf("AutoCombineDust sent transaction. Fee=%d, Total Value=%d Sending=%d\n", + nFeeRet, nTotalRewardsValue, vecSend[0].second); delete coinControl; } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 6d159f4e8055..2dad618aa5b7 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -279,6 +279,7 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface //Auto Combine Inputs bool fCombineDust; CAmount nAutoCombineThreshold; + int nAutoCombineBlockFrequency; CWallet() { @@ -330,6 +331,7 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface //Auto Combine Dust fCombineDust = false; nAutoCombineThreshold = 0; + nAutoCombineBlockFrequency = 15; } int getZeromintPercentage() diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index d5df4e390e37..b2715f7b882d 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -278,12 +278,11 @@ bool CWalletDB::EraseMSDisabledAddresses(std::vector vDisabledAddre } return ret; } -bool CWalletDB::WriteAutoCombineSettings(bool fEnable, CAmount nCombineThreshold) +bool CWalletDB::WriteAutoCombineSettings(bool fEnable, CAmount nCombineThreshold, int nBlockFrequency) { nWalletDBUpdated++; - std::pair pSettings; - pSettings.first = fEnable; - pSettings.second = nCombineThreshold; + std::pair enabledMS1(fEnable, nCombineThreshold); + std::pair,int> pSettings(enabledMS1, nBlockFrequency); return Write(std::string("autocombinesettings"), pSettings, true); } @@ -700,10 +699,11 @@ bool ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue, CW ssValue >> strDisabledAddress; pwallet->vDisabledAddresses.push_back(strDisabledAddress); } else if (strType == "autocombinesettings") { - std::pair pSettings; + std::pair,int> pSettings; ssValue >> pSettings; - pwallet->fCombineDust = pSettings.first; - pwallet->nAutoCombineThreshold = pSettings.second; + pwallet->fCombineDust = pSettings.first.first; + pwallet->nAutoCombineThreshold = pSettings.first.second; + pwallet->nAutoCombineBlockFrequency = pSettings.second; } else if (strType == "destdata") { std::string strAddress, strKey, strValue; ssKey >> strAddress; diff --git a/src/wallet/walletdb.h b/src/wallet/walletdb.h index 1f03d4a5b98d..5e290818cb93 100644 --- a/src/wallet/walletdb.h +++ b/src/wallet/walletdb.h @@ -125,7 +125,7 @@ class CWalletDB : public CDB bool WriteMSettings(bool fMultiSendStake, bool fMultiSendMasternode, int nLastMultiSendHeight); bool WriteMSDisabledAddresses(std::vector vDisabledAddresses); bool EraseMSDisabledAddresses(std::vector vDisabledAddresses); - bool WriteAutoCombineSettings(bool fEnable, CAmount nCombineThreshold); + bool WriteAutoCombineSettings(bool fEnable, CAmount nCombineThreshold, int nBlockFrequency); bool WriteDefaultKey(const CPubKey& vchPubKey); From ba7cd7f2ac75daeb5f1313f13e93e62df00c4b79 Mon Sep 17 00:00:00 2001 From: CaveSpectre11 <36988814+CaveSpectre11@users.noreply.github.com> Date: Wed, 1 May 2019 22:15:48 -0400 Subject: [PATCH 2/4] Correct porting typo --- src/wallet/rpcwallet.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 713ce92db371..4d64ebafabe1 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -2310,7 +2310,7 @@ UniValue autocombinerewards(const UniValue& params, bool fHelp) "autocombinerewards enable ( threshold ) ( frequency )\n" "\nWallet will automatically monitor for UTXOs with values below the threshold amount, " "and combine them into transactions sized to the threshold amount, if they reside with " - "the same UCC address.\n" + "the same PIVX address.\n" "\nA frequency value of \"0\" will run the combine once, on the next available block, " "and once again on each wallet startup.\n" "\nWhen autocombinerewards runs it will create a transaction, and therefore will be subject " From 3db3b1c9187aadb0930c5a248bf48c9df3d8843d Mon Sep 17 00:00:00 2001 From: Cave Spectre Date: Sat, 15 Jun 2019 17:48:35 -0400 Subject: [PATCH 3/4] Update autocombine settings to provide backwards compatibility with wallets that have used autocombinesettings in the past. If that is the case, we will load the default threshold of "15" in, and issue a message to the log file for them to update their settings. As soon as they use "autocombinerewards" again, it will save the old format with a "-1" for the threshold, signalling to walletdb that the new format is now being used and to disregard the old format. Also, a further improvement has been made, making a threshold of "0" much more useful. Rather than simply combining two dust piles, it is now used as a special value to indicate that there is no threshold set, and to combine as many dust piles as possible. --- src/wallet/rpcwallet.cpp | 10 ++++++---- src/wallet/wallet.cpp | 6 ++++-- src/wallet/walletdb.cpp | 28 +++++++++++++++++++++++++--- 3 files changed, 35 insertions(+), 9 deletions(-) diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 713ce92db371..89cb659ae410 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -2279,7 +2279,7 @@ UniValue getautocombineinfo(const UniValue& params, bool fHelp) if (pwalletMain->fCombineDust) { obj.push_back(Pair("threshold", int(pwalletMain->nAutoCombineThreshold))); - if (0 == pwalletMain->nAutoCombineBlockFrequency) { + if (pwalletMain->nAutoCombineBlockFrequency == 0) { obj.push_back(Pair("frequency", "nextblock")); } else { obj.push_back(Pair("frequency", @@ -2287,7 +2287,7 @@ UniValue getautocombineinfo(const UniValue& params, bool fHelp) } } else { - if (0 == pwalletMain->nAutoCombineBlockFrequency) { + if (pwalletMain->nAutoCombineBlockFrequency == 0) { obj.push_back(Pair("threshold", int(pwalletMain->nAutoCombineThreshold))); obj.push_back(Pair("frequency", "startup")); } @@ -2310,7 +2310,9 @@ UniValue autocombinerewards(const UniValue& params, bool fHelp) "autocombinerewards enable ( threshold ) ( frequency )\n" "\nWallet will automatically monitor for UTXOs with values below the threshold amount, " "and combine them into transactions sized to the threshold amount, if they reside with " - "the same UCC address.\n" + "the same PIVX address.\n" + "\nA threshold value of \"0\" will combine all the UTXOs, up to the maximum possible " + "within the maximum transaction size of a block.\n" "\nA frequency value of \"0\" will run the combine once, on the next available block, " "and once again on each wallet startup.\n" "\nWhen autocombinerewards runs it will create a transaction, and therefore will be subject " @@ -2354,7 +2356,7 @@ UniValue autocombinerewards(const UniValue& params, bool fHelp) obj.push_back(Pair("enabled", pwalletMain->fCombineDust ? "on" : "off")); if (pwalletMain->fCombineDust) { obj.push_back(Pair("threshold", int(pwalletMain->nAutoCombineThreshold))); - if (0 == pwalletMain->nAutoCombineBlockFrequency) { + if (pwalletMain->nAutoCombineBlockFrequency == 0) { obj.push_back(Pair("frequency", "nextblock")); } else { obj.push_back(Pair("frequency", int(pwalletMain->nAutoCombineBlockFrequency))); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 55a510702fdf..7ff8fe9cd2a5 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4190,7 +4190,7 @@ void CWallet::AutoCombineDust() return; } - if (0 != nAutoCombineBlockFrequency) { + if (nAutoCombineBlockFrequency != 0) { // If the block height hasn't exceeded our frequency; or is not a multiple of our frequency. if ((nAutoCombineBlockFrequency > chainActive.Tip()->nHeight) || (chainActive.Tip()->nHeight % nAutoCombineBlockFrequency)) { @@ -4229,7 +4229,9 @@ void CWallet::AutoCombineDust() vRewardCoins.push_back(out); nTotalRewardsValue += out.Value(); // Combine until our total is enough above the threshold to remain above after adjustments - if ((nTotalRewardsValue - nTotalRewardsValue / 10) > nAutoCombineThreshold * COIN) + // Unless no threshold is set; in which case we want to keep going until we hit MAX_STANDARD_TX_SIZE + if (nAutoCombineThreshold && + ((nTotalRewardsValue - nTotalRewardsValue / 10) > nAutoCombineThreshold * COIN)) break; // Around 180 bytes per input. We use 190 to be certain diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index b2715f7b882d..6235e15debc2 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -281,9 +281,19 @@ bool CWalletDB::EraseMSDisabledAddresses(std::vector vDisabledAddre bool CWalletDB::WriteAutoCombineSettings(bool fEnable, CAmount nCombineThreshold, int nBlockFrequency) { nWalletDBUpdated++; - std::pair enabledMS1(fEnable, nCombineThreshold); - std::pair,int> pSettings(enabledMS1, nBlockFrequency); - return Write(std::string("autocombinesettings"), pSettings, true); + // Overwrite the old format with a special flag + std::pair pSettingsOld; + pSettingsOld.first = fEnable; + pSettingsOld.second = -1; // Negatives doesn't make sense, so we can use them as flags + if (Write(std::string("autocombinesettings"), pSettingsOld, true)) { + // Now add the new format as v2 + std::pair enabledMS1(fEnable, nCombineThreshold); + std::pair,int> pSettings(enabledMS1, nBlockFrequency); + return Write(std::string("autocombinesettingsV2"), pSettings, true); + } else { + // report the old format write failed + return false; + } } bool CWalletDB::WriteDefaultKey(const CPubKey& vchPubKey) @@ -699,6 +709,18 @@ bool ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue, CW ssValue >> strDisabledAddress; pwallet->vDisabledAddresses.push_back(strDisabledAddress); } else if (strType == "autocombinesettings") { + // Old Format + std::pair pSettings; + ssValue >> pSettings; + if (pSettings.second >= 0) { + // Convert the old format + pwallet->fCombineDust = pSettings.first; + pwallet->nAutoCombineThreshold = pSettings.second; + pwallet->nAutoCombineBlockFrequency = 15; // Default + LogPrintf("autocombinerewards settings are stale, refresh your settings for the new format\n"); + } + } else if (strType == "autocombinesettingsV2") { + // New Format std::pair,int> pSettings; ssValue >> pSettings; pwallet->fCombineDust = pSettings.first.first; From abd6b1725d3f2cc8beba8bff12e408beefdabb9e Mon Sep 17 00:00:00 2001 From: CaveSpectre11 <36988814+CaveSpectre11@users.noreply.github.com> Date: Sat, 15 Jun 2019 18:09:17 -0400 Subject: [PATCH 4/4] remove stale variable --- src/wallet/rpcwallet.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 89cb659ae410..6d761caeda8c 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -2298,7 +2298,6 @@ UniValue getautocombineinfo(const UniValue& params, bool fHelp) UniValue autocombinerewards(const UniValue& params, bool fHelp) { - string strCommand; bool fEnable = false; if (params.size() >= 1) {