wallet: skip BnB when SFFO is enabled #28994

furszy · 2023-12-04T15:25:21Z

Solves #28918. Coming from #28918 (comment) discussion.

The intention is to decouple only the bugfix relevant commits from #28985, allowing them to be included in the 26.x release. This way, we can avoid disabling the coin selection fuzzing test for an entire release.

Note:
Have introduced few changes to the bug fix commit so that the unit tests pass without the additional burden introduced in #28985.

DrahtBot · 2023-12-04T15:25:25Z

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.

Type	Reviewers
ACK	josibake, murchandamus, achow101

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:

#28985 (Avoid changeless input sets when SFFO is active by murchandamus)
#25665 (refactor: Add util::Result failure values, multiple error and warning messages by ryanofsky)

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.

src/test/util/logging.h

src/wallet/test/coinselector_tests.cpp

src/test/util/logging.cpp

src/wallet/test/coinselector_tests.cpp

src/wallet/spend.cpp

murchandamus

Approach ACK: 233a07a

murchandamus · 2023-12-04T16:45:51Z

src/wallet/test/coinselector_tests.cpp

@@ -345,6 +348,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test)

        CoinsResult available_coins;

+        coin_selection_params_bnb.m_effective_feerate = CFeeRate(0);


Nit: This test was already a bit weird to me, because it is categorized as belonging to "BnB", but the composition of preset inputs and the selection of additional inputs is a SelectCoins-level functionality that has little to do with specific coin selection algorithms. Composability should be tested at SelectCoins level (which it is in spend_tests and wallet_fundrawtransaction.py, although it could probably be more extensive). Since we already tested whether we can select two UTXOs that exactly match an amount in absence of fees above, I expect that we could remove this test altogether without losing coverage.

achow101

Approach ACK

src/wallet/test/coinselector_tests.cpp

furszy · 2023-12-04T21:30:15Z

Updated per feedback. Thanks everyone.

