remyers
commented at 2:00 pm on May 10, 2024:
contributor
This PR replaces draft PR 29442 as a simpler alternative that only adds two new coin selection parameters: add_excess_to_recipient_position and max_excess.
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
commented at 2:00 pm on May 10, 2024:
contributor
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
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 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
DrahtBot added the label
Wallet
on May 10, 2024
DrahtBot added the label
CI failed
on May 10, 2024
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.
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):
8File"/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()
10File"/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()
12File"/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):
mozz30-tech approved
Theschorpioen approved
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.
bitcoin deleted a comment
on May 13, 2024
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?
remyers force-pushed
on May 14, 2024
remyers force-pushed
on May 15, 2024
remyers force-pushed
on May 15, 2024
remyers force-pushed
on May 15, 2024
remyers force-pushed
on May 15, 2024
DrahtBot removed the label
CI failed
on May 15, 2024
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_algosstd::bitset to all true, but there are a few other ways to do it.
DrahtBot added the label
CI failed
on May 20, 2024
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.
DrahtBot removed the label
CI failed
on May 20, 2024
DrahtBot added the label
Needs rebase
on Jun 5, 2024
remyers
commented at 9:27 am on August 2, 2024:
contributor
May want to first do a PR to fix the related issue 26466 or resolve that issue if it has been fixed. ht @willcl-ark
remyers force-pushed
on Aug 23, 2024
DrahtBot removed the label
Needs rebase
on Aug 23, 2024
remyers force-pushed
on Aug 28, 2024
remyers force-pushed
on Sep 2, 2024
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.
The BnB algorithm with this option set will primarily select the input set with the least fee waste and excess value less than the cost of adding change. When comparing input sets with the same fee waste, the input set with the least excess value will be selected.
324281f686
wallet: Add coin control option "max_excess"
If set, excess from changeless spends can not exceed the lesser of this amount and the current cost_of_change, otherwise just use cost_of_change by default
17bc70f80a
remyers force-pushed
on Sep 4, 2024
remyers marked this as ready for review
on Sep 11, 2024
wallet: Update BnB upper bound to be consistent with tx building
Use min_viable_change instead of cost_of_change as the upper limit of excess allowed when computing a changeless BnB solution.
This prevents a corner case where dust threshold > change_fee results in some changeless BnB solutions not being considered, despite change being dropped during tx building.
h/t @S3RK for identifying this issue in #26466 .
7df63761eb
achow101 requested review from achow101
on Oct 15, 2024
achow101 requested review from murchandamus
on Oct 15, 2024
bitcoin deleted a comment
on Nov 4, 2024
achow101
commented at 9:16 pm on November 4, 2024:
member
It’s not entirely clear to me why BnB should care about whether the excess is going to the recipient or not. I don’t think that should change the algorithm at all, so the changes to SelectCoinsBnB seem unnecessary.
Additionally, this implementation modifies the target, and I think that’s rather dangerous to do. I think the obvious way to implement this feature is to just detect when the fee paid is greater than the fee needed, and send the excess to the recipient specified, just as we already do when there are change outputs. That would also really shrink the diff.
remyers
commented at 9:33 am on November 5, 2024:
contributor
I think the obvious way to implement this feature is to just detect when the fee paid is greater than the fee needed, and send the excess to the recipient specified, just as we already do when there are change outputs.
Thanks for the feedback. I agree and think for our use we can even do that diff outside of bitcoind. I’ll update the PR.
remyers
commented at 9:58 am on November 7, 2024:
contributor
It’s not entirely clear to me why BnB should care about whether the excess is going to the recipient or not. I don’t think that should change the algorithm at all, so the changes to SelectCoinsBnB seem unnecessary.
Imagine this scenario:
target is 100
max_excess is 30
UTXOs: [20, 20, 20, 20, 20, 120]
fee to spend an input: 4
minimum spendable: 10
Input set A: [20,20,20,20,20]
Input set B: [120]
Current BnB computes waste as:
Input set A: 20 (best)
Input set B: 24
This PR with max_excess = 30 computes waste as:
Input set A: 20
Input set B: 4 (best)
In both cases, CG would select input set B and compute waste as something like 8 (1 input, 1 output). Current CS would choose the Knapsack solution(waste = 8 < 20). After this PR CS should select the BnB solution (waste = 4 < 8).
A sender that retains excess would prefer the less costly BnB solution using input set A.
This PR allows a sender to find an optimal solution using CS when excess value is retained by the sender.
Another approach would be to not change BnB at all, but instead modify CS to detect that the best solution has a change output amount (and fee) that is less than our specified max_excess and modify the final tx to add the change output amount (and fee) to the target output.
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-21 09:12 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me
