Skip to content

Commit

Permalink
Merge #2812: [Wallet] Fix AutoCombineDust and improve setautocombinet…
Browse files Browse the repository at this point in the history 
…hreshold

63488be Update functional test wallet_autocombine.py (Alessandro Rezzi)
e7437c9 Update functional test DeprecatedRpcTest (Alessandro Rezzi)
1c300a2 Remove deprecated autocombinerewards RPC command (Alessandro Rezzi)
9ea65d5 Fixed and improved setautocombinethreshold (Alessandro Rezzi)
66dbb2e Fixed current AutoCombineDust (Alessandro Rezzi)

Pull request description:

  AutoCombineDust has been bugged since an old PR see (#953  #518).

  To make an example of what's happening let's consider for simplicity  nAutoCombineThreshold = 100 and let's say that the user has 2 UTXOs one of 90 PIVs and the other of 15 PIVs.

  With the current code of AutoCombineDust we will therefore try to send a total of 90+15 = 105 PIVs to ourself, more precisely to avoid the "Insufficient funds" the primary output will contain only the 90% =94.5 PIVs and the remaining 10% = 10.5 PIVs is used to pay fees + sent as a change to ourself. As you can see we end up with two new UTXOs, which are still both smaller than the threshold.

  This process is iterated block by block until we end up with a single UTXO which is smaller than the threshold, i.e in this example 5 PIVs will be burn in fees.

  Moreover the process of "checking all UTXOs at each block for each address of your wallet"  can potentially consume a lot of resources especially if the user has a lot of small UTXOs, therefore I added a new parameter "frequency" which will check for dust every N blocks (where N is the value of frequency), and which is set by default at N = 30.

  Finally I removed the old autocombinerewards since it is deprecated and was planned to be removed in v6.0 (https://github.com/PIVX-Project/PIVX/blob/master/doc/release-notes.md)

ACKs for top commit:
  Fuzzbawls:
    ACK 63488be
  Liquid369:
    tACK 63488be

Tree-SHA512: b06046590d63399e4f579fbcd899f77346c0c5b8fd69613b60874f3920b2d776da97878a5794336c873012757ebef969467f4381441903231bf41c4d70c4635f
  • Loading branch information
@Fuzzbawls
Fuzzbawls committed Mar 8, 2023
2 parents 9240e15 + 63488be commit ac8a74d
Show file tree
Hide file tree
Showing 8 changed files with 72 additions and 87 deletions.
3 changes: 1 addition & 2 deletions src/rpc/client.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,6 @@ static const CRPCConvertParam vRPCConvertParams[] = {
{ "addmultisigaddress", 0, "nrequired" },
{ "addmultisigaddress", 1, "keys" },
{ "addpeeraddress", 1, "port" },
{ "autocombinerewards", 0, "enable" },
{ "autocombinerewards", 1, "threshold" },
{ "cleanbudget", 0, "try_sync" },
{ "createmultisig", 0, "nrequired" },
{ "createmultisig", 1, "keys" },
Expand Down Expand Up @@ -149,6 +147,7 @@ static const CRPCConvertParam vRPCConvertParams[] = {
{ "sendtoaddress", 4, "subtract_fee" },
{ "setautocombinethreshold", 0, "enable" },
{ "setautocombinethreshold", 1, "threshold" },
{ "setautocombinethreshold", 2, "frequency"},
{ "setnetworkactive", 0, "active"},
{ "setban", 2, "bantime" },
{ "setban", 3, "absolute" },
Expand Down
73 changes: 19 additions & 54 deletions src/wallet/rpcwallet.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4267,6 +4267,7 @@ UniValue getwalletinfo(const JSONRPCRequest& request)
// Autocombine settings
obj.pushKV("autocombine_enabled", pwallet->fCombineDust);
obj.pushKV("autocombine_threshold", ValueFromAmount(pwallet->nAutoCombineThreshold));
obj.pushKV("autocombine_frequency", pwallet->frequency);

// Keypool information
obj.pushKV("keypoololdest", pwallet->GetOldestKeyPoolTime());
Expand Down Expand Up @@ -4449,62 +4450,14 @@ UniValue getstakesplitthreshold(const JSONRPCRequest& request)
return ValueFromAmount(pwallet->GetStakeSplitThreshold());
}

UniValue autocombinerewards(const JSONRPCRequest& request)
{
if (!IsDeprecatedRPCEnabled("autocombinerewards")) {
if (request.fHelp) {
throw std::runtime_error("autocombinerewards (Deprecated, will be removed in v6.0. To use this command, start pivxd with -deprecatedrpc=autocombinerewards)");
}
throw JSONRPCError(RPC_METHOD_DEPRECATED, "autocombinerewards is deprecated and will be removed in v6.0. To use this command, start pivxd with -deprecatedrpc=autocombinerewards");
}

CWallet * const pwallet = GetWalletForJSONRPCRequest(request);

if (!EnsureWalletIsAvailable(pwallet, request.fHelp))
return NullUniValue;

bool fEnable = !request.params.empty() && request.params[0].get_bool();

if (request.fHelp || request.params.empty() || (fEnable && request.params.size() != 2) || request.params.size() > 2)
throw std::runtime_error(
"autocombinerewards enable ( threshold )\n"
"\nDEPRECATED!!! This command has been replaced with setautocombinethreshold and getautocombinethreshold and will be removed in a future version!!!\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"

"\nArguments:\n"
"1. enable (boolean, required) Enable auto combine (true) or disable (false)\n"
"2. threshold (numeric, optional, required if enable=True) Threshold amount (default: 0)\n"

"\nExamples:\n" +
HelpExampleCli("autocombinerewards", "true 500") + HelpExampleRpc("autocombinerewards", "true 500"));

WalletBatch batch(pwallet->GetDBHandle());
CAmount nThreshold = 0;

if (fEnable && request.params.size() > 1) {
nThreshold = AmountFromValue(request.params[1]);
if (nThreshold < COIN)
throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("The threshold value cannot be less than %s", FormatMoney(COIN)));
}

pwallet->fCombineDust = fEnable;
pwallet->nAutoCombineThreshold = nThreshold;

if (!batch.WriteAutoCombineSettings(fEnable, nThreshold))
throw std::runtime_error("Changed settings in wallet but failed to save to database\n");

return NullUniValue;
}

UniValue setautocombinethreshold(const JSONRPCRequest& request)
{
CWallet * const pwallet = GetWalletForJSONRPCRequest(request);

if (!EnsureWalletIsAvailable(pwallet, request.fHelp))
return NullUniValue;

if (request.fHelp || request.params.empty() || request.params.size() > 2)
if (request.fHelp || request.params.empty() || request.params.size() > 3)
throw std::runtime_error(
"setautocombinethreshold enable ( value )\n"
"\nThis will set the auto-combine threshold value.\n"
Expand All @@ -4514,21 +4467,24 @@ UniValue setautocombinethreshold(const JSONRPCRequest& request)
"\nArguments:\n"
"1. enable (boolean, required) Enable auto combine (true) or disable (false).\n"
"2. threshold (numeric, optional. required if enable is true) Threshold amount. Must be greater than 1.\n"
"3. frequency (numeric, optional. default value is 30 if not provided). Check for UTXOs to autocombine each N blocks where N is the frequency.\n"

"\nResult:\n"
"{\n"
" \"enabled\": true|false, (boolean) true if auto-combine is enabled, otherwise false\n"
" \"threshold\": n.nnn, (numeric) auto-combine threshold in PIV\n"
" \"frequency\": n.nnn, (numeric) auto-combine frequency in blocks\n"
" \"saved\": true|false (boolean) true if setting was saved to the database, otherwise false\n"
"}\n"

"\nExamples:\n" +
HelpExampleCli("setautocombinethreshold", "true 500.12") + HelpExampleRpc("setautocombinethreshold", "true, 500.12"));
HelpExampleCli("setautocombinethreshold", "true 500.12 40") + HelpExampleRpc("setautocombinethreshold", "true, 500.12, 40"));

RPCTypeCheck(request.params, {UniValue::VBOOL, UniValue::VNUM});
RPCTypeCheck(request.params, {UniValue::VBOOL, UniValue::VNUM, UniValue::VNUM});

bool fEnable = request.params[0].get_bool();
CAmount nThreshold = 0;
int frequency = 30;

if (fEnable) {
if (request.params.size() < 2) {
Expand All @@ -4537,6 +4493,12 @@ UniValue setautocombinethreshold(const JSONRPCRequest& request)
nThreshold = AmountFromValue(request.params[1]);
if (nThreshold < COIN)
throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("The threshold value cannot be less than %s", FormatMoney(COIN)));
if (request.params.size() == 3) {
frequency = request.params[2].get_int();
if (frequency <= 0) {
throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Frequency must be greater than 0"));
}
}
}

WalletBatch batch(pwallet->GetDBHandle());
Expand All @@ -4545,11 +4507,13 @@ UniValue setautocombinethreshold(const JSONRPCRequest& request)
LOCK(pwallet->cs_wallet);
pwallet->fCombineDust = fEnable;
pwallet->nAutoCombineThreshold = nThreshold;
pwallet->frequency = frequency;

UniValue result(UniValue::VOBJ);
result.pushKV("enabled", fEnable);
result.pushKV("threshold", ValueFromAmount(pwallet->nAutoCombineThreshold));
if (batch.WriteAutoCombineSettings(fEnable, nThreshold)) {
result.pushKV("frequency", frequency);
if (batch.WriteAutoCombineSettings(fEnable, nThreshold, frequency)) {
result.pushKV("saved", "true");
} else {
result.pushKV("saved", "false");
Expand All @@ -4569,12 +4533,13 @@ UniValue getautocombinethreshold(const JSONRPCRequest& request)
if (request.fHelp || !request.params.empty())
throw std::runtime_error(
"getautocombinethreshold\n"
"\nReturns the current threshold for auto combining UTXOs, if any\n"
"\nReturns the current threshold and frequency for auto combining UTXOs, if any\n"

"\nResult:\n"
"{\n"
" \"enabled\": true|false, (boolean) true if auto-combine is enabled, otherwise false\n"
" \"threshold\": n.nnn (numeric) the auto-combine threshold amount in PIV\n"
" \"frequency\": n.nnn (numeric) the auto-combine frequency in blocks\n"
"}\n"

"\nExamples:\n" +
Expand All @@ -4585,6 +4550,7 @@ UniValue getautocombinethreshold(const JSONRPCRequest& request)
UniValue result(UniValue::VOBJ);
result.pushKV("enabled", pwallet->fCombineDust);
result.pushKV("threshold", ValueFromAmount(pwallet->nAutoCombineThreshold));
result.pushKV("frequency", pwallet->frequency);

return result;
}
Expand Down Expand Up @@ -4724,7 +4690,6 @@ static const CRPCCommand commands[] =
{ // category name actor (function) okSafe argNames
// --------------------- ------------------------ ----------------------- ------ --------
{ "wallet", "getaddressinfo", &getaddressinfo, true, {"address"} },
{ "wallet", "autocombinerewards", &autocombinerewards, false, {"enable","threshold"} },
{ "wallet", "setautocombinethreshold", &setautocombinethreshold, false, {"enable","threshold"} },
{ "wallet", "getautocombinethreshold", &getautocombinethreshold, false, {} },
{ "wallet", "abandontransaction", &abandontransaction, false, {"txid"} },
Expand Down
11 changes: 6 additions & 5 deletions src/wallet/wallet.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1340,7 +1340,7 @@ void CWallet::BlockConnected(const std::shared_ptr<const CBlock>& pblock, const
// If turned on Auto Combine will scan wallet for dust to combine
// Outside of the cs_wallet lock because requires cs_main for now
// due CreateTransaction/CommitTransaction dependency.
if (fCombineDust) {
if (fCombineDust && pindex->nHeight % frequency == 0) {
AutoCombineDust(g_connman.get());
}
}
Expand Down Expand Up @@ -4054,8 +4054,8 @@ void CWallet::AutoCombineDust(CConnman* connman)
vRewardCoins.push_back(out);
nTotalRewardsValue += out.Value();

// Combine to the threshold and not way above
if (nTotalRewardsValue > nAutoCombineThreshold)
// Combine to the threshold and not way above considering the safety margin.
if ((nTotalRewardsValue - (nTotalRewardsValue / 10)) > nAutoCombineThreshold)
break;

// Around 180 bytes per input. We use 190 to be certain
Expand Down Expand Up @@ -4107,7 +4107,7 @@ void CWallet::AutoCombineDust(CConnman* connman)
}

//we don't combine below the threshold unless the fees are 0 to avoid paying fees over fees over fees
if (!maxSize && nTotalRewardsValue < nAutoCombineThreshold && nFeeRet > 0)
if (!maxSize && vecSend[0].nAmount < nAutoCombineThreshold && nFeeRet > 0)
continue;

const CWallet::CommitResult& res = CommitTransaction(wtx, keyChange, connman);
Expand All @@ -4116,7 +4116,7 @@ void CWallet::AutoCombineDust(CConnman* connman)
continue;
}

LogPrintf("AutoCombineDust sent transaction\n");
LogPrintf("AutoCombineDust sent transaction. Fee=%d, Total value=%d, Sending=%d\n", nFeeRet, nTotalRewardsValue, vecSend[0].nAmount);

delete coinControl;
}
Expand Down Expand Up @@ -4483,6 +4483,7 @@ void CWallet::SetNull()
//Auto Combine Dust
fCombineDust = false;
nAutoCombineThreshold = 0;
frequency = 30;

// Sapling.
m_sspk_man->nWitnessCacheSize = 0;
Expand Down
1 change: 1 addition & 0 deletions src/wallet/wallet.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -742,6 +742,7 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
//Auto Combine Inputs
bool fCombineDust;
CAmount nAutoCombineThreshold;
int frequency;

/** Get database handle used by this wallet. Ideally this function would
* not be necessary.
Expand Down
21 changes: 16 additions & 5 deletions src/wallet/walletdb.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ namespace DBKeys {

// Wallet custom settings
const std::string AUTOCOMBINE{"autocombinesettings"};
const std::string AUTOCOMBINE_V2{"autocombinesettingsV2"};
const std::string STAKE_SPLIT_THRESHOLD{"stakeSplitThreshold"};
const std::string USE_CUSTOM_FEE{"fUseCustomFee"};
const std::string CUSTOM_FEE_VALUE{"nCustomFee"};
Expand Down Expand Up @@ -241,12 +242,14 @@ bool WalletBatch::WriteCustomFeeValue(const CAmount& nFee)
return WriteIC(std::string(DBKeys::CUSTOM_FEE_VALUE), nFee);
}

bool WalletBatch::WriteAutoCombineSettings(bool fEnable, CAmount nCombineThreshold)
bool WalletBatch::WriteAutoCombineSettings(bool fEnable, CAmount nCombineThreshold, int frequency)
{
std::pair<bool, CAmount> pSettings;
pSettings.first = fEnable;
pSettings.second = nCombineThreshold;
return WriteIC(std::string(DBKeys::AUTOCOMBINE), pSettings, true);
std::pair<std::pair<bool, CAmount>, int> pSettings;
pSettings.first.first = fEnable;
pSettings.first.second = nCombineThreshold;
pSettings.second = frequency;
EraseIC(std::string(DBKeys::AUTOCOMBINE));
return WriteIC(std::string(DBKeys::AUTOCOMBINE_V2), pSettings, true);
}

bool WalletBatch::ReadPool(int64_t nPool, CKeyPool& keypool)
Expand Down Expand Up @@ -535,9 +538,17 @@ bool ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue, CW
ssValue >> pSettings;
pwallet->fCombineDust = pSettings.first;
pwallet->nAutoCombineThreshold = pSettings.second;
// Value used for old autocombine
pwallet->frequency = 1;
// originally saved as integer
if (pwallet->nAutoCombineThreshold < COIN)
pwallet->nAutoCombineThreshold *= COIN;
} else if (strType == DBKeys::AUTOCOMBINE_V2) {
std::pair<std::pair<bool, CAmount>, int> pSettings;
ssValue >> pSettings;
pwallet->fCombineDust = pSettings.first.first;
pwallet->nAutoCombineThreshold = pSettings.first.second;
pwallet->frequency = pSettings.second;
} else if (strType == DBKeys::DESTDATA) {
std::string strAddress, strKey, strValue;
ssKey >> strAddress;
Expand Down
2 changes: 1 addition & 1 deletion src/wallet/walletdb.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ class WalletBatch
bool WriteStakeSplitThreshold(const CAmount& nStakeSplitThreshold);
bool WriteUseCustomFee(bool fUse);
bool WriteCustomFeeValue(const CAmount& nCustomFee);
bool WriteAutoCombineSettings(bool fEnable, CAmount nCombineThreshold);
bool WriteAutoCombineSettings(bool fEnable, CAmount nCombineThreshold, int frequency);

bool ReadPool(int64_t nPool, CKeyPool& keypool);
bool WritePool(int64_t nPool, const CKeyPool& keypool);
Expand Down
12 changes: 3 additions & 9 deletions test/functional/rpc_deprecated.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,14 @@
"""Test deprecation of RPC calls."""

from test_framework.test_framework import PivxTestFramework
from test_framework.util import assert_raises_rpc_error
# from test_framework.util import assert_raises_rpc_error


class DeprecatedRpcTest(PivxTestFramework):
def set_test_params(self):
self.num_nodes = 2
self.setup_clean_chain = True
self.extra_args = [[], ["-deprecatedrpc=autocombinerewards"]]
# self.extra_args = [[], ["-deprecatedrpc=rpcname"]] add deprecated rpc here

def run_test(self):
# This test should be used to verify correct behaviour of deprecated
Expand All @@ -21,13 +21,7 @@ def run_test(self):
# self.log.info("Make sure that -deprecatedrpc=accounts allows it to take accounts")
# assert_raises_rpc_error(-32, "listaccounts is deprecated", self.nodes[0].listaccounts)
# self.nodes[1].listaccounts()

self.log.info("Test autocombinerewards deprecation")
# The autocombinerewards RPC method has been deprecated
assert_raises_rpc_error(-32, "autocombinerewards is deprecated", self.nodes[0].autocombinerewards, True, 500)
self.nodes[1].autocombinerewards(True, 500)

# self.log.info("No test cases to run") # remove this when adding any tests to this file
self.log.info("No test cases to run") # remove this when adding any tests to this file


if __name__ == '__main__':
Expand Down

0 comments on commit ac8a74d

Please sign in to comment.