Ancestor-aware CPFP: search on local bumps, report exact package pricing - #60
Draft
evanlinjin wants to merge 5 commits into
Draft
Ancestor-aware CPFP: search on local bumps, report exact package pricing#60evanlinjin wants to merge 5 commits into
evanlinjin wants to merge 5 commits into
Conversation
…call
A selector was built for one target and evaluated against it throughout,
but every method took the target as a parameter, so nothing stopped
`cs.excess(target_a, drain)` being followed by `cs.is_funded(target_b)`.
The correctness arguments in the metrics are all stated at a fixed target
-- `LowestFee::bound`'s proof that a changeless superset always costs
more, `Changeless::change_unavoidable`'s assumption that the drain
decision is monotone in the excess -- and were held together by
convention rather than by types.
`CoinSelector::new` now takes the target and owns it. Twenty signatures
*lose* a parameter rather than gaining one: fifteen public methods
(`excess`, `implied_fee`, `is_funded`, `drain`, `select_until_target_met`,
the four `*_excess`, ...), plus `bnb_solutions` and `run_bnb`, plus all
three `BnbMetric` methods.
The crate had already reached this conclusion one layer down: `BnbIter`
stored the target as a field, took it once in `BnbIter::new`, and then
re-passed it into `metric.score` and `metric.bound` at every node. That
field and the re-threading are both gone.
This is a breaking change, and it reaches `BnbMetric`, so metrics
implemented outside this crate need their signatures updated:
fn score(&mut self, cs: &CoinSelector<'_>) -> Option<Ordf32>;
fn bound(&mut self, cs: &CoinSelector<'_>) -> Option<Ordf32>;
fn drain(&mut self, cs: &CoinSelector<'_>) -> Drain;
`CoinSelector::target()` exposes the target for metrics that need to read
it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
When spending unconfirmed UTXOs, miners evaluate the transaction as a package with its unconfirmed ancestors; if the ancestors paid below the target feerate, the child must cover the difference (CPFP). This makes coin selection price that correctly, without banning anything from selection and without capping how many candidates may carry ancestors. `Cluster` holds the relevant piece of the mempool as a graph -- weights, fees, direct parent edges, and which candidate spends which -- and computes transitive ancestor closures itself, so a caller cannot under-price a package by listing only direct parents. `Cluster::mine` builds a mock block template at the target feerate using the same greedy ancestor-feerate loop Bitcoin Core's `MiniMiner` uses: whatever the template includes is already paying its own way, and a package's bump is what its remaining ancestors still owe. Package feerates are compared by cross-multiplication in `u128` so vbyte rounding cannot leak into the template's ordering. `BumpTable`, built from a cluster at one feerate, answers two different questions, and the difference is the whole design: - `individual(c)` -- what one candidate owes alone. Additive, so an over-estimate when ancestors are shared, but *local*: netted off by the new `CoinSelector::effective_value_of` and `value_pwu_of`, it makes the per-candidate figures the selection algorithms rank on tell the whole truth. - the combined package figure -- what a set of candidates owes together, exactly. What `excess`, `implied_fee`, `is_funded` and `drain` report, and what the transaction has to pay. A selector commits to one model at a time (`Pricing`), never mixing them: a bound computed against one and scored against the other is not a bound. `BnbIter` searches in the local model, and selections handed back are exactly priced, so `run_bnb`'s drain is sized by the true package cost -- the local over-estimate surfaces as a larger change output, never as a missing fee. This is the split Bitcoin Core draws between `calculateIndividualBumpFees` and `calculateCombinedBumpFee`. It relies on the local sum never falling below the combined figure, which the mock template guarantees (an overpaying ancestor is mined out, so whatever two candidates share is itself ancestor-closed and therefore deficient); `the_local_sum_never_undercuts_the_package_for_any_cluster` pins that over 2000 generated clusters. The accessors live on `CoinSelector` rather than on `Candidate`: a bump is `feerate * weight - fee_paid` over the unconfirmed ancestors, so it only means anything at the feerate it was derived for, and a `Candidate` has nowhere to record which. The selector holds the table and checks. `LowestFee::bound` needed three fixes, all the same mistake -- a bump is a fixed cost attached to an input, not a rate, so per-weight reasoning does not apply to it: - The greedy prefix charged bumps of candidates the node had not selected; a descendant may skip those candidates and never pay them, so only committed bumps may enter the deficit. - The hypothetical resized input must be priced bump-free, since scaling an input does not scale the ancestors it drags in. - The exclusion branch treated equal value and weight as interchangeable, which no longer holds when two inputs drag in different ancestors. Measured against an oracle over ~75k bound evaluations, these took over-estimates from 0.65%/2.03% of calls (k=1/k=3) to 0.01%/0.07%, and the answer gap versus the brute-forced optimum to at most 0.02% for k <= 8. `src/ancestor_search_experiment.rs` keeps the measurements runnable as ignored tests and records the two approaches this replaced: banning ancestor-carrying candidates (up to 3.07% worse answers, and insufficient-funds failures on up to 100% of instances once most candidates are unconfirmed), and repairing the bound with a `min_bump` floor over the exact model (prunes almost nothing, because the bound prices a greedy prefix and the least bump reachable from the root is always zero). Co-Authored-By: Noah Joeris <noahjoeris@gmail.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…nally
`with_bump_table` accepted a `BumpTable` built at any feerate, and two
runtime asserts caught the case where it disagreed with the selector's
target. But the selector owns its target now, so it can build the table
itself:
pub fn with_cluster(mut self, cluster: &Cluster) -> Self
The table is always derived at `target.fee.rate`, the mismatch stops
being representable, and both asserts disappear -- the same move that
took the bump off `Candidate`, applied one level up.
Since `from_ancestors` and `from_fn` were removed earlier, `from_cluster`
was `BumpTable`'s only constructor, so `with_bump_table` was not an
escape hatch for externally-computed figures; it was only a way to build
the table at the wrong rate. With it gone, nothing public accepts or
returns a `BumpTable`, so the type drops out of the public API entirely.
Callers see `Cluster` in and per-candidate figures out
(`ancestor_bump_fee_of`, `effective_value_of`, `value_pwu_of`,
`selected_ancestor_bump_fee`).
`selected_ancestor_bump_fee` loses its feerate parameter -- it existed
only to catch the now-unrepresentable mismatch. `effective_value_of` and
`CoinSelector::effective_value` keep theirs (the rate genuinely enters
the weight term) and now check it against the target's feerate, since a
bump computed at the target rate must not be netted off a
differently-rated figure.
The selector stores the table as `Arc<BumpTable>`: it is cloned at every
branch and bound node, and the table never changes after construction, so
clone stays a refcount bump.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
evanlinjin
force-pushed
the
feature/cpfp-on-target
branch
from
August 4, 2026 07:41
d917ed8 to
0596f4e
Compare
`Cluster::new` took transactions with parent edges expressed as indices
into the very `Vec` being assembled, and candidate spends as index
pairs. The caller had to invent a dense numbering for transactions they
already key by txid, could not append a parent after its child without
renumbering, and got errors back as positions.
`ClusterBuilder<Id: Ord + Clone>` takes the graph in the caller's own
vocabulary instead:
let mut builder = ClusterBuilder::new();
builder.tx(txid_a, 1_000, 500, []);
builder.tx(txid_b, 400, 0, [txid_a]);
builder.spent_by(txid_b, 3); // candidate 3 spends an output of b
let cluster = builder.build()?;
`Id` is generic rather than a txid type, so the crate stays
dependency-free -- the caller uses whatever they already have and the
ids are resolved to internal indices once, in `build`. Insertion order
stops mattering (parents are recorded as ids and resolved at the end),
and `ClusterError<Id>` reports problems by the caller's ids, including a
`DuplicateTx` case the index API could not even express as a mistake.
The candidate side deliberately stays an index: candidates are indices
everywhere else in this crate, and giving them a second identity here
would be inconsistent rather than convenient.
`MempoolTx` and the index-based constructor drop out of the public API;
the builder is the only way in. Anyone who genuinely has indices
instantiates `ClusterBuilder<usize>` and loses nothing.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
`ClusterBuilder::build` errored on a parent edge or spend naming a transaction that was never added, and on adding the same id twice. All three cases have natural meanings under the rule the cluster already lives by -- membership *is* the definition of unconfirmed, exactly as mempool membership is for a node: - A parent never added is a confirmed output. Dropped, not an error, so the caller lists every input's prevout id unfiltered instead of checking each against the mempool first. - A spend of a transaction never added spends a confirmed output and drags in nothing. Same rule, so `spent_by` can also be called for every candidate prevout unconditionally. - Adding the same id again is a no-op (the first record wins), tracked by an id map maintained as transactions are added. Overlapping ancestry walks -- two candidates sharing a parent -- each add it without coordinating. `ClusterError` shrinks to the one case with no sensible meaning: a cycle in the parent relation. What this trades away is a tripwire: an underpaying parent whose id is mistyped is now silently priced as confirmed, under-pricing the package. That tripwire was already half-blind -- omitting the parent entirely under-priced identically, uncaught -- so the docs now state the real contract instead: unconfirmed ancestors must all be present (missing ones under-price; the unsafe direction), while descendants and siblings are best-effort (missing ones overpay; safe). The `Default` impl carries an `Id: Ord` bound because `BTreeMap::new` demanded it before Rust 1.66, and the MSRV is 1.54. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
evanlinjin
force-pushed
the
feature/cpfp-on-target
branch
from
August 5, 2026 06:09
6c59038 to
6bcbd26
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note
Stacked on #59 (first commit). Review
5570dccandd917ed8here; the rest is #59.Closes #24. Successor to #43 (closed), taking a different approach after measurement; alternative to #38 / #42.
Why
When spending unconfirmed UTXOs, miners evaluate the transaction as a package with its unconfirmed ancestors — if the ancestors paid below the target feerate, the child must cover the difference (CPFP). The bump is a property of the package: an ancestor shared by two candidates is paid for once, so it is not additive across candidates. That non-additivity is what broke every earlier design — per-candidate figures like
Candidate::effective_valuestop telling the truth, and every bound in branch and bound stops being a bound.#43 answered that by banning ancestor-carrying candidates from automatic selection. Measured against brute force, the ban costs up to 3.07% in answer quality when parents mostly already pay the going rate, and reports insufficient funds against a wallet that can pay on up to 100% of instances once most candidates are unconfirmed. So this PR takes the split Bitcoin Core draws between
calculateIndividualBumpFeesandcalculateCombinedBumpFee:excess,implied_fee,is_funded,drain) uses the exact combined figure, which is what the transaction actually pays.The individual sum never falls below the combined figure (pinned by proptest over 2000 generated clusters), so the search over-reserves and the difference lands in the change output, never missing from the fee.
API
The caller supplies the mempool graph; transitive ancestor closures are computed internally, and a mock block template (
Cluster::mine, the same greedy ancestor-feerate loop as Bitcoin Core'sMiniMinerfrom bitcoin/bitcoin#27021) determines what still needs bumping at the target feerate — a parent already paying its way, or carried by an overpaying child in the cluster, correctly costs nothing. Because the selector owns its target (#59), the pricing is derived at the right feerate by construction; there is no way to inject figures computed at another rate.Queries:
ancestor_bump_fee_of/effective_value_of/value_pwu_of(per candidate, onCoinSelectorbecause a bump is only meaningful at the feerate it was derived for), andselected_ancestor_bump_fee()for the package. No cap on how many candidates may carry ancestors, nothing banned,Candidateunchanged.Measurements
src/ancestor_search_experiment.rskeeps them runnable (cargo test --release ancestor_search_experiment -- --ignored --nocapture). Sweeping how many of 12 candidates carry unconfirmed ancestors, optimum by brute force: answer gap ≤ 0.11% for k ≤ 8 (vs the ban's 0.07–0.46% plus its hard failures), search cost at or below the ancestor-free baseline. The module docs record the two designs this replaced and why: the ban, and repairingLowestFee::boundwith amin_bumpfloor over the exact model (prunes almost nothing — the bound prices a greedy prefix, and the least bump reachable from the root is always zero).Known residual: the prefix's stopping condition still uses the bump-aware
is_funded, worth ~0.01% of bound calls; documented in the commit message, deliberately left for a follow-up.Co-authored with @noahjoeris, who contributed the inverted ancestor→candidate edge the first iteration was built on.
🤖 Generated with Claude Code