RPCHelpMan: Check default values are given at compile-time #14918

maflcko · 2018-12-10T20:40:00Z

Remove the run time assertions on the default values and ensure that the correct default type and value is provided at compile time.

DrahtBot · 2018-12-11T01:20:58Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#15341 (rpc: Support specifying change address in bumpfee by promag)
#15006 (Add option to create an encrypted wallet by achow101)
#14707 ([RPC] Include coinbase transactions in receivedby RPCs by andrewtoth)
#14641 (rpc: Add min_conf option to fund transaction calls by promag)
#14481 (Add P2SH-P2WSH support to listunspent RPC by MeshCollider)
#13541 (wallet/rpc: sendrawtransaction maxfeerate by kallewoof)
#12674 (RPC: Support addnode onetry without making the connection priviliged by luke-jr)
#11484 (Optional update rescan option in importmulti RPC by pedrobranco)
#10593 (Relax punishment for peers relaying invalid blocks and headers by luke-jr)

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.

maflcko · 2019-02-11T13:49:12Z

@ryanofsky Mind to take a look here? You requested this in #14877 (review)

ryanofsky

utACK fa0ad4e

@ryanofsky Mind to take a look here? You requested this in #14877 (review)

This is a big monster change, but it does make things clearer. I reviewed the code and also diffed the output using #14726 (review). All the changes looked like improvements.

Output

@@ -59,7 +59,7 @@
        "key",      (string) bitcoin address or hex-encoded public key
        ...
      ]
-3. label           (string, optional, default=null) A label to assign the addresses to.
+3. label           (string, optional) A label to assign the addresses to.
 4. address_type    (string, optional, default=set by -addresstype) The address type to use. Options are "legacy", "p2sh-segwit", and "bech32".
 
 Result:
@@ -121,7 +121,7 @@
 
 Arguments:
 1. txid                           (string, required) The txid to be bumped