Test wise; replaced the logs debugger approach for a low level function call (per #28994 (comment)). And applied
the test variables naming suggestions.

josibake

Looks good! I am uncomfortable with adding the log message in a4c3294 as it has nothing to do with the bug fix and I think it could confuse (or worse, alarm) users to see "Waste" in their logs in the context of coin selection when waste is a not well-documented, implementation-specific term. Can we drop a4c3294 for now and revisit debug logging in a separate PR, unrelated to the v26 release?

src/wallet/test/coinselector_tests.cpp

furszy · 2023-12-05T13:10:47Z

I am uncomfortable with adding the log message as it has nothing to do with the bug fix and I think it could confuse (or worse, alarm) users to see "Waste" in their logs in the context of coin selection when waste is a not well-documented, implementation-specific term. Can we drop this commit for now and revisit debug logging in a separate PR, unrelated to the v26 release?

Logs aren't meant for end users; logs are useful for us to understand what is going on internally. Debug issues and provide better feedback to our users. This is not different to what we do for the fee calculation. And there is more documentation about the selection waste than doc about each of the logged fee reasons actually.

Right now, the transaction creation process lacks of outputting this information (thus why we are forced to call a low level function to test the bug). Moreover, as coin selection isn't a deterministic process, the lack of this info makes debugging and helping users harder than what it should.

That being said, if you and others agree on removing the logging after reading the rationale I just wrote, could drop it too. I'm not that hard on it.

josibake · 2023-12-05T13:18:45Z

Logs aren't meant for end users

Strongly disagree with this statement, and regardless of who they are intended for, users (people who run nodes) have access to these logs and do look at them.

This is not different to what we do for the fee calculation. And there is more documentation about the selection waste than doc about each of the logged fee reasons actually.

This is irrelevant to my point that this is not a bug fix and shouldn't be included in a bug fix PR. I also don't agree with the justification and claim that coin selection is well-documented, but I'd rather leave that discussion for a PR related to improving wallet logging and keep this one focused on the bug fix for v26 :)

furszy · 2023-12-05T13:26:41Z

This is irrelevant to my point that this is not a bug fix and shouldn't be included in a bug fix PR. I also don't agree with the justification and claim that coin selection is well-documented.

Where did I claim that coin selection is well-documented? I said that it is more documented than fee estimation. And fee estimation is being logged.

Strongly disagree with this statement, and regardless of who they are intended for, users (people who run nodes) have access to these logs and do look at them.

Everyone have access. That doesn't mean that they are meant for everyone. Thus why we have logging domains and levels. Moreover, most logging domains are disabled by default. Including the wallet one.

josibake · 2023-12-05T13:34:17Z

Where did I claim that coin selection is well-documented?

Sorry, my comment should say s/well-documented/more documented/

furszy · 2023-12-05T13:46:25Z

Where did I claim that coin selection is well-documented?

Sorry, my comment should say s/well-documented/more documented/

np. Happens on any convo.

I'm not that strong on the new logging introduction, still, sharing the change rationale and opening the convo helps moving forward.

murchandamus

I realized that we need to add the input fee to each UTXO rather than deducting it from the target for the two input set options to have the same waste score.

src/wallet/spend.cpp

src/wallet/test/coinselector_tests.cpp

Co-authored-by: furszy <matiasfurszyfer@protonmail.com>

Useful for understanding what is going on internally when the software is running. Debug issues, and provide more accurate feedback to users.

furszy · 2023-12-08T01:03:48Z

Updated per feedback. Thanks.
Applied @murchandamus suggestion of adding inputs fee to each UTXO rather than deduct them from the target. The latter one was breaking the equivalence of the input sets.

Note:
I did not improve the test code organization on purpose. Made the smallest change-set possible to not interfere with #28985. (I just reused the hardcoded number for the p2wpkh input size, just like we do in several other places inside the coinselector_tests file).

Note 2:
Haven't introduced the "BnB never produces change" assertion because it requires further restructuring.
The responsibility of asserting that a certain coin selection algorithm did not produce change shouldn't lie at the wallet's tx creation level. Instead, it should be internal to the selection algorithm. However, to achieve this, we need to modify the way the "create change" decision is made. For instance, it would be good to introduce a field inside SelectionResult that expresses the coin selection algo's expectation in terms of producing change or not.

josibake

ACK 1d3bc77

Applied @murchandamus suggestion of adding inputs fee to each UTXO rather than deduct them from the target. The latter one was breaking the equivalence of the input sets.

Wasn't the test still passing before, when you were deducting them from the target? If so, I would suggest we take a closer look at this test in #28985 to ensure it's testing the thing it's supposed to be testing.

murchandamus

ACK 18669d7

Edit:
ACK 576bee8

murchandamus · 2023-12-12T02:50:58Z

src/wallet/test/coinselector_tests.cpp

+    add_coin(available_coins, *wallet, COIN + params.m_cost_of_change, /*feerate=*/params.m_effective_feerate, /*nAge=*/6, /*fIsFromMe=*/true, /*nInput=*/0, /*spendable=*/true);
+    add_coin(available_coins, *wallet, 0.5 * COIN + params.m_cost_of_change, /*feerate=*/params.m_effective_feerate, /*nAge=*/6, /*fIsFromMe=*/true, /*nInput=*/0, /*spendable=*/true);
+    add_coin(available_coins, *wallet, 0.5 * COIN, /*feerate=*/params.m_effective_feerate, /*nAge=*/6, /*fIsFromMe=*/true, /*nInput=*/0, /*spendable=*/true);
+    // Knapsack will only find a changeless solution on an exact match to the satoshi, SRD doesn’t look for changeless
+    // If BnB were run, it would produce a single input solution with the best waste score


Good, now I’d expect:

SRD randomly returning two or three of the inputs

Knapsack return { COIN+coc, 0.5×COIN }

BnB return { COIN+coc }

Since the feerate is higher than the LFTR, two or three inputs with change are worse than one input without change, even when dropping coc into fees. Ergo, BnB not being the first solution proves that BnB wasn’t run.

josibake · 2023-12-12T09:20:37Z

ACK 576bee8

josibake · 2023-12-12T09:21:28Z

ACK 18669d7

guessing you meant to ack 576bee8 ? 😉

murchandamus · 2023-12-12T13:31:28Z

Yes, ACK 576bee8

achow101 · 2023-12-12T15:38:27Z

ACK 576bee8

Co-authored-by: furszy <matiasfurszyfer@protonmail.com> Github-Pull: bitcoin#28994 Rebased-From: 5cea25b

Useful for understanding what is going on internally when the software is running. Debug issues, and provide more accurate feedback to users. Github-Pull: bitcoin#28994 Rebased-From: 0c57557

Verify the transaction creation process does not produce a BnB solution when SFFO is enabled. This is currently problematic because it could require a change output. And BnB is specialized on changeless solutions. Co-authored-by: Andrew Chow <achow101@gmail.com> Co-authored-by: Murch <murch@murch.one> Github-Pull: bitcoin#28994 Rebased-From: 05e5ff1

Github-Pull: bitcoin#28994 Rebased-From: 576bee8

fanquake · 2023-12-12T16:04:20Z

Added this to #29011 for backporting to 26.x.

fanquake · 2023-12-13T09:22:03Z

See #28918 (comment). This doesn't seem to have fixed the fuzzer.

furszy · 2023-12-13T12:11:34Z

See #28918 (comment). This doesn't seem to have fixed the fuzzer.

It fixed the initially reported one (#28918 (comment)). The latest one is different.

Co-authored-by: furszy <matiasfurszyfer@protonmail.com> Github-Pull: bitcoin#28994 Rebased-From: 5cea25b

Useful for understanding what is going on internally when the software is running. Debug issues, and provide more accurate feedback to users. Github-Pull: bitcoin#28994 Rebased-From: 0c57557

Verify the transaction creation process does not produce a BnB solution when SFFO is enabled. This is currently problematic because it could require a change output. And BnB is specialized on changeless solutions. Co-authored-by: Andrew Chow <achow101@gmail.com> Co-authored-by: Murch <murch@murch.one> Github-Pull: bitcoin#28994 Rebased-From: 05e5ff1

Github-Pull: bitcoin#28994 Rebased-From: 576bee8

7b79e54 doc: update release notes for 26.x (fanquake) ccf00b1 wallet: Fix use-after-free in WalletBatch::EraseRecords (MarcoFalke) 40252e1 ci: Set `HOMEBREW_NO_INSTALLED_DEPENDENTS_CHECK` to avoid failures (Hennadii Stepanov) b06b14e rpc: getwalletinfo, return wallet 'birthtime' (furszy) 1283401 test: coverage for wallet birth time interaction with -reindex (furszy) 0fa47e2 wallet: fix legacy spkm default birth time (furszy) 84f4a6c wallet: birth time update during tx scanning (furszy) 074296d refactor: rename FirstKeyTimeChanged to MaybeUpdateBirthTime (furszy) 35039ac fuzz: disable BnB when SFFO is enabled (furszy) 903b462 test: add coverage for BnB-SFFO restriction (furszy) 05d0576 wallet: create tx, log resulting coin selection info (furszy) 5493ebb wallet: skip BnB when SFFO is active (Murch) b15e2e2 test: add regression test for the getrawtransaction segfault (Martin Zumsande) 5097bb3 rpc: fix getrawtransaction segfault (Martin Zumsande) 81e744a ci: Use Ubuntu 24.04 Noble for asan (MarcoFalke) 69e53d1 ci: Use Ubuntu 24.04 Noble for tsan,tidy,fuzz (MarcoFalke) d2c80b6 doc: Missing additions to 26.0 release notes (fanquake) 8dc2c75 doc: add historical release notes for 26.0 (fanquake) Pull request description: Backports for `26.x`. Currently: * #28920 * #28992 * #28994 * #29003 * #29023 * #29080 * #29176 ACKs for top commit: TheCharlatan: ACK 7b79e54 glozow: ACK 7b79e54, matches mine Tree-SHA512: 898aec76ed3ad35e0edd0980af5bcc21bd60003bbf69e0b4f473ed2aa38c4e3b360b930bc3747cf798195906a8f9fe66417524f5e5ef40fa68f1c1aaceebdeb0

DrahtBot added the Wallet label Dec 4, 2023

maflcko reviewed Dec 4, 2023

View reviewed changes

src/test/util/logging.h Outdated Show resolved Hide resolved

fanquake reviewed Dec 4, 2023

View reviewed changes

src/wallet/test/coinselector_tests.cpp Outdated Show resolved Hide resolved

murchandamus reviewed Dec 4, 2023

View reviewed changes

maflcko reviewed Dec 4, 2023

View reviewed changes

src/wallet/spend.cpp Outdated Show resolved Hide resolved

murchandamus reviewed Dec 4, 2023

View reviewed changes

DrahtBot added the CI failed label Dec 4, 2023

achow101 reviewed Dec 4, 2023

View reviewed changes

src/wallet/test/coinselector_tests.cpp Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Dec 4, 2023

Avoid changeless input sets when SFFO is active #28985

Closed

furszy force-pushed the 2023_wallet_sffo_skip_bnb branch from 233a07a to 7e1bf0d Compare December 4, 2023 21:14

This was referenced Dec 4, 2023

test: add end-to-end tests for CConnman and PeerManager #26812

Open

refactor: Add util::Result failure values, multiple error and warning messages #25665

Open

DrahtBot removed the CI failed label Dec 5, 2023

josibake reviewed Dec 5, 2023

View reviewed changes

src/wallet/test/coinselector_tests.cpp Outdated Show resolved Hide resolved

furszy force-pushed the 2023_wallet_sffo_skip_bnb branch from 7e1bf0d to efb54a2 Compare December 5, 2023 13:55

murchandamus suggested changes Dec 5, 2023

View reviewed changes

src/wallet/spend.cpp Outdated Show resolved Hide resolved

src/wallet/test/coinselector_tests.cpp Outdated Show resolved Hide resolved

src/wallet/test/coinselector_tests.cpp Outdated Show resolved Hide resolved

murchandamus and others added 2 commits December 7, 2023 21:47

wallet: skip BnB when SFFO is active

5cea25b

Co-authored-by: furszy <matiasfurszyfer@protonmail.com>

wallet: create tx, log resulting coin selection info

0c57557

Useful for understanding what is going on internally when the software is running. Debug issues, and provide more accurate feedback to users.

furszy force-pushed the 2023_wallet_sffo_skip_bnb branch from efb54a2 to 1d3bc77 Compare December 8, 2023 00:52

josibake reviewed Dec 11, 2023

View reviewed changes

DrahtBot requested review from murchandamus and achow101 December 11, 2023 10:44

furszy force-pushed the 2023_wallet_sffo_skip_bnb branch from 18669d7 to 576bee8 Compare December 12, 2023 02:40

murchandamus reviewed Dec 12, 2023

View reviewed changes

DrahtBot requested review from murchandamus and removed request for josibake December 12, 2023 09:20

DrahtBot removed the request for review from murchandamus December 12, 2023 13:31

DrahtBot removed the request for review from achow101 December 12, 2023 15:38

achow101 added the Needs backport (26.x) label Dec 12, 2023

achow101 merged commit d646ca3 into bitcoin:master Dec 12, 2023

fanquake added this to the 26.1 milestone Dec 12, 2023

fanquake mentioned this pull request Dec 12, 2023

[26.x] Backports #29011

Merged

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Dec 12, 2023

wallet: skip BnB when SFFO is active

64da0d4

Co-authored-by: furszy <matiasfurszyfer@protonmail.com> Github-Pull: bitcoin#28994 Rebased-From: 5cea25b

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Dec 12, 2023

fuzz: disable BnB when SFFO is enabled

832b7cf

Github-Pull: bitcoin#28994 Rebased-From: 576bee8

fanquake removed the Needs backport (26.x) label Dec 12, 2023

furszy deleted the 2023_wallet_sffo_skip_bnb branch December 12, 2023 16:33

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Jan 4, 2024

wallet: skip BnB when SFFO is active

5493ebb

Co-authored-by: furszy <matiasfurszyfer@protonmail.com> Github-Pull: bitcoin#28994 Rebased-From: 5cea25b

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Jan 4, 2024

fuzz: disable BnB when SFFO is enabled

35039ac

Github-Pull: bitcoin#28994 Rebased-From: 576bee8

murchandamus mentioned this pull request Jan 8, 2024

test: Add algo assert to bnb_search_test #29206

Closed

bitcoin locked and limited conversation to collaborators Dec 12, 2024

		@@ -345,6 +348,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test)

		CoinsResult available_coins;

		coin_selection_params_bnb.m_effective_feerate = CFeeRate(0);

wallet: skip BnB when SFFO is enabled #28994

wallet: skip BnB when SFFO is enabled #28994

Uh oh!

Conversation

furszy commented Dec 4, 2023

Uh oh!

DrahtBot commented Dec 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

murchandamus left a comment

Choose a reason for hiding this comment

Uh oh!

murchandamus Dec 4, 2023

Choose a reason for hiding this comment

Uh oh!

achow101 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

furszy commented Dec 4, 2023

Uh oh!

josibake left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

furszy commented Dec 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

josibake commented Dec 5, 2023

Uh oh!

furszy commented Dec 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

josibake commented Dec 5, 2023

Uh oh!

furszy commented Dec 5, 2023

Uh oh!

murchandamus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

furszy commented Dec 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

josibake left a comment

Choose a reason for hiding this comment

Uh oh!

murchandamus left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

murchandamus Dec 12, 2023

Choose a reason for hiding this comment

Uh oh!

josibake commented Dec 12, 2023

Uh oh!

josibake commented Dec 12, 2023

Uh oh!

murchandamus commented Dec 12, 2023

Uh oh!

achow101 commented Dec 12, 2023

Uh oh!

fanquake commented Dec 12, 2023

Uh oh!

fanquake commented Dec 13, 2023

Uh oh!

DrahtBot commented Dec 4, 2023 •

edited

Loading

josibake left a comment •

edited

Loading

furszy commented Dec 5, 2023 •

edited

Loading

furszy commented Dec 5, 2023 •

edited

Loading

furszy commented Dec 8, 2023 •

edited

Loading

murchandamus left a comment •

edited

Loading