#36255 Refactor CoinGrinder tests
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #36255 Refactor CoinGrinder tests | CI failing | 0 | Mild | 0 + 194 tests | |||
|
|
|
|
| |||
Reviewability: ReadyStalePaused Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs. Size: lines added or modified outside tests, then in tests; greener = smaller. | |||||||
Goal
This PR migrates CoinGrinder tests from the legacy `coinselector_tests.cpp` to `coinselection_tests.cpp`. The migrated tests use effective values and realistic feerates instead of synthetic parameters, and simplifies the test for search exhaustion.
Problem: CoinGrinder unit tests were residing in the older coin selector test suite using obsolete dummy parameters that do not reflect production coin selection behavior, fragmenting wallet coin selection testing across multiple files.
Reviewability
Ready · CI failing
CI is red; check whether the failure is the PR's before a deep review.
Waiting on (from the thread read):
- author: author response to l0rinc's test restructuring feedback and yancyribbens' parameter questions 2026-09-15
Agreement
Mild
objection that needs an answer has none yet (l0rinc)
Thread notes: Zeegaths agreed to open a separate PR for the TestSRDSuccess bugs, and yancyribbens agreed to review it there rather than holding up this PR, accepting both objections and the related question.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| l0rinc (member) | question, maintenance | Whether default_cs_params can be removed as well now | open | answered | no | 2026-09-14: 'Here we can remove the `default_cs_params` as well now, right?' |
| l0rinc (member) | suggestion, maintenance | Simplify the input equivalence helper using std::ranges::is_permutation or direct sorted comparison | open | answered | no | 2026-09-14: 'The previous implementation was indeed hard to understand. This is basically a permutation check of a subset of fields, and since we have very few elements here (given it's test code), we could either' |
| l0rinc (member) | suggestion, maintenance | Remove is_eff_value altogether since every caller uses effective values | open | answered | no | 2026-09-15: 'Since every caller uses effective values, could we remove `is_eff_value` altogether?' |
| l0rinc (member) | objection, maintenance | Helper assertions obscure test failure line numbers and prevent test scenarios from running independently | open | answered | no | 2026-09-15: 'Assertions inside these helpers report the helper's source line, so test_title has to identify the individual scenario. Could we organize the scenarios into named Boost test cases and share the repeat' |
| l0rinc (member) | suggestion, maintenance | Drop expected_amount, its accumulation, and its assertion since HaveEquivalentInputs already compares input values | open | answered | no | 2026-09-15: '`HaveEquivalentInputs` already compares the input values, and `GetSelectedValue()` just sums them. Could we drop `expected_amount`, its accumulation, and this assertion?' |
| l0rinc (member) | objection, correctness | Floating-point arithmetic followed by conversion to CAmount risks imprecision compared to integer satoshi amounts | open | answered | no | 2026-09-15: 'Could we use integer amounts throughout the migrated tests, e.g. 33 * CENT here and 2'533 * CENT for the target below? This expresses the intended satoshi amounts directly and avoids floating-point mu' |
| l0rinc (member) | suggestion, maintenance | Allow AddDuplicateCoins to take an optional input size so four loops become four helper calls | open | answered | no | 2026-09-15: 'Could `AddDuplicateCoins` take an optional input size so these four loops become four calls to the existing helper?' |
| l0rinc (member) | suggestion, maintenance | Build expected_inputs once and initialize utxo_pool from it before adding heavier clones to avoid duplicate coin constructions | open | answered | no | 2026-09-15: 'We could also build `expected_inputs` once and initialize `utxo_pool` from it before adding the heavier clones, which avoids constructing the four expected coins twice.' |
| yancyribbens (contributor) | suggestion, style | Clarify comment explaining input equivalence regardless of prevouts | open | answered | no | 2026-09-15: 'Two results are equivalent if they are composed of inputs with the same amounts and sizes, even if they are composed from different outputs (i.e., different prevouts).' |
| yancyribbens (contributor) | suggestion, maintenance | Use real values for max_selection_weight like 400,000 | open | answered | no | 2026-09-15: 'This is a strange max_selection_weight. It would be better to use real values, like max transaction weight of of 400_000 I think, although that's unrelated to this change.' |
| yancyribbens (contributor) | question, maintenance | Why a set and target of 40000 was chosen rather than 400000 | open | answered | no | 2026-09-15: 'Unrelated to this change, but why choose a set and target of 40000? I had thought at first this was 400000 which would make more sense, so I wonder if this was a typo.' |
| Zeegaths (none) | objection, correctness | TestSRDSuccess uses utxo_pool instead of mixed_weight_pool, so the tie-break is not checked and passes even when #33223 is reverted | accepted · Zeegaths agreed to address this in a separate PR | answered | by others | 2026-10-30: 'TestSRDSuccess calls utxo_pool instead of the mixed_weight_pool it builds. As a result, the tie-break test passes even when #33223 is reverted. There's nothing checking the tie-break.' Status from: 2026-11-01: Zeegaths: 'sure, I have a draft; I wanted to check before opening a PR' |
| Zeegaths (none) | objection, correctness | Coin eviction never happens in TestSRDSuccess because max weight fits any two coins | accepted · Zeegaths agreed to address this in a separate PR | answered | by others | 2026-10-30: 'Also coin eviction never happens because the current max weight (804 WU) fits any two coins (544 WU).' Status from: 2026-11-01: Zeegaths: 'sure, I have a draft; I wanted to check before opening a PR' |
| yancyribbens (contributor) | question, correctness | Whether TestSRDSuccess fails to test eviction because the target only takes two UTXOs to reach, and whether it should assert the resulting UTXOs on success | accepted · yancyribbens agreed to handle this in Zeegaths' separate PR | answered | by others | 2026-10-31: 'Looking a little closer at the test, I think there is more wrong than simply it should be `mixed_weight_pool` instead of `utxo_pool`.' Status from: 2026-11-01: yancyribbens: 'I will take a look if you want to open a PR to fix the test.' |
Support:
- yancyribbens (contributor) (Concept ACK, 2026-09-15): It would be nice to see the coinselector test file removed entirely in the future since having multiple test files has caused confusion.
- l0rinc (member) (Concept ACK, 2026-09-15) [read: src/wallet/test/coinselection_tests.cpp]: supports migration and provided branch with test simplification ideas
Participants:
- l0rinc (member, objection, 8 statements 2026-09-14 to 2026-09-15): Concept ACKed migration, but requested restructuring test cases to run independently, using integer amounts, and simplifying helpers
- yancyribbens (contributor, support, 7 statements 2026-09-15 to 2026-11-01): Concept ACKed migration; questioned TestSRDSuccess and agreed to review a separate PR for it
- Zeegaths (none, objection, 2 statements 2026-10-30 to 2026-11-01): pointed out bugs in TestSRDSuccess, but agreed to address them in a separate PR
Dependencies
Enables:
- Eventual removal of legacy coinselector_tests.cpp
Files
468 lines under test/bench/ci.
- src/wallet/test/coinselector_tests.cpp +0/-253
- src/wallet/test/coinselection_tests.cpp +194/-21
Card
The short text the ranking pass compares across PRs.
PR #36255 migrates the CoinGrinder unit tests from the older coinselector_tests.cpp into coinselection_tests.cpp, updating them to test with realistic feerates and effective values. This improves test suite maintainability and helps move toward retiring the redundant legacy coin selection test file. Reviewers support the concept with helpful code simplifications proposed, though CI is currently failing on head.
Data
thread read through 2026-11-01 (18 statements), head 08ee40f1de
labels: CI failed · draft
Categories
Wallet
P3 · test coverage
Migrates CoinGrinder tests from the legacy suite to use realistic effective values and feerates rather than synthetic parameters. This improves test reliability and works toward consolidating coin selection testing, but it is an internal test refactor reasonably deferrable behind functional fixes.
Membership: Modifies `src/wallet/test/coinselector_tests.cpp` and `src/wallet/test/coinselection_tests.cpp` to modernize CoinGrinder coin selection unit tests.
Factors: security/stability 0, bug 0, performance 0, user value 0, leverage 1