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

[remyers]

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.

[DrahtBot]

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.

[DrahtBot]

🚧 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}

[brunoerg]

From CI:

291/305 - 
rpc_help.py
 failed, Duration: 1 s
stdout:
2024-05-10T15:06:20.992000Z TestFramework (INFO): PRNG seed is: 4539310303736745094
2024-05-10T15:06:20.998000Z TestFramework (INFO): Initializing test directory /ci_container_base/ci/scratch/test_runner/test_runner_₿_🏃_20240510_150004/rpc_help_7
2024-05-10T15:06:21.349000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
  File "/ci_container_base/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 132, in main
    self.run_test()
  File "/ci_container_base/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/rpc_help.py", line 54, in run_test
    self.test_client_conversion_table()
  File "/ci_container_base/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/rpc_help.py", line 72, in test_client_conversion_table
    raise AssertionError("RPC client conversion table ({}) and RPC server named arguments mismatch!\n{}".format(
AssertionError: 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!
{('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')}
2024-05-10T15:06:21.401000Z TestFramework (INFO): Stopping nodes
2024-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
2024-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
2024-05-10T15:06:21.503000Z TestFramework (ERROR): 
2024-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
2024-05-10T15:06:21.503000Z TestFramework (ERROR): 
2024-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.
2024-05-10T15:06:21.504000Z TestFramework (ERROR): https://github.com/bitcoin/bitcoin/issues
2024-05-10T15:06:21.504000Z TestFramework (ERROR): 

[Saraeutsza]

[S3RK]

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.

[remyers]

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?

[remyers]

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.

[DrahtBot]

🚧 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}