wallet: fix BnB selection upper bound #28395

furszy · 2023-09-02T22:57:54Z

As BnB is an algorithm specialized in seeking changeless solutions, the
final selection amount (the sum of all the selected UTXOs amount) can be
as high as one unit below the amount where the transaction creation
process starts creating a change output. Which is: target + min_viable_change - 1.

In other words, the range of the possible solutions is:
target <= solution_amount < target + min_viable_change - 1.

Code level description:
We have a discrepancy on the threshold at which we create, or not, "change" between the BnB
algorithm and the, higher level, transaction creation process.

BnB selection upper bound uses cost_of_change, while the transaction creation process uses
the min_viable_change variable to obtain the resulting change amount from the coin selection
solution (here).

Essentially, this means that under certain circumstances; when the BnB solution excess is
in-between min_viable_change and cost_of_change, the BnB algorithm returns a
successful solution which, under the transaction creation process perspective, requires to create
change output. This, isn't an expected behavior as BnB is specialized to only return a changeless
solution.

This can happen when min_viable_change <= BnB_solution_amount - target < cost_of_change.

Note:
This should solve the issue presented in #28372.

Testing Note:
I have decoupled the test in a standalone commit so it can be easily cherry-picked on top of master to
verify the test failure vs the test passing here after the changes.

DrahtBot · 2023-09-02T22:57:56Z

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	brunoerg

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

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

brunoerg · 2023-09-02T23:35:39Z

Concept ACK

brunoerg · 2023-09-02T23:54:05Z

Cherry-picked this commit to test it with #28372 and did the changes to the harness to fit it. Previous crashes don't fail anymore, will leave it running overnight!

diff --git a/src/wallet/test/fuzz/coinselection.cpp b/src/wallet/test/fuzz/coinselection.cpp
index 07bb0f001c..2c96c79ea2 100644
--- a/src/wallet/test/fuzz/coinselection.cpp
+++ b/src/wallet/test/fuzz/coinselection.cpp
@@ -125,9 +125,9 @@ FUZZ_TARGET(coinselection)
     }
 
     // Run coinselection algorithms
-    auto result_bnb = SelectCoinsBnB(group_pos, target, coin_params.m_cost_of_change, MAX_STANDARD_TX_WEIGHT);
+    auto result_bnb = SelectCoinsBnB(group_pos, target, coin_params.min_viable_change, MAX_STANDARD_TX_WEIGHT);
     if (result_bnb) {
-        assert(result_bnb->GetChange(coin_params.m_cost_of_change, CAmount{0}) == 0);
+        assert(result_bnb->GetChange(coin_params.min_viable_change, CAmount{0}) == 0);
         assert(result_bnb->GetSelectedValue() >= target);
         (void)result_bnb->GetShuffledInputVector();
         (void)result_bnb->GetInputSet();

brunoerg · 2023-09-03T00:23:51Z

cc: @murchandamus

maflcko · 2023-09-03T07:52:46Z

/bip324_ecdh.cpp
clang-tidy-16 -p=/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu -quiet -load=/tidy-build/libbitcoin-tidy.so /ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/coins.cpp
clang-tidy-16 -p=/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu -quiet -load=/tidy-build/libbitcoin-tidy.so /ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/wallet/test/coinselector_tests.cpp
wallet/test/coinselector_tests.cpp:434:63: error: argument name 'cost_of_change' in comment does not match parameter name 'min_viable_change' [bugprone-argument-comment,-warnings-as-errors]
                                            selection_target, /*cost_of_change=*/0, MAX_STANDARD_TX_WEIGHT);
                                                              ^
