docs: add doc comment for SRD selection algorithm

yancyribbens commented at 1:05 pm on October 14, 2025: contributor

The params documentation is missing change_fee and the description is lacking recent changes.

DrahtBot added the label Docs on Oct 14, 2025

DrahtBot commented at 1:06 pm on October 14, 2025: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33622.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	murchandamus, brunoerg

If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

fanquake requested review from murchandamus on Oct 14, 2025

yancyribbens commented at 4:52 pm on October 14, 2025: contributor

cc @furszy as related to https://github.com/bitcoin/bitcoin/issues/33168

in src/wallet/coinselection.cpp:538 in e6c439dcb2

532@@ -533,6 +533,21 @@ class MinOutputGroupComparator
533     }
534 };
535 
536+/* Randomized Single Round Selection Algorithm.
537+*
538+* Full Description: This algorithm works by selecting inputs randomly until the target amount is

brunoerg commented at 6:46 pm on October 14, 2025:

e6c439dcb23e1a0d03261734fd89412a15b1932a: I don’t this it needs this “Full Description:”. It seems obvious that is the description of the algorithm.

0This algorithm works by selecting inputs randomly until the target amount is

yancyribbens commented at 8:20 pm on October 14, 2025:

Some of the other algorithms use that wording, for example see coin-grinder: https://github.com/bitcoin/bitcoin/pull/33622/files#diff-d473ed8396f9451afb848923cfcfaa630c9811a78e07f3ae1ffd3a65da218accR208. Therefore just copying a pattern that already exists. Do you not like that wording in coin-grinder also?

brunoerg commented at 8:42 pm on October 14, 2025:

I think coin grinder explanation is more complex, so that makes sense. But anyway, feel free to ignore it.

yancyribbens commented at 11:25 pm on October 14, 2025:

No worries I can remove it then.

in src/wallet/coinselection.cpp:544 in e6c439dcb2

539+* reached or exceeded.  If the maximum weight is exceeded, then the least valuable inputs are
540+* removed from the selection.  In so doing, minimize the number of UTXOs included in the result
541+* by preferring UTXOs with higher value.  Note that this selection algorithm should always produce
542+* a non-dust change output.
543+*
544+* @param std::vector<OutputGroup>& utxo_pool The UTXOs that we are choosing from.

brunoerg commented at 7:04 pm on October 14, 2025:

Aren’t there already documentation about the parameters in coinselection.h? If so, we should just add the description there instead.

yancyribbens commented at 8:25 pm on October 14, 2025:

There’s not much presidence for this when looking at how the other algorithms are documented. coin-grinder seems to have it documented in both the headers file and the cpp file.

yancyribbens commented at 8:26 pm on October 14, 2025:

I don’t care much either way where the doc comments are, just that it exists someplace that is findable.

brunoerg commented at 8:43 pm on October 14, 2025:

I think it’s weird to have a documentation about the paratemers in coinselection.h and another documentation about the same parameters but with different wording/explanation in coinselection.cpp.

yancyribbens commented at 11:35 pm on October 14, 2025:

Hmm it’s probably silly to have it both places. BnB for example does not have it’s params defined in .h at all, nor its description for that matter. And the params in .h for SRD are missing change_fee :face_palm:.

Honestly, all these comments should be in the header file, right? It would be really hard to get people to review that kind of a change though.

yancyribbens force-pushed on Oct 14, 2025

yancyribbens commented at 11:59 pm on October 14, 2025: contributor

@brunoerg thanks for reviewing. Instead of adding new docs I improved the comments in .h instead. If only the other selection algorithms did that as well..

yancyribbens force-pushed on Oct 15, 2025

in src/wallet/coinselection.h:454 in d18a88a203 outdated

450@@ -451,6 +451,7 @@ util::Result<SelectionResult> CoinGrinder(std::vector<OutputGroup>& utxo_pool, c
451  *
452  * @param[in]  utxo_pool    The positive effective value OutputGroups eligible for selection
453  * @param[in]  target_value The target value to select for
454+ * @param[in]  change_fee The amount added to CHANGE_LOWER which ensures a non-dust change output will be created.

murchandamus commented at 1:19 pm on November 3, 2025:

This comment is unhelpful. It provides the information that is already available from the code, but does not correctly explain the abstract context.

0 * [@param](/bitcoin-bitcoin/contributor/param/)[in]  change_fee The cost of adding the change output to the transaction at the transaction’s feerate. Budgeting separately ensures that the change’s amount will be at least CHANGE_LOWER.

yancyribbens commented at 5:16 pm on November 3, 2025:

This edit is a tautology. No matter what the change budgeting is, the change amount will be at least CHANGE_LOWER since this value is added to CHANGE_LOWER. Maybe it’s not the place to explain to users of the API why it matters that CHANGE_LOWER may not be enough of a change budget, although I would have no idea why without reading the commit that added this parameter.

yancyribbens commented at 11:16 am on November 9, 2025:

I think I see now what you are getting on when you say “Budgeting separately”. You’re saying that by budgeting separately you are providing a value to be used for change_fee and any value used for change fee will be at least CHANGE_LOWER. If there is no other discussion on this i’ll accept it and move on, but this is pretty confusing imo. It also doesn’t make clear that you can just not “budget separately” and the change_fee will be equal to CHANGE_LOWER.

yancyribbens commented at 11:26 am on November 9, 2025:

I think ideally a default parameter value is used here: CAmount change_fee = CHANGE_LOWER. That way it’s made more clear that either you budget separately for the change_fee or it will default to CHANGE_LOWER. In most cases CHANGE_LOWER should be plenty for a change_fee.

in src/wallet/coinselection.h:453 in bbdfe22902 outdated

446@@ -447,7 +447,10 @@ util::Result<SelectionResult> SelectCoinsBnB(std::vector<OutputGroup>& utxo_pool
447 util::Result<SelectionResult> CoinGrinder(std::vector<OutputGroup>& utxo_pool, const CAmount& selection_target, CAmount change_target, int max_selection_weight);
448 
449 /** Select coins by Single Random Draw. OutputGroups are selected randomly from the eligible
450- * outputs until the target is satisfied
451+ * outputs until the target is satisfied.  If the maximum weight is exceeded, then the least
452+ * valuable outputs are removed from the selection.  In so doing, minimize the number of UTXOs
453+ * included in the result by preferring UTXOs with higher value.  Note that this selection
454+ * algorithm should always produce a non-dust change output.

murchandamus commented at 1:35 pm on November 3, 2025:

SRD does NOT minimize the number of UTXOs included in the result. It only ensures that the selection adheres to the weight limit. Minimizing would imply that the selection is further pruned beyond adhering to the weight limit. The last sentence should also be amended to clarify that SRD does not always return a result, but when it does, the selection will always create a change output:

0 * outputs until the target is satisfied.  If the maximum weight is exceeded, the OutputGroups with the lowest
1 * effective value are removed from the selection until weight is acceptable. If a solution is found, the resulting
2 * selection will produce a change output with an amount of at least CHANGE_LOWER.

yancyribbens commented at 5:02 pm on November 3, 2025:

SRD does NOT minimize the number of UTXOs included in the result. It only ensures that the selection adheres to the weight limit. Minimizing would imply that the selection is further pruned beyond adhering to the weight limit.

Your edit’s make sense, thanks for the clarification. However, I thought it would be helpful to note why the OutputGroups with the lowest effective_value are removed when the weight is exceeded. This seems at first counter-intuitive since it’s not removing OutputGroups according to weight when the weight is exceeded.

Removing UTXO’s with the lowest effective_value minimizes the number of UTXOS included in the result when the maximum weight is exceeded. That is, by removing the lowest effective_value utxos leaves only large value utxos which means there are less required to reach a target.

murchandamus commented at 1:41 am on November 6, 2025:

Removing UTXO’s with the lowest effective_value minimizes the number of UTXOS included in the result when the maximum weight is exceeded.

Maybe we have diverging understandings of what the verb “minimize” means. I understand it to mean “reduce to a minimum”. Are you using it as a synonym for “reduce”? Or do you think that removing a single OutputGroup always reduces the selection to the minimum necessary selection?

Because, per my understanding of the verb, this algorithm does not minimize the selection when the weight is overshot. To give a counterexample, if the addition of a tenth OutputGroup causes the weight to be exceeded, maybe removing a single OutputGroup is sufficient to drop the selection below the weigh limit, leaving nine OutputGroups selected even if the single tenth OutputGroup among the selected were to be sufficient to fund the whole transaction. I would understand “minimizing” here as all other nine OGs being removed once the limit is exceeded, and that’s not what we do.

why the OutputGroups with the lowest effective_value are removed

When the maximum weight is exceeded, this implies that the current selection has an insufficient amount of value per weight. Most wallets only have one UTXO per OutputGroup, and many only one type of UTXOs. In that case, their weight would not differ in the first place. Removing the OutputGroup with the lowest effective_value is a simple algorithm that increases the average effective_value per weight of the selection. Especially if there are OutputGroups with multiple UTXOs or UTXOs with different weights, this simple heuristic is insufficient to guarantee that a solution will be found even when one exists, but in most cases, it will get the job done. To guarantee that an existing solution is found, a more complex approach such as CoinGrinder would be necessary.

yancyribbens commented at 12:47 pm on November 6, 2025:

Are you using it as a synonym for “reduce”?

Yes, that is correct. I was using it in terms of a minimization function, that is, it is a step in the direction of a more minimal solution. Agree to remove that wording though if your don’t like it; I can see why it’s confusing.

Removing the OutputGroup with the lowest effective_value is a simple algorithm that increases the average effective_value per weight of the selection.

Said another way, the selection density is biased towards a higher density selection when max weight is exceeded?

this simple heuristic is insufficient to guarantee that a solution will be found even when one exists, but in most cases, it will get the job done. To guarantee that an existing solution is found, a more complex approach such as CoinGrinder would be necessary.

Agree

yancyribbens commented at 11:57 am on November 9, 2025:

I updated the documentation per out discussion here to read:

If the maximum weight is exceeded, the OutputGroups with the lowest effective value are removed from the selection until weight is acceptable. By removing the lowest effective value, the average effective value per weight of the selection is increased and thus reducing the average selection size. If a solution is found, the resulting selection will produce a change output with an amount of at least CHANGE_LOWER.

murchandamus commented at 1:39 pm on November 3, 2025: contributor

Concept ACK on improving the documentation, but there is room for improvement on the specific phrasing.

yancyribbens force-pushed on Nov 9, 2025

DrahtBot added the label CI failed on Nov 9, 2025

yancyribbens force-pushed on Nov 9, 2025

DrahtBot removed the label CI failed on Nov 9, 2025

in src/wallet/coinselection.h:458 in c2d05bf0be

454+ * is increased and thus reducing the average selection size.  If a solution is found, the
455+ * resulting selection will produce a change output with an amount of at least CHANGE_LOWER.
456  *
457  * @param[in]  utxo_pool    The positive effective value OutputGroups eligible for selection
458  * @param[in]  target_value The target value to select for
459+ * @param[in]  change_fee The cost of adding the change output to the transaction at the transaction’s feerate. Budgeting separately ensures that the change’s amount will be at least CHANGE_LOWER.

murchandamus commented at 9:41 pm on January 20, 2026:

I see why my prior suggestion was confusing. Looking at this again, I would recommend to focus on the parameter itself here rather than explaining the success condition of the algorithm. The algorithm should rather be explained in the description of the function instead of this parameter.

0 * [@param](/bitcoin-bitcoin/contributor/param/)[in]  change_fee The cost of adding the change output to the transaction at the transaction’s feerate.

yancyribbens commented at 1:00 pm on January 23, 2026:

Sure, sounds good.

in src/wallet/coinselection.h:454 in c2d05bf0be outdated

446@@ -447,10 +447,15 @@ util::Result<SelectionResult> SelectCoinsBnB(std::vector<OutputGroup>& utxo_pool
447 util::Result<SelectionResult> CoinGrinder(std::vector<OutputGroup>& utxo_pool, const CAmount& selection_target, CAmount change_target, int max_selection_weight);
448 
449 /** Select coins by Single Random Draw. OutputGroups are selected randomly from the eligible
450- * outputs until the target is satisfied
451+ * outputs until the target is satisfied.  If the maximum weight is exceeded, the OutputGroups
452+ * with the lowest effective value are removed from the selection until weight is acceptable.
453+ * By removing the lowest effective value, the average effective value per weight of the selection
454+ * is increased and thus reducing the average selection size.  If a solution is found, the
455+ * resulting selection will produce a change output with an amount of at least CHANGE_LOWER.

murchandamus commented at 6:41 pm on January 21, 2026:

By removing the lowest effective value, the average effective value per weight of the selection is increased and thus reducing the average selection size.

That sentence is not correct. Consider this counterexample: You are selecting at 1 s/vB. The maximum weight is exceeded and you need to either kick out an OutputGroup with three UTXOs that weighs 300 vB and is worth 5100 sats (eff_value = 4800 sats) or an OutputGroup with one UTXO weighing 100 vB and worth 4000 sats (eff_value = 3900 sats). The one-UTXO OutputGroup has a “value density” (eff_value/vsize) of 39, the 3-UTXO OutputGroup has a value density of 16. However, the one-UTXO OutputGroup has the smaller effective value (3900 < 5100) even while it weighs a lot less and thus is evicted first.

How about this:

Select coins by Single Random Draw (SRD). SRD selects eligible OutputGroups from a shuffled ordering until the effective value of the input set suffices to create the recipient outputs and a change output with an amount of at least CHANGE_LOWER. While the maximum input weight is exceeded during selection, the OutputGroup with the lowest effective value is dropped from the selection before additional OutputGroups are selected. Due to its greedy approach, SRD can fail to discover possible solutions in pathological cases.

yancyribbens commented at 12:58 pm on January 23, 2026:

I think what I had is correct, if you consider every utxo group to have only one utxo. And in such a case where utxo groups are not considered, would SRD still fail in the pathological case? Part of the issue is the rust version does not use output groups, and there is disagreement on if output groups are even useful.

Could this be amended to say something like it works better if you chose to compile without output groups?

Select coins by Single Random Draw. OutputsGroups are considered for example to contain exactly one UTXO. In the case where multiple UTXOs are used per OutputGroup, SRD can fail to discover possible solutions in the pathological case.

murchandamus commented at 10:23 pm on January 23, 2026:

I think what I had is correct, if you consider every utxo group to have only one utxo.

No, even if OutputGroups are composed of single UTXOs, they can differ in weight.

And in such a case where utxo groups are not considered, would SRD still fail in the pathological case?

Yes, I could have constructed an equivalent example with an OutputGroups consisting of a P2PKH UTXO and one with a P2TR UTXO.

E.g., a P2PKH input of 592 WU with 5000 sats (vd: ~32.78) and a P2TR keypath input of 230 WU with 4000 sats (vd: ~68.57) would exhibit the same issue at 1 s/vB; the P2TR output would be dropped, even though it is lighter and has a higher value density.

Part of the issue is the rust version does not use output groups, and there is disagreement on if output groups are even useful.

You are proposing a documentation change to Bitcoin Core, so this needs to accurately describe Bitcoin Core’s behavior, not some other piece of software.

“In the case where multiple UTXOs are used per OutputGroup, SRD can fail to discover possible solutions in the pathological case.”

No, the cause is not that OutputGroups can have multiple UTXOs, but that OutputGroups generally can have differing weights, which means that both dimensions contributing to value density (amount and weight) can affect the value density in opposite directions.

murchandamus commented at 10:24 pm on January 23, 2026:

We are now 28 comments deep into a PR that was adding missing documentation for a single input parameter. Could we please try to get this over the finish line?

yancyribbens commented at 12:25 pm on January 24, 2026:

We are now 28 comments deep into a PR that was adding missing documentation for a single input parameter. Could we please try to get this over the finish line?

Yes, I would like that as well. This PR was open for quiet a while without any comment or review so I’d forgotten much of the detail pertaining to this. While I see now why my original wording was wrong, I feel like it’s not understandable in what cases solutions are undiscoverable in a “pathological case”. The remainder of your suggestion looks good.

I give you permission though if you want to close this and re-open your own with the correct wording as you quoted above to move this along. Or just simply ignore this PR and I will close it if no one else cares to review this docs addition. TBH it’s been sitting open so long I had already moved on and forgot much of the context pertaining to this PR. I do appreciate you getting back to this and following though.

yancyribbens commented at 12:44 pm on January 24, 2026:

I’ve updated this to what we both seem to agree on. Hopefully we can get on with it and merge that.

murchandamus commented at 0:29 am on January 26, 2026:

Yeah, I’m sorry. I was focused on outreach and BIPs for a while and work on Bitcoin Core fell short.

I feel like it’s not understandable in what cases solutions are undiscoverable in a “pathological case”.

Let me show you an example (expanding on the scenario from above).

Let’s say we are selecting at 1 s/vB and have four UTXOs:

One P2PKH input (A):
- weight: 592 WU
- amount: 5000 sats
- eff_value: 4852 sats
Three identical P2TR keypath inputs (B):
- weight: 230 WU
- amount: 4000 sats
- eff_value: 3942.5 sats (rounded down unless we have two)

Let

target_value = 10'000 sats
max_selection_weight = 800 WU.

This could execute for example as follows:

We shuffle the UTXOs and get B B A B.
We select one P2TR output
- selection: B
- eff_value: 3942 sats ↦ not enough
- weight: 230 WU
We select another P2TR output
- selection: B + B
- eff_value: 2×3942.5 = 7885 sats ↦ not enough
- weight: 2×230 = 460 WU
We select the P2PKH output
- selection: B + B + A
- eff_value: 2×3942.5 + 4852 = 12'737 ↦ enough
- weight: 2×230 + 592 = 1052 WU ↦ overweight
We remove the UTXO with the lowest eff_value: 3942.5 sats < 4852 sats, one of the P2TR outputs is dropped
- selection: B + A
- eff_value: 3942 + 4852 = 8794 sats ↦ not enough
- weight: 230 + 592 = 822 WU ↦ overweight
We remove the UTXO with the lowest eff_value: the other P2TR outputs is dropped
- selection: A
- eff_value:4852 sats ↦ not enough
- weight: 592 WU ↦ no longer overweight

Selecting the last P2TR UTXO afterwards will lead to the same overweight situation, and we promptly remove it again. We have traversed the entire random shuffle and never find a selection that exceeds target_value, but isn’t overweight.

In fact, in this scenario SRD fails to find the solution in 3 out of the 4 possible cases of ordering:

A B B B: After the P2PKH UTXO is selected, each P2TR UTXO is discarded due to being overweight
B A B B: One P2TR UTXO is selected, but when the P2PKH UTXO is selected, the P2TR UTXO is dropped. After that each P2TR UTXO is discarded as soon as it is selected.
B B A B: Seen above.
B B B A: This will produce a valid solution composed of the three P2TR UTXOs:
- selection: B + B + B
- eff_value: 3×3942.5 = 11'827 sats _ ↦ more than target_value of 10'000 sats
- weight: 3×230 WU = 690 WU ↦ less than max_selection_weight of 800 WU

Reliably discovering the solution composed of B B B would require backtracking, which SRD does not have. That would essentially be essentially what Coingrinder is: a search exploring all possible combinations.

Anyway, that’s why I suggested the text above that I did (slightly amended):

 0- /** Select coins by Single Random Draw. OutputGroups are selected randomly from the eligible
 1- * outputs until the target is satisfied.  If the maximum weight is exceeded, the OutputGroups
 2- * with the lowest effective value are removed from the selection until weight is acceptable.
 3- * By removing the lowest effective value, the average effective value per weight of the selection
 4- * is increased and thus reducing the average selection size.  If a solution is found, the
 5- * resulting selection will produce a change output with an amount of at least CHANGE_LOWER.
 6+ /** Select coins by Single Random Draw (SRD). SRD selects eligible OutputGroups from a shuffled
 7+ * ordering until the effective value of the input set suffices to create the recipient outputs and a 
 8+ * change output with an amount of at least CHANGE_LOWER. While the maximum selection 
 9+ * weight is exceeded during selection, the OutputGroup with the lowest effective value is dropped
10+ * from the selection before additional OutputGroups are selected. Due to this greedy approach,
11+ * SRD can fail to discover possible solutions in pathological cases.

I would prefer using this text as it would be helpful to indicate that SRD is not guaranteed to succeed even when solutions are possible.

yancyribbens commented at 1:28 pm on January 26, 2026:

Thanks! This thorough description really helps me understand why the approach can fail to find an otherwise discoverable solution. Although, part of my point is that I feel like to those reading this doc comment, it is not intuitive:

A) Why the greedy selection can fail B) What is the advantage of this greedy selection, particularly if it can fail as stated.

Reliably discovering the solution composed of B B B would require backtracking, which SRD does not have.

If the value with the lowest density was removed instead of the lowest effective value, then in this case, removing the P2PKH input would result in the desired solution.

Also, I’d normally try to recommend an alternative wording, particularly since the first sentence is a run-on sentence that IMO could be tailored a bit. However, in the interest of getting this done without further ado, I’ve just used your suggestion.

murchandamus commented at 7:59 pm on January 26, 2026:

Also, I’d normally try to recommend an alternative wording, particularly since the first sentence is a run-on sentence that IMO could be tailored a bit. However, in the interest of getting this done without further ado, I’ve just used your suggestion.

Thanks for using my suggestion!

Thanks! This thorough description really helps me understand why the approach can fail to find an otherwise discoverable solution. Although, part of my point is that I feel like to those reading this doc comment, it is not intuitive: A) Why the greedy selection can fail

It’s sufficient to describe what outcomes can be expected here. I don’t think it’s necessary to explain edge cases in which SRD can fail to find a possible solution in this high-level description of the algorithm. Thanks for surfacing that it should be explicitly stated that SRD does not always succeed.

B) What is the advantage of this greedy selection, particularly if it can fail as stated.

It’s easy to understand, often produces results that diverge from the other algorithms (e.g., exhibiting more consolidatory behavior at low feerates, will most of the time find a solution when one is possible when BnB does not, and broadens the possible selections which reduces information leak about the wallet’s UTXO pool), and is computationally inexpensive.

Reliably discovering the solution composed of B B B would require backtracking, which SRD does not have.

If the value with the lowest density was removed instead of the lowest effective value, then in this case, removing the P2PKH input would result in the desired solution.

Yes, in this case it would. However, that’s not generally the case. I can as simply construct an example in which removing the UTXO with the lowest value density would cause the selection to fail, because the only solution was to select UTXOs of lower value density but higher eff_value. E.g.:

Let target_value be 17'700 sats, max_selection_weight be 1800 WU, and the UTXO pool be composed by two types of UTXOs:

8× P2TR (B):
- weight: 230 WU
- eff_value: 2301 sats
3× P2PKH (A):
- weight: 592 WU
- eff_value: 5920 sats

Outcome: P2TR UTXOs (B) have a (minimally) higher value density. An input set of three P2PKH UTXOs AAA is a valid solution (17'760 sats > 17'700 sats) and acceptable weight (1776 WU < 1800 WU). Any combination with at least one B is overweight, so SRD will fail as soon as one B gets selected, because an A is evicted before a B:

AABB has too little funds, AABB(A|B) are too heavy (2236 WU or 1874 WU)
ABBBBB has too little funds, ABBBBB(A|B) are too heavy (2334 WU or 1972 WU)
BBBBBBB has too little funds, BBBBBBB(A|B) are too heavy (2202 WU or 1840 WU)

Likelihood to select three P2PKH UTXOs before one P2TR output: (3/11)×(2/10)×(1/9)=~0.6%. If it were evicting the UTXOs with the lowest effective value: 100%. So, basically, you get to pick your poison, but neither guarantees to find a solution when one exists.

yancyribbens commented at 8:07 pm on January 27, 2026:

It’s easy to understand, often produces results that diverge from the other algorithms (e.g., exhibiting more consolidatory behavior at low feerates, will most of the time find a solution when one is possible when BnB does not, and broadens the possible selections which reduces information leak about the wallet’s UTXO pool), and is computationally inexpensive.

The point I think I was trying to make long ago, is that dropping the lowest effective value when weight exceeded tends to produce a result set with fewer UTXOs. In other-words, on average a smaller selection size.. At least, that’s how I interpret the changes here: #26720. I’m fine with not trying to explain that detail in the docs, however it’s an important detail in how this algo functions.

yancyribbens commented at 8:14 pm on January 27, 2026:

Math combinatorics checks out :)

murchandamus commented at 10:46 pm on January 27, 2026:

In other-words, on average a smaller selection size.

On average smaller than what?

Smaller than BnB or CoinGrinder? Not at all.
Smaller than Knapsack? Dunno.
Smaller than the max_selection_weight? Absolutely. ;)

yancyribbens commented at 12:09 pm on January 31, 2026:

Smaller than the max_selection_weight? Absolutely. ;)

Yes, exactly, that felt implied but maybe it was not clear. Anyway, I’m happy to see this finally merged :)

yancyribbens force-pushed on Jan 24, 2026

DrahtBot added the label CI failed on Jan 24, 2026

DrahtBot removed the label CI failed on Jan 24, 2026

yancyribbens force-pushed on Jan 26, 2026

DrahtBot added the label CI failed on Jan 26, 2026

DrahtBot commented at 1:31 pm on January 26, 2026: contributor

🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/actions/runs/21359069488/job/61473442378 LLM reason (✨ experimental): Lint check failed due to trailing whitespace in src/wallet/coinselection.h.

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

DrahtBot removed the label CI failed on Jan 26, 2026

murchandamus commented at 8:00 pm on January 26, 2026: contributor

ACK fef18ef02680769b9bf00152ccd54f445420db57

Except it looks like there was trailing whitespace somewhere. Could you please fix that and push again?

When you do, you could also squash those two commits.

doc: add missing param description to SRD

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

3400db8040

yancyribbens force-pushed on Jan 27, 2026

yancyribbens commented at 7:57 pm on January 27, 2026: contributor

Except it looks like there was trailing whitespace somewhere. Could you please fix that and push again?

I’m not sure why you think so. I double checked using git diff HEAD~1. Also, the lints would fail if there was a trailing white space I think.

When you do, you could also squash those two commits.

Sure

murchandamus commented at 10:41 pm on January 27, 2026: contributor

ACK 3400db80401d65ba16b52e5055486c75cd1412ff

Weird, I could have sworn that there was a failed CI citing trailing whitespace. Maybe I mixed it up with something else I was looking at. Either way, it appears to be resolved now, thanks.

sedited requested review from brunoerg on Jan 28, 2026

brunoerg commented at 1:16 pm on January 28, 2026: contributor

code review ACK 3400db80401d65ba16b52e5055486c75cd1412ff

I think this is a clear improvement on the description of the SRD algorithm. Also, it correctly adds missing change_fee description. Seems fine as is, just some points/personal preferences:

3400db80401d65ba16b52e5055486c75cd1412ff: The commit message “doc: add missing param description to SRD” doesn’t reflect everything the commit does, because it also improves the algorithm description.
When explaining an algorithm, I prefer to be verbose and not use any implementation-related name (e.g. CHANGE_LOWER).

sedited merged this on Jan 28, 2026

sedited closed this on Jan 28, 2026

yancyribbens commented at 12:07 pm on January 31, 2026: contributor

https://github.com/bitcoin/bitcoin/commit/3400db80401d65ba16b52e5055486c75cd1412ff: The commit message “doc: add missing param description to SRD” doesn’t reflect everything the commit does, because it also improves the algorithm description.

Thanks for the feedback. I had this as two separate commits since there is a few things happening in this change, however since in review it was request to combine them into one commit, I did so in hopes of not delaying this PR any further.

docs: add doc comment for SRD selection algorithm #33622

Code Coverage & Benchmarks

Reviews