rpc: Uncouple non-wallet rpcs from maxTxFee global #15620

pull MarcoFalke wants to merge 3 commits into bitcoin:master from MarcoFalke:1903-rpcNoMaxTxFee changing 3 files +34 −13

MarcoFalke commented at 6:03 PM on March 18, 2019: member
This makes the rpcs a bit more stateless by falling back to their own default max fee instead of the global maxTxFee.

A follow up pull request will move -maxtxfee to the wallet.

See also related discussions:
- -maxtxfee should not be used by both node and wallet #15355
- [RFC] Long term plan for wallet command-line args #13044
rpc: Use IsValidNumArgs over hardcoded size checks fa965e03c7
rpc: Uncouple rpcs from maxTxFee global fa96d76421
MarcoFalke added the label RPC/REST/ZMQ on Mar 18, 2019
jnewbery commented at 6:54 PM on March 18, 2019: member

Concept ACK!
MarcoFalke removed the label RPC/REST/ZMQ on Mar 18, 2019
DrahtBot added the label RPC/REST/ZMQ on Mar 19, 2019
doc: Add release notes for 15620 fa1ad200d3
ryanofsky approved
ryanofsky commented at 3:36 PM on March 20, 2019: member

utACK fa96d7642178b5472b7aa76de9c8b86e7b9cde54. This looks great and is nicely done. But it definitely should have release notes noting change in behavior for sendrawtransaction and testmempoolaccept RPCs, which now ignore the -maxtxfee setting.
fanquake added the label Needs release note on Mar 20, 2019
jamesob commented at 4:02 PM on March 20, 2019: member
utACK https://github.com/bitcoin/bitcoin/pull/15620/commits/fa96d7642178b5472b7aa76de9c8b86e7b9cde54. Verified this replaces the only usages of maxTxFee global in rpc code.
```
$ git grep maxTxFee | grep rpc

src/rpc/rawtransaction.cpp:    const CAmount highfee{allowhighfees ? 0 : ::maxTxFee};
src/rpc/rawtransaction.cpp:    CAmount max_raw_tx_fee = ::maxTxFee;
```
promag commented at 4:38 PM on March 20, 2019: member

How about bumpfee? https://github.com/bitcoin/bitcoin/blob/93623eea71e7159e367b1b1888418099a5307983/src/wallet/feebumper.cpp#L161-L166

😮 space in L162
ryanofsky commented at 4:47 PM on March 20, 2019: member

Bumpfee is a wallet function, so it's reasonable if it continues to use maxtxfee if maxtxfee becomes a wallet option.
in src/rpc/rawtransaction.cpp:42 in fa96d76421 outdated
```
  38 | @@ -39,6 +39,7 @@
  39 |  
  40 |  #include <univalue.h>
  41 |  
  42 | +constexpr static CAmount DEFAULT_MAX_RAW_TX_FEE{COIN / 10};
```
promag commented at 5:03 PM on March 20, 2019:

There is already DEFAULT_TRANSACTION_MAXFEE in validation.h, isn't a new constant confusing?
jnewbery commented at 8:02 PM on March 20, 2019:

Suggest a comment here. Something like:

/** High fee for sendrawtransaction and testmempoolaccept. * By default, transaction with a fee higher than this will be rejected by * sendrawtransaction and testmempoolaccept. This can be overriden with the * maxfeerate argument. * */
MarcoFalke commented at 8:17 PM on March 20, 2019:

I think the release notes and rpc documentation are pretty clear on this. Lets not bloat developers with redundant or excessive documentation

jnewbery commented at 8:23 PM on March 20, 2019:

Release notes and RPC documentation have nothing to do with code constants. @promag has already said that this constant name is confusing.

MarcoFalke commented at 8:59 PM on March 20, 2019:

promag was worried that there are two constants where one would theoretically be sufficient

Happy to change the name if there is a better one, though.

MarcoFalke commented at 9:07 PM on March 20, 2019:

Anyway, changed my mind

promag commented at 2:59 AM on March 24, 2019:

nit, anonymous namespace instead?
promag commented at 5:03 PM on March 20, 2019: member

Misinterpreted the goal sorry, IMO could have the title Uncouple non-wallet rpcs from maxTxFee global.

Agree with adding release notes.
MarcoFalke renamed this:
~~rpc: Uncouple rpcs from maxTxFee global~~
rpc: Uncouple non-wallt rpcs from maxTxFee global
on Mar 20, 2019
MarcoFalke renamed this:
~~rpc: Uncouple non-wallt rpcs from maxTxFee global~~
rpc: Uncouple non-wallet rpcs from maxTxFee global
on Mar 20, 2019
MarcoFalke removed the label Needs release note on Mar 20, 2019
MarcoFalke commented at 5:08 PM on March 20, 2019: member

