[rpc] fundrawtransaction: Fix help text #8144

pull MarcoFalke wants to merge 1 commits into bitcoin:master from MarcoFalke:Mf1606-rpcDoc changing 2 files +3 −3

MarcoFalke commented at 4:56 PM on June 3, 2016: member

Fixing my nits from #7967#ref-commit-8c1e49b:

The feeRate default is not "estimate" but it is not set at all. Also the unit is satoshis per KB.
MarcoFalke added the label Docs and Output on Jun 3, 2016
MarcoFalke force-pushed on Jun 3, 2016
jonasschnelli commented at 5:55 PM on June 3, 2016: contributor

Thanks! utACK. nit: default not set could be misinterpret.
sipa commented at 5:58 PM on June 3, 2016: member

I'm confused. What does "not set" mean? Can an average user guess what the resulting behaviour will be?
jonasschnelli commented at 6:00 PM on June 3, 2016: contributor

"not set" stands for "not setting" a specific feerate. IMO it's better if we say the default value is the estimated feerate.
jonasschnelli commented at 6:01 PM on June 3, 2016: contributor

Or also include something about txconfirmtarget (which fundrawtransaction uses for fee estimation).
MarcoFalke commented at 6:15 PM on June 3, 2016: member

With "default not set" I mean that the option is disabled/not considered/ignored. Better suggestions welcome.

Currently it says that the default will be the estimated fee rate, which is wrong. Also it may change in the future so you can't "hardcode" it.
sipa commented at 6:18 PM on June 3, 2016: member

Even better would be to add another option to fundrawtransaction to set the confirmation target for estimation. Then you can say that if feerate is not set, the set target is used for estimating it. If that target is not set, the default is used.
MarcoFalke commented at 6:27 PM on June 3, 2016: member

As I understand this, you'd be breaking compatibility (basically a weak version of #7633, so at least mention in the release notes would be required). Though, I consider new features out of scope for this pull request.

in src/wallet/rpcwallet.cpp:None in fa9b65d229 outdated

2457 | @@ -2458,13 +2458,13 @@ UniValue fundrawtransaction(const UniValue& params, bool fHelp)
2458 |                              "     \"changePosition\"    (numeric, optional, default random) The index of the change output\n"
2459 |                              "     \"includeWatching\"   (boolean, optional, default false) Also select inputs which are watch only\n"
2460 |                              "     \"lockUnspents\"      (boolean, optional, default false) Lock selected unspent outputs\n"
2461 | -                            "     \"feeRate\"           (numeric, optional, default 0=estimate) Set a specific feerate (fee per KB)\n"
2462 | +                            "     \"feeRate\"           (numeric, optional, default not set) Set a specific feerate (satoshis per KB)\n"

MarcoFalke commented at 7:13 PM on June 3, 2016:

Maybe (numeric, optional, no default)?

MarcoFalke force-pushed on Jun 4, 2016
MarcoFalke commented at 2:45 PM on June 4, 2016: member

Pushed a commit which says "default not set: makes wallet determine the fee". Is this easier to understand?
[rpc] fundrawtransaction: Fix help text and interface faf82e8fc8

in src/wallet/rpcwallet.cpp:None in fadae2be03 outdated

2457 | @@ -2458,13 +2458,13 @@ UniValue fundrawtransaction(const UniValue& params, bool fHelp)
2458 |                              "     \"changePosition\"    (numeric, optional, default random) The index of the change output\n"
2459 |                              "     \"includeWatching\"   (boolean, optional, default false) Also select inputs which are watch only\n"
2460 |                              "     \"lockUnspents\"      (boolean, optional, default false) Lock selected unspent outputs\n"
2461 | -                            "     \"feeRate\"           (numeric, optional, default 0=estimate) Set a specific feerate (fee per KB)\n"
2462 | +                            "     \"feeRate\"           (numeric, optional, default not set: makes wallet determine the fee) Set a specific feerate (satoshis per KB)\n"

laanwj commented at 1:31 PM on June 6, 2016:

Not this pull's fault, but I wonder why was it chosen to use satoshis ere instead of just BTC/kB (converted using the AmountFromValue function)? This is inconsistent with the rest of the API (e.g. settxfee).

MarcoFalke commented at 6:06 PM on June 6, 2016:

Let's leave this for another pull.

jonasschnelli commented at 6:07 PM on June 6, 2016:

I kinda agree with @laanwj. The feeRate option has not been deployed. We could still change it to BTC/kb to be consistent with the rest of the API.

MarcoFalke commented at 6:46 PM on June 6, 2016:

See #8153

MarcoFalke force-pushed on Jun 6, 2016
MarcoFalke commented at 6:06 PM on June 6, 2016: member

(Squashed commits)
laanwj merged this on Jun 8, 2016
laanwj closed this on Jun 8, 2016
MarcoFalke deleted the branch on Jun 8, 2016
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#7967 [RPC] add feerate option to fundrawtransaction