rpc: Keep default argument value in correct type

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:

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

0- {"nblocks", RPCArg::Type::NUM, /* default */ "one month", "Size of the window in number of blocks"}
1+ {"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:

0                    {"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

 0--- a/src/rpc/blockchain.cpp
 1+++ b/src/rpc/blockchain.cpp
 2@@ -883,7 +883,7 @@ static RPCHelpMan getblock()
 3                 "If verbosity is 2, returns an Object with information about block <hash> and information about each transaction. \n",
 4                 {
 5                     {"blockhash", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The block hash"},
 6-                    {"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"},
 7+                    {"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"},
 8                 },
 9                 {
10                     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)

 0[...]
 12021-04-16T18:02:03Z Using 16 MiB out of 32/2 requested for script execution cache, able to store 524288 elements
 22021-04-16T18:02:03Z Script verification uses 7 additional threads
 32021-04-16T18:02:03Z scheduler thread start
 42021-04-16T18:02:03Z 
 5
 6************************
 7EXCEPTION: 18NonFatalCheckError       
 8./rpc/util.h:170 (CheckFallback)
 9Internal bug detected: 'm_type == Type::STR || m_type == Type::STR_HEX || m_type == Type::AMOUNT'
10You may report this issue here: https://github.com/bitcoin/bitcoin/issues
11       
12bitcoin in AppInit()       
13
14
15
16************************
17EXCEPTION: 18NonFatalCheckError       
18./rpc/util.h:170 (CheckFallback)
19Internal bug detected: 'm_type == Type::STR || m_type == Type::STR_HEX || m_type == Type::AMOUNT'
20You may report this issue here: https://github.com/bitcoin/bitcoin/issues
21       
22bitcoin in AppInit()       
23
242021-04-16T18:02:03Z Shutdown: In progress...
252021-04-16T18:02:03Z scheduler thread exit
262021-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

  0diff --git a/bumpfee b/bumpfee
  1index 81181d8..accdd42 100644
  2--- a/bumpfee
  3+++ b/bumpfee
  4@@ -35 +35 @@ Arguments:
  5-       "estimate_mode": "str",    (string, optional, default=unset) The fee estimate mode, must be one of (case insensitive):
  6+       "estimate_mode": "str",    (string, optional, default="unset") The fee estimate mode, must be one of (case insensitive):
  7diff --git a/createmultisig b/createmultisig
  8index e2d97b0..aff8a24 100644
  9--- a/createmultisig
 10+++ b/createmultisig
 11@@ -13 +13 @@ Arguments:
 12-3. address_type    (string, optional, default=legacy) The address type to use. Options are "legacy", "p2sh-segwit", and "bech32".
 13+3. address_type    (string, optional, default="legacy") The address type to use. Options are "legacy", "p2sh-segwit", and "bech32".
 14diff --git a/estimatesmartfee b/estimatesmartfee
 15index 6a9d6c0..d2aa633 100644
 16--- a/estimatesmartfee
 17+++ b/estimatesmartfee
 18@@ -10 +10 @@ Arguments:
 19-2. estimate_mode    (string, optional, default=conservative) The fee estimate mode.
 20+2. estimate_mode    (string, optional, default="conservative") The fee estimate mode.
 21diff --git a/fundrawtransaction b/fundrawtransaction
 22index 28ea142..472a817 100644
 23--- a/fundrawtransaction
 24+++ b/fundrawtransaction
 25@@ -29 +29 @@ Arguments:
 26-       "subtractFeeFromOutputs": [    (json array, optional, default=empty array) The integers.
 27+       "subtractFeeFromOutputs": [    (json array, optional, default=[]) The integers.
 28@@ -39 +39 @@ Arguments:
 29-       "estimate_mode": "str",        (string, optional, default=unset) The fee estimate mode, must be one of (case insensitive):
 30+       "estimate_mode": "str",        (string, optional, default="unset") The fee estimate mode, must be one of (case insensitive):
 31diff --git a/getblockfilter b/getblockfilter
 32index 75c3f03..2d3ca93 100644
 33--- a/getblockfilter
 34+++ b/getblockfilter
 35@@ -7 +7 @@ Arguments:
 36-2. filtertype    (string, optional, default=basic) The type name of the filter
 37+2. filtertype    (string, optional, default="basic") The type name of the filter
 38diff --git a/gettxoutsetinfo b/gettxoutsetinfo
 39index a7b3271..6a3d574 100644
 40--- a/gettxoutsetinfo
 41+++ b/gettxoutsetinfo
 42@@ -7 +7 @@ Arguments:
 43-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'.
 44+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'.
 45diff --git a/importdescriptors b/importdescriptors
 46index 4fb645e..a88c5de 100644
 47--- a/importdescriptors
 48+++ b/importdescriptors
 49@@ -22 +22 @@ Arguments:
 50-         "label": "str",                    (string, optional, default='') Label to assign to the address, only allowed with internal=false
 51+         "label": "str",                    (string, optional, default="") Label to assign to the address, only allowed with internal=false
 52diff --git a/importmulti b/importmulti
 53index 22a13df..6a40330 100644
 54--- a/importmulti
 55+++ b/importmulti
 56@@ -25 +25 @@ Arguments:
 57-         "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).
 58+         "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).
 59@@ -29 +29 @@ Arguments:
 60-         "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.
 61+         "keys": [                                                  (json array, optional, default=[]) Array of strings giving private keys to import. The corresponding public keys must occur in the output or redeemscript.
 62@@ -36 +36 @@ Arguments:
 63-         "label": "str",                                            (string, optional, default='') Label to assign to the address, only allowed with internal=false
 64+         "label": "str",                                            (string, optional, default="") Label to assign to the address, only allowed with internal=false
 65diff --git a/listunspent b/listunspent
 66index 8d0777d..84ce469 100644
 67--- a/listunspent
 68+++ b/listunspent
 69@@ -10 +10 @@ Arguments:
 70-3. addresses                          (json array, optional, default=empty array) The bitcoin addresses to filter
 71+3. addresses                          (json array, optional, default=[]) The bitcoin addresses to filter
 72@@ -19 +19 @@ Arguments:
 73-       "minimumAmount": amount,       (numeric or string, optional, default=0) Minimum value of each UTXO in BTC
 74+       "minimumAmount": amount,       (numeric or string, optional, default="0.00") Minimum value of each UTXO in BTC
 75diff --git a/lockunspent b/lockunspent
 76index f870bcc..a3ebb80 100644
 77--- a/lockunspent
 78+++ b/lockunspent
 79@@ -14 +14 @@ Arguments:
 80-2. transactions            (json array, optional, default=empty array) The transaction outputs and within each, the txid (string) vout (numeric).
 81+2. transactions            (json array, optional, default=[]) The transaction outputs and within each, the txid (string) vout (numeric).
 82diff --git a/psbtbumpfee b/psbtbumpfee
 83index f4f5f7c..ac15520 100644
 84--- a/psbtbumpfee
 85+++ b/psbtbumpfee
 86@@ -36 +36 @@ Arguments:
 87-       "estimate_mode": "str",    (string, optional, default=unset) The fee estimate mode, must be one of (case insensitive):
 88+       "estimate_mode": "str",    (string, optional, default="unset") The fee estimate mode, must be one of (case insensitive):
 89diff --git a/send b/send
 90index ec9fc47..2d6579f 100644
 91--- a/send
 92+++ b/send
 93@@ -21 +21 @@ Arguments:
 94-3. estimate_mode                         (string, optional, default=unset) The fee estimate mode, must be one of (case insensitive):
 95+3. estimate_mode                         (string, optional, default="unset") The fee estimate mode, must be one of (case insensitive):
 96@@ -34 +34 @@ Arguments:
 97-       "estimate_mode": "str",           (string, optional, default=unset) The fee estimate mode, must be one of (case insensitive):
 98+       "estimate_mode": "str",           (string, optional, default="unset") The fee estimate mode, must be one of (case insensitive):
 99@@ -42 +42 @@ Arguments:
100-       "inputs": [                       (json array, optional, default=empty array) Specify inputs instead of adding them automatically. A JSON array of JSON objects
101+       "inputs": [                       (json array, optional, default=[]) Specify inputs instead of adding them automatically. A JSON array of JSON objects
102@@ -51 +51 @@ Arguments:
103-       "subtract_fee_from_outputs": [    (json array, optional, default=empty array) Outputs to subtract the fee from, specified as integer indices.
104+       "subtract_fee_from_outputs": [    (json array, optional, default=[]) Outputs to subtract the fee from, specified as integer indices.
105diff --git a/sendmany b/sendmany
106index 33a5d29..50b5b7f 100644
107--- a/sendmany
108+++ b/sendmany
109@@ -24 +24 @@ Arguments:
110-8. estimate_mode             (string, optional, default=unset) The fee estimate mode, must be one of (case insensitive):
111+8. estimate_mode             (string, optional, default="unset") The fee estimate mode, must be one of (case insensitive):
112diff --git a/sendrawtransaction b/sendrawtransaction
113index 74bc0cc..1a8aa57 100644
114--- a/sendrawtransaction
115+++ b/sendrawtransaction
116@@ -15 +15 @@ Arguments:
117-2. maxfeerate    (numeric or string, optional, default=0.10) Reject transactions whose fee rate is higher than the specified value, expressed in BTC/kB.
118+2. maxfeerate    (numeric or string, optional, default="0.10") Reject transactions whose fee rate is higher than the specified value, expressed in BTC/kB.
119diff --git a/sendtoaddress b/sendtoaddress
120index 11b06e9..e9cd7df 100644
121--- a/sendtoaddress
122+++ b/sendtoaddress
123@@ -18 +18 @@ Arguments:
124-8. estimate_mode            (string, optional, default=unset) The fee estimate mode, must be one of (case insensitive):
125+8. estimate_mode            (string, optional, default="unset") The fee estimate mode, must be one of (case insensitive):
126diff --git a/signrawtransactionwithkey b/signrawtransactionwithkey
127index 995361f..b46f2c7 100644
128--- a/signrawtransactionwithkey
129+++ b/signrawtransactionwithkey
130@@ -28 +28 @@ Arguments:
131-4. sighashtype                      (string, optional, default=ALL) The signature hash type. Must be one of:
132+4. sighashtype                      (string, optional, default="ALL") The signature hash type. Must be one of:
133diff --git a/signrawtransactionwithwallet b/signrawtransactionwithwallet
134index c926338..7a7b228 100644
135--- a/signrawtransactionwithwallet
136+++ b/signrawtransactionwithwallet
137@@ -22 +22 @@ Arguments:
138-3. sighashtype                      (string, optional, default=ALL) The signature hash type. Must be one of
139+3. sighashtype                      (string, optional, default="ALL") The signature hash type. Must be one of
140diff --git a/testmempoolaccept b/testmempoolaccept
141index ed26db3..8d18759 100644
142--- a/testmempoolaccept
143+++ b/testmempoolaccept
144@@ -16 +16 @@ Arguments:
145-2. maxfeerate      (numeric or string, optional, default=0.10) Reject transactions whose fee rate is higher than the specified value, expressed in BTC/kB
146+2. maxfeerate      (numeric or string, optional, default="0.10") Reject transactions whose fee rate is higher than the specified value, expressed in BTC/kB
147diff --git a/walletcreatefundedpsbt b/walletcreatefundedpsbt
148index 552d46e..02a21f0 100644
149--- a/walletcreatefundedpsbt
150+++ b/walletcreatefundedpsbt
151@@ -40 +40 @@ Arguments:
152-       "subtractFeeFromOutputs": [    (json array, optional, default=empty array) The outputs to subtract the fee from.
153+       "subtractFeeFromOutputs": [    (json array, optional, default=[]) The outputs to subtract the fee from.
154@@ -50 +50 @@ Arguments:
155-       "estimate_mode": "str",        (string, optional, default=unset) The fee estimate mode, must be one of (case insensitive):
156+       "estimate_mode": "str",        (string, optional, default="unset") The fee estimate mode, must be one of (case insensitive):
157diff --git a/walletprocesspsbt b/walletprocesspsbt
158index b1c41fd..56ba928 100644
159--- a/walletprocesspsbt
160+++ b/walletprocesspsbt
161@@ -10 +10 @@ Arguments:
162-3. sighashtype    (string, optional, default=ALL) The signature hash type to sign with if not specified by the PSBT. Must be one of
163+3. sighashtype    (string, optional, default="ALL") The signature hash type to sign with if not specified by the PSBT. Must be one of

MarcoFalke approved

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

ACK bee56c78e94417f89b1f48682404e2821b57bdec 🦅

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK bee56c78e94417f89b1f48682404e2821b57bdec 🦅
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUgEdAwAiuOG/NUD53BK/VU2qI66wA7J5C2x27kffCSwQ5U4YEkrAkFLUyd2kWsy
 8O2GZiBVgCj5P1fJFDpBD6uMNglmt3kQOKKPronSaQBNRi7c/dXvAFaOUJT8WswjM
 9YfKXimirpD548eNUkobx4KkuHT6EBbXNaMNR+MWZ8Glf8TbkUUQuKuCAVTem2YQD
10eK8hq86tychJL/4am6yGSBL6Zv3TPVbZxpouxZ02FDRLX37cXCq2QtZcmpuuyJ/B
11Ml51Aqy3n4gyd8k8z6DACoOIIPOtF539avoU297pGmNU83Ebbos6TargwR8VPMD0
12h4oIqDUv6rv2tCl1brn4/tqjXDqjaNBwIv3JClR2eK8mZt9kcXKjcEywnj8Do6yA
13MblmohF082OAZihYTNs7M2syrSMmKjyEW5FIqubwL31vKKKqHtQq6OJu1/xfgQYP
14Zt/6eLKIXNVX7+JLOT7/e6goKZLIwhpxCEMq8GHDMhexTJV9rcrGxuWO5wctM48+
15NZR8MIFK
16=uKa7
17-----END PGP SIGNATURE-----

Timestamp of file with hash ef653cb4e3ec22e909cd2fc37137ce65651a9301233bf904e68f42f1574f2b54 -

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.

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

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

rpc: Keep default argument value in correct type #21679

Conflicts