./wallet/coinselection.h:412:131: note: 'min_viable_change' declared here
util::Result<SelectionResult> SelectCoinsBnB(std::vector<OutputGroup>& utxo_pool, const CAmount& selection_target, const CAmount& min_viable_change,
                                                                                                                                  ^
--

brunoerg · 2023-09-03T10:21:00Z

It needs to change unit tests and fuzz.

…ound As BnB is an algorithm specialized in seeking changeless solutions, the final selection amount (the sum of all the selected UTXOs amount) can be as high as one unit below the amount where the transaction creation process starts creating a change output. Which is: target + min_viable_change - 1. In other words, the range of the possible solutions is: target <= solution_amount < target + min_viable_change - 1.

The following cases are covered: 1) The only available solution contain an excess equal to min_viable_change. The expected behavior here is BnB search to fail. Because, if BnB would return a valid solution, the transaction creation process would create a change output (which is what we don't expect from a BnB solution). 2) The only available solution contain an excess one unit below the min_viable_change. The expected behavior here is BnB search returning a successful solution.

furszy

Updated the PR and its description. Added test coverage in a standalone commit so it can easily be cherry-picked on top of master to verify the failure.

Also, about the changes.
I wouldn't be against continue using cost_of_change on the BnB upper bound. I just didn't do it because that would require further modifications at the transaction creation process level rather than at the coin selection algorithm level. But, happy to discuss this possible approach too.

achow101 · 2023-09-07T19:27:34Z

I don't think this is correct. The upper bound is the cost of change, not the minimum viable change. The minimum viable change is the minimum we're willing to make and this is not inherently tied to feerates. Cost of change is tied to the current feerate and the long term feerate. The minimum viable change is generally much higher than the cost of change. This PR will result in BnB solutions throwing away far more money.

src/wallet/coinselection.cpp

murchandamus · 2023-09-07T19:41:59Z

Cost of change is tied to the current feerate and the long term feerate.

If I recall correctly, the Bitcoin Core definition of cost_of_change is tied to the discard_feerate for the input, rather than the LTFRE

furszy

I don't think this is correct. The upper bound is the cost of change, not the minimum viable change. The minimum viable change is the minimum we're willing to make and this is not inherently tied to feerates. Cost of change is tied to the current feerate and the long term feerate. The minimum viable change is generally much higher than the cost of change. This PR will result in BnB solutions throwing away far more money.

Ok, will expand. Thats partially correct. Check the following. It was all taken from the sources.

"cost of change" is computed by (link):

change_fee = effective_feerate.GetFee(change_output_size);
change_spend_fee = discard_feerate.GetFee(change_spend_size)
cost_of_change = change_spend_fee + change_fee;

And, the "minimum viable change" is computed by (link):

dust = GetDustThreshold(change_prototype_txout, discard_feerate);
change_spend_fee = discard_feerate.GetFee(change_spend_size);
min_viable_change = std::max(change_spend_fee + 1, dust);

So, as generally change_spend_fee > dust. We can say that

min_viable_change = change_spend_fee + 1

Which means that, the minimum viable change is lower than cost of change. Exactly by change_fee - 1:

cost_of_change = min_viable_change + change_fee - 1

Therefore, conclusion is that cost_of_change > min_viable_change.

Now, let's look at the two possible GetChange() outcomes. Using the outputs' effective value vs SFFO.

Using the outputs' effective value:
GetChange() expands min_viable_change in change_fee, which makes min_viable_change > cost_of_change. Exactly by 1.
Using SFFO:
GetChange() uses min_viable_change as is. Keeping the cost_of_change > min_viable_change inequality. The diff is exactly change_fee - 1.

So, it means that.. we are both correct.
The first scenario describes what you are saying, while the second one describes what I'm saying. Essentially, BnB, in master, can currently throw up to change_fee - 1 to the change output during SFFO.

Will work on this. Thanks.

murchandamus · 2023-09-11T16:05:47Z

I am confused by your conclusion

So, as generally change_spend_fee > dust.

According to GetDustThreshold(…), dust = (output_size + input_size) × discard_feerate while change_spend_fee is only change_spend_size × discard_feerate. So shouldn’t that rather be:

change_spend_fee < dust

furszy

Thanks Murch. Summary of the off-github convo:

I am confused by your conclusion

So, as generally change_spend_fee > dust.

According to GetDustThreshold(…), dust = (output_size + input_size) × discard_feerate while change_spend_fee is only change_spend_size × discard_feerate. So shouldn’t that rather be:

change_spend_fee < dust

Yeah. Thats true for the common change output types (p2pkh and p2wpkh), dust is greater than change_spend_fee. But, for p2sh and p2wsh the change output redeem script size can turn around the inequality. GetDustThreshold input size calculation is fixed, while change_spend_fee uses a dummy input mimicking the real input size.

So.. the conclusion that I had above is not accurate. As regular output types are mostly used in the change output, the bare min_viable_change is usually higher than cost_of_change. Unless the effective feerate is high enough. But, that is a no-prob, because GetChange expands min_viable_change by change_fee which covers for the difference (change_fee is the field linked to the effective feerate).

As far as I can deduce now, the only scenario where min_viable_change could be lower than cost_of_change is when SFFO is enabled and the effective feerate is high enough. Because BnB upper bound includes cost_of_change, which includes change_fee (the cost of the output). And.. we don't extend min_viable_change in ´GetChange´ when SFFO is enabled. So, change_fee is not part of the min_viable_change expansion. Which ends up with a BnB result requiring a change output.

Apologies if this looks a bit intricate; it's a subject that really drills down into a niche within a niche..

murchandamus · 2023-09-12T14:41:19Z

Given that we had so much confusion on the exact details of this, maybe we should add tests that confirm our understanding of all the edge cases here.

Yet again, it is revealed that SFFO is the root of all evil in Bitcoin Core wallet. ;)

furszy

