test: SRD tie-break test #36740

Zeegaths commented at 6:26 AM on November 3, 2026: none

I was researching coin selection when I came across this bug, so I thought I'd fix it

Pr #33223 adjusts weight selection to keep the lighter coin when two coins have the same value, but the test has two mistakes, as confirmed in this comment

It builds mixed_weight_pool but passes utxo_pool to TestSRDSuccess.
There is nothing enforcing coin eviction because the current max weight (4 * 3 * 67 = 804 WU) fits any two coins (544 WU)

If you reverse #33223, it still passes, which means it misses the tie-break

The fix

The test now uses `mixed_weight_pool and adds an extra light coin so that only two light coins can fit together and evicts a coin on every tie to pass. Lowers the max weight to 536 WU; SRD now has to evict the heavier coin on every tie to succeed

This should show failed tests with bitcoin#33223 reverted and pass on master.

DrahtBot added the label Tests on Nov 3, 2026

DrahtBot commented at 6:26 AM on November 3, 2026: contributor

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

External sites

Code coverage & benchmarks: https://corecheck.dev/bitcoin/bitcoin/pulls/36740
Ralph review <img src=https://review.fish.foo/assets/bitcoin/bitcoin/36740.png alt="Ralph review status"> https://git.fish.foo/bitcoin/bitcoin/pulls/36740#author-ralph

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
ACK	Zippo76, frankomosh, mercie-ux
Stale ACK	yancyribbens

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#36255 <sub><img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36255.svg"></sub> (Refactor CoinGrinder tests 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.

Zeegaths force-pushed on Nov 3, 2026

yancyribbens commented at 3:04 PM on November 3, 2026: contributor

Concept ACK.

PR 33223 added a tie breaker condition such that if two UTXOs are the same effective_value, sort such that the lighter UTXO is preferred. This creates a lighter and thus cheaper transaction.

The sort is defined by 33223 here:

class MinOutputGroupComparator
{
public:
    int operator() (const OutputGroup& group1, const OutputGroup& group2) const
    {
        return descending_effval_weight(group1, group2);
    }
};

However, the test added did not actually test the new behavior since selecting two UTXOs needed to hit the target did not cause an eviction to take place. Confirmed that this PR fixes that by lowering the weight limit which causes the heap to pop an item, and in so doing, must sort and then perform the eviction. The second lighter UTXO is added such that the only solution that causes completion is when both lighter UTXOs remain and the heavier UTXOs evicted.

Approach ACK

in src/wallet/test/coinselection_tests.cpp:321 in 3ed719de18

 318 | +        mixed_weight_pool.push_back(MakeCoin(5 * CENT, true, cs_params, /*custom_spending_vsize=*/P2WPKH_INPUT_VSIZE - 1));
 319 | +        const auto result = SelectCoinsSRD(mixed_weight_pool, /*target_value=*/9 * CENT, cs_params.m_change_fee, cs_params.rng_fast,
 320 | +                                           /*max_selection_weight=*/4 * 2 * (P2WPKH_INPUT_VSIZE - 1));
 321 | +        BOOST_REQUIRE(result);
 322 | +        BOOST_CHECK_EQUAL(result->GetInputSet().size(), 2U);
 323 | +        for (const auto& input : result->GetInputSet()) {

yancyribbens commented at 3:06 PM on November 3, 2026:

just a nit, but I think this for loop just adds test complexity. There are only two utxos, so it would be more clean to just assert the weight of each UTXO without a loop.

Zeegaths commented at 8:54 AM on November 4, 2026:

replaced the loop with a weight check

in src/wallet/test/coinselection_tests.cpp:319 in 3ed719de18

 312 | @@ -313,8 +313,15 @@ BOOST_AUTO_TEST_CASE(srd_test)
 313 |          std::vector<OutputGroup> mixed_weight_pool;
 314 |          AddDuplicateCoins(mixed_weight_pool, /*count=*/100, /*amount=*/5 * CENT, cs_params);
 315 |          mixed_weight_pool.push_back(MakeCoin(5 * CENT, true, cs_params, /*custom_spending_vsize=*/P2WPKH_INPUT_VSIZE - 1));
 316 | -        TestSRDSuccess("Tie-break same effective value with lower weight", utxo_pool, /*selection_target=*/9 * CENT, cs_params,
 317 | -        /*max_selection_weight=*/4 * 3 * (P2WPKH_INPUT_VSIZE - 1));
 318 | +        mixed_weight_pool.push_back(MakeCoin(5 * CENT, true, cs_params, /*custom_spending_vsize=*/P2WPKH_INPUT_VSIZE - 1));
 319 | +        const auto result = SelectCoinsSRD(mixed_weight_pool, /*target_value=*/9 * CENT, cs_params.m_change_fee, cs_params.rng_fast,
 320 | +                                           /*max_selection_weight=*/4 * 2 * (P2WPKH_INPUT_VSIZE - 1));
 321 | +        BOOST_REQUIRE(result);

yancyribbens commented at 3:07 PM on November 3, 2026:

nit, I think this require is superfluous since even without it, the next asserts couldn't pass.

Zeegaths commented at 8:49 AM on November 4, 2026:

done, removed

Zeegaths force-pushed on Nov 4, 2026

winterrdog commented at 9:15 AM on November 4, 2026: contributor

From https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md

The description for a new pull request should not contain any @ mentions. The PR description will be included in the commit message when the PR is merged and any users mentioned in the description will be annoyingly notified each time a fork of Bitcoin Core copies the merge. Instead, make any username mentions in a subsequent comment to the PR.

Zeegaths commented at 9:31 AM on November 4, 2026: none

From https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md

The description for a new pull request should not contain any @ mentions. The PR description will be included in the commit message when the PR is merged and any users mentioned in the description will be annoyingly notified each time a fork of Bitcoin Core copies the merge. Instead, make any username mentions in a subsequent comment to the PR.

noted, Thanks for this

yancyribbens commented at 3:02 PM on November 4, 2026: contributor

ACK 2ade73da7ace76d6f6cc630fe01b275c39fb9401

Zippo76 commented at 7:03 AM on November 6, 2026: none

tACK 2ade73da7a

Tested on macOS 27.0.1 arm64, using master fcf76ea549 for comparison. srd_test passes on the PR. With the comparator change from #33223 reverted, the PR’s test fails while master’s test passes. I also checked that the 536 WU limit only allows the two lighter coins together. Whenever a heavier coin makes the selection exceed that limit, it must be evicted for selection to succeed.

sedited commented at 12:37 PM on November 6, 2026: contributor

@Zeegaths please write your pull request descriptions in your own words. Not doing so is in violation of our AI policy. Most of the lines are basically redundant statements. Reviewers are expected to know basic git usage, how to run the tests and make them fail by undoing prior work, and read a diff to figure out which tests are changed. What it should say is why the change is important to you.

Zeegaths commented at 1:09 PM on November 6, 2026: none

@Zeegaths please write your pull request descriptions in your own words. Not doing so is in violation of our AI policy. Most of the lines are basically redundant statements. Reviewers are expected to know basic git usage, how to run the tests and make them fail by undoing prior work, and read a diff to figure out which tests are changed. What it should say is why the change is important to you.

i see what you mean. lmk if that works

sedited commented at 1:34 PM on November 6, 2026: contributor

lmk if that works

It does read a bit better. I think people can count for themselves how many tests are being run though.

Zeegaths commented at 1:44 PM on November 6, 2026: none

lmk if that works

It does read a bit better. I think people can count for themselves how many tests are being run though.

got it, thankyou

yancyribbens commented at 2:48 PM on November 6, 2026: contributor

If you reverse #33223, it still passes, which means it misses the tie-break

Note that this is a non-deterministic code path being tested. It looks like because of issue 1) it would never fail if 33223 is revered. However, I think 2) may fail if 33223 is undone and the random selection happened to select the lighter coin on it's first attempt even with no eviction, although this is very unlikely.

Zeegaths commented at 2:58 PM on November 6, 2026: none

If you reverse #33223, it still passes, which means it misses the tie-break

Note that this is a non-deterministic code path being tested. It looks like because of issue 1) it would never fail if 33223 is revered. However, I think 2) may fail if 33223 is undone and the random selection happened to select the lighter coin on it's first attempt even with no eviction, although this is very unlikely.

noted

DrahtBot added the label CI failed on Nov 6, 2026

test: fix SRD tie-break test

The test in #33223 builds `mixed_weight_pool` but passes
`utxo_pool` to `TestSRDSuccess`, and the max weight (804 WU) fits
any two coins, so SRD never evicts and the tie-break never runs.

Pass `mixed_weight_pool` instead of `utxo_pool`.
add an extra light coin and lower the max weight to 536 WU so that
the heavier coin gets evicted on every tie.
add checks to assert the resulting utxos on success
Call `SelectCoinsSRD` directly and check the number and total weight
of the selected inputs.

b1240a856e

Zeegaths force-pushed on Nov 6, 2026

DrahtBot removed the label CI failed on Nov 6, 2026

yancyribbens commented at 7:27 PM on November 6, 2026: contributor

@Zeegaths Zeegaths force-pushed the srd-tiebreak-check branch from 2ade73d to b1240a8 2 hours ago

When you force push like this it dismisses the reviews people have done. And if you don't explain why you are force pushing, it requires people to need to re-review all changes. Is this just a rebase? If so, it's nice to say that so people reviewing save time trying to figure out what changed.

Zeegaths commented at 7:55 PM on November 6, 2026: none

@Zeegaths Zeegaths force-pushed the srd-tiebreak-check branch from 2ade73d to b1240a8 2 hours ago

When you force push like this it dismisses the reviews people have done. And if you don't explain why you are force pushing, it requires people to need to re-review all changes. Is this just a rebase? If so, it's nice to say that so people reviewing save time trying to figure out what changed.

yes, It's just a rebase

Zippo76 commented at 5:00 PM on November 7, 2026: none

re-ACK b1240a856e

The range-diff against 2ade73da7a shows an unchanged patch.

DrahtBot requested review from yancyribbens on Nov 7, 2026

frankomosh commented at 3:13 PM on November 9, 2026: contributor

Tested ACK b1240a856e1a8f45e0dff4b600bf07605c23ed1a.

fanquake commented at 3:15 PM on November 9, 2026: member

cc @murchandamus

mercie-ux commented at 7:44 PM on November 9, 2026: contributor

ACK b1240a856e1a8f45e0dff4b600bf07605c23ed1a