rpc: Improve documentation and return value of settxfee #18467

fjahr · 2020-03-29T21:48:32Z

~~Closes 18315~~

settxfee can be deactivated by passing 0 as the fee. That does not seem to be documented for the user so this PR adds it in the description. The return value of a simple boolean seems also too simplified given the multiple dimensions that this deactivation feature enables. I.e. it does not seem intuitive if the returned boolean shows that the call succeeded or if means that settxfee is active. My suggested solution is to change the return value to a JSON object that included the "active" state and the currently set fee rate.

Examples:

$ src/bitcoin-cli settxfee 0.0000000
{
  "active": false,
  "fee_rate": "0.00000000 BTC/kB"
}
$ src/bitcoin-cli settxfee 0.0001
{
  "active": true,
  "fee_rate": "0.00010000 BTC/kB"
}

maflcko · 2020-03-29T22:07:14Z

src/wallet/rpcwallet.cpp

Nice documentation. Not sure on changing the format without an -rpcdeprecated cycle or whether it should be changed at all.

Thanks, I agree the documentation is the minimum and it might be good enough. If the change in return the value is too much hassle or if there is not enough interest, I can remove it.

I can't really see the value. Why would an interface return the value you passed in. Should the caller compare the result["feerate"] with the one passed in? Same goes for "active", which is simply the value passed in and cast to a bool.

I am repeating a pattern I am seeing in setwalletflag and a few other RPCs. I see the value in being explicit what is actually going on, i.e. that setting to 0 means the parameter is not active. Do you have an alternative change you would prefer, for example returning the current fee rate or just the active status? Or would you prefer to not make any change to the return value?

The current return true unconditionally is completely useless in the same sense. An API client wouldn't know what to do with the value. Should it check if it is false and then throw an error? In that case we might as well throw the error on the server.

So I can see your desire to clean it up, but the cost of breaking every API client for this doesn't seem justified. On top, just because we use that pattern in other calls, doesn't mean that it is a good pattern. I am not really aware of any setter method that returns the set value again, e.g. https://en.cppreference.com/w/cpp/atomic/atomic/store returns void.

I see the value in being explicit what is actually going on

This is what documentation is for, because the RPC interface is mostly used by machines and not humans interpreting the response to get feedback on what was going.

Maybe what could make sense here is to return the fee reason in send* and fund* rpcs. Something like
-> sendtoaddress
<- { "txid" : "deadbeef", "fee_reason" : "paytxfee" }

Maybe what could make sense here is to return the fee reason in send* and fund* rpcs. Something like
-> sendtoaddress
<- { "txid" : "deadbeef", "fee_reason" : "paytxfee" }

Nice -- worth proposing as a follow-up.

DrahtBot · 2020-03-30T13:15:37Z

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

Conflicts

No conflicts as of last run.

fjahr · 2020-03-30T20:03:03Z

Added a commit that makes settxfee respect the setting -maxtxfee as suggested by @MarcoFalke here: #18315 (comment)

jonasschnelli · 2020-03-31T08:09:01Z

Concept ACK. Light code review ACK.

I guess this breaks the RPC API. Should probably be mentioned in the release notes.

maflcko · 2020-03-31T12:43:35Z

Improving documentation and breaking the API should probably be two pull requests.

fjahr · 2020-03-31T16:04:33Z

Thanks for the feedback! I split documentation and return value changes into separate commits and added release notes.

maflcko · 2020-03-31T17:19:37Z

According to the documentation, breaking changes to the RPC API need to go through an -rpcdeprecated cycle. See https://github.com/bitcoin/bitcoin/blob/master/doc/JSON-RPC-interface.md#versioning

jonatack

Concept ACK, good changes. I agree that the API changes need to go through a deprecation process and should be in a separate PR from the non-breaking changes.

doc/release-notes-18467.md

fjahr · 2020-04-02T21:06:19Z

Added the -deprecatedrpc=settxfee option and mentioned it in the release notes.

maflcko · 2020-04-10T17:38:38Z

ACK bda84a0 (nice documentation)
ACK 6c1502768e72ec883a8a5d1682e335f62c8109aa (with previous commit removed, nice behaviour change to avoid setting an invalid value)
Not sure about the other commits in this pull

fjahr · 2020-04-14T14:19:59Z

@MarcoFalke thanks for the feedback.

I have removed the two commits related to the RPC return value change since it seems controversial and I agree that the improvement is probably too small to warrant an API change including deprecation cycle. What's left is documentation improvement and that settxfee respects -maxtxfee now. I am not if this should close #18315 now and will leave it to the user who reported it.

maflcko · 2020-04-14T14:37:10Z

ACK 3867727, seems useful to error out early instead of later #16257 🕍

Show signature and timestamp

Signature:

Timestamp of file with hash 262b14486a279571b51bb4e1b641d530dfa6be4293fff0bb9eb0be1f38a9424b -

jonatack

ACK 3867727

jonatack · 2020-04-14T14:51:54Z

