rpc: Support specifying change address in bumpfee #15341

pull promag wants to merge 3 commits into bitcoin:master from promag:2019-01-bumpfee-changeaddress changing 4 files +63 −12
  1. promag commented at 3:39 PM on February 4, 2019: member

    The bumpfee RPC has now the option to specify the change address which is used to identify which output will pay the fee bump.

    Fixes #15331.

  2. DrahtBot commented at 3:46 PM on February 4, 2019: member

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #11413 ([wallet] [rpc] sendtoaddress/sendmany: Add explicit feerate option by kallewoof)

    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.

  3. promag commented at 7:18 PM on February 4, 2019: member

    I knew this was familiar @DrahtBot, thanks! This is an alternative/complement to #12096.

  4. laanwj added the label Wallet on Feb 4, 2019
  5. in src/wallet/rpcwallet.cpp:3223 in 1a655efe96 outdated 
    3219 | @@ -3220,6 +3220,7 @@ static UniValue bumpfee(const JSONRPCRequest& request)
3220 |              "                         so the new transaction will not be explicitly bip-125 replaceable (though it may\n"
3221 |              "                         still be replaceable in practice, for example if it has unconfirmed ancestors which\n"
3222 |              "                         are replaceable)."},
3223 | +                            {"changeAddress", RPCArg::Type::STR_HEX, /* opt */ true, /* default_val */ "", "The change address to pay the fee bump"},
    laanwj commented at 7:56 PM on February 4, 2019:

    please follow the developer guidelines for naming new arguments:

     - Argument naming: use snake case `fee_delta` (and not, e.g. camel case `feeDelta`)
    promag commented at 7:58 PM on February 4, 2019:

    err, bad paste.. ty

  6. gmaxwell commented at 11:00 PM on February 4, 2019: contributor

    This should verify that the selected change output is in the wallet. Bumpfee is intentionally constructed so you can't accidentally rip off the payee using it.

  7. promag force-pushed on Feb 5, 2019
  8. promag force-pushed on Feb 5, 2019
  9. promag commented at 12:50 AM on February 5, 2019: member

    @gmaxwell Thanks, updated to verify that the specified address is spendable. Also improved test.

  10. jonasschnelli commented at 6:36 AM on February 5, 2019: contributor

    Nice. Concept ACK.

  11. promag force-pushed on Feb 5, 2019
  12. benthecarman deleted a comment on Feb 5, 2019
  13. in test/functional/wallet_bumpfee.py:135 in 752e2791eb outdated 
     128 | @@ -128,6 +129,20 @@ def test_segwit_bumpfee_succeeds(rbf_node, dest_address):
 129 |      assert rbfid not in rbf_node.getrawmempool()
 130 |  
 131 |  
 132 | +def test_change_address_succeeds(rbf_node, peer_node):
 133 | +    dest_address = peer_node.getnewaddress()
 134 | +    change_address = rbf_node.getnewaddress()
 135 | +    hex = rbf_node.createrawtransaction([], {dest_address: '0.00100000'})
    practicalswift commented at 9:57 AM on February 8, 2019:

    The built-in function hex is redefined here. Choose another name :-)

    promag commented at 3:31 PM on February 25, 2019:

    Done, now using raw.

    practicalswift commented at 8:49 AM on March 25, 2019:

    The built-in function hex is still redefined here, no? :-)

  14. MarcoFalke added the label Needs rebase on Feb 12, 2019
  15. in src/wallet/rpcwallet.cpp:3223 in 752e2791eb outdated 
    3219 | @@ -3220,6 +3220,7 @@ static UniValue bumpfee(const JSONRPCRequest& request)
3220 |              "                         so the new transaction will not be explicitly bip-125 replaceable (though it may\n"
3221 |              "                         still be replaceable in practice, for example if it has unconfirmed ancestors which\n"
3222 |              "                         are replaceable)."},
3223 | +                            {"change_address", RPCArg::Type::STR_HEX, /* opt */ true, /* default_val */ "", "The change address to pay the fee bump"},
    MarcoFalke commented at 11:50 PM on February 12, 2019: 
                                {"change_address", RPCArg::Type::STR_HEX, RPCArg::Optional::OMITTED_NAMED_ARG, "The change address to pay the fee bump"},

    Better: Mention what the default/fallback is

    promag commented at 3:32 PM on February 25, 2019:

    Done, please review new text.

  16. promag force-pushed on Feb 25, 2019
  17. DrahtBot removed the label Needs rebase on Feb 25, 2019
  18. DrahtBot added the label Needs rebase on Mar 2, 2019
  19. rpc: Support specifying change address in bumpfee 4d4be4bc2d
  20. qa: Test change_address option in bumpfee 7e5b136ddc
  21. doc: Add release note for new change_address option in bumpfee 0843bde34e
  22. promag force-pushed on Mar 6, 2019
  23. DrahtBot removed the label Needs rebase on Mar 6, 2019
  24. kallewoof commented at 12:37 AM on April 8, 2019: member

    Should I close #12096 then? I'm confused why this needed a new PR, aside from me not being good at review-begging.

  25. luke-jr commented at 7:50 AM on April 17, 2019: member

    I think the approach in #12096 is better:

    • I think we should move away from the idea that change has an address.
    • It allows reducing destination (non-ismine) outputs without as much risk of accidentally doing so.

    However, if there is currently no change address, some way may be desired to specify one explicitly?

  26. promag commented at 9:49 AM on April 22, 2019: member

    This should verify that the selected change output is in the wallet. Bumpfee is intentionally constructed so you can't accidentally rip off the payee using it. @gmaxwell there is no such check when specifying change address with fundrawtransaction, so why do it here? @luke-jr I think this API is preferable for those that use fundrawtransaction with custom change address. I don't consider this and #12096 exclusive, but happily close if #12096 is preferable. @kallewoof like @luke-jr said above, if there is no change output and an input must be added then a change output could be also added - which would use the specified change.

  27. kallewoof commented at 4:33 AM on April 24, 2019: member

    @promag I didn't realize this was a separate fix from #12096. I'll leave it up.

  28. promag commented at 2:19 PM on October 10, 2019: member

    Ping.

  29. DrahtBot closed this on Mar 9, 2020
  30. DrahtBot commented at 8:34 PM on March 9, 2020: member

    <!--5d09a71f8925f3f132321140b44b946d-->The last travis run for this pull request was 364 days ago and is thus outdated. To trigger a fresh travis build, this pull request should be closed and re-opened.

  31. DrahtBot reopened this on Mar 9, 2020
  32. DrahtBot added the label Needs rebase on Mar 10, 2020
  33. DrahtBot commented at 8:12 PM on March 10, 2020: member

    <!--cf906140f33d8803c4a75a2196329ecb-->

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

  34. promag commented at 9:05 PM on March 10, 2020: member

    Closing due to lack of interest.

  35. promag closed this on Mar 10, 2020
  36. promag deleted the branch on Mar 10, 2020
  37. promag restored the branch on Mar 10, 2020
  38. promag deleted the branch on Mar 10, 2020
  39. promag restored the branch on Mar 10, 2020
  40. DrahtBot locked this on Feb 15, 2022
Contributors
promagDrahtBotlaanwjgmaxwelljonasschnellipracticalswiftMarcoFalkekallewoofluke-jr
Labels
WalletNeeds rebase
Linked (view graph)
#15331 Unable use replace-by-fee if i use my own address for change
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-04-21 15:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me