Given that we had so much confusion on the exact details of this, maybe we should add tests that confirm our understanding of all the edge cases here.

Sure. Made a test for this here furszy@23333b4.
As the test is on top of master. It is failing outputting: "Error: BnB selection produced a change output!".

Now.. will spend some time thinking on an elegant way to solve this. The simplest one, and the more targeted one, would be to decrease the cost of change by change_fee for the BnB selection function argument. But there might be other ways to fix it.

furszy

Closing per discussion. Instead of modifying BnB upper bound (as is proposed here), there was consensus on disabling BnB when SFFO is enabled. @murchandamus should be pushing a new PR shortly.
The unit test from my previous comment can still be used there. Feel free to cherry-pick the following branch commits (containing a slightly different version of my previous test, using the logs instead of adding a new return value). This test branch fails without the bugfix and will pass with it.

DrahtBot added the Wallet label Sep 2, 2023

brunoerg mentioned this pull request Sep 2, 2023

fuzz: improve coinselection #27585

Merged

DrahtBot mentioned this pull request Sep 3, 2023

Fix waste calculation in SelectionResult #28366

Open

DrahtBot added the CI failed label Sep 3, 2023

maflcko added this to the 26.0 milestone Sep 3, 2023

furszy force-pushed the 2023_coinselection_fix_bnb_upper_bound branch from 1167ab9 to 62dca3c Compare September 3, 2023 14:12

furszy changed the title ~~wallet: bugfix, use min viable change on BnB selection upper bound~~ wallet: fix BnB selection upper bound Sep 3, 2023

furszy added 2 commits September 3, 2023 12:47

furszy force-pushed the 2023_coinselection_fix_bnb_upper_bound branch from 62dca3c to f81047b Compare September 3, 2023 15:49

furszy commented Sep 3, 2023

View reviewed changes

DrahtBot removed the CI failed label Sep 5, 2023

murchandamus reviewed Sep 7, 2023

View reviewed changes

src/wallet/coinselection.cpp Show resolved Hide resolved

furszy commented Sep 8, 2023

View reviewed changes

furszy commented Sep 11, 2023

View reviewed changes

furszy commented Sep 12, 2023

View reviewed changes

furszy commented Oct 1, 2023

View reviewed changes

furszy closed this Oct 1, 2023

furszy mentioned this pull request Oct 5, 2023

fuzz: coinselection, improve min_viable_change/change_output_size #28372

Merged

dergoegge mentioned this pull request Nov 20, 2023

fuzz, coinselection: Assertion 'result_bnb->GetChange(coin_params.m_cost_of_change, CAmount{0}) == 0' failed #28918

Closed

furszy mentioned this pull request Dec 6, 2023

wallet: skip BnB when SFFO is enabled #28994

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wallet: fix BnB selection upper bound #28395

wallet: fix BnB selection upper bound #28395

furszy commented Sep 2, 2023 •

edited

DrahtBot commented Sep 2, 2023 •

edited

brunoerg commented Sep 2, 2023 •

edited

brunoerg commented Sep 2, 2023 •

edited

brunoerg commented Sep 3, 2023

maflcko commented Sep 3, 2023

brunoerg commented Sep 3, 2023

furszy left a comment

achow101 commented Sep 7, 2023

murchandamus commented Sep 7, 2023

furszy left a comment •

edited

murchandamus commented Sep 11, 2023

furszy left a comment

murchandamus commented Sep 12, 2023 •

edited

furszy left a comment

furszy left a comment •

edited

wallet: fix BnB selection upper bound #28395

wallet: fix BnB selection upper bound #28395

Conversation

furszy commented Sep 2, 2023 • edited

DrahtBot commented Sep 2, 2023 • edited

Reviews

Conflicts

brunoerg commented Sep 2, 2023 • edited

brunoerg commented Sep 2, 2023 • edited

brunoerg commented Sep 3, 2023

maflcko commented Sep 3, 2023

brunoerg commented Sep 3, 2023

furszy left a comment

Choose a reason for hiding this comment

achow101 commented Sep 7, 2023

murchandamus commented Sep 7, 2023

furszy left a comment • edited

Choose a reason for hiding this comment

murchandamus commented Sep 11, 2023

furszy left a comment

Choose a reason for hiding this comment

murchandamus commented Sep 12, 2023 • edited

furszy left a comment

Choose a reason for hiding this comment

furszy left a comment • edited

Choose a reason for hiding this comment

furszy commented Sep 2, 2023 •

edited

DrahtBot commented Sep 2, 2023 •

edited

brunoerg commented Sep 2, 2023 •

edited

brunoerg commented Sep 2, 2023 •

edited

furszy left a comment •

edited

murchandamus commented Sep 12, 2023 •

edited

furszy left a comment •

edited