rpc: add the add_inputs option to bumpfee/psbtbumpfee #21284

pull danben wants to merge 1 commits into bitcoin:master from danben:add-add-inputs-option-to-bumpfee changing 2 files +18 −2
  1. danben commented at 6:46 pm on February 23, 2021: contributor
    As in fundrawtransaction, this gives the user the option to specify whether new inputs should be added when bumping the fee. See #20935
  2. DrahtBot added the label RPC/REST/ZMQ on Feb 23, 2021
  3. DrahtBot added the label Wallet on Feb 23, 2021
  4. in src/wallet/rpcwallet.cpp:3447 in 40bde1a889 outdated
    3443@@ -3444,6 +3444,7 @@ static RPCHelpMan bumpfee_helper(std::string method_name)
    3444                              "are replaceable).\n"},
    3445                     {"estimate_mode", RPCArg::Type::STR, /* default */ "unset", std::string() + "The fee estimate mode, must be one of (case insensitive):\n"
    3446     "         \"" + FeeModes("\"\n\"") + "\""},
    3447+                    {"add_inputs", RPCArg::Type::BOOL, /* default */ "true", "For a transaction with existing inputs, automatically include more if they are not enough."},
    


    luke-jr commented at 3:36 am on February 24, 2021:
    Default should probably be “as needed”, and let “true” (perhaps not now, but someday) be used to force additional inputs?
  5. luke-jr changes_requested
  6. ghost commented at 8:16 pm on February 24, 2021: none

    Compiled successfully. Tests failed.

    Tried experimenting with add_inputs:

    1. Transaction with 2 inputs selected manually and custom change address (A new receive address created and used as change):

    FALSE

    Error:

    TRUE

    1. Transaction with 2 inputs selected manually and custom change address (A new change address created and used as change):

    FALSE

    No Error:

    Not sure if this option is really solving the issue or will help users until we fix other related issues.

  7. Added the add_inputs option to the bumpfee RPC. Behavior is identical to that of the same option in the fundrawtransaction RPC. 115b461b63
  8. danben force-pushed on Feb 25, 2021
  9. danben commented at 6:19 pm on February 25, 2021: contributor
    @luke-jr I made the suggested change but am a little worried that it won’t be clear to users what value to use if they don’t want new inputs to be added. @prayank23 I will look into what causes a change address to behave differently and possibly propose something. I agree that the current change will make things worse if the behavior is inconsistent.
  10. danben commented at 2:35 am on February 27, 2021: contributor
    @prayank23 How did you create the change address in your second test case (i.e. the one that behaved wrongly)? Did you use getrawchangeaddress or something else?
  11. ghost commented at 10:46 pm on February 28, 2021: none

    @danben I used getnewaddress and getrawchangeaddress

    Problem is with 2 things:

    1. Using address returned in getnewaddress or something that doesn’t belong to wallet
    2. Using address which has a label even if it was created with getrawchangeaddress
  12. in src/wallet/rpcwallet.cpp:3447 in 115b461b63
    3443@@ -3444,6 +3444,7 @@ static RPCHelpMan bumpfee_helper(std::string method_name)
    3444                              "are replaceable).\n"},
    3445                     {"estimate_mode", RPCArg::Type::STR, /* default */ "unset", std::string() + "The fee estimate mode, must be one of (case insensitive):\n"
    3446     "         \"" + FeeModes("\"\n\"") + "\""},
    3447+                    {"add_inputs", RPCArg::Type::STR, /* default */ "as_needed", "For a transaction with existing inputs, automatically include more if they are not enough."},
    


    MarcoFalke commented at 1:07 pm on March 29, 2021:
    Wouldn’t this be better as a plain bool?

    danben commented at 1:09 pm on April 1, 2021:
    @MarcoFalke That is what I had originally, but changed it to a string as per @luke-jr’s suggestion from 2/23 (see above)

    MarcoFalke commented at 1:57 pm on April 1, 2021:
    what is the use case to force adding inputs if none are needed?

    danben commented at 3:28 pm on April 1, 2021:
    I will defer to @luke-jr on that; I am not sure

    luke-jr commented at 2:12 am on June 23, 2021:

    I think you misunderstood me. I meant the documentation should be accurate, not that “as_needed” should be a value. Documenting it as “true” suggested that inputs would be added even if not needed.

    If the user wants “as needed”, they just would omit “add_inputs” (or specify it as null). But I suppose defaults should be possible to specify explicitly, too, so not sure how to handle that. :)

  13. DrahtBot commented at 1:57 pm on March 29, 2021: contributor

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #21544 (rpc: Missing doc updates for bumpfee psbt update by MarcoFalke)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  14. DrahtBot added the label Needs rebase on Apr 1, 2021
  15. DrahtBot commented at 9:05 am on April 1, 2021: contributor

    🐙 This pull request conflicts with the target branch and needs rebase.

    Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

  16. DrahtBot commented at 12:28 pm on December 22, 2021: contributor
    • Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
    • Is it no longer relevant? ➡️ Please close.
    • Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.
  17. fanquake added the label Up for grabs on Apr 26, 2022
  18. fanquake closed this on Apr 26, 2022

  19. fanquake removed the label Up for grabs on Aug 4, 2022
  20. fanquake removed the label Needs rebase on Aug 4, 2022
  21. bitcoin locked this on Aug 4, 2023

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-11-17 18:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me