Skip to content

wallet, doc: clarify the coin selection filters that enforce cluster count#34037

Merged
fanquake merged 2 commits intobitcoin:masterfrom
glozow:2025-12-ancestry
Mar 9, 2026
Merged

wallet, doc: clarify the coin selection filters that enforce cluster count#34037
fanquake merged 2 commits intobitcoin:masterfrom
glozow:2025-12-ancestry

Conversation

@glozow
Copy link
Member

@glozow glozow commented Dec 10, 2025 •
edited by fanquake
Loading

Followup to #33629.

Fix misleading docs and variable names. Namely, getTransactionAncestry returns the cluster count, not max descendant count of ancestor set (not worth reimplementing as it is merely a heuristic). No behavior changes - I don’t think much needs to be changed for the first release containing cluster mempool.

Current CoinEligibilityFilters enforce a maximum ancestor count (summed across all outputs, potentially overestimating) and max descendant count across ancestors of the output.

Since #33629, these filters have started using cluster count instead of max desc count across ancestors. The change isn’t dangerous, as the cluster count bounds descendant count as well. Currently, the wallet is essentially enforcing a mixture of both limits - this is good while we are transitioning. Note that the cluster count enforced is 25, not 64, since it's grabbing the node's descendant count limit. While it is not an apples-to-apples comparison, a cluster count limit of 25 helps us avoid busting legacy descendant limits (which will be common on the network for a while).

Potential things for the future, out of scope for this PR:

  • When we get rid of the ancestor/descendant config options, getPackageLimits can probably be replaced with hard-coded values.
  • Change the OutputGroups to track the actual cluster count that results from spending these outputs and merging their clusters.
  • Loosen from 25 after that policy is no longer common.
  • Clean up getPackageLimits.

@DrahtBot
Copy link
Contributor

DrahtBot commented Dec 10, 2025 •
edited
Loading

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/34037.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK achow101, rkrux, ismaelsadeeq

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

Conflicts

No conflicts as of last run.

LLM Linter (✨ experimental)

Possible places where named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):

  • CoinEligibilityFilter(0, 1, 2) in src/wallet/spend.cpp

2026-02-11 19:10:35

@DrahtBot
Copy link
Contributor

DrahtBot commented Dec 10, 2025

🚧 At least one of the CI tasks failed.
Task tidy: https://github.com/bitcoin/bitcoin/actions/runs/20084176691/job/57617902247
LLM reason (✨ experimental): clang-tidy errors (bugprone-argument-comment) due to mismatched argument comments for the cluster_count parameter in coinselection tests.

Hints

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.

@rkrux
Copy link
Contributor

rkrux commented Dec 12, 2025

Since #33639, these filters have started using cluster count instead of max desc count across ancestors.

The linked PR doesn't seem correct to me. Is it supposed to be #33629 instead?

rkrux
rkrux reviewed Dec 12, 2025
View reviewed changes
Copy link
Contributor

@rkrux rkrux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK dc5e20c

Nice, this change comes at a right time. Recently, I got confused by getTransaction Ancestry() in a previous PR: #33528 (comment) - not anymore.

I have limited idea of Cluster Mempool currently, thus I have based this review off of the GetTransactionAncestry implementation in src/txmempool.h|cpp.

@DrahtBot DrahtBot mentioned this pull request Dec 15, 2025
Open
@fanquake
Copy link
Member

fanquake commented Dec 17, 2025

cc @murchandamus

@ismaelsadeeq
Copy link
Member

ismaelsadeeq commented Dec 22, 2025

Concept ACK

@achow101 achow101 self-requested a review December 23, 2025 21:59
@glozow
Copy link
Member Author

glozow commented Jan 8, 2026

I will circle back soon.

@glozow
Copy link
Member Author

glozow commented Jan 12, 2026

Ready for review again, thanks @rkrux and @ismaelsadeeq

@DrahtBot DrahtBot mentioned this pull request Jan 25, 2026
Merged
@glozow
Copy link
Member Author

glozow commented Feb 10, 2026

Rebased

@DrahtBot
Copy link
Contributor

DrahtBot commented Feb 10, 2026

🚧 At least one of the CI tasks failed.
Task tidy: https://github.com/bitcoin/bitcoin/actions/runs/21882146698/job/63167330045
LLM reason (✨ experimental): clang-tidy failed due to a bugprone-argument-comment error (comment names don’t match parameter), causing the CI build to fail.

Hints

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.

@glozow
[doc] coin selection filters by max cluster count, not descendant
a067ca3 
Avoid confusion by clarifying the docs and renaming the variables that
now hold cluster count rather than descendant count. No behavior change.
@glozow
Copy link
Member Author

glozow commented Mar 7, 2026

Would be nice to get this for the release. It should be easy to review.

@sedited sedited requested review from ismaelsadeeq and rkrux March 7, 2026 17:35
@fanquake fanquake added this to the 31.0 milestone Mar 8, 2026
@achow101
Copy link
Member

achow101 commented Mar 9, 2026

ACK a067ca3

rkrux
rkrux reviewed Mar 9, 2026
View reviewed changes
Copy link
Contributor

@rkrux rkrux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

crACK a067ca3

git range-diff dc5e20c...a067ca3

@ismaelsadeeq
Copy link
Member

ismaelsadeeq commented Mar 9, 2026

reACK a067ca3

@fanquake fanquake merged commit 3a22250 into bitcoin:master Mar 9, 2026
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@achow101 achow101 Awaiting requested review from achow101

@ismaelsadeeq ismaelsadeeq Awaiting requested review from ismaelsadeeq

1 more reviewer

@rkrux rkrux rkrux left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Docs Wallet

Projects

None yet

Milestone

31.0

Development

Successfully merging this pull request may close these issues.

6 participants

@glozow @DrahtBot @rkrux @fanquake @ismaelsadeeq @achow101