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
  1. 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
  2. rpc: Use IsValidNumArgs over hardcoded size checks fa965e03c7
  3. rpc: Uncouple rpcs from maxTxFee global fa96d76421
  4. MarcoFalke added the label RPC/REST/ZMQ on Mar 18, 2019
  5. jnewbery commented at 6:54 PM on March 18, 2019: member

    Concept ACK!

  6. MarcoFalke removed the label RPC/REST/ZMQ on Mar 18, 2019
  7. DrahtBot added the label RPC/REST/ZMQ on Mar 19, 2019
  8. doc: Add release notes for 15620 fa1ad200d3
  9. ryanofsky approved
  10. ryanofsky commented at 3:36 PM on March 20, 2019: contributor

    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.

  11. fanquake added the label Needs release note on Mar 20, 2019
  12. 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;
  13. 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

  14. ryanofsky commented at 4:47 PM on March 20, 2019: contributor

    Bumpfee is a wallet function, so it's reasonable if it continues to use maxtxfee if maxtxfee becomes a wallet option.

  15. 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?

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

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

    Renamed title and added release notes as requested by @promag

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

  22. ryanofsky approved
  23. ryanofsky commented at 7:37 PM on March 20, 2019: contributor

    utACK faadaae4795038d77936f36483f1410cd92b3238, only change is release notes

  24. MarcoFalke force-pushed on Mar 20, 2019
  25. MarcoFalke force-pushed on Mar 20, 2019
  26. 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.

  27. MarcoFalke force-pushed on Mar 20, 2019
  28. MarcoFalke commented at 9:07 PM on March 20, 2019: member

    Changed my mind as requested by @jnewbery

  29. jnewbery commented at 10:15 PM on March 20, 2019: member

    utACK fa1ad200d378fc3a4dc4c54214965d3c852db7d7

  31. promag cross-referenced this on Mar 23, 2019 from issue wallet: Keep all outputs in bumpfee by promag
  32. promag commented at 3:00 AM on March 24, 2019: member

    The above reference was a mistake.

    utACK fa1ad20.

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

    Concept ACK

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

    utACK fa1ad200d378fc3a4dc4c54214965d3c852db7d7

  35. MarcoFalke referenced this in commit 656a15e539 on Mar 27, 2019
  36. MarcoFalke merged this on Mar 27, 2019
  37. MarcoFalke closed this on Mar 27, 2019
  38. MarcoFalke deleted the branch on Mar 27, 2019
  39. jnewbery cross-referenced this on Apr 9, 2019 from issue [wallet] Move maxtxfee from node to wallet by jnewbery
  40. MarcoFalke referenced this in commit 3356799ee3 on Apr 27, 2019
  41. SomberNight cross-referenced this on Aug 2, 2019 from issue wallet: lower -txmaxfee default from 0.1 to 0.01 BTC by Sjors
  42. vansergen referenced this in commit 761f0d9633 on Mar 26, 2020
  43. deadalnix referenced this in commit 6c784a6125 on May 27, 2020
  44. ZmnSCPxj cross-referenced this on Jul 20, 2020 from issue `onchaind` should scorch-earth our penatly txes by ZmnSCPxj
  45. vijaydasmp referenced this in commit e9cbf74c5e on Oct 30, 2021
  46. vijaydasmp referenced this in commit 3a9c0dc4f0 on Oct 30, 2021
  47. vijaydasmp referenced this in commit d1e396828e on Oct 30, 2021
  48. vijaydasmp cross-referenced this on Oct 30, 2021 from issue Merge 15919, 15897, 15763, 16912, 17001, 16656, 16804 by vijaydasmp
  49. vijaydasmp referenced this in commit a4a882873f on Nov 2, 2021
  50. vijaydasmp referenced this in commit f0cf08dc7c on Nov 7, 2021
  51. vijaydasmp referenced this in commit e39241e7b2 on Nov 7, 2021
  52. vijaydasmp referenced this in commit f4c049f2f9 on Nov 7, 2021
  53. kwvg referenced this in commit 5ac548f127 on Dec 3, 2021
  54. kwvg referenced this in commit a48a7d8b3c on Dec 4, 2021
  55. kwvg referenced this in commit d61f3da5d1 on Dec 6, 2021
  56. kwvg referenced this in commit 5d293646b4 on Dec 8, 2021
  57. kwvg referenced this in commit 87915f4e68 on Dec 8, 2021
  58. kwvg referenced this in commit 894dc02b14 on Dec 8, 2021
  59. kwvg referenced this in commit 15d363a4b7 on Dec 11, 2021
  60. kwvg referenced this in commit ab34959eb0 on Dec 12, 2021
  61. kwvg referenced this in commit 399d8b3ddc on Dec 12, 2021
  62. kwvg referenced this in commit f00d5d3b37 on Dec 12, 2021
  63. kwvg referenced this in commit eecc714414 on Dec 12, 2021
  64. kwvg referenced this in commit 254f45a4f7 on Dec 12, 2021
  65. kwvg referenced this in commit 3284d82296 on Dec 12, 2021
  66. bitcoin locked this on Dec 16, 2021
Contributors
MarcoFalkejnewberyryanofskyjamesobpromagEmpactsipa
Labels
RPC/REST/ZMQ
Linked (view graph)
#13044 [RFC] Long term plan for wallet command-line args#15355 `-maxtxfee` should not be used by both node and wallet#15656 wallet: Keep all outputs in bumpfee#15778 [wallet] Move maxtxfee from node to wallet#16539 wallet: lower -txmaxfee default from 0.1 to 0.01 BTC
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-19 06:54 UTC