Skip to content
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

[RPC] Cleanup startmasternode command #2833

Merged
merged 3 commits into from
Apr 19, 2023
Merged

[RPC] Cleanup startmasternode command #2833

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b86632f
[RPC] Cleanup startmasternode command
Fuzzbawls Apr 1, 2023
189f263
[Tests] Test newly invalid startmasternode set options
Fuzzbawls Apr 1, 2023
815bbc2
[Tests] Cover startmasternode all option
Fuzzbawls Apr 1, 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: 2 additions & 1 deletion src/rpc/client.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,8 @@ static const CRPCConvertParam vRPCConvertParams[] = {
{ "signrawtransaction", 1, "prevtxs" },
{ "signrawtransaction", 2, "privkeys" },
{ "spork", 1, "value" },
{ "startmasternode", 3, "lockwallet" },
{ "startmasternode", 1, "lock_wallet" },
{ "startmasternode", 3, "reload_conf" },
{ "submitbudget", 2, "npayments" },
{ "submitbudget", 3, "start" },
{ "submitbudget", 5, "montly_payment" },
Expand Down
81 changes: 33 additions & 48 deletions src/rpc/masternode.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -413,46 +413,40 @@ UniValue startmasternode(const JSONRPCRequest& request)
throw JSONRPCError(RPC_MISC_ERROR, "startmasternode is not supported when deterministic masternode list is active (DIP3)");
}

CWallet * const pwallet = GetWalletForJSONRPCRequest(request);
CWallet* const pwallet = GetWalletForJSONRPCRequest(request);

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

std::string strCommand;
if (request.params.size() >= 1) {
if (!request.params.empty()) {
strCommand = request.params[0].get_str();

// Backwards compatibility with legacy 'masternode' super-command forwarder
if (strCommand == "start") strCommand = "local";
if (strCommand == "start-alias") strCommand = "alias";
if (strCommand == "start-all") strCommand = "all";
if (strCommand == "start-many") strCommand = "many";
if (strCommand == "start-missing") strCommand = "missing";
if (strCommand == "start-disabled") strCommand = "disabled";
}

if (strCommand == "local")
throw JSONRPCError(RPC_INVALID_PARAMETER, "Local start is deprecated. Start your masternode from the controller wallet instead.");
if (strCommand == "many")
throw JSONRPCError(RPC_INVALID_PARAMETER, "Many set is deprecated. Use either 'all', 'missing', or 'disabled'.");

if (request.fHelp || request.params.size() < 2 || request.params.size() > 4 ||
(request.params.size() == 2 && (strCommand != "local" && strCommand != "all" && strCommand != "many" && strCommand != "missing" && strCommand != "disabled")) ||
( (request.params.size() == 3 || request.params.size() == 4) && strCommand != "alias"))
(strCommand == "alias" && request.params.size() < 3))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Improve this if:
if the strCommand is "alias" you must use either 3 or 4 params
if the strCommand is valid but not "alias" you must use 2 params
if the strCommand is not valid you should throw error anyways

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the strCommand is "alias" you must use either 3 or 4 params

"alias" requires at least 3 arguments, this is specifically covered in this conditional check.

if the strCommand is valid but not "alias" you must use 2 params

This is no longer the case, as shown in the changes to the functional tests that use named arguments with the "all" command whilst reloading the masternode.conf file (here and here).

if the strCommand is not valid you should throw error anyways

This does that, and is tested in the functional tests (here) by returning a RPC_INVALID_PARAMETER error code. This is done towards the end of the RPC function call and not at the beginning intentionally (here).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the strCommand is not valid you should throw error anyways

This does that, and is tested in the functional tests (here) by returning a RPC_INVALID_PARAMETER error code. This is done towards the end of the RPC function call and not at the beginning intentionally (here).

Yep I saw that my main point was that (as a user) I would prefer that if I inserted an invalid strCommand the output would be error + how to use that rpc command (as it was done before) instead of a simple "Invalid strCommand" and same for the number of parameters

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved to this approach for a few reasons:

  1. It allows greater code coverage and testability.
  2. A user's natural instinct upon getting an RPC_INVALID_PARAMETER error (SHOULD) be to then check the help output.
  3. This new behavior is much much more inline with the rest of our RPC commands, and greatly simplifies the logic checks being done for when to show the actual help output.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok still don't totally agree but I'm not blocking the PR for this small thing