src/wallet/rpcwallet.cpp

            RPCHelpMan{"settxfee",
-                "\nSet the transaction fee per kB for this wallet. Overrides the global -paytxfee command line parameter.\n",
+                "\nSet the transaction fee per kB for this wallet. Overrides the global -paytxfee command line parameter.\n"
+                "Can be deactivated by passing 0 as the fee. In that case automatic fee selection will be used by default.\n",


suggestion if you retouch or for a follow-up:

- "Can be deactivated by passing 0 as the fee. In that case automatic fee selection will be used by default.\n", + "Can be deactivated by passing 0 as the fee, in which case automatic fee selection will be used by default.\n",

jonatack · 2020-04-14T15:17:35Z

src/wallet/rpcwallet.cpp

Maybe what could make sense here is to return the fee reason in send* and fund* rpcs. Something like
-> sendtoaddress
<- { "txid" : "deadbeef", "fee_reason" : "paytxfee" }

Nice -- worth proposing as a follow-up.

jonatack · 2020-04-14T15:26:25Z

test/functional/wallet_bumpfee.py


+    # check that settxfee respects -maxtxfee
+    self.restart_node(1, ['-maxtxfee=0.000025'] + self.extra_args[1])
+    assert_raises_rpc_error(-8, "txfee cannot be more than wallet max tx fee", rbf_node.settxfee, Decimal('0.00003'))


Verified this new assertion fails without the changes in rpcwallet.cpp.

meshcollider

LGTM, utACK 3867727

…ettxfee 3867727 rpc: settxfee respects -maxtxfee wallet setting (Fabian Jahr) bda84a0 rpc: Add documentation for deactivating settxfee (Fabian Jahr) Pull request description: ~~Closes 18315~~ `settxfee` can be deactivated by passing 0 as the fee. That does not seem to be documented for the user so this PR adds it in the description. The return value of a simple boolean seems also too simplified given the multiple dimensions that this deactivation feature enables. I.e. it does not seem intuitive if the returned boolean shows that the call succeeded or if means that `settxfee` is active. My suggested solution is to change the return value to a JSON object that included the "active" state and the currently set fee rate. Examples: ``` $ src/bitcoin-cli settxfee 0.0000000 { "active": false, "fee_rate": "0.00000000 BTC/kB" } $ src/bitcoin-cli settxfee 0.0001 { "active": true, "fee_rate": "0.00010000 BTC/kB" } ``` ACKs for top commit: MarcoFalke: ACK 3867727, seems useful to error out early instead of later bitcoin#16257 🕍 jonatack: ACK 3867727 meshcollider: LGTM, utACK 3867727 Tree-SHA512: 642813b5cf6612abb4b6cb63728081a6bd1659d809e0149c8f56060b6da7253fee989b3b202854f3051df3773c966799af30b612648c466b099f00590f356548

…etting Summary: PR description: > settxfee can be deactivated by passing 0 as the fee. That does not seem to be documented for the user so this PR adds it in the description. > Added a commit that makes settxfee respect the setting -maxtxfee This is a backport of Core [[bitcoin/bitcoin#18467 | PR18467]] Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D8939

fjahr mentioned this pull request Mar 29, 2020

settxfee 0.00000000 returns true #18315

Closed

maflcko reviewed Mar 29, 2020

View reviewed changes

maflcko added RPC/REST/ZMQ Wallet labels Mar 29, 2020

DrahtBot mentioned this pull request Mar 30, 2020

Use shared pointers only in validation interface #18354

Closed

rpc: Add documentation for deactivating settxfee

bda84a0

fjahr force-pushed the txfee0 branch from a193b51 to efff79a Compare March 31, 2020 16:02

jonatack reviewed Mar 31, 2020

View reviewed changes

doc/release-notes-18467.md Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Apr 5, 2020

rpc: remove deprecated CRPCCommand constructor #18531

Merged

rpc: settxfee respects -maxtxfee wallet setting

3867727

fjahr force-pushed the txfee0 branch from 14ba837 to 3867727 Compare April 14, 2020 14:14

jonatack reviewed Apr 14, 2020

View reviewed changes

meshcollider reviewed Apr 17, 2020

View reviewed changes

maflcko merged commit c2e53ff into bitcoin:master Apr 17, 2020

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

rpc: Improve documentation and return value of settxfee #18467

rpc: Improve documentation and return value of settxfee #18467

Uh oh!

Conversation

fjahr commented Mar 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko Mar 29, 2020

Choose a reason for hiding this comment

Uh oh!

fjahr Mar 29, 2020

Choose a reason for hiding this comment

Uh oh!

maflcko Apr 5, 2020

Choose a reason for hiding this comment

Uh oh!

fjahr Apr 6, 2020

Choose a reason for hiding this comment

Uh oh!

maflcko Apr 6, 2020

Choose a reason for hiding this comment

Uh oh!

jonatack Apr 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Mar 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

fjahr commented Mar 30, 2020

Uh oh!

jonasschnelli commented Mar 31, 2020

Uh oh!

maflcko commented Mar 31, 2020

Uh oh!

fjahr commented Mar 31, 2020

Uh oh!

maflcko commented Mar 31, 2020

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fjahr commented Apr 2, 2020

Uh oh!

maflcko commented Apr 10, 2020

Uh oh!

fjahr commented Apr 14, 2020

Uh oh!

maflcko commented Apr 14, 2020

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack Apr 14, 2020

Choose a reason for hiding this comment

Uh oh!

jonatack Apr 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonatack Apr 14, 2020

Choose a reason for hiding this comment

Uh oh!

meshcollider left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

fjahr commented Mar 29, 2020 •

edited

Loading

jonatack Apr 14, 2020 •

edited

Loading

DrahtBot commented Mar 30, 2020 •

edited

Loading

jonatack Apr 14, 2020 •

edited

Loading