Skip to content

Commit 4c2ef86

Browse files
MarcoFalkesidhujag
MarcoFalke
authored and
sidhujag
committed
Merge bitcoin#19717: rpc: Assert that RPCArg names are equal to CRPCCommand ones (mining,zmq,rpcdump)
fa3d9ce rpc: Assert that RPCArg names are equal to CRPCCommand ones (rpcdump) (MarcoFalke) fa32c1d rpc: Assert that RPCArg names are equal to CRPCCommand ones (zmq) (MarcoFalke) faaa46d rpc: Assert that RPCArg names are equal to CRPCCommand ones (mining) (MarcoFalke) fa93bc1 rpc: Remove unused return type from appendCommand (MarcoFalke) Pull request description: This is split out from bitcoin#18531 to just touch the RPC methods in misc. Description from the main pr: ### Motivation RPCArg names in the rpc help are currently only used for documentation. However, in the future they could be used to teach the server the named arguments. Named arguments are currently registered by the `CRPCCommand`s and duplicate the RPCArg names from the documentation. This redundancy is fragile, and has lead to errors in the past (despite having linters to catch those kind of errors). See section "bugs found" for a list of bugs that have been found as a result of the changes here. ### Changes The changes here add an assert in the `CRPCCommand` constructor that the RPCArg names are identical to the ones in the `CRPCCommand`. ### Future work > Here or follow up, makes sense to also assert type of returned UniValue? Sure, but let's not get ahead of ourselves. I am going to submit any further works as follow-ups, including: * Removing the CRPCCommand arguments, now that they are asserted to be equal and thus redundant * Removing all python regex linters on the args, now that RPCMan can be used to generate any output, including the cli.cpp table * Auto-formatting and sanity checking the RPCExamples with RPCMan * Checking passed-in json in self-check. Removing redundant checks * Checking returned json against documentation to avoid regressions or false documentation * Compile the RPC documentation at compile-time to ensure it doesn't change at runtime and is completely static ### Bugs found * The assert identified issue bitcoin#18607 * The changes itself fixed bug bitcoin#19250 ACKs for top commit: fjahr: tested ACK fa3d9ce promag: Code review ACK fa3d9ce. Tree-SHA512: 068ade4b55cc195868d53b7f9a27151d45b440857bb069e261a49d102a49a38fdba5d68868516a1d66a54a73ba34681362f934ded7349e894042bde873b75719
1 parent dd60812 commit 4c2ef86

8 files changed

Lines changed: 171 additions & 125 deletions

File tree

‎src/rpc/mining.cpp‎

Lines changed: 74 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -87,9 +87,9 @@ static UniValue GetNetworkHashPS(int lookup, int height) {
8787
return workDiff.getdouble() / timeDiff;
8888
}
8989

90-
static UniValue getnetworkhashps(const JSONRPCRequest& request)
90+
static RPCHelpMan getnetworkhashps()
9191
{
92-
RPCHelpMan{"getnetworkhashps",
92+
return RPCHelpMan{"getnetworkhashps",
9393
"\nReturns the estimated network hashes per second based on the last n blocks.\n"
9494
"Pass in [blocks] to override # of blocks, -1 specifies since last difficulty change.\n"
9595
"Pass in [height] to estimate the network speed at the time when a certain block was found.\n",
@@ -103,10 +103,12 @@ static UniValue getnetworkhashps(const JSONRPCRequest& request)
103103
HelpExampleCli("getnetworkhashps", "")
104104
+ HelpExampleRpc("getnetworkhashps", "")
105105
},
106-
}.Check(request);
107-
106+
[&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
107+
{
108108
LOCK(cs_main);
109109
return GetNetworkHashPS(!request.params[0].isNull() ? request.params[0].get_int() : 120, !request.params[1].isNull() ? request.params[1].get_int() : -1);
110+
},
111+
};
110112
}
111113

112114
static bool GenerateBlock(ChainstateManager& chainman, CBlock& block, uint64_t& max_tries, unsigned int& extra_nonce, uint256& block_hash)
@@ -206,9 +208,9 @@ static bool getScriptFromDescriptor(const std::string& descriptor, CScript& scri
206208
}
207209
}
208210