throw std::runtime_error(
"startmasternode \"local|all|many|missing|disabled|alias\" lockwallet ( \"alias\" reload_conf )\n"
"\nAttempts to start one or more masternode(s)\n"
"startmasternode \"all|missing|disabled|alias\" lock_wallet ( \"alias\" reload_conf )\n"
"\nAttempts to start one or more masternode(s)\n" +
HelpRequiringPassphrase(pwallet) + "\n"

"\nArguments:\n"
"1. set (string, required) Specify which set of masternode(s) to start.\n"
"2. lockwallet (boolean, required) Lock wallet after completion.\n"
"3. alias (string) Masternode alias. Required if using 'alias' as the set.\n"
"4. reload_conf (boolean) if true and \"alias\" was selected, reload the masternodes.conf data from disk"
"1. set (string, required) Specify which set of masternode(s) to start.\n"
"2. lock_wallet (boolean, required) Lock wallet after completion.\n"
"3. alias (string, optional) Masternode alias. Required if using 'alias' as the set.\n"
"4. reload_conf (boolean, optional, default=False) reload the masternodes.conf data from disk"

"\nResult: (for 'local' set):\n"
"\"status\" (string) Masternode status message\n"

"\nResult: (for other sets):\n"
"\nResult:\n"
"{\n"
" \"overall\": \"xxxx\", (string) Overall status message\n"
" \"detail\": [\n"
" {\n"
" \"node\": \"xxxx\", (string) Node name or alias\n"
" \"alias\": \"xxxx\", (string) Node alias\n"
" \"result\": \"xxxx\", (string) 'success' or 'failed'\n"
" \"error\": \"xxxx\" (string) Error message, if failed\n"
" }\n"
Expand All @@ -461,25 +455,25 @@ UniValue startmasternode(const JSONRPCRequest& request)
"}\n"

"\nExamples:\n" +
HelpExampleCli("startmasternode", "\"alias\" \"0\" \"my_mn\"") + HelpExampleRpc("startmasternode", "\"alias\" \"0\" \"my_mn\""));
HelpExampleCli("startmasternode", "\"alias\" false \"my_mn\"") + HelpExampleRpc("startmasternode", "\"alias\" false \"my_mn\""));

bool fLock = (request.params[1].get_str() == "true" ? true : false);
RPCTypeCheck(request.params, {UniValue::VSTR, UniValue::VBOOL, UniValue::VSTR, UniValue::VBOOL}, true);

EnsureWalletIsUnlocked(pwallet);

