test: add coin-grinder TOTAL

HouseOfHufflepuff commented at 11:25 pm on March 21, 2026: contributor

The TOTAL_TRIES exit condition in CoinGrinder (src/wallet/coinselection.cpp) has no test coverage — the guard and SetAlgoCompleted(false) can be removed without any test failing (see corecheck mutation report).

This adds a unit test in coinselection_tests.cpp that constructs a UTXO set large enough to exhaust TOTAL_TRIES iterations before finding an optimal solution, then asserts the result has GetAlgoCompleted() == false.

Modeled after the existing BnB TOTAL_TRIES test at L166 of the same file.

Closes #33419

DrahtBot added the label Tests on Mar 21, 2026

DrahtBot commented at 11:25 pm on March 21, 2026: contributor

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

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

HouseOfHufflepuff commented at 11:28 pm on March 21, 2026: contributor

@yancyribbens @murchandamus

in src/wallet/test/coinselection_tests.cpp:270 in 7f72d0edfb outdated

265+            utxo_pool.push_back(MakeCoin(1 * COIN + i));
266+        }
267+
268+        auto result = CoinGrinder(utxo_pool, selection_target, change_target, max_selection_weight);
269+        BOOST_CHECK(result);
270+        BOOST_CHECK(!result->GetAlgoCompleted());

murchandamus commented at 4:18 am on March 22, 2026:

CoinGrinder returns the count of selections that were evaluated. It would be neat to check here that it matches the limit from the algorithm!

Similarly, the success case could hard code the expected attempts as well, as the algorithm is deterministic and should always return the same count.

murchandamus commented at 5:02 pm on March 22, 2026:

What I meant here was to add another check along the lines of the following, and another with the respective number of attempts above:

0        BOOST_CHECK(!result->GetAlgoCompleted());
1         size_t expected_attempts = 100'000;
2        BOOST_CHECK_MESSAGE(result->GetSelectionsEvaluated() == expected_attempts, strprintf("Expected %i attempts, but got %i", expected_attempts, res->GetSelectionsEvaluated()));
3    }

in src/wallet/test/coinselection_tests.cpp:242 in 7f72d0edfb

237+     * With the chosen selection_target and change_target, exactly 9 coins are needed: 8 coins cannot reach
238+     * selection_target + change_target, so only 9-coin combinations are valid. We set max_selection_weight to exactly
239+     * 9 input weights so that 10-coin combinations exceed the weight limit.
240+     *
241+     * With 17 UTXOs, the search space is small enough to complete within TOTAL_TRIES.
242+     * With 30 UTXOs, the search space vastly exceeds TOTAL_TRIES, triggering the attempt limit.

murchandamus commented at 4:20 am on March 22, 2026:

17 to 30 leaves quite a gap, perhaps this test could use the highest number of UTXOs that allow the search to complete, and the lowest number of UTXOs for which the search fails to complete instead?

HouseOfHufflepuff commented at 3:39 pm on March 22, 2026:

Thank you for the review and feedback. I have tightened to 17/18 — the exact boundary. With m_min_change_target as the change target and identical-weight UTXOs, 17 completes within TOTAL_TRIES and 18 exceeds it.

murchandamus commented at 4:59 pm on March 22, 2026:

Great, thanks!

in src/wallet/test/coinselection_tests.cpp:246 in 7f72d0edfb

241+     * With 17 UTXOs, the search space is small enough to complete within TOTAL_TRIES.
242+     * With 30 UTXOs, the search space vastly exceeds TOTAL_TRIES, triggering the attempt limit.
243+     */
244+
245+    CAmount selection_target = 8 * COIN;
246+    CAmount change_target = default_cs_params.m_cost_of_change;

murchandamus commented at 4:22 am on March 22, 2026:

While this will work fine for the specific test, the amount of m_cost_of_change is quite small. In practice it would not suffice to pay for the weight of the change output and additionally ensure that there would be enough funds for the change output to exceed the dust limit. Perhaps check the default_cs_params for another parameter that would fit better?

HouseOfHufflepuff commented at 3:40 pm on March 22, 2026:

Good point. Changed to m_min_change_target, which is what all production call sites pass to CoinGrinder (e.g., spend.cpp:770).

murchandamus commented at 4:59 pm on March 22, 2026:

That’s the one. :)

murchandamus commented at 4:34 am on March 22, 2026: member

Nice work, thanks for picking that up!

test: Add CoinGrinder TOTAL_TRIES attempt limit test 9ee8895877

HouseOfHufflepuff force-pushed on Mar 22, 2026

Update src/wallet/test/coinselection_tests.cpp

Thank you 
@murchandamus.

Co-authored-by: Mark "Murch" Erhardt <murch@murch.one>

610082c214

yancyribbens commented at 6:50 pm on March 22, 2026: contributor

Isn’t this a duplicate of #33701? The referenced #33419 should have been closed by 33701, although it appears like it was left open.

Note that the coin-selection tests are sort of in a transitory place of existing both in ‎src/wallet/test/coinselector_tests.cppand ‎src/wallet/test/coinselection_tests.cpp which I think is part of the confusion here.

HouseOfHufflepuff commented at 7:11 pm on March 22, 2026: contributor

Thanks for pointing this out! It looks like #33701 already covers this. Issue #33419 should have been closed by that merge. Closing this as a duplicate.

HouseOfHufflepuff closed this on Mar 22, 2026

maflcko commented at 3:11 pm on March 24, 2026: member

Isn’t this a duplicate of #33701? The referenced #33419 should have been closed by 33701, although it appears like it was left open. @yancyribbens If you want it to be closed on merge, you’ll have to put it in the pull request description, not in a random other comment. Also, you can close your own issues, if they are no longer relevant/fixed. This takes off burden of maintainers to do it for you.

yancyribbens commented at 6:47 pm on March 24, 2026: contributor

@yancyribbens If you want it to be closed on merge, you’ll have to put it in the pull request description, not in a random other comment. Also, you can close your own issues, if they are no longer relevant/fixed. This takes off burden of maintainers to do it for you. @maflcko right, thanks, I will try to take notice next time that I add the closes thing in the PR description. Apologies for leaving the issue open.

yancyribbens commented at 6:49 pm on March 24, 2026: contributor

I would also add, though, that if one looks closely at the issue, Github mentions that the issue is linked to a PR #33701.

murchandamus commented at 8:45 pm on March 24, 2026: member

@HouseOfHufflepuff: Sorry that I missed that the issue had been addressed already. I currently have a PR open to move the SRD tests, #34886. After that, the Coingrinder and Knapsack tests would be outstanding. We could use your commit when the CoinGrinder tests are moved.

test: add coin-grinder TOTAL_TRIES coverage #34890

Reviews