209-
static UniValue generatetodescriptor(const JSONRPCRequest& request)
211+
static RPCHelpMan generatetodescriptor()
210212
{
211-
RPCHelpMan{
213+
return RPCHelpMan{
212214
"generatetodescriptor",
213215
"\nMine blocks immediately to a specified descriptor (before the RPC call returns)\n",
214216
{
@@ -224,9 +226,8 @@ static UniValue generatetodescriptor(const JSONRPCRequest& request)
224226
},
225227
RPCExamples{
226228
"\nGenerate 11 blocks to mydesc\n" + HelpExampleCli("generatetodescriptor", "11 \"mydesc\"")},
227-
}
228-
.Check(request);
229-
229+
[&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
230+
{
230231
const int num_blocks{request.params[0].get_int()};
231232
const uint64_t max_tries{request.params[2].isNull() ? DEFAULT_MAX_TRIES : request.params[2].get_int()};
232233

@@ -240,22 +241,25 @@ static UniValue generatetodescriptor(const JSONRPCRequest& request)
240241
ChainstateManager& chainman = EnsureChainman(request.context);
241242

242243
return generateBlocks(chainman, mempool, coinbase_script, num_blocks, max_tries);
244+
},
245+
};
243246
}
244247

245-
static UniValue generate(const JSONRPCRequest& request)
248+
static RPCHelpMan generate()
246249
{
247-
const std::string help_str{"generate ( nblocks maxtries ) has been replaced by the -generate cli option. Refer to -help for more information."};
250+
return RPCHelpMan{"generate", "has been replaced by the -generate cli option. Refer to -help for more information.", {}, {}, RPCExamples{""}, [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue {
248251

249252
if (request.fHelp) {
250-
throw std::runtime_error(help_str);
253+
throw std::runtime_error(self.ToString());
251254
} else {
252-
throw JSONRPCError(RPC_METHOD_NOT_FOUND, help_str);
255+
throw JSONRPCError(RPC_METHOD_NOT_FOUND, self.ToString());
253256
}
257+
}};
254258
}
255259

256-
static UniValue generatetoaddress(const JSONRPCRequest& request)
260+
static RPCHelpMan generatetoaddress()
257261
{
258-
RPCHelpMan{"generatetoaddress",
262+
return RPCHelpMan{"generatetoaddress",
259263
"\nMine blocks immediately to a specified address (before the RPC call returns)\n",
260264
{
261265
{"nblocks", RPCArg::Type::NUM, RPCArg::Optional::NO, "How many blocks are generated immediately."},
@@ -273,8 +277,8 @@ static UniValue generatetoaddress(const JSONRPCRequest& request)
273277
+ "If you are using the " PACKAGE_NAME " wallet, you can get a new address to send the newly generated syscoin to with:\n"
274278
+ HelpExampleCli("getnewaddress", "")
275279
},
276-
}.Check(request);
277-
280+
[&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
281+
{
278282
const int num_blocks{request.params[0].get_int()};
279283
const uint64_t max_tries{request.params[2].isNull() ? DEFAULT_MAX_TRIES : request.params[2].get_int()};
280284

@@ -289,11 +293,13 @@ static UniValue generatetoaddress(const JSONRPCRequest& request)
289293
CScript coinbase_script = GetScriptForDestination(destination);
290294

291295
return generateBlocks(chainman, mempool, coinbase_script, num_blocks, max_tries);
296+
},
297+
};
292298
}
293299

294-
static UniValue generateblock(const JSONRPCRequest& request)
300+
static RPCHelpMan generateblock()
295301
{
296-
RPCHelpMan{"generateblock",
302+
return RPCHelpMan{"generateblock",
297303
"\nMine a block with a set of ordered transactions immediately to a specified address or descriptor (before the RPC call returns)\n",
298304
{
299305
{"output", RPCArg::Type::STR, RPCArg::Optional::NO, "The address or descriptor to send the newly generated syscoin to."},
@@ -315,8 +321,8 @@ static UniValue generateblock(const JSONRPCRequest& request)
315321
"\nGenerate a block to myaddress, with txs rawtx and mempool_txid\n"
316322
+ HelpExampleCli("generateblock", R"("myaddress" '["rawtx", "mempool_txid"]')")
317323
},
318-
}.Check(request);
319-
324+
[&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
325+
{
320326
const auto address_or_descriptor = request.params[0].get_str();
321327
CScript coinbase_script;
322328
std::string error;
@@ -398,11 +404,13 @@ static UniValue generateblock(const JSONRPCRequest& request)
398404
UniValue obj(UniValue::VOBJ);
399405
obj.pushKV("hash", block_hash.GetHex());
400406
return obj;
407+
},
408+
};
401409
}
402410

403-
static UniValue getmininginfo(const JSONRPCRequest& request)
411+
static RPCHelpMan getmininginfo()
404412
{
405-
RPCHelpMan{"getmininginfo",
413+
return RPCHelpMan{"getmininginfo",
406414
"\nReturns a json object containing mining-related information.",
407415
{},
408416
RPCResult{
@@ -421,8 +429,8 @@ static UniValue getmininginfo(const JSONRPCRequest& request)
421429
HelpExampleCli("getmininginfo", "")
422430
+ HelpExampleRpc("getmininginfo", "")
423431
},
424-
}.Check(request);
425-
432+
[&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
433+
{
426434
LOCK(cs_main);
427435
const CTxMemPool& mempool = EnsureMemPool(request.context);
428436

@@ -431,18 +439,20 @@ static UniValue getmininginfo(const JSONRPCRequest& request)
431439
if (BlockAssembler::m_last_block_weight) obj.pushKV("currentblockweight", *BlockAssembler::m_last_block_weight);
432440
if (BlockAssembler::m_last_block_num_txs) obj.pushKV("currentblocktx", *BlockAssembler::m_last_block_num_txs);
433441
obj.pushKV("difficulty", (double)GetDifficulty(::ChainActive().Tip()));
434-
obj.pushKV("networkhashps", getnetworkhashps(request));
442+
obj.pushKV("networkhashps", getnetworkhashps().HandleRequest(request));
435443
obj.pushKV("pooledtx", (uint64_t)mempool.size());
436444
obj.pushKV("chain", Params().NetworkIDString());
437445
obj.pushKV("warnings", GetWarnings(false).original);
438446
return obj;
447+
},
448+
};
439449
}
440450

441451

442452
// NOTE: Unlike wallet RPC (which use SYS values), mining RPCs follow GBT (BIP 22) in using satoshi amounts
443-
static UniValue prioritisetransaction(const JSONRPCRequest& request)
453+
static RPCHelpMan prioritisetransaction()
444454
{
445-
RPCHelpMan{"prioritisetransaction",
455+
return RPCHelpMan{"prioritisetransaction",
446456
"Accepts the transaction into mined blocks at a higher (or lower) priority\n",
447457
{
448458
{"txid", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The transaction id."},
@@ -459,8 +469,8 @@ static UniValue prioritisetransaction(const JSONRPCRequest& request)
459469
HelpExampleCli("prioritisetransaction", "\"txid\" 0.0 10000")
460470
+ HelpExampleRpc("prioritisetransaction", "\"txid\", 0.0, 10000")
461471
},
462-
}.Check(request);
463-
472+
[&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
473+
{
464474
LOCK(cs_main);
465475

466476
uint256 hash(ParseHashV(request.params[0], "txid"));
@@ -472,6 +482,8 @@ static UniValue prioritisetransaction(const JSONRPCRequest& request)
472482

473483
EnsureMemPool(request.context).PrioritiseTransaction(hash, nAmount);
474484
return true;
485+
},
486+
};
475487
}
476488

477489

@@ -503,9 +515,9 @@ static std::string gbt_vb_name(const Consensus::DeploymentPos pos) {
503515
return s;
504516
}
505517

506-
static UniValue getblocktemplate(const JSONRPCRequest& request)
518+
static RPCHelpMan getblocktemplate()
507519
{
508-
RPCHelpMan{"getblocktemplate",
520+
return RPCHelpMan{"getblocktemplate",
509521
"\nIf the request parameters include a 'mode' key, that is used to explicitly select between the default 'template' request or a 'proposal'.\n"
510522
"It returns data needed to construct a block to work on.\n"
511523
"For full specification, see BIPs 22, 23, 9, and 145:\n"
@@ -585,15 +597,15 @@ static UniValue getblocktemplate(const JSONRPCRequest& request)
585597
RPCExamples{
586598
HelpExampleCli("getblocktemplate", "'{\"rules\": [\"segwit\"]}'")
587599
+ HelpExampleRpc("getblocktemplate", "{\"rules\": [\"segwit\"]}")
588-
}
589-
}.Check(request);
600+
},
601+
[&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
602+
{
603+
LOCK(cs_main);
590604
// SYSCOIN RPC_MISC_ERROR
591605
std::string errorMessage = "";
592606
if(!fRegTest && !CheckSpecs(errorMessage, true)){
593607
throw JSONRPCError(RPC_MISC_ERROR, errorMessage);
594608
}
595-
LOCK(cs_main);
596-
597609
std::string strMode = "template";
598610
UniValue lpval = NullUniValue;
599611
std::set<std::string> setClientRules;
@@ -948,6 +960,8 @@ static UniValue getblocktemplate(const JSONRPCRequest& request)
948960
result.pushKV("default_witness_commitment_extra", HexStr(pblocktemplate->vchCoinbaseCommitmentExtra));
949961
}
950962
return result;
963+
},
964+
};
951965
}
952966

953967
class submitblock_StateCatcher final : public CValidationInterface
@@ -968,10 +982,10 @@ class submitblock_StateCatcher final : public CValidationInterface
968982
}
969983
};
970984

971-
static UniValue submitblock(const JSONRPCRequest& request)
985+
static RPCHelpMan submitblock()
972986
{
973987
// We allow 2 arguments for compliance with BIP22. Argument 2 is ignored.
974-
RPCHelpMan{"submitblock",
988+
return RPCHelpMan{"submitblock",
975989
"\nAttempts to submit new block to network.\n"
976990
"See https://en.syscoin.it/wiki/BIP_0022 for full specification.\n",
977991
{
@@ -983,8 +997,8 @@ static UniValue submitblock(const JSONRPCRequest& request)
983997
HelpExampleCli("submitblock", "\"mydata\"")
984998
+ HelpExampleRpc("submitblock", "\"mydata\"")
985999
},
986-
}.Check(request);
987-
1000+
[&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
1001+
{
9881002
std::shared_ptr<CBlock> blockptr = std::make_shared<CBlock>();
9891003
CBlock& block = *blockptr;
9901004
if (!DecodeHexBlk(block, request.params[0].get_str())) {
@@ -1029,11 +1043,13 @@ static UniValue submitblock(const JSONRPCRequest& request)
10291043
return "inconclusive";
10301044
}
10311045
return BIP22ValidationResult(sc->state);
1046+
},
1047+
};
10321048
}
10331049

1034-
static UniValue submitheader(const JSONRPCRequest& request)
1050+
static RPCHelpMan submitheader()
10351051
{
1036-
RPCHelpMan{"submitheader",
1052+
return RPCHelpMan{"submitheader",
10371053
"\nDecode the given hexdata as a header and submit it as a candidate chain tip if valid."
10381054
"\nThrows when the header is invalid.\n",
10391055
{
@@ -1045,8 +1061,8 @@ static UniValue submitheader(const JSONRPCRequest& request)
10451061
HelpExampleCli("submitheader", "\"aabbcc\"") +
10461062
HelpExampleRpc("submitheader", "\"aabbcc\"")
10471063
},
1048-
}.Check(request);
1049-
1064+
[&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
1065+
{
10501066
CBlockHeader h;
10511067
if (!DecodeHexBlockHeader(h, request.params[0].get_str())) {
10521068
throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "Block header decode failed");
@@ -1065,11 +1081,13 @@ static UniValue submitheader(const JSONRPCRequest& request)
10651081
throw JSONRPCError(RPC_VERIFY_ERROR, state.ToString());
10661082
}
10671083
throw JSONRPCError(RPC_VERIFY_ERROR, state.GetRejectReason());
1084+
},
1085+
};
10681086
}
10691087

1070-
static UniValue estimatesmartfee(const JSONRPCRequest& request)
1088+
static RPCHelpMan estimatesmartfee()
10711089
{
1072-
RPCHelpMan{"estimatesmartfee",
1090+
return RPCHelpMan{"estimatesmartfee",
10731091
"\nEstimates the approximate fee per kilobyte needed for a transaction to begin\n"
10741092
"confirmation within conf_target blocks if possible and return the number of blocks\n"
10751093
"for which the estimate is valid. Uses virtual transaction size as defined\n"
@@ -1103,8 +1121,8 @@ static UniValue estimatesmartfee(const JSONRPCRequest& request)
11031121
RPCExamples{
11041122
HelpExampleCli("estimatesmartfee", "6")
11051123
},
1106-
}.Check(request);
1107-
1124+
[&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
1125+
{
11081126
RPCTypeCheck(request.params, {UniValue::VNUM, UniValue::VSTR});
11091127
RPCTypeCheckArgument(request.params[0], UniValue::VNUM);
11101128
unsigned int max_target = ::feeEstimator.HighestTargetTracked(FeeEstimateHorizon::LONG_HALFLIFE);
@@ -1130,11 +1148,13 @@ static UniValue estimatesmartfee(const JSONRPCRequest& request)
11301148
}
11311149
result.pushKV("blocks", feeCalc.returnedTarget);
11321150
return result;
1151+
},
1152+
};
11331153
}
11341154

1135-
static UniValue estimaterawfee(const JSONRPCRequest& request)
1155+
static RPCHelpMan estimaterawfee()
11361156
{
1137-
RPCHelpMan{"estimaterawfee",
1157+
return RPCHelpMan{"estimaterawfee",
11381158
"\nWARNING: This interface is unstable and may disappear or change!\n"
11391159
"\nWARNING: This is an advanced API call that is tightly coupled to the specific\n"
11401160
" implementation of fee estimation. The parameters it can be called with\n"
@@ -1186,8 +1206,8 @@ static UniValue estimaterawfee(const JSONRPCRequest& request)
11861206
RPCExamples{
11871207
HelpExampleCli("estimaterawfee", "6 0.9")
11881208
},
1189-
}.Check(request);
1190-
1209+
[&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
1210+
{
11911211
RPCTypeCheck(request.params, {UniValue::VNUM, UniValue::VNUM}, true);
11921212
RPCTypeCheckArgument(request.params[0], UniValue::VNUM);
11931213
unsigned int max_target = ::feeEstimator.HighestTargetTracked(FeeEstimateHorizon::LONG_HALFLIFE);
@@ -1246,6 +1266,8 @@ static UniValue estimaterawfee(const JSONRPCRequest& request)
12461266
result.pushKV(StringForFeeEstimateHorizon(horizon), horizon_result);
12471267
}
12481268
return result;
1269+
},
1270+
};
12491271
}
12501272
/* ************************************************************************** */
12511273
/* Merge mining. */

‎src/rpc/server.cpp‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -277,13 +277,11 @@ CRPCTable::CRPCTable()
277277
}
278278
}
279279

280-
bool CRPCTable::appendCommand(const std::string& name, const CRPCCommand* pcmd)
280+
void CRPCTable::appendCommand(const std::string& name, const CRPCCommand* pcmd)
281281
{
282-
if (IsRPCRunning())
283-
return false;
282+
CHECK_NONFATAL(!IsRPCRunning()); // Only add commands before rpc is running
284283

285284
mapCommands[name].push_back(pcmd);
286-
return true;
287285
}
288286

289287
bool CRPCTable::removeCommand(const std::string& name, const CRPCCommand* pcmd)

‎src/rpc/server.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -162,7 +162,7 @@ class CRPCTable
162162
/**
163163
* Appends a CRPCCommand to the dispatch table.
164164
*
165-
* Returns false if RPC server is already running (dump concurrency protection).
165+
* Precondition: RPC server is not running
166166
*
167167
* Commands with different method names but the same unique_id will
168168
* be considered aliases, and only the first registered method name will
@@ -171,7 +171,7 @@ class CRPCTable
171171
* between calls based on method name, and aliased commands can also
172172
* register different names, types, and numbers of parameters.
173173
*/
174-
bool appendCommand(const std::string& name, const CRPCCommand* pcmd);
174+
void appendCommand(const std::string& name, const CRPCCommand* pcmd);
175175
bool removeCommand(const std::string& name, const CRPCCommand* pcmd);
176176
};
177177

0 commit comments

Comments
 (0)