rpc: Document default values for optional arguments by maflcko · Pull Request #14877 · bitcoin/bitcoin

maflcko · 2018-12-05T22:00:30Z

No description provided.

DrahtBot · 2018-12-05T22:53:05Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#14707 ([RPC] Include coinbase transactions in receivedby RPCs by andrewtoth)
#14641 (rpc: Add min_conf option to fund transaction calls by promag)
#11413 ([wallet] [rpc] sendtoaddress/sendmany: Add explicit feerate option by kallewoof)

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.

src/rpc/rawtransaction.cpp

maflcko · 2018-12-06T18:38:19Z

Thanks @practicalswift. Fixed my language

jonasschnelli · 2018-12-09T23:09:27Z

src/rpc/mining.cpp

                {
                    {"conf_target", RPCArg::Type::NUM, /* opt */ false, /* default_val */ "", "Confirmation target in blocks (1 - 1008)"},
-                    {"threshold", RPCArg::Type::NUM, /* opt */ true, /* default_val */ "", "The proportion of transactions in a given feerate range that must have been\n"
+                    {"threshold", RPCArg::Type::NUM, /* opt */ true, /* default_val */ "0.95", "The proportion of transactions in a given feerate range that must have been\n"


what about using a stringprint of the threshold variable?

jonasschnelli · 2018-12-09T23:14:34Z

utACK fa0c24c
Somehow I would be in favour of putting brackets around descriptive values ((null) instead of null and (fallback to nodeid) instead of fallback to nodeid) to avoid confusion between real default values and behavioural description.

kristapsk · 2018-12-10T00:33:29Z

ACK fa0c24c

ryanofsky

ACK fa0c24c. I checked help output using the hack from #14726 (review)

This is a nice documentation change. If you wanted to follow up with more improvements I have some suggestions below:

The fallback descriptions like "fallback to wallet's default" are vague and could be described in more detail.
I don't think saying (default=null) or (default=omitted) is ever helpful. Just because of the way we check arguments, omitting an argument is always equivalent to passing null. And (default=omitted) is just a tautology. The point of the default documentation should be to say what an RPC call will do when an argument is null or omitted. I actually think it would be better to drop the all the (default=null) and (default=omitted) strings if they can't be replaced with something more constructive, to avoid creating a bad precedent for future changes.

ryanofsky · 2018-12-10T17:33:27Z

src/rpc/net.cpp

-                    {"bantime", RPCArg::Type::NUM, /* opt */ true, /* default_val */ "", "time in seconds how long (or until when if [absolute] is set) the IP is banned (0 or empty means using the default time of 24h which can also be overwritten by the -bantime startup argument)"},
-                    {"absolute", RPCArg::Type::BOOL, /* opt */ true, /* default_val */ "", "If set, the bantime must be an absolute timestamp in seconds since epoch (Jan 1 1970 GMT)"},
+                    {"bantime", RPCArg::Type::NUM, /* opt */ true, /* default_val */ "0", "time in seconds how long (or until when if [absolute] is set) the IP is banned (0 or empty means using the default time of 24h which can also be overwritten by the -bantime startup argument)"},
+                    {"absolute", RPCArg::Type::BOOL, /* opt */ true, /* default_val */ "false", "If set, the bantime must be an absolute timestamp in seconds since epoch (Jan 1 1970 GMT)"},


/If set/If true/ might be more accurate

kristapsk · 2018-12-10T17:58:02Z

I don't think saying (default=null) or (default=omitted) is ever helpful. (...) I actually think it would be better to drop the all the (default=null) and (default=omitted) strings if they can't be replaced with something more constructive, to avoid creating a bad precedent for future changes.

If there is "default=" string outputted for each optional argument, functional test could be made that checks that people don't forget to document optional argument defaults for a new RPCs.

ryanofsky · 2018-12-10T18:29:08Z

If there is "default=" string outputted for each optional argument, functional test could be made that checks that people don't forget to document optional argument defaults for a new RPCs.

IMO, better to write that test with a list of exceptions for grandfathered, undocumented default arguments, than to leave bad documentation scattered over the codebase, especially since existing documentation is often used as a template for new documentation

kristapsk · 2018-12-10T18:41:58Z

Oh, yes, short list of a few exceptions probably is better idea.

maflcko · 2018-12-10T18:56:52Z

I have changes piled up locally to enforce that it is either a required arg or a default value is provided at compile time.

fa0c24c rpc: Document default values for optional arguments (MarcoFalke) Pull request description: Tree-SHA512: e1f5ea67d7ac67526ae87bffaeb308a9ad68632e161fe0148cd431a340bb7a30def18f1dbc7e98c6c1c269ac8942fd5d5334c85c48e4fb1cead70a42536b6eef

ryanofsky · 2018-12-10T20:52:30Z

@MarcoFalke what do you want to do with all the (default=null) / (default=omitted) cruft? Do you want to address it in a future PR or maybe mark this one up for grabs so someone else can fill in useful default values?

maflcko · 2018-12-10T20:57:52Z

@ryanofsky Your suggestion is a one-line change after #14918, so either you leave feedback there or create a pull request after #14918 is merged.

fa5e6ef wallet: Fixup rescanblockchain result doc (MarcoFalke) Pull request description: This was probably accidentally added to the wrong line when addressing the feedback here: #7061 (comment) I already added the default values in #14877, but it could be clarified more that this really has no specific block height as default value, since the tip can change during a rescan. Tree-SHA512: 48a3c5143e2b7129ee8f396d2e77550cb393fbe45f5936aeebeb7a201d61560336a3ae47b26bb757a4dbbe217e06abfd67a5a673aef266b6c4d7a80d049a2b49

Summary: This is a backport of Core [[bitcoin/bitcoin#14877 | PR14877]] Test Plan: ninja all check-all Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D6218

fa5e6ef wallet: Fixup rescanblockchain result doc (MarcoFalke) Pull request description: This was probably accidentally added to the wrong line when addressing the feedback here: bitcoin#7061 (comment) I already added the default values in bitcoin#14877, but it could be clarified more that this really has no specific block height as default value, since the tip can change during a rescan. Tree-SHA512: 48a3c5143e2b7129ee8f396d2e77550cb393fbe45f5936aeebeb7a201d61560336a3ae47b26bb757a4dbbe217e06abfd67a5a673aef266b6c4d7a80d049a2b49

maflcko added Docs RPC/REST/ZMQ labels Dec 5, 2018

practicalswift suggested changes Dec 6, 2018

View reviewed changes

src/rpc/rawtransaction.cpp Outdated Show resolved Hide resolved

src/rpc/rawtransaction.cpp Outdated Show resolved Hide resolved

src/rpc/rawtransaction.cpp Outdated Show resolved Hide resolved

maflcko force-pushed the Mf1812-rpcDefault branch from eeee62f to fadea91 Compare December 6, 2018 18:37

DrahtBot added the Needs rebase label Dec 7, 2018

rpc: Document default values for optional arguments

fa0c24c

maflcko force-pushed the Mf1812-rpcDefault branch from fadea91 to fa0c24c Compare December 7, 2018 16:53

DrahtBot removed the Needs rebase label Dec 7, 2018

jonasschnelli reviewed Dec 9, 2018

View reviewed changes

ryanofsky approved these changes Dec 10, 2018

View reviewed changes

maflcko merged commit fa0c24c into bitcoin:master Dec 10, 2018

maflcko deleted the Mf1812-rpcDefault branch December 10, 2018 19:05

dooglus mentioned this pull request Dec 11, 2018

gui, wallet: random abort (segmentation fault) running master/HEAD #9683

Closed

maflcko mentioned this pull request Jan 28, 2019

wallet: Clarify rescanblockchain doc #15279

Merged

maflcko mentioned this pull request Feb 11, 2019

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

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Conversation

maflcko commented Dec 5, 2018

Uh oh!

DrahtBot commented Dec 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maflcko commented Dec 6, 2018

Uh oh!

jonasschnelli Dec 9, 2018

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Dec 9, 2018

Uh oh!

kristapsk commented Dec 10, 2018

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Dec 10, 2018

Choose a reason for hiding this comment

Uh oh!

kristapsk commented Dec 10, 2018

Uh oh!

ryanofsky commented Dec 10, 2018

Uh oh!

kristapsk commented Dec 10, 2018

Uh oh!

maflcko commented Dec 10, 2018

Uh oh!

ryanofsky commented Dec 10, 2018

Uh oh!

maflcko commented Dec 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

DrahtBot commented Dec 5, 2018 •

edited

Loading