Renamed title and added release notes as requested by @promag
in doc/release-notes-15620.md:4 in faadaae479 outdated
```
   0 | @@ -0,0 +1,11 @@
   1 | +Updated RPCs
   2 | +------------
   3 | +
   4 | +* The `sendrawtransaction` and `testmempoolaccept` previouly accepted an
```
ryanofsky commented at 7:33 PM on March 20, 2019:

In commit "doc: Add release notes for 15620" (faadaae4795038d77936f36483f1410cd92b3238)

s/previouly accepted/RPC methods previously accepted/

ryanofsky commented at 7:36 PM on March 20, 2019:

In commit "doc: Add release notes for 15620" (faadaae):

This is a nice, detailed description, but I think it would be nice to have a sentence at the beginning summarizing the whole paragraph, like "The -maxtxfee setting no longer has any effect on non-wallet RPCs. [...]"

MarcoFalke commented at 8:01 PM on March 20, 2019:

Done
ryanofsky approved
ryanofsky commented at 7:37 PM on March 20, 2019: member

utACK faadaae4795038d77936f36483f1410cd92b3238, only change is release notes
MarcoFalke force-pushed on Mar 20, 2019
MarcoFalke force-pushed on Mar 20, 2019
jnewbery commented at 8:07 PM on March 20, 2019: member

Oops. I'd already written a branch for this too.

One comment inline, otherwise utACK fa4230b8a708ad9b848c97a5142da804afc43a4b.
MarcoFalke force-pushed on Mar 20, 2019
MarcoFalke commented at 9:07 PM on March 20, 2019: member

Changed my mind as requested by @jnewbery
jnewbery commented at 10:15 PM on March 20, 2019: member

utACK fa1ad200d378fc3a4dc4c54214965d3c852db7d7
Empact commented at 8:24 AM on March 21, 2019: member

utACK https://github.com/bitcoin/bitcoin/pull/15620/commits/fa1ad200d378fc3a4dc4c54214965d3c852db7d7
promag commented at 3:00 AM on March 24, 2019: member

The above reference was a mistake.

utACK fa1ad20.
sipa commented at 11:40 PM on March 25, 2019: member

Concept ACK
jnewbery commented at 4:34 PM on March 26, 2019: member

utACK fa1ad200d378fc3a4dc4c54214965d3c852db7d7
MarcoFalke referenced this in commit 656a15e539 on Mar 27, 2019
MarcoFalke merged this on Mar 27, 2019
MarcoFalke closed this on Mar 27, 2019
MarcoFalke deleted the branch on Mar 27, 2019
MarcoFalke referenced this in commit 3356799ee3 on Apr 27, 2019
vansergen referenced this in commit 761f0d9633 on Mar 26, 2020
deadalnix referenced this in commit 6c784a6125 on May 27, 2020
vijaydasmp referenced this in commit e9cbf74c5e on Oct 30, 2021
vijaydasmp referenced this in commit 3a9c0dc4f0 on Oct 30, 2021
vijaydasmp referenced this in commit d1e396828e on Oct 30, 2021
vijaydasmp referenced this in commit a4a882873f on Nov 2, 2021
vijaydasmp referenced this in commit f0cf08dc7c on Nov 7, 2021
vijaydasmp referenced this in commit e39241e7b2 on Nov 7, 2021
vijaydasmp referenced this in commit f4c049f2f9 on Nov 7, 2021
kittywhiskers referenced this in commit 5ac548f127 on Dec 3, 2021
kittywhiskers referenced this in commit a48a7d8b3c on Dec 4, 2021
kittywhiskers referenced this in commit d61f3da5d1 on Dec 6, 2021
kittywhiskers referenced this in commit 5d293646b4 on Dec 8, 2021
kittywhiskers referenced this in commit 87915f4e68 on Dec 8, 2021
kittywhiskers referenced this in commit 894dc02b14 on Dec 8, 2021
kittywhiskers referenced this in commit 15d363a4b7 on Dec 11, 2021
kittywhiskers referenced this in commit ab34959eb0 on Dec 12, 2021
kittywhiskers referenced this in commit 399d8b3ddc on Dec 12, 2021
kittywhiskers referenced this in commit f00d5d3b37 on Dec 12, 2021
kittywhiskers referenced this in commit eecc714414 on Dec 12, 2021
kittywhiskers referenced this in commit 254f45a4f7 on Dec 12, 2021
kittywhiskers referenced this in commit 3284d82296 on Dec 12, 2021
DrahtBot locked this on Dec 16, 2021

Contributors

Labels

Linked (view graph)

#13044 [RFC] Long term plan for wallet command-line args #15355 `-maxtxfee` should not be used by both node and wallet #15778 [wallet] Move maxtxfee from node to wallet