-2. options                        (json object, optional, default=null)
+2. options                        (json object, optional)
      {
        "confTarget": n,           (numeric, optional, default=fallback to wallet's default) Confirmation target (in blocks)
        "totalFee": n,             (numeric, optional, default=fallback to 'confTarget') Total fee (NOT feerate) to pay, in satoshis.
@@ -782,7 +782,7 @@
 
 Arguments:
 1. hexstring                          (string, required) The hex string of the raw transaction
-2. options                            (json object, optional, default=null) for backward compatibility: passing in a true instead of an object will result in {"includeWatching":true}
+2. options                            (json object, optional) for backward compatibility: passing in a true instead of an object will result in {"includeWatching":true}
      {
        "changeAddress": "str",        (string, optional, default=pool address) The bitcoin address to receive the change
        "changePosition": n,           (numeric, optional, default=random) The index of the change output
@@ -972,7 +972,7 @@
 thus affected by options which limit spendability such as -spendzeroconfchange.
 
 Arguments:
-1. dummy                (string, optional, default=null) Remains for backward compatibility. Must be excluded or set to "*".
+1. dummy                (string, optional) Remains for backward compatibility. Must be excluded or set to "*".
 2. minconf              (numeric, optional, default=0) Only include transactions confirmed at least this many times.
 3. include_watchonly    (boolean, optional, default=false) Also include balance in watch-only addresses (see 'importaddress')
 
@@ -1682,7 +1682,7 @@
 so payments received with the address will be associated with 'label'.
 
 Arguments:
-1. label           (string, optional, default=null) The label name for the address to be linked to. If not provided, the default label "" is used. It can also be set to the empty string "" to represent the default label. The label does not need to exist, it will be created if there is no label by the given name.
+1. label           (string, optional, default="") The label name for the address to be linked to. It can also be set to the empty string "" to represent the default label. The label does not need to exist, it will be created if there is no label by the given name.
 2. address_type    (string, optional, default=set by -addresstype) The address type to use. Options are "legacy", "p2sh-segwit", and "bech32".
 
 Result:
@@ -1859,7 +1859,7 @@
 Arguments:
 1. txid         (string, required) The transaction id
 2. verbose      (boolean, optional, default=false) If false, return a string, otherwise return a json object
-3. blockhash    (string, optional, default=null) The block in which to look for the transaction
+3. blockhash    (string, optional) The block in which to look for the transaction
 
 Result (if verbose is not set or set to false):
 "data"      (string) The serialized, hex-encoded data for 'txid'
@@ -2080,7 +2080,7 @@
        "txid",    (string) A transaction hash
        ...
      ]
-2. blockhash      (string, optional, default=null) If specified, looks for txid in the block with this hash
+2. blockhash      (string, optional) If specified, looks for txid in the block with this hash
 
 Result:
 "data"           (string) A string that is a serialized, hex-encoded data for the proof.
@@ -2191,7 +2191,7 @@
 1. requests                                                         (json array, required) Data to be imported
      [
        {                                                            (json object)
-         "desc": "str",                                             (string, optional) Descriptor to import. If using descriptor, do not also provide address/scriptPubKey, scripts, or pubkeys
+         "desc": "str",                                             (string) Descriptor to import. If using descriptor, do not also provide address/scriptPubKey, scripts, or pubkeys
          "scriptPubKey": "<script>" | { "address":"<address>" },    (string / json, required) Type of scriptPubKey (string for script, json for address). Should not be provided if using a descriptor
          "timestamp": timestamp | "now",                            (integer / string, required) Creation time of the key in seconds since epoch (Jan 1 1970 GMT),
                                                                     or the string "now" to substitute the current synced blockchain time. The timestamp of the oldest
@@ -2199,8 +2199,8 @@
                                                                     "now" can be specified to bypass scanning, for keys which are known to never have been used, and
                                                                     0 can be specified to scan the entire blockchain. Blocks up to 2 hours before the earliest key
                                                                     creation time of all keys being imported by the importmulti call will be scanned.
-         "redeemscript": "str",                                     (string, optional, default=omitted) Allowed only if the scriptPubKey is a P2SH or P2SH-P2WSH address/scriptPubKey
-         "witnessscript": "str",                                    (string, optional, default=omitted) Allowed only if the scriptPubKey is a P2SH-P2WSH or P2WSH address/scriptPubKey
+         "redeemscript": "str",                                     (string) Allowed only if the scriptPubKey is a P2SH or P2SH-P2WSH address/scriptPubKey
+         "witnessscript": "str",                                    (string) Allowed only if the scriptPubKey is a P2SH-P2WSH or P2WSH address/scriptPubKey
          "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).
            "pubKey",                                                (string)
            ...
@@ -2209,7 +2209,7 @@
            "key",                                                   (string)
            ...
          ],
-         {                                                          (json object, optional) If a ranged descriptor is used, this specifies the start and end of the range to import
+         {                                                          (json object) If a ranged descriptor is used, this specifies the start and end of the range to import
            "start": n,                                              (numeric, optional, default=0) Start of the range to import
            "end": n,                                                (numeric, required) End of the range to import (inclusive)
          },
@@ -2219,7 +2219,7 @@
        },
        ...
      ]
-2. options                                                          (json object, optional, default=null)
+2. options                                                          (json object, optional)
      {
        "rescan": bool,                                              (boolean, optional, default=true) Stating if should rescan the blockchain after all imports
      }
@@ -2379,7 +2379,7 @@
 Returns the list of all labels, or labels that are assigned to addresses with a specific purpose.
 
 Arguments:
-1. purpose    (string, optional, default=null) Address purpose to list labels for ('send','receive'). An empty string is the same as not providing this argument.
+1. purpose    (string, optional) Address purpose to list labels for ('send','receive'). An empty string is the same as not providing this argument.
 
 Result:
 [               (json array of string)
@@ -2442,7 +2442,7 @@
 1. minconf              (numeric, optional, default=1) The minimum number of confirmations before payments are included.
 2. include_empty        (boolean, optional, default=false) Whether to include addresses that haven't received any payments.
 3. include_watchonly    (boolean, optional, default=false) Whether to include watch-only addresses (see 'importaddress').
-4. address_filter       (string, optional, default=null) If present, only return information on this address.
+4. address_filter       (string, optional) If present, only return information on this address.
 
 Result:
 [
@@ -2500,7 +2500,7 @@
 Additionally, if include_removed is set, transactions affecting the wallet which were removed are returned in the "removed" array.
 
 Arguments:
-1. blockhash               (string, optional, default=null) If set, the block hash to list transactions since, otherwise list all transactions.
+1. blockhash               (string, optional) If set, the block hash to list transactions since, otherwise list all transactions.
 2. target_confirmations    (numeric, optional, default=1) Return the nth block hash from the main chain. e.g. 1 would mean the best block hash. Note: this is not used as a filter, but only affects [lastblock] in the return value
 3. include_watchonly       (boolean, optional, default=false) Include transactions to watch-only addresses (see 'importaddress')
 4. include_removed         (boolean, optional, default=true) Show transactions that were removed due to a reorg in the "removed" array
@@ -2555,7 +2555,7 @@
 Returns up to 'count' most recent transactions skipping the first 'from' transactions.
 
 Arguments:
-1. label                (string, optional, default=null) If set, should be a valid label name to return only incoming transactions
+1. label                (string, optional) If set, should be a valid label name to return only incoming transactions
                         with the specified label, or "*" to disable filtering and return all transactions.
 2. count                (numeric, optional, default=10) The number of transactions to return
 3. skip                 (numeric, optional, default=0) The number of transactions to skip
@@ -2622,7 +2622,7 @@
      ]
 4. include_unsafe                     (boolean, optional, default=true) Include outputs that are not safe to spend
                                       See description of "safe" attribute below.
-5. query_options                      (json object, optional, default=null) JSON with query options
+5. query_options                      (json object, optional) JSON with query options
      {
        "minimumAmount": amount,       (numeric or string, optional, default=0) Minimum value of each UTXO in BTC
        "maximumAmount": amount,       (numeric or string, optional, default=unlimited) Maximum value of each UTXO in BTC
@@ -2766,12 +2766,12 @@
   - "none", "0" : even if other logging categories are specified, ignore all of them.
 
 Arguments:
-1. include                    (json array, optional, default=null) A json array of categories to add debug logging
+1. include                    (json array, optional) A json array of categories to add debug logging
      [
        "include_category",    (string) the valid logging category
        ...
      ]
-2. exclude                    (json array, optional, default=null) A json array of categories to remove debug logging
+2. exclude                    (json array, optional) A json array of categories to remove debug logging
      [
        "exclude_category",    (string) the valid logging category
        ...
@@ -2820,7 +2820,7 @@
 
 Arguments:
 1. txid         (string, required) The transaction id.
-2. dummy        (numeric, optional, default=null) API-Compatibility for previous API. Must be zero or null.
+2. dummy        (numeric, optional) API-Compatibility for previous API. Must be zero or null.
                 DEPRECATED. For forward compatibility use named arguments and omit this parameter.
 3. fee_delta    (numeric, required) The fee value (in satoshis) to add (or subtract, if negative).
                 Note, that this value is not a fee rate. It is a value to modify absolute fee of the TX.
@@ -2975,8 +2975,8 @@
        "address": amount,    (numeric or string, required) The bitcoin address is the key, the numeric amount (can be string) in BTC is the value
      }
 3. minconf                   (numeric, optional, default=1) Only use the balance confirmed at least this many times.
-4. comment                   (string, optional, default=null) A comment
-5. subtractfeefrom           (json array, optional, default=null) A json array with addresses.
+4. comment                   (string, optional) A comment
+5. subtractfeefrom           (json array, optional) A json array with addresses.
                              The fee will be equally deducted from the amount of each selected address.
                              Those recipients will receive less bitcoins than you enter in their corresponding amount field.
                              If no addresses are specified here, the sender pays the fee.
@@ -3044,9 +3044,9 @@
 Arguments:
 1. address                  (string, required) The bitcoin address to send to.
 2. amount                   (numeric or string, required) The amount in BTC to send. eg 0.1
-3. comment                  (string, optional, default=null) A comment used to store what the transaction is for.
+3. comment                  (string, optional) A comment used to store what the transaction is for.
                             This is not part of the transaction, just kept in your wallet.
-4. comment_to               (string, optional, default=null) A comment to store the name of the person or organization
+4. comment_to               (string, optional) A comment to store the name of the person or organization
                             to which you're sending the transaction. This is not part of the 
                             transaction, just kept in your wallet.
 5. subtractfeefromamount    (boolean, optional, default=false) The fee will be deducted from the amount being sent.
@@ -3217,13 +3217,13 @@
        "privatekey",               (string) private key in base58-encoding
        ...
      ]
-3. prevtxs                         (json array, optional, default=null) A json array of previous dependent transaction outputs
+3. prevtxs                         (json array, optional) A json array of previous dependent transaction outputs
      [
        {                           (json object)
          "txid": "hex",            (string, required) The transaction id
          "vout": n,                (numeric, required) The output number
          "scriptPubKey": "hex",    (string, required) script key
-         "redeemScript": "hex",    (string, optional, default=omitted) (required for P2SH or P2WSH) redeem script
+         "redeemScript": "hex",    (string) (required for P2SH or P2WSH) redeem script
          "amount": amount,         (numeric or string, required) The amount spent
        },
        ...
@@ -3266,13 +3266,13 @@
 
 Arguments:
 1. hexstring                       (string, required) The transaction hex string
-2. prevtxs                         (json array, optional, default=null) A json array of previous dependent transaction outputs
+2. prevtxs                         (json array, optional) A json array of previous dependent transaction outputs
      [
        {                           (json object)
          "txid": "hex",            (string, required) The transaction id
          "vout": n,                (numeric, required) The output number
          "scriptPubKey": "hex",    (string, required) script key
-         "redeemScript": "hex",    (string, optional, default=omitted) (required for P2SH or P2WSH)
+         "redeemScript": "hex",    (string) (required for P2SH or P2WSH) redeem script
          "amount": amount,         (numeric or string, required) The amount spent
        },
        ...
@@ -3586,7 +3586,7 @@
        ...
      ]
 3. locktime                           (numeric, optional, default=0) Raw locktime. Non-0 value also locktime-activates inputs
-4. options                            (json object, optional, default=null)
+4. options                            (json object, optional)
      {
        "changeAddress": "hex",        (string, optional, default=pool address) The bitcoin address to receive the change
        "changePosition": n,           (numeric, optional, default=random) The index of the change output

ryanofsky · 2019-02-12T23:30:38Z

src/rpc/util.h

+        /** Required arg */
+        NO,
+        /**
+         * Optinal arg that is a named argument and has a default value of


spelling optinal

yea.. why was this ignored?

yea.. why was this ignored?

Just in order to able to merge the pr quickly (it was big and needed to be rebased a lot). The spelling could be fixed later.

…mpile-time fa0ad4e RPCHelpMan: Check default values are given at compile-time (MarcoFalke) Pull request description: Remove the run time assertions on the default values and ensure that the correct default type and value is provided at compile time. Tree-SHA512: 80df2f3fab4379b500c773c27da63f22786c58be5963fe99744746320e43627a5d433eedf8b32209158df7805ebdce65ed4d242c829c4fe6e5d13deb4799ed42

Summary: This is a backport of Core [[bitcoin/bitcoin#14918 | PR14918]] In order to automate somewhat the process, I used the following commands: ``` sed -n '1h; 1!H; ${ g; s@/\* opt \*/ true,\n \+@/* opt */ true, @g; p}' -i ../src/wallet/rpc*.cpp ../src/rpc/*.cpp sed -n '1h; 1!H; ${ g; s@/\* opt \*/ false,\n \+@/* opt */ false, @g; p}' -i ../src/wallet/rpc*.cpp ../src/rpc/*.cpp sed -e 's@/\* opt \*/ false, /\* default_val \*/ ""@RPCArg::Optional::NO@' -i ../src/wallet/rpc*.cpp ../src/rpc/*.cpp # Manual edit for OMITTED and OMITTED_NAMED_ARG sed -e 's@/\* opt \*/ true, /\* default_val \*/@/* default */@' -i ../src/wallet/rpc*.cpp ../src/rpc/*.cpp ``` Test Plan: ninja all check-all Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D6064

…mpile-time fa0ad4e RPCHelpMan: Check default values are given at compile-time (MarcoFalke) Pull request description: Remove the run time assertions on the default values and ensure that the correct default type and value is provided at compile time. Tree-SHA512: 80df2f3fab4379b500c773c27da63f22786c58be5963fe99744746320e43627a5d433eedf8b32209158df7805ebdce65ed4d242c829c4fe6e5d13deb4799ed42

Merge bitcoin#14987 and bitcoin#14918

…mpile-time fa0ad4e RPCHelpMan: Check default values are given at compile-time (MarcoFalke) Pull request description: Remove the run time assertions on the default values and ensure that the correct default type and value is provided at compile time. Tree-SHA512: 80df2f3fab4379b500c773c27da63f22786c58be5963fe99744746320e43627a5d433eedf8b32209158df7805ebdce65ed4d242c829c4fe6e5d13deb4799ed42

maflcko added Refactoring Docs RPC/REST/ZMQ labels Dec 10, 2018

maflcko force-pushed the Mf1812-rpcOptionalCompile branch from fa4381d to faf4ab5 Compare December 10, 2018 20:43

maflcko mentioned this pull request Dec 10, 2018

rpc: Document default values for optional arguments #14877

Merged

DrahtBot added the Needs rebase label Dec 21, 2018

maflcko force-pushed the Mf1812-rpcOptionalCompile branch from faf4ab5 to fa3292c Compare December 21, 2018 13:23

DrahtBot removed the Needs rebase label Dec 21, 2018

maflcko force-pushed the Mf1812-rpcOptionalCompile branch 5 times, most recently from fa9f5a3 to faff487 Compare December 21, 2018 15:44

DrahtBot added the Needs rebase label Dec 24, 2018

maflcko force-pushed the Mf1812-rpcOptionalCompile branch from faff487 to faa60e7 Compare December 24, 2018 12:24

DrahtBot removed the Needs rebase label Dec 24, 2018

maflcko force-pushed the Mf1812-rpcOptionalCompile branch 2 times, most recently from fafd087 to 2758c66 Compare January 15, 2019 22:10

DrahtBot added the Needs rebase label Jan 16, 2019

maflcko force-pushed the Mf1812-rpcOptionalCompile branch from 2758c66 to fa8903a Compare January 16, 2019 17:05

DrahtBot added Needs rebase and removed Needs rebase labels Jan 16, 2019

maflcko force-pushed the Mf1812-rpcOptionalCompile branch from fa8903a to fa46ada Compare January 25, 2019 03:07

DrahtBot added Needs rebase and removed Needs rebase labels Jan 25, 2019

maflcko force-pushed the Mf1812-rpcOptionalCompile branch from fa46ada to fa5d914 Compare January 29, 2019 16:54

DrahtBot added Needs rebase and removed Needs rebase labels Jan 29, 2019

maflcko force-pushed the Mf1812-rpcOptionalCompile branch from fa5d914 to aaaaaeb Compare February 6, 2019 19:47

DrahtBot removed the Needs rebase label Feb 6, 2019

maflcko force-pushed the Mf1812-rpcOptionalCompile branch 2 times, most recently from fa4f46e to fa63bea Compare February 7, 2019 20:01

DrahtBot added the Needs rebase label Feb 7, 2019

maflcko force-pushed the Mf1812-rpcOptionalCompile branch from fa63bea to faec234 Compare February 8, 2019 14:14

DrahtBot added Needs rebase and removed Needs rebase labels Feb 8, 2019

RPCHelpMan: Check default values are given at compile-time

fa0ad4e

maflcko force-pushed the Mf1812-rpcOptionalCompile branch from faec234 to fa0ad4e Compare February 11, 2019 13:40

DrahtBot removed the Needs rebase label Feb 11, 2019

ryanofsky approved these changes Feb 12, 2019

View reviewed changes

maflcko merged commit fa0ad4e into bitcoin:master Feb 12, 2019

maflcko deleted the Mf1812-rpcOptionalCompile branch February 13, 2019 00:00

Sjors mentioned this pull request Feb 13, 2019

Descriptor checksums #15368

Merged

PastaPastaPasta added a commit to dashpay/dash that referenced this pull request Nov 13, 2021

Merge pull request #4567 from dzutte-cpp/merge_14987_14918

4bf6dc0

Merge bitcoin#14987 and bitcoin#14918

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RPCHelpMan: Check default values are given at compile-time #14918

RPCHelpMan: Check default values are given at compile-time #14918

maflcko commented Dec 10, 2018 •

edited

DrahtBot commented Dec 11, 2018 •

edited

maflcko commented Feb 11, 2019

ryanofsky left a comment

ryanofsky Feb 12, 2019

thijstriemstra Feb 13, 2019

ryanofsky Feb 13, 2019

RPCHelpMan: Check default values are given at compile-time #14918

RPCHelpMan: Check default values are given at compile-time #14918

Conversation

maflcko commented Dec 10, 2018 • edited

DrahtBot commented Dec 11, 2018 • edited

Conflicts

maflcko commented Feb 11, 2019

ryanofsky left a comment

Choose a reason for hiding this comment

ryanofsky Feb 12, 2019

Choose a reason for hiding this comment

thijstriemstra Feb 13, 2019

Choose a reason for hiding this comment

ryanofsky Feb 13, 2019

Choose a reason for hiding this comment

maflcko commented Dec 10, 2018 •

edited

DrahtBot commented Dec 11, 2018 •

edited