Skip to content

Commit 89a8299

Browse files
author
MarcoFalke
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
2 parents 068bc21 + fa3d9ce commit 89a8299

8 files changed

Lines changed: 167 additions & 122 deletions

File tree

‎src/rpc/mining.cpp‎

Lines changed: 72 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -81,9 +81,9 @@ static UniValue GetNetworkHashPS(int lookup, int height) {
8181
return workDiff.getdouble() / timeDiff;
8282
}
8383

84-
static UniValue getnetworkhashps(const JSONRPCRequest& request)
84+
static RPCHelpMan getnetworkhashps()
8585
{
86-
RPCHelpMan{"getnetworkhashps",
86+
return RPCHelpMan{"getnetworkhashps",
8787
"\nReturns the estimated network hashes per second based on the last n blocks.\n"
8888
"Pass in [blocks] to override # of blocks, -1 specifies since last difficulty change.\n"
8989
"Pass in [height] to estimate the network speed at the time when a certain block was found.\n",
@@ -97,10 +97,12 @@ static UniValue getnetworkhashps(const JSONRPCRequest& request)
9797
HelpExampleCli("getnetworkhashps", "")
9898
+ HelpExampleRpc("getnetworkhashps", "")
9999
},
100-
}.Check(request);
101-
100+
[&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
101+
{
102102
LOCK(cs_main);
103103
return GetNetworkHashPS(!request.params[0].isNull() ? request.params[0].get_int() : 120, !request.params[1].isNull() ? request.params[1].get_int() : -1);
104+
},
105+
};
104106
}
105107

106108
static bool GenerateBlock(ChainstateManager& chainman, CBlock& block, uint64_t& max_tries, unsigned int& extra_nonce, uint256& block_hash)
@@ -200,9 +202,9 @@ static bool getScriptFromDescriptor(const std::string& descriptor, CScript& scri
200202
}
201203
}
202204

203-
static UniValue generatetodescriptor(const JSONRPCRequest& request)
205+
static RPCHelpMan generatetodescriptor()
204206
{
205-
RPCHelpMan{
207+
return RPCHelpMan{
206208
"generatetodescriptor",
207209
"\nMine blocks immediately to a specified descriptor (before the RPC call returns)\n",
208210
{
@@ -218,9 +220,8 @@ static UniValue generatetodescriptor(const JSONRPCRequest& request)
218220
},
219221
RPCExamples{
220222
"\nGenerate 11 blocks to mydesc\n" + HelpExampleCli("generatetodescriptor", "11 \"mydesc\"")},
221-
}
222-
.Check(request);
223-
223+
[&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
224+
{
224225
const int num_blocks{request.params[0].get_int()};
225226
const uint64_t max_tries{request.params[2].isNull() ? DEFAULT_MAX_TRIES : request.params[2].get_int()};
226227

@@ -234,22 +235,25 @@ static UniValue generatetodescriptor(const JSONRPCRequest& request)
234235
ChainstateManager& chainman = EnsureChainman(request.context);
235236

236237
return generateBlocks(chainman, mempool, coinbase_script, num_blocks, max_tries);
238+
},
239+
};
237240
}
238241

239-
static UniValue generate(const JSONRPCRequest& request)
242+
static RPCHelpMan generate()
240243
{
241-
const std::string help_str{"generate ( nblocks maxtries ) has been replaced by the -generate cli option. Refer to -help for more information."};
244+
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 {
242245

243246
if (request.fHelp) {
244-
throw std::runtime_error(help_str);
247+
throw std::runtime_error(self.ToString());
245248
} else {
246-
throw JSONRPCError(RPC_METHOD_NOT_FOUND, help_str);
249+
throw JSONRPCError(RPC_METHOD_NOT_FOUND, self.ToString());
247250
}
251+
}};
248252
}
249253

