rpc: Keep default argument value in correct type #21679

promag commented at 2:09 PM on April 14, 2021: member

Store default values of RPC arguments in the corresponding type instead of a string. The value is then serialized when the help output is needed. This change simplifies #20017.

The following examples illustrates how to use the new RPCArg::Default and RPCArg::DefaultHint:

- {"verbose", RPCArg::Type::BOOL, /* default */ "false", "True for a json object, false for array of transaction ids"}
+ {"verbose", RPCArg::Type::BOOL, RPCArg::Default(false), "True for a json object, false for array of transaction ids"}

- {"nblocks", RPCArg::Type::NUM, /* default */ "one month", "Size of the window in number of blocks"}
+ {"nblocks", RPCArg::Type::NUM, RPCArg::DefaultHint("one month"), "Size of the window in number of blocks"}

No behavior change is expected.

fanquake added the label RPC/REST/ZMQ on Apr 14, 2021

in src/wallet/rpcwallet.cpp:3417 in 9195574361 outdated

3416 |                               "original transaction that were less than 0xfffffffe will be increased to 0xfffffffe\n"
3417 |                               "so the new transaction will not be explicitly bip-125 replaceable (though it may\n"
3418 |                               "still be replaceable in practice, for example if it has unconfirmed ancestors which\n"
3419 |                               "are replaceable).\n"},
3420 | -                    {"estimate_mode", RPCArg::Type::STR, /* default */ "unset", std::string() + "The fee estimate mode, must be one of (case insensitive):\n"
3421 | +                    {"estimate_mode", RPCArg::Type::STR, RPCArg::Default("unset"), std::string() + "The fee estimate mode, must be one of (case insensitive):\n"

MarcoFalke commented at 2:22 PM on April 14, 2021:

                    {"estimate_mode", RPCArg::Type::STR, RPCArg::Default("unset"), "The fee estimate mode, must be one of (case insensitive):\n"

unrelated nit (feel free to fix in a new commit or not at all)

promag commented at 2:39 PM on April 14, 2021:

Done.

MarcoFalke commented at 2:25 PM on April 14, 2021: member

cr ACK 9195574361a6b81930679ece7fd28956d2ba6436

style-nit: Could use { instead of ( :sweat_smile: (feel free to ignore, if the replacement can't be done with a scripted-diff)

in src/rpc/mining.cpp:245 in 9195574361 outdated

 240 | @@ -241,8 +241,8 @@ static RPCHelpMan generatetodescriptor()
 241 |  
 242 |  static RPCHelpMan generate()
 243 |  {
 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 {
 245 | -        throw JSONRPCError(RPC_METHOD_NOT_FOUND, self.ToString());
 246 | +    return RPCHelpMan{"generate", "has been replaced by the -generate cli option. Refer to -help for more information.", {}, {}, RPCExamples{""}, [](const RPCContext& ctx) -> UniValue {
 247 | +        throw JSONRPCError(RPC_METHOD_NOT_FOUND, ctx.m_helpman.ToString());

MarcoFalke commented at 2:25 PM on April 14, 2021:

what is this change?

promag commented at 2:38 PM on April 14, 2021:

Bad fixup

promag force-pushed on Apr 14, 2021

promag commented at 2:41 PM on April 14, 2021: member

style-nit: Could use { instead of ( 😅 (feel free to ignore, if the replacement can't be done with a scripted-diff)

Done, thanks for your review!

promag force-pushed on Apr 14, 2021

DrahtBot commented at 8:47 PM on April 14, 2021: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#21359 (rpc: include_unsafe option for fundrawtransaction by t-bast)
#21283 (Implement BIP 370 PSBTv2 by achow101)
#19521 (Coinstats Index by fjahr)
#18418 (wallet: Increase OUTPUT_GROUP_MAX_ENTRIES to 100 by fjahr)
#17211 (Allow fundrawtransaction and walletcreatefundedpsbt to take external inputs by achow101)
#14707 ([RPC] Include coinbase transactions in receivedby RPCs by andrewtoth)

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.

promag commented at 8:58 AM on April 15, 2021: member

@kallewoof @LarryRuane mind taking a look here?

in src/rpc/blockchain.cpp:546 in b9e18b5494 outdated

 541 | @@ -542,8 +542,8 @@ static RPCHelpMan getrawmempool()
 542 |                  "\nReturns all transaction ids in memory pool as a json array of string transaction ids.\n"
 543 |                  "\nHint: use getmempoolentry to fetch a specific transaction from the mempool.\n",
 544 |                  {
 545 | -                    {"verbose", RPCArg::Type::BOOL, /* default */ "false", "True for a json object, false for array of transaction ids"},
 546 | -                    {"mempool_sequence", RPCArg::Type::BOOL, /* default */ "false", "If verbose=false, returns a json object with transaction list and mempool sequence number attached."},
 547 | +                    {"verbose", RPCArg::Type::BOOL, RPCArg::Default{0}, "True for a json object, false for array of transaction ids"},
 548 | +                    {"mempool_sequence", RPCArg::Type::BOOL, RPCArg::Default{0}, "If verbose=false, returns a json object with transaction list and mempool sequence number attached."},

MarcoFalke commented at 9:09 AM on April 15, 2021:

?

promag commented at 9:26 AM on April 15, 2021:

Do you think we should check types?

promag force-pushed on Apr 15, 2021

in src/rpc/blockchain.cpp:595 in 4a99d88978 outdated

 591 | @@ -592,7 +592,7 @@ static RPCHelpMan getmempoolancestors()
 592 |                  "\nIf txid is in the mempool, returns all in-mempool ancestors.\n",
 593 |                  {
 594 |                      {"txid", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The transaction id (must be in mempool)"},
 595 | -                    {"verbose", RPCArg::Type::BOOL, /* default */ "false", "True for a json object, false for array of transaction ids"},
 596 | +                    {"verbose", RPCArg::Type::BOOL, RPCArg::Default{0}, "True for a json object, false for array of transaction ids"},

MarcoFalke commented at 9:54 AM on April 15, 2021:

It was just an example. This is wrong everywhere since my last review

promag force-pushed on Apr 15, 2021

in src/rpc/util.h:180 in e294e3e784 outdated

 175 | +        case UniValue::VBOOL:
 176 | +            CHECK_NONFATAL(m_type == Type::BOOL);
 177 | +            break;
 178 | +        case UniValue::VNULL:
 179 | +        default:
 180 | +            break;

MarcoFalke commented at 5:38 PM on April 15, 2021:

CHECK_NONFATAL(false);

promag commented at 2:15 PM on April 16, 2021:

Added just for default: case.

in src/rpc/util.h:201 in e294e3e784 outdated

 197 | @@ -174,6 +198,7 @@ struct RPCArg {
 198 |            m_type_str{std::move(type_str)}
 199 |      {
 200 |          CHECK_NONFATAL(type != Type::ARR && type != Type::OBJ);
 201 | +        CheckFallback();

MarcoFalke commented at 5:38 PM on April 15, 2021:

Woudn't this be better checked in the RPCHelpMan constructor, which also checks the RPCArg name?

MarcoFalke commented at 7:13 AM on April 17, 2021:

:facepalm: RPCArg is a recursive datastructure, so it makes more sense to check this here (like you did initially)

in src/rpc/util.h:182 in e294e3e784 outdated

 177 | +            break;
 178 | +        case UniValue::VNULL:
 179 | +        default:
 180 | +            break;
 181 | +        }
 182 | +    }

MarcoFalke commented at 5:39 PM on April 15, 2021:

I am not really a fan of putting a 20-line function in a header

promag commented at 5:45 PM on April 15, 2021:

Yeah I made the smallest change to test this approach. I’ll move this.

promag force-pushed on Apr 15, 2021

promag force-pushed on Apr 16, 2021

in src/rpc/blockchain.cpp:785 in e3cc7902a7 outdated

 781 | @@ -782,7 +782,7 @@ static RPCHelpMan getblockheader()
 782 |                  "If verbose is true, returns an Object with information about blockheader <hash>.\n",
 783 |                  {
 784 |                      {"blockhash", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The block hash"},
 785 | -                    {"verbose", RPCArg::Type::BOOL, /* default */ "true", "true for a json object, false for the hex-encoded data"},
 786 | +                    {"verbose", RPCArg::Type::BOOL, RPCArg::Default{false}, "true for a json object, false for the hex-encoded data"},

LarryRuane commented at 5:24 PM on April 16, 2021:

Did you intend to change the default?

in src/rpc/blockchain.cpp:886 in e3cc7902a7 outdated

 882 | @@ -883,7 +883,7 @@ static RPCHelpMan getblock()
 883 |                  "If verbosity is 2, returns an Object with information about block <hash> and information about each transaction. \n",
 884 |                  {
 885 |                      {"blockhash", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The block hash"},
 886 | -                    {"verbosity|verbose", RPCArg::Type::NUM, /* default */ "1", "0 for hex-encoded data, 1 for a json object, and 2 for json object with transaction data"},
 887 | +                    {"verbosity|verbose", RPCArg::Type::NUM, RPCArg::Default{0}, "0 for hex-encoded data, 1 for a json object, and 2 for json object with transaction data"},

LarryRuane commented at 5:25 PM on April 16, 2021:

Default change?

in src/rpc/blockchain.cpp:1108 in e3cc7902a7 outdated

1104 | @@ -1105,7 +1105,7 @@ static RPCHelpMan gettxout()
1105 |          {
1106 |              {"txid", RPCArg::Type::STR, RPCArg::Optional::NO, "The transaction id"},
1107 |              {"n", RPCArg::Type::NUM, RPCArg::Optional::NO, "vout number"},
1108 | -            {"include_mempool", RPCArg::Type::BOOL, /* default */ "true", "Whether to include the mempool. Note that an unspent output that is spent in the mempool won't appear."},
1109 | +            {"include_mempool", RPCArg::Type::BOOL, RPCArg::Default{false}, "Whether to include the mempool. Note that an unspent output that is spent in the mempool won't appear."},

LarryRuane commented at 5:27 PM on April 16, 2021:

Default change?

in src/rpc/blockchain.cpp:1190 in e3cc7902a7 outdated

1184 | @@ -1185,9 +1185,9 @@ static RPCHelpMan verifychain()
1185 |      return RPCHelpMan{"verifychain",
1186 |                  "\nVerifies blockchain database.\n",
1187 |                  {
1188 | -                    {"checklevel", RPCArg::Type::NUM, /* default */ strprintf("%d, range=0-4", DEFAULT_CHECKLEVEL),
1189 | +                    {"checklevel", RPCArg::Type::NUM, RPCArg::DefaultHint{strprintf("%d, range=0-4", DEFAULT_CHECKLEVEL)},
1190 |                          strprintf("How thorough the block verification is:\n - %s", Join(CHECKLEVEL_DOC, "\n- "))},
1191 | -                    {"nblocks", RPCArg::Type::NUM, /* default */ strprintf("%d, 0=all", DEFAULT_CHECKBLOCKS), "The number of blocks to check."},
1192 | +                    {"nblocks", RPCArg::Type::NUM, RPCArg::DefaultHint{strprintf("%d, range=0-4", DEFAULT_CHECKLEVEL)}, "The number of blocks to check."},

LarryRuane commented at 5:43 PM on April 16, 2021:

Default (hint) change?

promag commented at 11:06 PM on April 16, 2021:

😱 how did this happen?

in src/rpc/blockchain.cpp:1663 in e3cc7902a7 outdated

1658 | @@ -1659,8 +1659,8 @@ static RPCHelpMan getchaintxstats()
1659 |      return RPCHelpMan{"getchaintxstats",
1660 |                  "\nCompute statistics about the total number and rate of transactions in the chain.\n",
1661 |                  {
1662 | -                    {"nblocks", RPCArg::Type::NUM, /* default */ "one month", "Size of the window in number of blocks"},
1663 | -                    {"blockhash", RPCArg::Type::STR_HEX, /* default */ "chain tip", "The hash of the block that ends the window."},
1664 | +                    {"nblocks", RPCArg::Type::NUM, RPCArg::DefaultHint{strprintf("%d, range=0-4", DEFAULT_CHECKLEVEL)}, "Size of the window in number of blocks"},
1665 | +                    {"blockhash", RPCArg::Type::STR_HEX, RPCArg::DefaultHint{strprintf("%d, range=0-4", DEFAULT_CHECKLEVEL)}, "The hash of the block that ends the window."},

LarryRuane commented at 5:45 PM on April 16, 2021:

Default (hint) change?

in src/rpc/blockchain.cpp:1809 in e3cc7902a7 outdated

1805 | @@ -1806,7 +1806,7 @@ static RPCHelpMan getblockstats()
1806 |                  "It won't work for some heights with pruning.\n",
1807 |                  {
1808 |                      {"hash_or_height", RPCArg::Type::NUM, RPCArg::Optional::NO, "The block hash or height of the target block", "", {"", "string or numeric"}},
1809 | -                    {"stats", RPCArg::Type::ARR, /* default */ "all values", "Values to plot (see result below)",
1810 | +                    {"stats", RPCArg::Type::ARR, RPCArg::DefaultHint{strprintf("%d, range=0-4", DEFAULT_CHECKLEVEL)}, "Values to plot (see result below)",

LarryRuane commented at 5:46 PM on April 16, 2021:

Default (hint) change?

promag commented at 11:20 PM on April 16, 2021:

Thanks.

in src/rpc/mining.cpp:516 in e3cc7902a7 outdated

 512 | @@ -513,7 +513,7 @@ static RPCHelpMan getblocktemplate()
 513 |          "    https://github.com/bitcoin/bips/blob/master/bip-0009.mediawiki#getblocktemplate_changes\n"
 514 |          "    https://github.com/bitcoin/bips/blob/master/bip-0145.mediawiki\n",
 515 |          {
 516 | -            {"template_request", RPCArg::Type::OBJ, "{}", "Format of the template",
 517 | +            {"template_request", RPCArg::Type::OBJ, RPCArg::Default{UniValue::VOBJ}, "Format of the template",

LarryRuane commented at 5:48 PM on April 16, 2021:

nit, consider prefacing with the comment "empty object" (I had to think about what this was doing).

promag commented at 11:18 PM on April 16, 2021:

Kept as-is for simplicity.

in src/rpc/util.cpp:502 in e3cc7902a7 outdated

 497 | @@ -498,6 +498,33 @@ RPCHelpMan::RPCHelpMan(std::string name, std::string description, std::vector<RP
 498 |          for (const std::string& name : names) {
 499 |              CHECK_NONFATAL(named_args.insert(name).second);
 500 |          }
 501 | +        // Default value type should match argument type
 502 | +        if (arg.m_fallback.index() == 2) {

LarryRuane commented at 5:54 PM on April 16, 2021:

nit, perhaps comment like "only the Default (variant 2) needs to be [or can be?] checked" (to explain where the 2 came from).

promag commented at 11:20 PM on April 16, 2021:

Just appended "only when defined", didn't want to duplicate what's in code, and I think is obvious after checking m_fallback type.

LarryRuane commented at 6:04 PM on April 16, 2021: contributor

cr tested ACK, very nice PR! Tested by making the following change

--- a/src/rpc/blockchain.cpp
+++ b/src/rpc/blockchain.cpp
@@ -883,7 +883,7 @@ static RPCHelpMan getblock()
                 "If verbosity is 2, returns an Object with information about block <hash> and information about each transaction. \n",
                 {
                     {"blockhash", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The block hash"},
-                    {"verbosity|verbose", RPCArg::Type::NUM, RPCArg::Default{0}, "0 for hex-encoded data, 1 for a json object, and 2 for json object with transaction data"},
+                    {"verbosity|verbose", RPCArg::Type::NUM, RPCArg::Default{"0"}, "0 for hex-encoded data, 1 for a json object, and 2 for json object with transaction data"},
                 },
                 {
                     RPCResult{"for verbosity = 0",

and verified that bitcoind failed during startup (I don't know why the errors printed twice, maybe that should be fixed)

[...]
2021-04-16T18:02:03Z Using 16 MiB out of 32/2 requested for script execution cache, able to store 524288 elements
2021-04-16T18:02:03Z Script verification uses 7 additional threads
2021-04-16T18:02:03Z scheduler thread start
2021-04-16T18:02:03Z 

************************
EXCEPTION: 18NonFatalCheckError       
./rpc/util.h:170 (CheckFallback)
Internal bug detected: 'm_type == Type::STR || m_type == Type::STR_HEX || m_type == Type::AMOUNT'
You may report this issue here: https://github.com/bitcoin/bitcoin/issues
       
bitcoin in AppInit()       



************************
EXCEPTION: 18NonFatalCheckError       
./rpc/util.h:170 (CheckFallback)
Internal bug detected: 'm_type == Type::STR || m_type == Type::STR_HEX || m_type == Type::AMOUNT'
You may report this issue here: https://github.com/bitcoin/bitcoin/issues
       
bitcoin in AppInit()       

2021-04-16T18:02:03Z Shutdown: In progress...
2021-04-16T18:02:03Z scheduler thread exit
2021-04-16T18:02:03Z Shutdown: done

promag force-pushed on Apr 16, 2021

rpc: Keep default argument value in correct type f81ef4303e

rpc: Check default value type againts argument type bee56c78e9

promag force-pushed on Apr 16, 2021

MarcoFalke commented at 6:54 AM on April 17, 2021: member

<details><summary>Rendered diff first commit:</summary>

diff --git a/bumpfee b/bumpfee
index 81181d8..accdd42 100644
--- a/bumpfee
+++ b/bumpfee
@@ -35 +35 @@ Arguments:
-       "estimate_mode": "str",    (string, optional, default=unset) The fee estimate mode, must be one of (case insensitive):
+       "estimate_mode": "str",    (string, optional, default="unset") The fee estimate mode, must be one of (case insensitive):
diff --git a/createmultisig b/createmultisig
index e2d97b0..aff8a24 100644
--- a/createmultisig
+++ b/createmultisig
@@ -13 +13 @@ Arguments:
-3. address_type    (string, optional, default=legacy) The address type to use. Options are "legacy", "p2sh-segwit", and "bech32".
+3. address_type    (string, optional, default="legacy") The address type to use. Options are "legacy", "p2sh-segwit", and "bech32".
diff --git a/estimatesmartfee b/estimatesmartfee
index 6a9d6c0..d2aa633 100644
--- a/estimatesmartfee
+++ b/estimatesmartfee
@@ -10 +10 @@ Arguments:
-2. estimate_mode    (string, optional, default=conservative) The fee estimate mode.
+2. estimate_mode    (string, optional, default="conservative") The fee estimate mode.
diff --git a/fundrawtransaction b/fundrawtransaction
index 28ea142..472a817 100644
--- a/fundrawtransaction
+++ b/fundrawtransaction
@@ -29 +29 @@ Arguments:
-       "subtractFeeFromOutputs": [    (json array, optional, default=empty array) The integers.
+       "subtractFeeFromOutputs": [    (json array, optional, default=[]) The integers.
@@ -39 +39 @@ Arguments:
-       "estimate_mode": "str",        (string, optional, default=unset) The fee estimate mode, must be one of (case insensitive):
+       "estimate_mode": "str",        (string, optional, default="unset") The fee estimate mode, must be one of (case insensitive):
diff --git a/getblockfilter b/getblockfilter
index 75c3f03..2d3ca93 100644
--- a/getblockfilter
+++ b/getblockfilter
@@ -7 +7 @@ Arguments:
-2. filtertype    (string, optional, default=basic) The type name of the filter
+2. filtertype    (string, optional, default="basic") The type name of the filter
diff --git a/gettxoutsetinfo b/gettxoutsetinfo
index a7b3271..6a3d574 100644
--- a/gettxoutsetinfo
+++ b/gettxoutsetinfo
@@ -7 +7 @@ Arguments:
-1. hash_type    (string, optional, default=hash_serialized_2) Which UTXO set hash should be calculated. Options: 'hash_serialized_2' (the legacy algorithm), 'muhash', 'none'.
+1. hash_type    (string, optional, default="hash_serialized_2") Which UTXO set hash should be calculated. Options: 'hash_serialized_2' (the legacy algorithm), 'muhash', 'none'.
diff --git a/importdescriptors b/importdescriptors
index 4fb645e..a88c5de 100644
--- a/importdescriptors
+++ b/importdescriptors
@@ -22 +22 @@ Arguments:
-         "label": "str",                    (string, optional, default='') Label to assign to the address, only allowed with internal=false
+         "label": "str",                    (string, optional, default="") Label to assign to the address, only allowed with internal=false
diff --git a/importmulti b/importmulti
index 22a13df..6a40330 100644
--- a/importmulti
+++ b/importmulti
@@ -25 +25 @@ Arguments:
-         "pubkeys": [                                               (json array, optional, default=empty array) Array of strings giving pubkeys to import. They must occur in P2PKH or P2WPKH scripts. They are not required when the private key is also provided (see the "keys" argument).
+         "pubkeys": [                                               (json array, optional, default=[]) Array of strings giving pubkeys to import. They must occur in P2PKH or P2WPKH scripts. They are not required when the private key is also provided (see the "keys" argument).
@@ -29 +29 @@ Arguments:
-         "keys": [                                                  (json array, optional, default=empty array) Array of strings giving private keys to import. The corresponding public keys must occur in the output or redeemscript.
+         "keys": [                                                  (json array, optional, default=[]) Array of strings giving private keys to import. The corresponding public keys must occur in the output or redeemscript.
@@ -36 +36 @@ Arguments:
-         "label": "str",                                            (string, optional, default='') Label to assign to the address, only allowed with internal=false
+         "label": "str",                                            (string, optional, default="") Label to assign to the address, only allowed with internal=false
diff --git a/listunspent b/listunspent
index 8d0777d..84ce469 100644
--- a/listunspent
+++ b/listunspent
@@ -10 +10 @@ Arguments:
-3. addresses                          (json array, optional, default=empty array) The bitcoin addresses to filter
+3. addresses                          (json array, optional, default=[]) The bitcoin addresses to filter
@@ -19 +19 @@ Arguments:
-       "minimumAmount": amount,       (numeric or string, optional, default=0) Minimum value of each UTXO in BTC
+       "minimumAmount": amount,       (numeric or string, optional, default="0.00") Minimum value of each UTXO in BTC
diff --git a/lockunspent b/lockunspent
index f870bcc..a3ebb80 100644
--- a/lockunspent
+++ b/lockunspent
@@ -14 +14 @@ Arguments:
-2. transactions            (json array, optional, default=empty array) The transaction outputs and within each, the txid (string) vout (numeric).
+2. transactions            (json array, optional, default=[]) The transaction outputs and within each, the txid (string) vout (numeric).
diff --git a/psbtbumpfee b/psbtbumpfee
index f4f5f7c..ac15520 100644
--- a/psbtbumpfee
+++ b/psbtbumpfee
@@ -36 +36 @@ Arguments:
-       "estimate_mode": "str",    (string, optional, default=unset) The fee estimate mode, must be one of (case insensitive):
+       "estimate_mode": "str",    (string, optional, default="unset") The fee estimate mode, must be one of (case insensitive):
diff --git a/send b/send
index ec9fc47..2d6579f 100644
--- a/send
+++ b/send
@@ -21 +21 @@ Arguments:
-3. estimate_mode                         (string, optional, default=unset) The fee estimate mode, must be one of (case insensitive):
+3. estimate_mode                         (string, optional, default="unset") The fee estimate mode, must be one of (case insensitive):
@@ -34 +34 @@ Arguments:
-       "estimate_mode": "str",           (string, optional, default=unset) The fee estimate mode, must be one of (case insensitive):
+       "estimate_mode": "str",           (string, optional, default="unset") The fee estimate mode, must be one of (case insensitive):
@@ -42 +42 @@ Arguments:
-       "inputs": [                       (json array, optional, default=empty array) Specify inputs instead of adding them automatically. A JSON array of JSON objects
+       "inputs": [                       (json array, optional, default=[]) Specify inputs instead of adding them automatically. A JSON array of JSON objects
@@ -51 +51 @@ Arguments:
-       "subtract_fee_from_outputs": [    (json array, optional, default=empty array) Outputs to subtract the fee from, specified as integer indices.
+       "subtract_fee_from_outputs": [    (json array, optional, default=[]) Outputs to subtract the fee from, specified as integer indices.
diff --git a/sendmany b/sendmany
index 33a5d29..50b5b7f 100644
--- a/sendmany
+++ b/sendmany
@@ -24 +24 @@ Arguments:
-8. estimate_mode             (string, optional, default=unset) The fee estimate mode, must be one of (case insensitive):
+8. estimate_mode             (string, optional, default="unset") The fee estimate mode, must be one of (case insensitive):
diff --git a/sendrawtransaction b/sendrawtransaction
index 74bc0cc..1a8aa57 100644
--- a/sendrawtransaction
+++ b/sendrawtransaction
@@ -15 +15 @@ Arguments:
-2. maxfeerate    (numeric or string, optional, default=0.10) Reject transactions whose fee rate is higher than the specified value, expressed in BTC/kB.
+2. maxfeerate    (numeric or string, optional, default="0.10") Reject transactions whose fee rate is higher than the specified value, expressed in BTC/kB.
diff --git a/sendtoaddress b/sendtoaddress
index 11b06e9..e9cd7df 100644
--- a/sendtoaddress
+++ b/sendtoaddress
@@ -18 +18 @@ Arguments:
-8. estimate_mode            (string, optional, default=unset) The fee estimate mode, must be one of (case insensitive):
+8. estimate_mode            (string, optional, default="unset") The fee estimate mode, must be one of (case insensitive):
diff --git a/signrawtransactionwithkey b/signrawtransactionwithkey
index 995361f..b46f2c7 100644
--- a/signrawtransactionwithkey
+++ b/signrawtransactionwithkey
@@ -28 +28 @@ Arguments:
-4. sighashtype                      (string, optional, default=ALL) The signature hash type. Must be one of:
+4. sighashtype                      (string, optional, default="ALL") The signature hash type. Must be one of:
diff --git a/signrawtransactionwithwallet b/signrawtransactionwithwallet
index c926338..7a7b228 100644
--- a/signrawtransactionwithwallet
+++ b/signrawtransactionwithwallet
@@ -22 +22 @@ Arguments:
-3. sighashtype                      (string, optional, default=ALL) The signature hash type. Must be one of
+3. sighashtype                      (string, optional, default="ALL") The signature hash type. Must be one of
diff --git a/testmempoolaccept b/testmempoolaccept
index ed26db3..8d18759 100644
--- a/testmempoolaccept
+++ b/testmempoolaccept
@@ -16 +16 @@ Arguments:
-2. maxfeerate      (numeric or string, optional, default=0.10) Reject transactions whose fee rate is higher than the specified value, expressed in BTC/kB
+2. maxfeerate      (numeric or string, optional, default="0.10") Reject transactions whose fee rate is higher than the specified value, expressed in BTC/kB
diff --git a/walletcreatefundedpsbt b/walletcreatefundedpsbt
index 552d46e..02a21f0 100644
--- a/walletcreatefundedpsbt
+++ b/walletcreatefundedpsbt
@@ -40 +40 @@ Arguments:
-       "subtractFeeFromOutputs": [    (json array, optional, default=empty array) The outputs to subtract the fee from.
+       "subtractFeeFromOutputs": [    (json array, optional, default=[]) The outputs to subtract the fee from.
@@ -50 +50 @@ Arguments:
-       "estimate_mode": "str",        (string, optional, default=unset) The fee estimate mode, must be one of (case insensitive):
+       "estimate_mode": "str",        (string, optional, default="unset") The fee estimate mode, must be one of (case insensitive):
diff --git a/walletprocesspsbt b/walletprocesspsbt
index b1c41fd..56ba928 100644
--- a/walletprocesspsbt
+++ b/walletprocesspsbt
@@ -10 +10 @@ Arguments:
-3. sighashtype    (string, optional, default=ALL) The signature hash type to sign with if not specified by the PSBT. Must be one of
+3. sighashtype    (string, optional, default="ALL") The signature hash type to sign with if not specified by the PSBT. Must be one of

</details>

MarcoFalke approved

MarcoFalke commented at 7:14 AM on April 17, 2021: member

ACK bee56c78e94417f89b1f48682404e2821b57bdec 🦅

<details><summary>Show signature and timestamp</summary>

Signature:

Timestamp of file with hash ef653cb4e3ec22e909cd2fc37137ce65651a9301233bf904e68f42f1574f2b54 -

</details>

promag commented at 8:53 AM on April 17, 2021: member

@MarcoFalke how did you generated that?

MarcoFalke commented at 8:57 AM on April 17, 2021: member

See bbbbb3f8850907d413db4715c10ef6df055234f6

dump_dir needs to be a git dir. For example, git init /tmp/git_dir. Then modify the test to set dump_dir = '/tmp/git_dir'. Then run the test on the commits you like and observe the difference in the git_dir.

<details><summary>Example patch:</summary>

diff --git a/src/rpc/server.cpp b/src/rpc/server.cpp
index 5d816ba5bb..36e027d8e4 100644
--- a/src/rpc/server.cpp
+++ b/src/rpc/server.cpp
@@ -95,7 +95,7 @@ std::string CRPCTable::help(const std::string& strCommand, const JSONRPCRequest&
     {
         const CRPCCommand *pcmd = command.second;
         std::string strMethod = pcmd->name;
-        if ((strCommand != "" || pcmd->category == "hidden") && strMethod != strCommand)
+        if ((strCommand != "") && strMethod != strCommand)
             continue;
         jreq.strMethod = strMethod;
         try
diff --git a/test/functional/rpc_help.py b/test/functional/rpc_help.py
index de21f43747..753ee771ad 100755
--- a/test/functional/rpc_help.py
+++ b/test/functional/rpc_help.py
@@ -100,7 +100,7 @@ class HelpRpcTest(BitcoinTestFramework):
         # command titles
         titles = [line[3:-3] for line in node.help().splitlines() if line.startswith('==')]
 
-        components = ['Blockchain', 'Control', 'Generating', 'Mining', 'Network', 'Rawtransactions', 'Util']
+        components = ['Blockchain', 'Control', 'Generating', 'Hidden', 'Mining', 'Network', 'Rawtransactions', 'Util']
 
         if self.is_wallet_compiled():
             components.append('Wallet')
@@ -116,7 +116,8 @@ class HelpRpcTest(BitcoinTestFramework):
     def dump_help(self):
         dump_dir = os.path.join(self.options.tmpdir, 'rpc_help_dump')
         os.mkdir(dump_dir)
-        calls = [line.split(' ', 1)[0] for line in self.nodes[0].help().splitlines() if line and not line.startswith('==')]
+        dump_dir = '/tmp/temp_git/' ##HACK
+        calls = [line.split(' ', 1)[0].split('|', 1)[0] for line in self.nodes[0].help().splitlines() if line and not line.startswith('==')]
         for call in calls:
             with open(os.path.join(dump_dir, call), 'w', encoding='utf-8') as f:
                 # Make sure the node can generate the help at runtime without crashing

</details>

promag commented at 9:01 AM on April 17, 2021: member

Cool! I thought there was already something like that but couldn't find it. Maybe it could be made a script.

LarryRuane commented at 9:09 PM on April 17, 2021: contributor

ACK bee56c78e94417f89b1f48682404e2821b57bdec

in src/rpc/blockchain.cpp:1188 in bee56c78e9

1184 | @@ -1185,9 +1185,9 @@ static RPCHelpMan verifychain()
1185 |      return RPCHelpMan{"verifychain",
1186 |                  "\nVerifies blockchain database.\n",
1187 |                  {
1188 | -                    {"checklevel", RPCArg::Type::NUM, /* default */ strprintf("%d, range=0-4", DEFAULT_CHECKLEVEL),

kiminuo commented at 11:36 AM on April 18, 2021:

Would it make sense to attempt to make this a scripted-diff?

It looks like one can change /* default */ to /* default-hint */ in one commit (by hand) and then write a sed script that does the replacements (all or a subset) for you. It would probably be hard unless done in a partial way. Just thinking out loud.

in src/rpc/util.cpp:512 in bee56c78e9

 507 | +                break;
 508 | +            case UniValue::VARR:
 509 | +                CHECK_NONFATAL(type == RPCArg::Type::ARR);
 510 | +                break;
 511 | +            case UniValue::VSTR:
 512 | +                CHECK_NONFATAL(type == RPCArg::Type::STR || type == RPCArg::Type::STR_HEX || type == RPCArg::Type::AMOUNT);

kiminuo commented at 12:00 PM on April 18, 2021:

Just FMI: RPCArg::Type::RANGE is represented by a number or with an array with two numbers. So RPCArg::Type::RANGE does not belong here (it's correct as it is).

in src/rpc/util.cpp:520 in bee56c78e9

 515 | +                CHECK_NONFATAL(type == RPCArg::Type::NUM || type == RPCArg::Type::AMOUNT || type == RPCArg::Type::RANGE);
 516 | +                break;
 517 | +            case UniValue::VBOOL:
 518 | +                CHECK_NONFATAL(type == RPCArg::Type::BOOL);
 519 | +                break;
 520 | +            case UniValue::VNULL:

kiminuo commented at 12:00 PM on April 18, 2021:

Just for the sake of completeness: All UniValue enum values are here.

kiminuo commented at 12:01 PM on April 18, 2021: contributor

I went through the replacements of /* default */ occurrences and it looks correct to me.

Nit: Commit rpc: Check default value type againts argument type should be rpc: Check default value type against argument type

Nit 2: Could changes like {} to RPCArg::Default{UniValue::VOBJ} be replaced with a comment like:

/* empty object */ RPCArg::Default{UniValue::VOBJ} or possibly by some C++ equivalent of a constant?

promag commented at 8:31 PM on April 18, 2021: member

@kiminuo thanks for reviewing, will fix typo if I have to push again.

MarcoFalke commented at 6:52 AM on April 19, 2021: member

@promag Are you going to address #21679 (review) here or in a follow-up?

promag commented at 6:58 AM on April 19, 2021: member

@MarcoFalke I'm fine either way. Maybe do it later? I'm thinking that the recursive check could be reused to check for actual RPC argument values. Also, #20017 doesn't need that change.

MarcoFalke merged this on Apr 19, 2021

MarcoFalke closed this on Apr 19, 2021

promag deleted the branch on Apr 19, 2021

sidhujag referenced this in commit d9997829bf on Apr 19, 2021

DrahtBot locked this on Aug 16, 2022