wallet: add coin selection parameter add_excess_to_recipient_position for changeless txs with excess that would be added to fees #30080

1. 
   remyers commented at 2:00 pm on May 10, 2024: contributor

   This PR replaces draft PR 29442 as a simpler alternative that only adds new coin selection parameters, primarily add_excess_to_recipient_position.

   I am opening this PR as a draft to get feedback and suggestions on the concept and my implementation.

   I have also included two additional coin selection parameters to this PR for specifying the target change amount and for disabling different coin selection algorithms. I can break them out into individual PRs if the primary excess amount coin selection option would be easier to review on its own. These additional parameters can also help lower fees for the LSP liquidity provider use case.

   motivation

   A changeless transaction may select inputs with excess value over what is needed to reach the desired fee rate and output targets. Currently that excess value is considered waste and burned as fees. In some situations you may prefer to add any excess value to an output you control. When fees are high the cost of adding a change output increases and so does the amount of potential excess value spent as fees. One example of a situation where excess value should be added to the output amount is when splicing in value to a Lightning channel because the excess value is retained off-chain.

2. wallet: Add coin control option "change_target"

   Add a coin control rpc parameter to optionally force a particular change target for coin selection algorithms that result in a change output.

   c9c8b09878
3. 
   DrahtBot commented at 2:00 pm on May 10, 2024: contributor

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

   Code Coverage

   For detailed information about the code coverage, see the test coverage report.

   Reviews

   See the guideline for information on the review process. A summary of reviews will appear here.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #30050 (refactor, wallet: get serialized size of CRecipients directly by josibake)
   • #29523 (Wallet: Add max_tx_weight to transaction funding options (take 2) by ismaelsadeeq)
   • #28366 (Fix waste calculation in SelectionResult by murchandamus)

   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.

4. DrahtBot renamed this:
   wallet: add coin selection parameter `add_excess_to_recipient_position` for changeless txs with excess that would be added to fees
   wallet: add coin selection parameter `add_excess_to_recipient_position` for changeless txs with excess that would be added to fees
   on May 10, 2024
5. DrahtBot added the label Wallet on May 10, 2024
6. DrahtBot added the label CI failed on May 10, 2024
7. 
   DrahtBot commented at 3:30 pm on May 10, 2024: contributor

   🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

   Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

   Leave a comment here, if you need help tracking down a confusing failure.

   Debug: https://github.com/bitcoin/bitcoin/runs/24823503231