if (strCommand == "local") {
if (!fMasterNode) throw std::runtime_error("you must set masternode=1 in the configuration\n");
bool fLock = request.params[1].get_bool();
bool fReload = request.params.size() > 3 ? request.params[3].get_bool() : false;

if (activeMasternode.GetStatus() != ACTIVE_MASTERNODE_STARTED) {
activeMasternode.ResetStatus();
if (fLock)
pwallet->Lock();
// Check reload param
if (fReload) {
masternodeConfig.clear();
std::string error;
if (!masternodeConfig.read(error)) {
throw std::runtime_error("Error reloading masternode.conf, " + error);
}

return activeMasternode.GetStatusMessage();
}

if (strCommand == "all" || strCommand == "many" || strCommand == "missing" || strCommand == "disabled") {
if (strCommand == "all" || strCommand == "missing" || strCommand == "disabled") {
if ((strCommand == "missing" || strCommand == "disabled") &&
(g_tiertwo_sync_state.GetSyncPhase() <= MASTERNODE_SYNC_LIST ||
g_tiertwo_sync_state.GetSyncPhase() == MASTERNODE_SYNC_FAILED)) {
Expand All @@ -491,7 +485,7 @@ UniValue startmasternode(const JSONRPCRequest& request)

UniValue resultsObj(UniValue::VARR);

for (CMasternodeConfig::CMasternodeEntry mne : masternodeConfig.getEntries()) {
for (const CMasternodeConfig::CMasternodeEntry& mne : masternodeConfig.getEntries()) {
UniValue statusObj(UniValue::VOBJ);
CMasternodeBroadcast mnb;
std::string errorMessage;
Expand All @@ -514,28 +508,19 @@ UniValue startmasternode(const JSONRPCRequest& request)
if (strCommand == "alias") {
std::string alias = request.params[2].get_str();

// Check reload param
if(request.params[3].getBool()) {
masternodeConfig.clear();
std::string error;
if (!masternodeConfig.read(error)) {
throw std::runtime_error("Error reloading masternode.conf, " + error);
}
}

bool found = false;

UniValue resultsObj(UniValue::VARR);
UniValue statusObj(UniValue::VOBJ);

for (CMasternodeConfig::CMasternodeEntry mne : masternodeConfig.getEntries()) {
for (const CMasternodeConfig::CMasternodeEntry& mne : masternodeConfig.getEntries()) {
if (mne.getAlias() == alias) {
CMasternodeBroadcast mnb;
found = true;
std::string errorMessage;
bool fSuccess = false;
if (!StartMasternodeEntry(statusObj, mnb, fSuccess, mne, errorMessage, strCommand))
continue;
continue;
RelayMNB(mnb, fSuccess);
break;
}
Expand All @@ -552,7 +537,7 @@ UniValue startmasternode(const JSONRPCRequest& request)

return statusObj;
}
return NullUniValue;
throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid set name %s.", strCommand));
}

UniValue createmasternodekey(const JSONRPCRequest& request)
Expand Down Expand Up @@ -1125,7 +1110,7 @@ static const CRPCCommand commands[] =
{ "masternode", "listmasternodes", &listmasternodes, true, {"filter"} },
{ "masternode", "masternodecurrent", &masternodecurrent, true, {} },
{ "masternode", "relaymasternodebroadcast", &relaymasternodebroadcast, true, {"hexstring"} },
{ "masternode", "startmasternode", &startmasternode, true, {"set","lockwallet","alias","reload_conf"} },
{ "masternode", "startmasternode", &startmasternode, true, {"set","lock_wallet","alias","reload_conf"} },

/* Not shown in help */
{ "hidden", "getcachedblockhashes", &getcachedblockhashes, true, {} },
Expand Down
10 changes: 9 additions & 1 deletion test/functional/test_framework/test_framework.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1030,11 +1030,19 @@ def wait_until_mn_vinspent(self, mnTxHash, _timeout, _with_ping_mns=[]):


def controller_start_masternode(self, mnOwner, masternodeAlias):
ret = mnOwner.startmasternode("alias", "false", masternodeAlias, True)
ret = mnOwner.startmasternode("alias", False, masternodeAlias, True)
assert_equal(ret["result"], "success")
time.sleep(1)


def controller_start_masternodes(self, mnOwner, aliases=[]):
ret = mnOwner.startmasternode(set="all", lock_wallet=False, reload_conf=True)
for i in range(len(aliases)):
assert_equal(ret["detail"][i]["alias"], aliases[i])
assert_equal(ret["detail"][i]["result"], "success")
time.sleep(1)


def send_pings(self, mnodes):
for node in mnodes:
try:
Expand Down
3 changes: 1 addition & 2 deletions test/functional/tiertwo_governance_reorg.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,7 @@ def run_test(self):
mn2.initmasternode(self.mnTwoPrivkey, "127.0.0.1:"+str(remoteTwoPort))
self.stake_and_ping(self.minerAPos, 1, [])
self.wait_until_mnsync_finished()
self.controller_start_masternode(minerA, self.masternodeOneAlias)
self.controller_start_masternode(minerA, self.masternodeTwoAlias)
self.controller_start_masternodes(minerA, [self.masternodeOneAlias, self.masternodeTwoAlias])
self.wait_until_mn_preenabled(self.mnOneCollateral.hash, 40)
self.wait_until_mn_preenabled(self.mnOneCollateral.hash, 40)
self.send_3_pings([mn1, mn2])
Expand Down
8 changes: 7 additions & 1 deletion test/functional/tiertwo_masternode_ping.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
from test_framework.util import (
assert_equal,
assert_greater_than,
assert_raises_rpc_error,
Decimal,
p2p_port,
)
Expand Down Expand Up @@ -90,6 +91,12 @@ def run_test(self):
self.sync_blocks()
time.sleep(1)

# Exercise invalid startmasternode methods
self.log.info("exercising invalid startmasternode methods...")
assert_raises_rpc_error(-8, "Local start is deprecated.", remote.startmasternode, "local", False)
assert_raises_rpc_error(-8, "Many set is deprecated.", owner.startmasternode, "many", False)
assert_raises_rpc_error(-8, "Invalid set name", owner.startmasternode, "foo", False)

# Send Start message
self.log.info("sending masternode broadcast...")
self.controller_start_masternode(owner, masternodeAlias)
Expand Down Expand Up @@ -119,6 +126,5 @@ def run_test(self):
self.log.info("All good.")



if __name__ == '__main__':
MasternodePingTest().main()