250-
static UniValue generatetoaddress(const JSONRPCRequest& request)
254+
static RPCHelpMan generatetoaddress()
251255
{
252-
RPCHelpMan{"generatetoaddress",
256+
return RPCHelpMan{"generatetoaddress",
253257
"\nMine blocks immediately to a specified address (before the RPC call returns)\n",
254258
{
255259
{"nblocks", RPCArg::Type::NUM, RPCArg::Optional::NO, "How many blocks are generated immediately."},
@@ -267,8 +271,8 @@ static UniValue generatetoaddress(const JSONRPCRequest& request)
267271
+ "If you are using the " PACKAGE_NAME " wallet, you can get a new address to send the newly generated bitcoin to with:\n"
268272
+ HelpExampleCli("getnewaddress", "")
269273
},
270-
}.Check(request);
271-
274+
[&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
275+
{
272276
const int num_blocks{request.params[0].get_int()};
273277
const uint64_t max_tries{request.params[2].isNull() ? DEFAULT_MAX_TRIES : request.params[2].get_int()};
274278

@@ -283,11 +287,13 @@ static UniValue generatetoaddress(const JSONRPCRequest& request)
283287
CScript coinbase_script = GetScriptForDestination(destination);
284288

285289
return generateBlocks(chainman, mempool, coinbase_script, num_blocks, max_tries);
290+
},
291+
};
286292
}
287293

288-
static UniValue generateblock(const JSONRPCRequest& request)
294+
static RPCHelpMan generateblock()
289295
{
290-
RPCHelpMan{"generateblock",
296+
return RPCHelpMan{"generateblock",
291297
"\nMine a block with a set of ordered transactions immediately to a specified address or descriptor (before the RPC call returns)\n",
292298
{
293299
{"output", RPCArg::Type::STR, RPCArg::Optional::NO, "The address or descriptor to send the newly generated bitcoin to."},
@@ -309,8 +315,8 @@ static UniValue generateblock(const JSONRPCRequest& request)
309315
"\nGenerate a block to myaddress, with txs rawtx and mempool_txid\n"
310316
+ HelpExampleCli("generateblock", R"("myaddress" '["rawtx", "mempool_txid"]')")
311317
},
312-
}.Check(request);
313-
318+
[&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
319+
{
314320
const auto address_or_descriptor = request.params[0].get_str();
315321
CScript coinbase_script;
316322
std::string error;
@@ -390,11 +396,13 @@ static UniValue generateblock(const JSONRPCRequest& request)
390396
UniValue obj(UniValue::VOBJ);
391397
obj.pushKV("hash", block_hash.GetHex());
392398
return obj;
399+
},
400+
};
393401
}
394402

395-
static UniValue getmininginfo(const JSONRPCRequest& request)
403+
static RPCHelpMan getmininginfo()
396404
{
397-
RPCHelpMan{"getmininginfo",
405+
return RPCHelpMan{"getmininginfo",
398406
"\nReturns a json object containing mining-related information.",
399407
{},
400408
RPCResult{
@@ -413,8 +421,8 @@ static UniValue getmininginfo(const JSONRPCRequest& request)
413421
HelpExampleCli("getmininginfo", "")
414422
+ HelpExampleRpc("getmininginfo", "")
415423
},
416-
}.Check(request);
417-
424+
[&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
425+
{
418426
LOCK(cs_main);
419427
const CTxMemPool& mempool = EnsureMemPool(request.context);
420428

@@ -423,18 +431,20 @@ static UniValue getmininginfo(const JSONRPCRequest& request)
423431
if (BlockAssembler::m_last_block_weight) obj.pushKV("currentblockweight", *BlockAssembler::m_last_block_weight);
424432
if (BlockAssembler::m_last_block_num_txs) obj.pushKV("currentblocktx", *BlockAssembler::m_last_block_num_txs);
425433
obj.pushKV("difficulty", (double)GetDifficulty(::ChainActive().Tip()));
426-
obj.pushKV("networkhashps", getnetworkhashps(request));
434+
obj.pushKV("networkhashps", getnetworkhashps().HandleRequest(request));
427435
obj.pushKV("pooledtx", (uint64_t)mempool.size());
428436
obj.pushKV("chain", Params().NetworkIDString());
429437
obj.pushKV("warnings", GetWarnings(false).original);
430438
return obj;
439+
},
440+
};
431441
}
432442

433443

434444
// NOTE: Unlike wallet RPC (which use BTC values), mining RPCs follow GBT (BIP 22) in using satoshi amounts
435-
static UniValue prioritisetransaction(const JSONRPCRequest& request)
445+
static RPCHelpMan prioritisetransaction()
436446
{
437-
RPCHelpMan{"prioritisetransaction",
447+
return RPCHelpMan{"prioritisetransaction",
438448
"Accepts the transaction into mined blocks at a higher (or lower) priority\n",
439449
{
440450
{"txid", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The transaction id."},
@@ -451,8 +461,8 @@ static UniValue prioritisetransaction(const JSONRPCRequest& request)
451461
HelpExampleCli("prioritisetransaction", "\"txid\" 0.0 10000")
452462
+ HelpExampleRpc("prioritisetransaction", "\"txid\", 0.0, 10000")
453463
},
454-
}.Check(request);
455-
464+
[&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
465+
{
456466
LOCK(cs_main);
457467

458468
uint256 hash(ParseHashV(request.params[0], "txid"));
@@ -464,6 +474,8 @@ static UniValue prioritisetransaction(const JSONRPCRequest& request)
464474

465475
EnsureMemPool(request.context).PrioritiseTransaction(hash, nAmount);
466476
return true;
477+
},
478+
};
467479
}
468480

469481

@@ -495,9 +507,9 @@ static std::string gbt_vb_name(const Consensus::DeploymentPos pos) {
495507
return s;
496508
}
497509

498-
static UniValue getblocktemplate(const JSONRPCRequest& request)
510+
static RPCHelpMan getblocktemplate()
499511
{
500-
RPCHelpMan{"getblocktemplate",
512+
return RPCHelpMan{"getblocktemplate",
501513
"\nIf the request parameters include a 'mode' key, that is used to explicitly select between the default 'template' request or a 'proposal'.\n"
502514
"It returns data needed to construct a block to work on.\n"
503515
"For full specification, see BIPs 22, 23, 9, and 145:\n"
@@ -578,8 +590,8 @@ static UniValue getblocktemplate(const JSONRPCRequest& request)
578590
HelpExampleCli("getblocktemplate", "'{\"rules\": [\"segwit\"]}'")
579591
+ HelpExampleRpc("getblocktemplate", "{\"rules\": [\"segwit\"]}")
580592
},
581-
}.Check(request);
582-
593+
[&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
594+
{
583595
LOCK(cs_main);
584596

585597
std::string strMode = "template";
@@ -887,6 +899,8 @@ static UniValue getblocktemplate(const JSONRPCRequest& request)
887899
}
888900

889901
return result;
902+
},
903+
};
890904
}
891905

892906
class submitblock_StateCatcher final : public CValidationInterface
@@ -907,10 +921,10 @@ class submitblock_StateCatcher final : public CValidationInterface
907921
}
908922
};
909923

910-
static UniValue submitblock(const JSONRPCRequest& request)
924+
static RPCHelpMan submitblock()
911925
{
912926
// We allow 2 arguments for compliance with BIP22. Argument 2 is ignored.
913-
RPCHelpMan{"submitblock",
927+
return RPCHelpMan{"submitblock",
914928
"\nAttempts to submit new block to network.\n"
915929
"See https://en.bitcoin.it/wiki/BIP_0022 for full specification.\n",
916930
{
@@ -922,8 +936,8 @@ static UniValue submitblock(const JSONRPCRequest& request)
922936
HelpExampleCli("submitblock", "\"mydata\"")
923937
+ HelpExampleRpc("submitblock", "\"mydata\"")
924938
},
925-
}.Check(request);
926-
939+
[&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
940+
{
927941
std::shared_ptr<CBlock> blockptr = std::make_shared<CBlock>();
928942
CBlock& block = *blockptr;
929943
if (!DecodeHexBlk(block, request.params[0].get_str())) {
@@ -968,11 +982,13 @@ static UniValue submitblock(const JSONRPCRequest& request)
968982
return "inconclusive";
969983
}
970984
return BIP22ValidationResult(sc->state);
985+
},
986+
};
971987
}
972988

973-
static UniValue submitheader(const JSONRPCRequest& request)
989+
static RPCHelpMan submitheader()
974990
{
975-
RPCHelpMan{"submitheader",
991+
return RPCHelpMan{"submitheader",
976992
"\nDecode the given hexdata as a header and submit it as a candidate chain tip if valid."
977993
"\nThrows when the header is invalid.\n",
978994
{
@@ -984,8 +1000,8 @@ static UniValue submitheader(const JSONRPCRequest& request)
9841000
HelpExampleCli("submitheader", "\"aabbcc\"") +
9851001
HelpExampleRpc("submitheader", "\"aabbcc\"")
9861002
},
987-
}.Check(request);
988-
1003+
[&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
1004+
{
9891005
CBlockHeader h;
9901006
if (!DecodeHexBlockHeader(h, request.params[0].get_str())) {
9911007
throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "Block header decode failed");
@@ -1004,11 +1020,13 @@ static UniValue submitheader(const JSONRPCRequest& request)
10041020
throw JSONRPCError(RPC_VERIFY_ERROR, state.ToString());
10051021
}
10061022
throw JSONRPCError(RPC_VERIFY_ERROR, state.GetRejectReason());
1023+
},
1024+
};
10071025
}
10081026

1009-
static UniValue estimatesmartfee(const JSONRPCRequest& request)
1027+
static RPCHelpMan estimatesmartfee()
10101028
{
1011-
RPCHelpMan{"estimatesmartfee",
1029+
return RPCHelpMan{"estimatesmartfee",
10121030
"\nEstimates the approximate fee per kilobyte needed for a transaction to begin\n"
10131031
"confirmation within conf_target blocks if possible and return the number of blocks\n"
10141032
"for which the estimate is valid. Uses virtual transaction size as defined\n"
@@ -1042,8 +1060,8 @@ static UniValue estimatesmartfee(const JSONRPCRequest& request)
10421060
RPCExamples{
10431061
HelpExampleCli("estimatesmartfee", "6")
10441062
},
1045-
}.Check(request);
1046-
1063+
[&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
1064+
{
10471065
RPCTypeCheck(request.params, {UniValue::VNUM, UniValue::VSTR});
10481066
RPCTypeCheckArgument(request.params[0], UniValue::VNUM);
10491067
unsigned int max_target = ::feeEstimator.HighestTargetTracked(FeeEstimateHorizon::LONG_HALFLIFE);
@@ -1069,11 +1087,13 @@ static UniValue estimatesmartfee(const JSONRPCRequest& request)
10691087
}
10701088
result.pushKV("blocks", feeCalc.returnedTarget);
10711089
return result;
1090+
},
1091+
};
10721092
}
10731093

1074-
static UniValue estimaterawfee(const JSONRPCRequest& request)
1094+
static RPCHelpMan estimaterawfee()
10751095
{
1076-
RPCHelpMan{"estimaterawfee",
1096+
return RPCHelpMan{"estimaterawfee",
10771097
"\nWARNING: This interface is unstable and may disappear or change!\n"
10781098
"\nWARNING: This is an advanced API call that is tightly coupled to the specific\n"
10791099
" implementation of fee estimation. The parameters it can be called with\n"
@@ -1125,8 +1145,8 @@ static UniValue estimaterawfee(const JSONRPCRequest& request)
11251145
RPCExamples{
11261146
HelpExampleCli("estimaterawfee", "6 0.9")
11271147
},
1128-
}.Check(request);
1129-
1148+
[&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
1149+
{
11301150
RPCTypeCheck(request.params, {UniValue::VNUM, UniValue::VNUM}, true);
11311151
RPCTypeCheckArgument(request.params[0], UniValue::VNUM);
11321152
unsigned int max_target = ::feeEstimator.HighestTargetTracked(FeeEstimateHorizon::LONG_HALFLIFE);
@@ -1185,6 +1205,8 @@ static UniValue estimaterawfee(const JSONRPCRequest& request)
11851205
result.pushKV(StringForFeeEstimateHorizon(horizon), horizon_result);
11861206
}
11871207
return result;
1208+
},
1209+
};
11881210
}
11891211

11901212
void RegisterMiningRPCCommands(CRPCTable &t)

‎src/rpc/server.cpp‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -261,13 +261,11 @@ CRPCTable::CRPCTable()
261261
}
262262
}
263263

264-
bool CRPCTable::appendCommand(const std::string& name, const CRPCCommand* pcmd)
264+
void CRPCTable::appendCommand(const std::string& name, const CRPCCommand* pcmd)
265265
{
266-
if (IsRPCRunning())
267-
return false;
266+
CHECK_NONFATAL(!IsRPCRunning()); // Only add commands before rpc is running
268267

269268
mapCommands[name].push_back(pcmd);
270-
return true;
271269
}
272270

273271
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
@@ -160,7 +160,7 @@ class CRPCTable
160160
/**
161161
* Appends a CRPCCommand to the dispatch table.
162162
*
163-
* Returns false if RPC server is already running (dump concurrency protection).
163+
* Precondition: RPC server is not running
164164
*
165165
* Commands with different method names but the same unique_id will
166166
* be considered aliases, and only the first registered method name will
@@ -169,7 +169,7 @@ class CRPCTable
169169
* between calls based on method name, and aliased commands can also
170170
* register different names, types, and numbers of parameters.
171171
*/
172-
bool appendCommand(const std::string& name, const CRPCCommand* pcmd);
172+
void appendCommand(const std::string& name, const CRPCCommand* pcmd);
173173
bool removeCommand(const std::string& name, const CRPCCommand* pcmd);
174174
};
175175

0 commit comments

Comments
 (0)