-maxtxfee should not be used by both node and wallet #15355

issue jnewbery openend this issue on February 6, 2019
  1. jnewbery commented at 4:28 pm on February 6, 2019: member

    EDITED 2019-02-08

    maxTxFee is a global variable, used by both the raw transaction and wallet RPCs. It’s set from -maxtxfee.

    listed in the global help text in init.cpp, but is only set in WalletInit::ParameterInteraction(). If the wallet is disabled, then -maxtxfee is ignored and the default is used.

    This behaviour has existed since the mempool was changed to limit acceptance based on maxTxFee in https://github.com/bitcoin/bitcoin/commit/fa331db68bcc68e4c93fb45aaa30f911b0ecfe1a.

    Short-term fix is to move the -maxtxfee handling to InitParameterInteraction() in init.cpp. EDIT: maxTxFee initiation bug fixed in #15357

    I don’t think we should share this setting between the node and wallet.

  2. jnewbery commented at 4:28 pm on February 6, 2019: member
    @MarcoFalke
  3. MarcoFalke added this to the milestone 0.18.0 on Feb 6, 2019
  4. MarcoFalke commented at 4:59 pm on February 6, 2019: member

    Longer term, I don’t think we should share this setting between the node and wallet.

    The wallet should still be aware of what the value is, which is why I made it shared between the node and wallet in the first place. For that to work you’d have to parse it before parsing the wallet specific max fee. So moving it to src/init.cpp as you suggest is probably a requirement for that.

    Short-term fix is to move the -maxtxfee handling to InitParameterInteraction() in init.cpp.

    init.cpp is also the place where AddArgs is happening, so ACK from me.

  5. MarcoFalke added the label Wallet on Feb 6, 2019
  6. MarcoFalke added the label RPC/REST/ZMQ on Feb 6, 2019
  7. MarcoFalke added the label Mempool on Feb 6, 2019
  8. MarcoFalke removed the label Mempool on Feb 6, 2019
  9. MarcoFalke removed the label Wallet on Feb 6, 2019
  10. MarcoFalke added the label Wallet on Feb 6, 2019
  11. MarcoFalke added the label Mempool on Feb 6, 2019
  12. MarcoFalke added the label Up for grabs on Feb 6, 2019
  13. MarcoFalke added the label good first issue on Feb 6, 2019
  14. MarcoFalke removed the label Up for grabs on Feb 6, 2019
  15. jnewbery commented at 10:45 pm on February 6, 2019: member

    The wallet should still be aware of what the value is

    Right - this command line option was added in aa279d613152e87ea25edfdf76c86779c0632f18 to mean “I don’t want my wallet to create a transaction that has a fee higher than this”. After a few refactors, the same maxTxFee was recycled for use by the raw transaction RPCs. My point is that there should be a separation - -maxtxfee should be used to control wallet behaviour, and something else (probably an RPC parameter) should be used to control the raw transaction RPC behaviour.

    We both agree that the minimal fix (move -maxtxfee to init.cpp) is what we should do now.

  16. gmaxwell commented at 11:02 pm on February 7, 2019: contributor
    Creating a plethora of overlapping fee configuration options is not good for users. The result from doing that is that users set the wrong ones then are surprised when they don’t get the behaviour they’re expecting. Maxtxfee shouldn’t be ignored when the wallet is disabled, for sure, but what benefit to users of the software is there in splitting “fee level above which we treat as something has gone wrong” between the raw transactions interface and the wallet? I don’t believe we’ve ever gotten a request “I want to set this one to X and the other one to Y”.
  17. jnewbery commented at 2:17 pm on February 8, 2019: member

    Creating a plethora of overlapping fee configuration options…

    I agree that having multiple config options would be confusing, which is why I suggested an RPC parameter for the sendrawtransaction and testmempoolaccept RPCs, which are the only places that maxTxFee is used outside the wallet. In fact, both of those RPCs already have a parameter allowhighfees which submits the transaction to the mempool without a nAbsurdFee parameter.

    My reasoning is that those RPCs are lower-level than wallet operations, and should be separate from the wallet. The existing behaviour doesn’t make really make sense to me:

    • testmempoolaccept currently means “would my wallet be able to submit this to the mempool?” rather than “would this be accepted to the mempool over P2P?”, unless allowhighfees is set to true, in which case it means “would this be accepted to the mempool over P2P?”.
    • sendrawtransaction currently means “broadcast this transaction if the wallet would submit it to the mempool” rather than “broadcast this transaction”, unless allowhighfees is set to true, in which case it means “broadcast this transaction”.
  18. MarcoFalke closed this on Feb 8, 2019
  19. MarcoFalke referenced this in commit 2945492424 on Feb 8, 2019
  20. jnewbery reopened this on Feb 8, 2019
  21. jnewbery commented at 7:45 pm on February 8, 2019: member
    #15357 doesn’t fully resolve the issue here. Reopening.
  22. jnewbery renamed this:
    `-maxtxfee` is ignored when wallet is disabled
    `-maxtxfee` should not be used by both node and wallet
    on Feb 8, 2019
  23. MarcoFalke removed the label good first issue on Feb 8, 2019
  24. MarcoFalke commented at 8:42 pm on February 8, 2019: member

    I don’t think there is anything left to do here after moving the parsing back to the node (init.cpp).

    Re (about the implicit use in the wallet): The wallet should never create transactions that have such a high fee that this test could fail (at least in the default setting) and currently this serves as a “last-mile” belt-and-suspenders check for the wallet.

    Re (about the implicit use in the rpc interface): I don’t object changing the rpcs to explicitly accept a fee (instead of the bool), but that seems like it should be handled in a separate feature-request.

  25. MarcoFalke removed this from the milestone 0.18.0 on Feb 13, 2019
  26. MarcoFalke commented at 5:14 pm on February 13, 2019: member
    Removed milestone, but I haven’t received a reply either, so I intend to close this
  27. jnewbery commented at 5:24 pm on February 13, 2019: member
    This is partially fixed by #13541. If the same change is made to testmempoolaccept then maxTxFee could be removed from the node entirely and moved to the wallet.
  28. kallewoof commented at 7:48 am on February 14, 2019: member
    I plan to do did the same change to testmempoolaccept later today.
  29. MarcoFalke referenced this in commit 656a15e539 on Mar 27, 2019
  30. MarcoFalke referenced this in commit 3356799ee3 on Apr 27, 2019
  31. MarcoFalke closed this on Apr 27, 2019
  32. Munkybooty referenced this in commit 139a98561d on Aug 27, 2021
  33. Munkybooty referenced this in commit 56a4ffa535 on Aug 28, 2021
  34. Munkybooty referenced this in commit 5e9a6baeab on Aug 29, 2021
  35. Munkybooty referenced this in commit 504f40e3de on Sep 1, 2021
  36. Munkybooty referenced this in commit e47609493c on Sep 5, 2021
  37. Munkybooty referenced this in commit 4efa0a69b6 on Sep 7, 2021
  38. Munkybooty referenced this in commit 3a9a1cb2eb on Sep 7, 2021
  39. Munkybooty referenced this in commit c4b8697a7f on Sep 9, 2021
  40. Munkybooty referenced this in commit 2d5cdd45b8 on Sep 10, 2021
  41. Munkybooty referenced this in commit ae4bea6052 on Sep 13, 2021
  42. vijaydasmp referenced this in commit e9cbf74c5e on Oct 30, 2021
  43. vijaydasmp referenced this in commit 3a9c0dc4f0 on Oct 30, 2021
  44. vijaydasmp referenced this in commit d1e396828e on Oct 30, 2021
  45. DrahtBot locked this on Dec 16, 2021


jnewbery MarcoFalke gmaxwell kallewoof

Labels
Wallet RPC/REST/ZMQ Mempool

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: 2024-12-18 15:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me