rpc: Improve documentation and return value of settxfee #18467

fjahr commented at 9:48 PM on March 29, 2020: member

~~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"
}

in src/wallet/rpcwallet.cpp:2322 in 253402a1b4 outdated

2317 | @@ -2318,12 +2318,17 @@ static UniValue settxfee(const JSONRPCRequest& request)
2318 |      }
2319 |  
2320 |              RPCHelpMan{"settxfee",
2321 | -                "\nSet the transaction fee per kB for this wallet. Overrides the global -paytxfee command line parameter.\n",
2322 | +                "\nSet the transaction fee per kB for this wallet. Overrides the global -paytxfee command line parameter.\n"
2323 | +                "Can be deactivated by passing 0 as the fee. In that case automatic fee selection will be used by default.\n",

MarcoFalke commented at 10:07 PM on March 29, 2020:

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

fjahr commented at 10:32 PM on March 29, 2020:

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.

MarcoFalke commented at 7:51 PM on April 5, 2020:

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.

fjahr commented at 1:38 PM on April 6, 2020:

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?

MarcoFalke commented at 2:50 PM on April 6, 2020:

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" }

jonatack commented at 3:17 PM on April 14, 2020:

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.

MarcoFalke added the label RPC/REST/ZMQ on Mar 29, 2020

MarcoFalke added the label Wallet on Mar 29, 2020

DrahtBot commented at 1:15 PM on March 30, 2020: member

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

Conflicts

No conflicts as of last run.

fjahr commented at 8:03 PM on March 30, 2020: member

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

jonasschnelli commented at 8:09 AM on March 31, 2020: contributor

Concept ACK. Light code review ACK.

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

MarcoFalke commented at 12:43 PM on March 31, 2020: member

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

rpc: Add documentation for deactivating settxfee bda84a08a0

fjahr force-pushed on Mar 31, 2020

fjahr commented at 4:04 PM on March 31, 2020: member

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

MarcoFalke commented at 5:19 PM on March 31, 2020: member

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

in doc/release-notes-18467.md:4 in efff79aadb outdated

   0 | @@ -0,0 +1,7 @@
   1 | +Updated RPCs
   2 | +------------
   3 | +
   4 | +`settxfee` now returns a JSON result object instead of a simple boolean in

jonatack commented at 7:59 PM on March 31, 2020:

a few nits: omit "result", s/reflect/reflect the/, and s/errors/raises|returns an error/

fjahr commented at 9:04 PM on April 2, 2020:

done

jonatack commented at 8:04 PM on March 31, 2020: member

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.

fjahr commented at 9:06 PM on April 2, 2020: member

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

MarcoFalke commented at 5:38 PM on April 10, 2020: member

ACK bda84a08a0ac92dff6cadc99cf9bb8c3fadd7e13 (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

rpc: settxfee respects -maxtxfee wallet setting 38677274f9

fjahr force-pushed on Apr 14, 2020

fjahr commented at 2:19 PM on April 14, 2020: member

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

MarcoFalke commented at 2:37 PM on April 14, 2020: member

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

<details><summary>Show signature and timestamp</summary>

Signature:

Timestamp of file with hash 262b14486a279571b51bb4e1b641d530dfa6be4293fff0bb9eb0be1f38a9424b -

</details>

in src/wallet/rpcwallet.cpp:2322 in bda84a08a0 outdated

2317 | @@ -2318,7 +2318,8 @@ static UniValue settxfee(const JSONRPCRequest& request)
2318 |      }
2319 |  
2320 |              RPCHelpMan{"settxfee",
2321 | -                "\nSet the transaction fee per kB for this wallet. Overrides the global -paytxfee command line parameter.\n",
2322 | +                "\nSet the transaction fee per kB for this wallet. Overrides the global -paytxfee command line parameter.\n"
2323 | +                "Can be deactivated by passing 0 as the fee. In that case automatic fee selection will be used by default.\n",

jonatack commented at 2:51 PM on April 14, 2020:

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",

in test/functional/wallet_bumpfee.py:286 in 38677274f9

 280 | @@ -281,9 +281,15 @@ def test_settxfee(self, rbf_node, dest_address):
 281 |      assert_greater_than(Decimal("0.00001000"), abs(requested_feerate - actual_feerate))
 282 |      rbf_node.settxfee(Decimal("0.00000000"))  # unset paytxfee
 283 |  
 284 | +    # check that settxfee respects -maxtxfee
 285 | +    self.restart_node(1, ['-maxtxfee=0.000025'] + self.extra_args[1])
 286 | +    assert_raises_rpc_error(-8, "txfee cannot be more than wallet max tx fee", rbf_node.settxfee, Decimal('0.00003'))

jonatack commented at 3:26 PM on April 14, 2020:

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

jonatack commented at 3:27 PM on April 14, 2020: member

ACK 38677274f931088218eeb

meshcollider commented at 11:29 AM on April 17, 2020: contributor

LGTM, utACK 38677274f931088218eeb1f258077d3387f39c89

MarcoFalke merged this on Apr 17, 2020

MarcoFalke closed this on Apr 17, 2020

sidhujag referenced this in commit a041dfaaf0 on Apr 17, 2020

Fabcien referenced this in commit 634c08afae on Jan 18, 2021

DrahtBot locked this on Feb 15, 2022