Skip to content

[Wallet] Fix AutoCombineDust and improve setautocombinethreshold #2812

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Fuzzbawls merged 5 commits into from
Mar 8, 2023
Merged

[Wallet] Fix AutoCombineDust and improve setautocombinethreshold #2812

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
66dbb2e
Fixed current AutoCombineDust
panleone Feb 23, 2023
9ea65d5
Fixed and improved setautocombinethreshold
panleone Mar 5, 2023
1c300a2
Remove deprecated autocombinerewards RPC command
panleone Mar 5, 2023
e7437c9
Update functional test DeprecatedRpcTest
panleone Mar 5, 2023
63488be
Update functional test wallet_autocombine.py
panleone Mar 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions src/rpc/client.cpp
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Loading