Avoid changeless input sets when SFFO is active #28985
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
This was referenced Dec 2, 2023
18bd137 to
6dabce5
Compare
|
Rebased on top of #28994 |
achow101
added a commit
that referenced
this pull request
Dec 12, 2023
576bee8 fuzz: disable BnB when SFFO is enabled (furszy) 05e5ff1 test: add coverage for BnB-SFFO restriction (furszy) 0c57557 wallet: create tx, log resulting coin selection info (furszy) 5cea25b wallet: skip BnB when SFFO is active (Murch) Pull request description: 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. ACKs for top commit: josibake: ACK 576bee8 murchandamus: ACK 576bee8 achow101: ACK 576bee8 Tree-SHA512: f5d90eb3f3f524265afe4719495c9bf30f98b9af26cf039f7df5a7db977abae72caa7a3478cdd0ab10cd143bc1662e8fc5286b5bc10fc10f0dd582a45b45c31a
5b8c165 to
ad7e31c
Compare
ad7e31c to
832a384
Compare
Originally these tests verified that at a SelectCoins level the solution with fewer inputs gets preferred at high feerates, and the solution with more inputs gets preferred at low feerates. This actually relies on the behavior of BnB, so these tests are moved to the BnB tests. Originally these tests relied on SFFO to work.
- Testing that negative effective value filters a UTXO is a behavior of AvailableCoins and has nothing to do with BnB - Combining Preset inputs with additional inputs resulting from coin selection algorithms is a SelectCoins-level behavior and has nothing to do with BnB
Also removes `bnb_search_tests` from coinselector_tests altogether
The behavior of test_22670 needed to be amended as we no longer are searching for changeless solutions with knapsack on SFFO and thus the trick to calculating the fee for the change output no longer worked.
832a384 to
8b66c6e
Compare
This was referenced Jan 11, 2024
9415863 to
07b201c
Compare
07b201c to
4ce5ee6
Compare
This was referenced Jan 18, 2024
|
🐙 This pull request conflicts with the target branch and needs rebase. |
Merged
|
⌛ There hasn't been much activity lately and the patch still needs rebase. What is the status here?
|
achow101
added a commit
that referenced
this pull request
May 6, 2025
85368aa test: Run simple tests at various feerates (Murch) d610951 test: Recreate BnB iteration exhaustion test (Murch) 2a1b275 test: Remove redundant repeated test (Murch) 4781f5c test: Recreate simple BnB failure tests (Murch) a94030a test: Recreate BnB clone skipping test (Murch) 7db6f01 test: Move BnB feerate sensitivity tests (Murch) 2bafc46 test: Recreate simple BnB success tests (Murch) Pull request description: This PR is splitting off some of the improvements made in #28985 and starts addressing the issues raised in #27754. I aim to completely replace `coinselector_tests` with `coinselection_tests`. The goal is to generally use coins created per a nominal _effective value_ so we can get away from testing with `CoinSelectionParams` that are non-representative and effectuate counterintuitive behavior such as `feerate = 0` or `cost_of_change = 0` ACKs for top commit: achow101: ACK 85368aa monlovesmango: ACK 85368aa w0xlt: ACK 85368aa Tree-SHA512: 1a984837b4efddc0d8abe11668898fb207fb539e784bf911d4038211274b82e0fe1f8fffe7e5a19e0e013ccb7dc40e3f62d853a2a729980d0d935e66f12b9156
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes a bunch of issues around tests for coinselection, and disables changeless solutions when building transactions with SFFO.
Depends on #28994