rpc: add the add_inputs option to bumpfee/psbtbumpfee

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

DrahtBot added the label RPC/REST/ZMQ on Feb 23, 2021

DrahtBot added the label Wallet on Feb 23, 2021

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?

luke-jr changes_requested

ghost commented at 8:16 pm on February 24, 2021: none

Compiled successfully. Tests failed.

Tried experimenting with add_inputs:

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

FALSE

Error:

TRUE

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.

Added the add_inputs option to the bumpfee RPC. Behavior is identical to that of the same option in the fundrawtransaction RPC. 115b461b63

danben force-pushed on Feb 25, 2021

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.

ghost commented at 6:57 pm on February 25, 2021: none

@danben Check #20795, https://github.com/bitcoin-core/gui/pull/186 and https://github.com/bitcoin/bitcoin/issues/20598

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?

ghost commented at 10:46 pm on February 28, 2021: none

@danben I used getnewaddress and getrawchangeaddress

Problem is with 2 things:

Using address returned in getnewaddress or something that doesn’t belong to wallet
Using address which has a label even if it was created with getrawchangeaddress

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. :)

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.

DrahtBot added the label Needs rebase on Apr 1, 2021

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

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.

fanquake added the label Up for grabs on Apr 26, 2022

fanquake closed this on Apr 26, 2022

fanquake removed the label Up for grabs on Aug 4, 2022

fanquake removed the label Needs rebase on Aug 4, 2022

bitcoin locked this on Aug 4, 2023

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

Conflicts