8. 
   brunoerg commented at 6:29 pm on May 10, 2024: contributor

   From CI:

    0291/305 - 
    1rpc_help.py
    2 failed, Duration: 1 s
    3stdout:
    42024-05-10T15:06:20.992000Z TestFramework (INFO): PRNG seed is: 4539310303736745094
    52024-05-10T15:06:20.998000Z TestFramework (INFO): Initializing test directory /ci_container_base/ci/scratch/test_runner/test_runner_₿_🏃_20240510_150004/rpc_help_7
    62024-05-10T15:06:21.349000Z TestFramework (ERROR): Assertion failed
    7Traceback (most recent call last):
    8  File "/ci_container_base/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 132, in main
    9    self.run_test()
   10  File "/ci_container_base/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/rpc_help.py", line 54, in run_test
   11    self.test_client_conversion_table()
   12  File "/ci_container_base/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/rpc_help.py", line 72, in test_client_conversion_table
   13    raise AssertionError("RPC client conversion table ({}) and RPC server named arguments mismatch!\n{}".format(
   14AssertionError: RPC client conversion table (/ci_container_base/ci/scratch/build/bitcoin-i686-pc-linux-gnu/src/rpc/client.cpp) and RPC server named arguments mismatch!
   15{('walletcreatefundedpsbt', 3, 'add_excess_to_recipient_position'), ('fundrawtransaction', 1, 'add_excess_to_recipient_position'), ('walletcreatefundedpsbt', 3, 'disable_algos'), ('send', 4, 'add_excess_to_recipient_position')}
   162024-05-10T15:06:21.401000Z TestFramework (INFO): Stopping nodes
   172024-05-10T15:06:21.503000Z TestFramework (WARNING): Not cleaning up dir /ci_container_base/ci/scratch/test_runner/test_runner_₿_🏃_20240510_150004/rpc_help_7
   182024-05-10T15:06:21.503000Z TestFramework (ERROR): Test failed. Test logging available at /ci_container_base/ci/scratch/test_runner/test_runner_₿_🏃_20240510_150004/rpc_help_7/test_framework.log
   192024-05-10T15:06:21.503000Z TestFramework (ERROR): 
   202024-05-10T15:06:21.503000Z TestFramework (ERROR): Hint: Call /ci_container_base/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/combine_logs.py '/ci_container_base/ci/scratch/test_runner/test_runner_₿_🏃_20240510_150004/rpc_help_7' to consolidate all logs
   212024-05-10T15:06:21.503000Z TestFramework (ERROR): 
   222024-05-10T15:06:21.503000Z TestFramework (ERROR): If this failure happened unexpectedly or intermittently, please file a bug and provide a link or upload of the combined log.
   232024-05-10T15:06:21.504000Z TestFramework (ERROR): https://github.com/bitcoin/bitcoin/issues
   242024-05-10T15:06:21.504000Z TestFramework (ERROR): 
   

9. mozz30-tech approved
10. 
    Saraeutsza commented at 0:21 am on May 13, 2024: none

    
11. Theschorpioen approved
12. 
    S3RK commented at 7:55 am on May 13, 2024: contributor

    Here are my initial thoughts on each of the options individually

    change_target IIUC you want a specific min value for the change UTXO. Would a following alternative work? You provide your change address in the list of recipients with desired min amount and specify it as change UTXO using change_positionparameter. The downside compared to a new option is that we always have a change output. Not sure if that’s a problem in your case.

    disable_algos This makes sense, and could be a first step to pluggable coin selection in core. But it should be in settings, not in RPC parameters. I doubt many users need to change it at runtime and so it’s better to keep the RPC parameters to a minimum.

    add_excess_to_recipient_position This options seems like a compromise that makes nobody happy. From your perspective you’re okay to overfund a channel, but you have no control by how much, since excess is not configurable. From the perspective of users not doing splicing, this option is not useful at all.

    It seems like what you really want is to search for a changeless solution in a given target range. And then if failed to fallback to a solution with change.

13. bitcoin deleted a comment on May 13, 2024
14. 
    remyers commented at 12:54 pm on May 13, 2024: contributor

    Thanks for the feedback! I agree that it is worth considering how to make these parameters more useful for general users.

    change_target IIUC you want a specific min value for the change UTXO. Would a following alternative work? You provide your change address in the list of recipients with desired min amount and specify it as change UTXO using change_positionparameter. The downside compared to a new option is that we always have a change output. Not sure if that’s a problem in your case.

    Always having a change output would be a problem because we always prefer a changeless tx if the excess is not too large. But when that is not possible the change should be large enough to potentially spend later as a changeless tx.

    A possible alternative would be to add a setting for CHANGE_LOWER rather than always use the hard coded value of 50,000 sats. It is probably not necessary for my use case to set the change_target every time we fund a tx; we can just make sure it is always above the minimum value we want for all of our UTXOs.

    disable_algos This makes sense, and could be a first step to pluggable coin selection in core. But it should be in settings, not in RPC parameters. I doubt many users need to change it at runtime and so it’s better to keep the RPC parameters to a minimum.

    I agree that generally it’s too in the weeds for most users. I’ll move it to settings.

    add_excess_to_recipient_position This options seems like a compromise that makes nobody happy. From your perspective you’re okay to overfund a channel, but you have no control by how much, since excess is not configurable. From the perspective of users not doing splicing, this option is not useful at all.

    Good point. The maximum excess currently built into BnB is cost_of_change, defined as the cost to create and spend a change output. This is an arbitrary value to use if you are happy to add some amount of excess to a changeless spend output. It would be better to also have a max_excess parameter that defaults to cost_of_change if not otherwise defined.

    It seems like what you really want is to search for a changeless solution in a given target range. And then if failed to fallback to a solution with change.

    Exactly! I believe that is how CS works now. If no set of inputs are found with excess in the range from zero to cost_of_change then BnB will fail and CS will fallback to a solution with change. But adding a new max_excess parameter would make it explicit what that range is.

    From the perspective of not over complicating the coin selection options, do you think it would be better to have two new options: add_excess_to_recipient_position and max_excess, OR just add max_excess and always default to add the excess to the first spending target?

15. remyers force-pushed on May 14, 2024
16. remyers force-pushed on May 15, 2024
17. remyers force-pushed on May 15, 2024
18. wallet: Add coin control option "enable_algos"

    A new rpc option to specify a list of enabled coin selection algorithms. If not set, then by default all algorithms are enabled.

    3154f833d7
19. remyers force-pushed on May 15, 2024
20. wallet: Add coin control option "add_excess_to_recipient_position"

    If this coin selection RPC option is set, then the excess value for a changeless BnB tx will be added to the selected recipient output position instead of added to fees.

    6f594a7d3e
21. remyers force-pushed on May 15, 2024
22. DrahtBot removed the label CI failed on May 15, 2024
23. 
    remyers commented at 7:33 am on May 16, 2024: contributor

    I changed disable_algos to enable_algos in 3154f83. This was suggested by @t-bast because it won’t cause problems for existing users when new cs algorithms are added. I believe I have the simplest and safest solution for initializing the m_enable_algos std::bitset to all true, but there are a few other ways to do it.
24. DrahtBot added the label CI failed on May 20, 2024
25. 
    DrahtBot commented at 11:16 am on May 20, 2024: contributor

    🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

    Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    Leave a comment here, if you need help tracking down a confusing failure.

    Debug: https://github.com/bitcoin/bitcoin/runs/25171023419

26. wallet: Add coin control option "max_excess"

    If set, excess from changeless spends can not exceed this amount, otherwise use cost_of_change by default

    1307d1ff44
27. remyers force-pushed on May 20, 2024
28. DrahtBot removed the label CI failed on May 20, 2024
29. 
    DrahtBot commented at 0:09 am on June 5, 2024: contributor

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

30. DrahtBot added the label Needs rebase on Jun 5, 2024

Contributors
remyers DrahtBot brunoerg mozz30-tech Saraeutsza Theschorpioen S3RK

Labels
Wallet Needs rebase