The Arg() helper has many benefits:
isNull checksSo use it more broadly.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline and AI policy for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | stickies-v, fanquake |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
-Wunused by fanquake)CAmount a class by hodlinator)If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed.
<sub>Task iwyu: https://github.com/bitcoin/bitcoin/actions/runs/36623882963/job/109595800670</sub>
<sub>LLM reason (✨ experimental): CI failed because IWYU detected incorrect/missing includes and triggered a “Failure generated from IWYU” (returned exit code 1).</sub>
<details><summary>Hints</summary>
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
</details>
Concept ACK
Forgot to say that everything here is a refactor, except for the cleanup commit that changes the schema of one RPC:
diff --git a/getopenrpcinfo.full.json b/getopenrpcinfo.full.json
index e0cebef..cbbf785 100644
--- a/getopenrpcinfo.full.json
+++ b/getopenrpcinfo.full.json
@@ -15035,5 +15035,5 @@
"type": "number",
- "x-bitcoin-default-hint": "3, range=0-4"
+ "default": 3
},
- "description": "How thorough the block verification is:\n- level 0 reads the blocks from disk\n- level 1 verifies block validity\n- level 2 verifies undo data\n- level 3 checks disconnection of tip blocks\n- level 4 tries to reconnect the blocks\n- each level includes the checks of the previous levels"
+ "description": "How thorough the block verification is (range 0-4):\n- level 0 reads the blocks from disk\n- level 1 verifies block validity\n- level 2 verifies undo data\n- level 3 checks disconnection of tip blocks\n- level 4 tries to reconnect the blocks\n- each level includes the checks of the previous levels"
},
@@ -15044,5 +15044,5 @@
"type": "number",
- "x-bitcoin-default-hint": "6, 0=all"
+ "default": 6
},
- "description": "The number of blocks to check."
+ "description": "The number of blocks to check (0=all)."
}
diff --git a/verifychain b/verifychain
index b742f17..1863a80 100644
--- a/verifychain
+++ b/verifychain
@@ -5,3 +5,3 @@ Verifies blockchain database.
Arguments:
-1. checklevel (numeric, optional, default=3, range=0-4) How thorough the block verification is:
+1. checklevel (numeric, optional, default=3) How thorough the block verification is (range 0-4):
- level 0 reads the blocks from disk
@@ -12,3 +12,3 @@ Arguments:
- each level includes the checks of the previous levels
-2. nblocks (numeric, optional, default=6, 0=all) The number of blocks to check.
+2. nblocks (numeric, optional, default=6) The number of blocks to check (0=all).
29 | @@ -30,8 +30,8 @@ 30 | #include <util/translation.h> 31 | 32 | #include <algorithm> 33 | -#include <iterator>
@RalphLLM removing this is correct when ranges is included, see https://eel.is/c++draft/ranges.syn
rebased and replied to ralph. edit: Another push for ralph
Concept ACK
768 | @@ -769,9 +769,7 @@ std::vector<std::pair<std::string, bool>> RPCMethod::GetArgNames() const 769 | 770 | size_t RPCMethod::GetParamIndex(std::string_view key) const 771 | { 772 | - auto it{std::find_if( 773 | - m_args.begin(), m_args.end(), [&key](const auto& arg) { return arg.GetName() == key;} 774 | - )}; 775 | + auto it{std::ranges::find_if(m_args, [&key](const auto& arg) { return arg.GetFirstName() == key; })};
nit: would be useful to document this in the interface too:
<details> <summary>git diff on fada6e1dcb</summary>
diff --git a/src/rpc/util.h b/src/rpc/util.h
index 949a275910..bf17885b9a 100644
--- a/src/rpc/util.h
+++ b/src/rpc/util.h
@@ -462,6 +462,7 @@ public:
*
* The instantiation of this helper for type R must match the corresponding RPCArg::Type.
*
+ * [@param](/bitcoin-bitcoin/contributor/param/) key The first name of the argument. Aliases are not supported.
* [@return](/bitcoin-bitcoin/contributor/return/) The value of the RPC argument (or the default value) cast to type R.
*
* [@see](/bitcoin-bitcoin/contributor/see/) MaybeArg for handling optional arguments without default values.
@@ -492,6 +493,7 @@ public:
*
* The instantiation of this helper for type R must match the corresponding RPCArg::Type.
*
+ * [@param](/bitcoin-bitcoin/contributor/param/) key The first name of the argument. Aliases are not supported.
* [@return](/bitcoin-bitcoin/contributor/return/) For trivially copyable types, a std::optional<R> is returned.
* For other types, a R* pointer to the argument is returned. If the
* argument is not provided, std::nullopt or a null pointer is returned.
@@ -533,7 +535,7 @@ private:
mutable const JSONRPCRequest* m_req{nullptr}; // A pointer to the request for the duration of m_fun()
template <typename R>
R ArgValue(size_t i) const;
- //! Return positional index of a parameter using its name as key.
+ //! Return positional index of a parameter using its first name as key.
size_t GetParamIndex(std::string_view key) const;
};
</details>
nit: people might be tempted to "upgrade" this to accept aliases too (I spent a few cycles thinking about it), so perhaps worth briefly documenting why we only accept first name here?
<details> <summary>git diff on fada6e1dcb</summary>
diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp
index 2c7f6a96ee..e2cf363a97 100644
--- a/src/rpc/util.cpp
+++ b/src/rpc/util.cpp
@@ -771,6 +771,7 @@ std::vector<std::pair<std::string, bool>> RPCMethod::GetArgNames() const
size_t RPCMethod::GetParamIndex(std::string_view key) const
{
+ // Use only the first name, so that callers are uniform and legacy aliases can be removed.
auto it{std::ranges::find_if(m_args, [&key](const auto& arg) { return arg.GetFirstName() == key; })};
CHECK_NONFATAL(it != m_args.end()); // TODO: ideally this is checked at compile time
</details>
646 | @@ -647,10 +647,10 @@ BOOST_AUTO_TEST_CASE(rpc_arg_helper) 647 | // Default arg 648 | {"def_uint64_t", RPCArg::Type::NUM, RPCArg::Default{DEFAULT_UINT64_T}, ""}, 649 | {"def_string", RPCArg::Type::STR, RPCArg::Default{DEFAULT_STRING}, ""}, 650 | - {"def_bool", RPCArg::Type::BOOL, RPCArg::Default{DEFAULT_BOOL}, ""}, 651 | + {"def_bool|legacy_def_bool", RPCArg::Type::BOOL, RPCArg::Default{DEFAULT_BOOL}, ""},
nit: commit message states "The test is modified to show the second legacy alias is ignored." but that's not true, unless we do something like BOOST_CHECK_THROW(self.Arg<bool>("legacy_def_bool"), NonFatalCheckError); - which I think we should if only allowing the first name is intentional (cfr this comment)
760 | - bool include_mempool_sequence = false; 761 | - if (!request.params[1].isNull()) { 762 | - include_mempool_sequence = request.params[1].get_bool(); 763 | - } 764 | + const bool fVerbose{self.Arg<bool>("verbose")}; 765 | + const bool include_mempool_sequence{self.Arg<bool>("mempool_sequence")};
nit: helper variables often seem to bring more noise than clarity after this clean up, so i think they could be removed fully in many cases?
<details> <summary>git diff on fa005e85e9</summary>
diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
index a6bbc308f7..96ceff6d23 100644
--- a/src/rpc/blockchain.cpp
+++ b/src/rpc/blockchain.cpp
@@ -671,8 +671,6 @@ static RPCMethod getblockheader()
{
uint256 hash(ParseHashV(request.params[0], "hash"));
- const bool fVerbose{self.Arg<bool>("verbose")};
-
const CBlockIndex* pblockindex;
const CBlockIndex* tip;
ChainstateManager& chainman = EnsureAnyChainman(request.context);
@@ -686,8 +684,7 @@ static RPCMethod getblockheader()
throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block not found");
}
- if (!fVerbose)
- {
+ if (!self.Arg<bool>("verbose")) {
DataStream ssBlock{};
ssBlock << pblockindex->GetBlockHeader();
std::string strHex = HexStr(ssBlock);
@@ -1249,13 +1246,12 @@ static RPCMethod gettxout()
auto hash{Txid::FromUint256(ParseHashV(request.params[0], "txid"))};
COutPoint out{hash, request.params[1].getInt<uint32_t>()};
- const bool fMempool{self.Arg<bool>("include_mempool")};
Chainstate& active_chainstate = chainman.ActiveChainstate();
CCoinsViewCache* coins_view = &active_chainstate.CoinsTip();
std::optional<Coin> coin;
- if (fMempool) {
+ if (self.Arg<bool>("include_mempool")) {
const CTxMemPool& mempool = EnsureMemPool(node);
LOCK(mempool.cs);
CCoinsViewMemPool view(coins_view, mempool);
@@ -2922,9 +2918,7 @@ static RPCMethod getdescriptoractivity()
}
}
- const bool search_mempool{self.Arg<bool>("include_mempool")};
-
- if (search_mempool) {
+ if (self.Arg<bool>("include_mempool")) {
const CTxMemPool& mempool = EnsureMemPool(node);
LOCK(::cs_main);
LOCK(mempool.cs);
diff --git a/src/rpc/mempool.cpp b/src/rpc/mempool.cpp
index c128723a6f..a0008708ad 100644
--- a/src/rpc/mempool.cpp
+++ b/src/rpc/mempool.cpp
@@ -753,10 +753,7 @@ static RPCMethod getrawmempool()
},
[](const RPCMethod& self, const JSONRPCRequest& request) -> UniValue
{
- const bool fVerbose{self.Arg<bool>("verbose")};
- const bool include_mempool_sequence{self.Arg<bool>("mempool_sequence")};
-
- return MempoolToJSON(EnsureAnyMemPool(request.context), fVerbose, include_mempool_sequence);
+ return MempoolToJSON(EnsureAnyMemPool(request.context), self.Arg<bool>("verbose"), self.Arg<bool>("mempool_sequence"));
},
};
}
@@ -786,8 +783,6 @@ static RPCMethod getmempoolancestors()
},
[](const RPCMethod& self, const JSONRPCRequest& request) -> UniValue
{
- const bool fVerbose{self.Arg<bool>("verbose")};
-
auto txid{Txid::FromUint256(ParseHashV(request.params[0], "txid"))};
const CTxMemPool& mempool = EnsureAnyMemPool(request.context);
@@ -800,7 +795,7 @@ static RPCMethod getmempoolancestors()
auto ancestors{mempool.CalculateMemPoolAncestors(*entry)};
- if (!fVerbose) {
+ if (!self.Arg<bool>("verbose")) {
UniValue o(UniValue::VARR);
for (CTxMemPool::txiter ancestorIt : ancestors) {
o.push_back(ancestorIt->GetTx().GetHash().ToString());
@@ -845,8 +840,6 @@ static RPCMethod getmempooldescendants()
},
[](const RPCMethod& self, const JSONRPCRequest& request) -> UniValue
{
- const bool fVerbose{self.Arg<bool>("verbose")};
-
auto txid{Txid::FromUint256(ParseHashV(request.params[0], "txid"))};
const CTxMemPool& mempool = EnsureAnyMemPool(request.context);
@@ -862,7 +855,7 @@ static RPCMethod getmempooldescendants()
// CTxMemPool::CalculateDescendants will include the given tx
setDescendants.erase(*it);
- if (!fVerbose) {
+ if (!self.Arg<bool>("verbose")) {
UniValue o(UniValue::VARR);
for (CTxMemPool::txiter descendantIt : setDescendants) {
o.push_back(descendantIt->GetTx().GetHash().ToString());
diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp
index e407b95c6b..ef507d3b15 100644
--- a/src/rpc/net.cpp
+++ b/src/rpc/net.cpp
@@ -1057,7 +1057,6 @@ static RPCMethod addpeeraddress()
const std::string& addr_string{request.params[0].get_str()};
const auto port{request.params[1].getInt<uint16_t>()};
- const bool tried{self.Arg<bool>("tried")};
UniValue obj(UniValue::VOBJ);
std::optional<CNetAddr> net_addr{LookupHost(addr_string, false)};
@@ -1074,7 +1073,7 @@ static RPCMethod addpeeraddress()
// announcing itself.
if (addrman.Add({address}, address)) {
success = true;
- if (tried) {
+ if (self.Arg<bool>("tried")) {
// Attempt to move the address to the tried addresses table.
if (!addrman.Good(address)) {
success = false;
diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp
index 031ec21c6a..2279315239 100644
--- a/src/rpc/rawtransaction.cpp
+++ b/src/rpc/rawtransaction.cpp
@@ -1693,8 +1693,6 @@ static RPCMethod finalizepsbt()
}
PartiallySignedTransaction psbtx = *psbt_res;
- const bool extract{self.Arg<bool>("extract")};
-
CMutableTransaction mtx;
bool complete = FinalizeAndExtractPSBT(psbtx, mtx);
@@ -1702,7 +1700,7 @@ static RPCMethod finalizepsbt()
DataStream ssTx{};
std::string result_str;
- if (complete && extract) {
+ if (complete && self.Arg<bool>("extract")) {
ssTx << TX_WITH_WITNESS(mtx);
result_str = HexStr(ssTx);
result.pushKV("hex", result_str);
@@ -2117,15 +2115,13 @@ RPCMethod descriptorprocesspsbt()
}
std::optional<int> sighash_type = ParseSighashString(request.params[2]);
- const bool bip32derivs{self.Arg<bool>("bip32derivs")};
- const bool finalize{self.Arg<bool>("finalize")};
const PartiallySignedTransaction& psbtx = ProcessPSBT(
request.params[0].get_str(),
request.context,
- HidingSigningProvider(&provider, /*hide_secret=*/false, !bip32derivs),
+ HidingSigningProvider(&provider, /*hide_secret=*/false, !self.Arg<bool>("bip32derivs")),
sighash_type,
- finalize);
+ self.Arg<bool>("finalize"));
// Check whether or not all of the inputs are now correctly signed
bool complete = true;
diff --git a/src/wallet/rpc/spend.cpp b/src/wallet/rpc/spend.cpp
index 8a78a612d9..03b0a01fb3 100644
--- a/src/wallet/rpc/spend.cpp
+++ b/src/wallet/rpc/spend.cpp
@@ -335,9 +335,8 @@ RPCMethod sendtoaddress()
}
std::vector<CRecipient> recipients{CreateRecipients(ParseOutputs(address_amounts), sffo_set)};
- const bool verbose{self.Arg<bool>("verbose")};
- return SendMoney(*pwallet, coin_control, recipients, comment, comment_to, verbose);
+ return SendMoney(*pwallet, coin_control, recipients, comment, comment_to, self.Arg<bool>("verbose"));
},
};
}
@@ -431,9 +430,8 @@ RPCMethod sendmany()
ParseOutputs(sendTo),
InterpretSubtractFeeFromOutputInstructions(request.params[4], sendTo.getKeys())
);
- const bool verbose{self.Arg<bool>("verbose")};
- return SendMoney(*pwallet, coin_control, recipients, comment, /*comment_to=*/std::nullopt, verbose);
+ return SendMoney(*pwallet, coin_control, recipients, comment, /*comment_to=*/std::nullopt, self.Arg<bool>("verbose"));
},
};
}
@@ -1825,9 +1823,8 @@ RPCMethod walletcreatefundedpsbt()
PartiallySignedTransaction psbtx(CMutableTransaction(*txr.tx), psbt_version);
// Fill transaction with our data but don't sign
- const bool bip32derivs{self.Arg<bool>("bip32derivs")};
bool complete = true;
- const auto err{wallet.FillPSBT(psbtx, {.sign = false, .bip32_derivs = bip32derivs}, complete)};
+ const auto err{wallet.FillPSBT(psbtx, {.sign = false, .bip32_derivs = self.Arg<bool>("bip32derivs")}, complete)};
if (err) {
throw JSONRPCPSBTError(*err);
}
diff --git a/src/wallet/rpc/transactions.cpp b/src/wallet/rpc/transactions.cpp
index dd5361ce66..4be755b717 100644
--- a/src/wallet/rpc/transactions.cpp
+++ b/src/wallet/rpc/transactions.cpp
@@ -247,11 +247,9 @@ RPCMethod listreceivedbyaddress()
// the user could have gotten from another RPC command prior to now
pwallet->BlockUntilSyncedToCurrentChain();
- const bool include_immature_coinbase{self.Arg<bool>("include_immature_coinbase")};
-
LOCK(pwallet->cs_wallet);
- return ListReceived(*pwallet, request.params, false, include_immature_coinbase);
+ return ListReceived(*pwallet, request.params, false, self.Arg<bool>("include_immature_coinbase"));
},
};
}
@@ -292,11 +290,9 @@ RPCMethod listreceivedbylabel()
// the user could have gotten from another RPC command prior to now
pwallet->BlockUntilSyncedToCurrentChain();
- const bool include_immature_coinbase{self.Arg<bool>("include_immature_coinbase")};
-
LOCK(pwallet->cs_wallet);
- return ListReceived(*pwallet, request.params, true, include_immature_coinbase);
+ return ListReceived(*pwallet, request.params, true, self.Arg<bool>("include_immature_coinbase"));
},
};
}
@@ -889,8 +885,6 @@ RPCMethod gettransaction()
Txid hash{Txid::FromUint256(ParseHashV(request.params[0], "txid"))};
- const bool verbose{self.Arg<bool>("verbose")};
-
UniValue entry(UniValue::VOBJ);
auto it = pwallet->mapWallet.find(hash);
if (it == pwallet->mapWallet.end()) {
@@ -908,7 +902,7 @@ RPCMethod gettransaction()
entry.pushKV("hex", EncodeHexTx(*wtx.GetTx()));
- if (verbose) {
+ if (self.Arg<bool>("verbose")) {
PushTxDecoded(*pwallet, wtx, entry);
}
</details>
Hmm, this may change the parsing order, so I think I'll leave this as-is for now to keep it a "refactor" that is easy to review.
ACK fa005e85e9495b9c3cb41e382cd695d317f301f9
Increased self.Arg usage makes RPC code easier and more robust, and this PR is easy to review.
GetName does not support aliases at all. However, it should be fine for
GetParamIndex (used in Arg and MaybeArg) to support lookup by the first
name.
The test is modified to show the second legacy alias is ignored.
The test-only's generatetoaddress's maxtries is now u64. It is
consistent with the other maxtries parsing, which was changed earlier:
```
$ git show 30a6c999351041d6a1e8712a9659be1296a1b46a | grep '"maxtries"'
+ const auto max_tries{self.Arg<uint64_t>("maxtries")};
```
Once it is parsed as i32, otherwise as u32. Use u32 consistently, which
is also used in the ctor.
The Arg() helper has many benefits:
* It does not require manual isNull checks
* It does not require hard-coding the fallback/default value a second time
* It does not require manually specifying the getter for the type. Specifying the type is enough.
So use it in this refactor, which does not change any behavior.
Use same wording as in walletprocesspsbt
re-ACK fa7b2d05387ef2735b1a8b0b1a91ab4d3a070b4b
No changes except for addressing doc and test change suggestions.
ACK fa7b2d05387ef2735b1a8b0b1a91ab4d3a070b4b
We can do more similar cleanups in future. i.e:
@@ -465,7 +465,7 @@ RPCMethod restorewallet()
- std::optional<bool> load_on_start = request.params[2].isNull() ? std::nullopt : std::optional<bool>(request.params[2].get_bool());
+ std::optional<bool> load_on_start{self.MaybeArg<bool>("load_on_startup")};
diff --git a/src/wallet/rpc/spend.cpp b/src/wallet/rpc/spend.cpp
@@ -313,9 +313,7 @@ RPCMethod sendtoaddress()
CCoinControl coin_control;
- if (!request.params[5].isNull()) {
- coin_control.m_signal_bip125_rbf = request.params[5].get_bool();
- }
+ coin_control.m_signal_bip125_rbf = self.MaybeArg<bool>("replaceable");
@@ -421,9 +419,7 @@ RPCMethod sendmany()
CCoinControl coin_control;
- if (!request.params[5].isNull()) {
- coin_control.m_signal_bip125_rbf = request.params[5].get_bool();
- }
+ coin_control.m_signal_bip125_rbf = self.MaybeArg<bool>("replaceable");
diff --git a/src/wallet/rpc/wallet.cpp b/src/wallet/rpc/wallet.cpp
@@ -263,7 +263,7 @@ static RPCMethod loadwallet()
- std::optional<bool> load_on_start = request.params[1].isNull() ? std::nullopt : std::optional<bool>(request.params[1].get_bool());
+ std::optional<bool> load_on_start{self.MaybeArg<bool>("load_on_startup")};
@@ -398,8 +398,8 @@ static RPCMethod createwallet()
- if (!request.params[3].isNull()) {
- passphrase = std::string_view{request.params[3].get_str()};
+ if (const auto pass{self.MaybeArg<std::string_view>("passphrase")}) {
+ passphrase = *pass;
@@ -428,7 +428,7 @@ static RPCMethod createwallet()
- std::optional<bool> load_on_start = request.params[6].isNull() ? std::nullopt : std::optional<bool>(request.params[6].get_bool());
+ std::optional<bool> load_on_start{self.MaybeArg<bool>("load_on_startup")};
@@ -624,8 +624,8 @@ static RPCMethod migratewallet()
- if (!request.params[1].isNull()) {
- wallet_pass = std::string_view{request.params[1].get_str()};
+ if (const auto pass{self.MaybeArg<std::string_view>("passphrase")}) {
+ wallet_pass = *pass;
Nice!