Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rpc: Keep default argument value in correct type #21679

Merged
merged 2 commits into from
Apr 19, 2021

Conversation

promag
Copy link
Member

@promag promag commented Apr 14, 2021

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.

Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cr ACK 9195574

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

src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved
src/rpc/mining.cpp Outdated Show resolved Hide resolved
@promag
Copy link
Member Author

promag commented Apr 14, 2021

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 promag force-pushed the 2021-04-rpc-defaults branch 2 times, most recently from 2f5c526 to b9e18b5 Compare April 14, 2021 14:58
@DrahtBot
Copy link
Contributor

DrahtBot commented Apr 14, 2021

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

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
Copy link
Member Author

promag commented Apr 15, 2021

@kallewoof @LarryRuane mind taking a look here?

src/rpc/blockchain.cpp Outdated Show resolved Hide resolved
src/rpc/blockchain.cpp Outdated Show resolved Hide resolved
src/rpc/util.h Outdated Show resolved Hide resolved
src/rpc/util.h Outdated
@@ -174,6 +198,7 @@ struct RPCArg {
m_type_str{std::move(type_str)}
{
CHECK_NONFATAL(type != Type::ARR && type != Type::OBJ);
CheckFallback();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/rpc/util.h Outdated Show resolved Hide resolved
@promag promag force-pushed the 2021-04-rpc-defaults branch 3 times, most recently from 33bc2f1 to e3cc790 Compare April 16, 2021 14:14
Copy link
Contributor

@LarryRuane LarryRuane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/rpc/blockchain.cpp Outdated Show resolved Hide resolved
src/rpc/blockchain.cpp Outdated Show resolved Hide resolved
src/rpc/blockchain.cpp Outdated Show resolved Hide resolved
src/rpc/blockchain.cpp Outdated Show resolved Hide resolved
src/rpc/blockchain.cpp Outdated Show resolved Hide resolved
src/rpc/blockchain.cpp Outdated Show resolved Hide resolved
src/rpc/mining.cpp Show resolved Hide resolved
src/rpc/util.cpp Show resolved Hide resolved
@maflcko
Copy link
Member

maflcko commented Apr 17, 2021

Rendered diff first commit:
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

Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK bee56c7 🦅

Show signature and timestamp

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK bee56c78e94417f89b1f48682404e2821b57bdec 🦅
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUgEdAwAiuOG/NUD53BK/VU2qI66wA7J5C2x27kffCSwQ5U4YEkrAkFLUyd2kWsy
O2GZiBVgCj5P1fJFDpBD6uMNglmt3kQOKKPronSaQBNRi7c/dXvAFaOUJT8WswjM
YfKXimirpD548eNUkobx4KkuHT6EBbXNaMNR+MWZ8Glf8TbkUUQuKuCAVTem2YQD
eK8hq86tychJL/4am6yGSBL6Zv3TPVbZxpouxZ02FDRLX37cXCq2QtZcmpuuyJ/B
Ml51Aqy3n4gyd8k8z6DACoOIIPOtF539avoU297pGmNU83Ebbos6TargwR8VPMD0
h4oIqDUv6rv2tCl1brn4/tqjXDqjaNBwIv3JClR2eK8mZt9kcXKjcEywnj8Do6yA
MblmohF082OAZihYTNs7M2syrSMmKjyEW5FIqubwL31vKKKqHtQq6OJu1/xfgQYP
Zt/6eLKIXNVX7+JLOT7/e6goKZLIwhpxCEMq8GHDMhexTJV9rcrGxuWO5wctM48+
NZR8MIFK
=uKa7
-----END PGP SIGNATURE-----

Timestamp of file with hash ef653cb4e3ec22e909cd2fc37137ce65651a9301233bf904e68f42f1574f2b54 -

@promag
Copy link
Member Author

promag commented Apr 17, 2021

@MarcoFalke how did you generated that?

@maflcko
Copy link
Member

maflcko commented Apr 17, 2021

See bbbbb3f

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.

Example patch:
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

@promag
Copy link
Member Author

promag commented Apr 17, 2021

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

@LarryRuane
Copy link
Contributor

ACK bee56c7

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

@kiminuo kiminuo Apr 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

CHECK_NONFATAL(type == RPCArg::Type::ARR);
break;
case UniValue::VSTR:
CHECK_NONFATAL(type == RPCArg::Type::STR || type == RPCArg::Type::STR_HEX || type == RPCArg::Type::AMOUNT);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

case UniValue::VBOOL:
CHECK_NONFATAL(type == RPCArg::Type::BOOL);
break;
case UniValue::VNULL:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@kiminuo kiminuo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

promag commented Apr 18, 2021

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

@maflcko
Copy link
Member

maflcko commented Apr 19, 2021

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

@promag
Copy link
Member Author

promag commented Apr 19, 2021

@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.

@maflcko maflcko merged commit d4300a1 into bitcoin:master Apr 19, 2021
@promag promag deleted the 2021-04-rpc-defaults branch April 19, 2021 07:31
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 19, 2021
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants