Prefer exact-sum token combinations in selector to avoid unnecessary change outputs Part: K=2

[Effi-S]

Fixes #2404

Part: K=2

Summary

Design proposal for reducing unnecessary change outputs in token selection by preferring exact-sum combinations of available tokens over the current greedy first-fit walk. Follows up on the lock-contention work in #2395 (PRs #2397-#2403) and the amount-blind-selection tracking issue #2017 — this is scoped specifically to change avoidance, not full amount-aware/input-minimizing selection.

This has already been through two rounds of independent adversarial review (checked against the actual sherdlock selector code, the real batched Postgres locking path, and the fetcher/cache implementation) — the design below reflects both rounds of fixes, not a first draft.

Design: Exact-Amount Token Selection (Change Avoidance)

Status: Proposal, not implemented. Tracked under
issue #2017 as a follow-on to the
lock-contention work in issue #2395
(PRs #2397–#2403). This document does not change current selector behaviour; see
selector.md for the implemented algorithm.

Revision history: two independent reviews have run against this document.

• Review 1 found the original mechanism was designed against a per-candidate "about to lock"
  decision point that doesn't exist in the batch (Postgres) locking path, and understated
  round-trip/indexing costs. Fixed by moving the completion logic out of the per-candidate
  lock call and into window construction.
• Review 2 found that fix was still wrong: checking for a completing candidate only at the
  moment the next candidate would overshoot is a strictly narrower rule than the problem
  statement needs — this document's own [30,40,70,200]/100 example is not solved by
  that rule, because by the time the check fires against gap=30, the 30-token has already
  been swept into the window and isn't available to complete it. This revision replaces the
  "check only at the boundary" rule with a full pre-search over the cached candidate set,
  run before any greedy accumulation, which does solve the example (see below).

Problem

Selector.selectInternal (token/services/selector/sherdlock/selector.go) is a greedy
first-fit: it walks candidates ascending by amount (same-amount runs shuffled, per #2399)
and stops as soon as the running sum reaches the requested quantity. It never looks for a
subset of candidates that sums to exactly the requested quantity — it stops at the first
candidate that tips the sum over the target, even when a different, still-unpicked candidate
would have completed the request with zero remainder.

Example: wallet holds [30, 40, 70, 200] (same currency), request is 100. Greedy walk:
30 → sum 30, 40 → sum 70, 70 → sum 140 ≥ 100, stop. Result: 3 inputs, sum 140, and the
transaction must produce a change output of 40 back to the same wallet. But 30 + 70 = 100 was available the whole time: 2 inputs, no change. Note this combination is not
reachable by checking only "is there a completing candidate for whatever gap remains once the
greedy walk is about to overshoot" — by the time the walk is about to overshoot (after 30
and 40 are already committed to the window), the 30 token is no longer available to pair
with 70. Finding 30 + 70 requires looking at combinations before committing to the greedy
order, not patching the greedy order at its last step.

Every unnecessary change output is a new token the wallet immediately has to track, fund a
future lock cycle for, and — per #2395 — a new opportunity for lock contention. Reducing
change outputs is therefore not just a UX/fee concern; it also shrinks the token population
that PRs #2398–#2403 are protecting from collisions.

Goals / non-goals

Goal: when an exact-sum combination of available (unlocked) candidates exists, prefer it
over a combination that overshoots and requires change — without regressing anything #2395's
stack already fixed, and in particular without adding a per-selection database round trip
beyond what the batched Postgres path (#2402) already does.

Non-goals:

• Not a general subset-sum solver. Wallets can hold many tokens; an unbounded search is a
  denial-of-service surface (a request can be crafted, or simply occur naturally under load,
  that makes selection scan or backtrack arbitrarily). The search below is bounded in both
  input count (k) and candidates scanned, and falls back to the existing greedy behaviour on
  any budget exhaustion or outright miss.
• Not a replacement for the ascending-order/bucket-shuffle candidate ordering from fix(selector): anti-join locked tokens and order candidates by amount #2399. That
  ordering exists specifically to spread contention across same-amount candidates; the fallback
  greedy walk is untouched by this proposal.
• Scoped to sherdlock only. simple has no ordered, windowed fetcher and no anti-join —
  building a bounded search on top of it would mean adding most of fix(selector): anti-join locked tokens and order candidates by amount #2399's machinery to a
  driver whose whole point is to stay minimal. If exact-match selection is wanted under
  simple later, that is a separate proposal.

Design

Core idea: a bounded pre-search over the cache, run before greedy accumulation starts

The mechanism is a distinct step that runs before selectInternal's existing window-growth
loop (sherdlock/selector.go:264-351) is entered for a given remaining = quantity - sum, not
a patch applied inside that loop:

1. Pre-search: look for a small set of up to k unlocked, not-yet-blacklisted candidates in
   the fetcher's already-loaded cache for this wallet+currency whose amounts sum to exactly
   remaining. This never calls s.next() and never touches the iterator cursor — it reads a
   snapshot of the cache's sorted-by-amount data (see Fetcher/cache interaction).
2. If found: that subset becomes window directly — the existing window-growth loop is
   skipped entirely for this attempt — and is claimed through the exact same single
   TryLockBatch call, with the exact same win/loss handling (sherdlock/selector.go:301-351)
   that already exists today. No new locking logic, no new race handling: the win/loss/blacklist
   code at lines 322-336 already tolerates a window where not every entry wins the race, so a
   partially-contended exact-sum window degrades exactly the way a partially-contended greedy
   window does today — the losers are blacklisted, sum includes only the winners, and the
   outer loop continues (which will re-run the pre-search against the new, smaller remaining
   before falling back to greedy again).
3. If not found within the scan/depth budget: fall through unchanged to today's window-growth
   loop. Because the pre-search never consumed anything from the iterator, nothing is displaced,
   discarded, or double-counted — the greedy walk starts exactly where it would have without this
   feature.

Running the search before any greedy commitment, over the full cached candidate set rather
than a shrinking suffix, is what makes 30 + 70 = 100 reachable: both tokens are still
"available" from the pre-search's point of view, because neither has been swept into a window
yet.

Algorithm

Given remaining = quantity - sum and the cache's ascending-sorted candidate list for this
wallet+currency (minus already-blacklisted/locked entries):

• k = 1 (single completing token): binary search the sorted cache for amount == remaining. O(log n).
• k = 2 (a completing pair): a standard two-pointer scan over the sorted cache — lo = 0,
  hi = n-1; while lo < hi: if cache[lo]+cache[hi] == remaining, record the pair; if < remaining, lo++; if >, hi--. O(n) in the cache size for this wallet+currency, one pass,
  no nested loop and no per-candidate point-query — this replaces the first revision's "for each
  candidate a, look up remaining - a" sketch, which could not be bounded cleanly against a
  point-query budget. Continue the scan (bounded by maxTieBreakCandidates, default 4) to collect
  a handful of valid pairs rather than stopping at the first, then shuffle among them before
  picking one — the same anti-hotspot rationale as fix(selector): anti-join locked tokens and order candidates by amount #2399's bucket shuffle, now stated as an
  actual rule instead of an open question.
• k > 2: not proposed. Cost grows combinatorially and the marginal benefit over k = 1..2
  is expected to shrink; left as a future, separately-justified change if data says otherwise.

Recommended default: k = 1 first, since it is the cheapest and covers the common
single-completing-token case (including cases the ascending-order fix in #2399 does not
already cover — see the worked example above). k = 2 is the next increment, costed and
bounded above rather than left vague.

Point-query fallback, and why it is now a backstop rather than the common path. The pre-search
above assumes the cache holds the wallet+currency's full spendable set, not a small lookahead
window. That is in fact how the cache is populated: cachedFetcher.update() loads via an
unfiltered SpendableTokensIteratorBy call and buckets the entire result by wallet+currency
key, with no per-key truncation — so for the wired fetcher configuration, the pre-search
ordinarily has the complete candidate set to work with already, in memory, with no additional
query. A point-query fallback (amount = target_gap, same anti-join predicate as
buildSpendableTokensIteratorByQuery, LIMIT a small constant) is kept only as a backstop for
the case the in-cache search comes up empty because the cache is momentarily stale relative to
the DB (a token created after the last refresh) — capped at maxPointQueries (default: 1) per
Select() call, and only ever attempted for k = 1 (the k = 2 two-pointer scan has no
per-candidate point-query fallback at all, by design — see Budgets below).

Costs and tradeoffs

• Indexing: a new index is required, not optional. TokenStore.GetSchema
  (token/services/storage/db/sql/common/tokens.go) defines five indexes on Tokens, none of
  which reference amount; the ORDER BY amount added in fix(selector): anti-join locked tokens and order candidates by amount #2399 is an unindexed sort today,
  affordable only because per-wallet candidate sets are small. The point-query backstop above
  needs a composite index, e.g. (owner_wallet_id, token_type, amount), matching the existing
  partial-index style already used for idx_owner_wallet_part, to stay a cheap lookup rather
  than a sequential scan. This index has a write-path cost — one more B-tree entry maintained on
  every token creation and consumption — that has to be weighed against the read-side benefit in
  the benchmark below; this document does not yet have production insert/delete-rate numbers to
  quantify that tradeoff and treats it as an open rollout question, not a settled one.
• Round-trip cost in the common case is genuinely zero, verified against the real batch-lock
  call. TryLockBatch/the Postgres LockBatch implementation treats its token-ID list as an
  unordered set — multi-row INSERT ... ON CONFLICT and the FOR UPDATE SKIP LOCKED join
  impose no ordering or contiguity requirement — so handing it a pre-search-selected window
  instead of a greedily-grown one is exactly as costly as today's single TryLockBatch call:
  no second lock round trip is introduced. The only added round trip is the point-query
  backstop, and it fires only on a stale-cache miss, which should be rare given the cache
  contains the full candidate set by construction (see above) — but "should be rare" still needs
  the benchmark in Rollout to become "is rare in practice."
• Budgets are split by cost, not conflated. maxLookaheadInputs (k) bounds combinatorial
  depth; a separate maxPointQueries (default 1, k = 1 only) bounds actual DB round trips;
  a separate maxTieBreakCandidates (default 4) bounds how many same-sum pairs the k = 2 scan
  collects before shuffling, all independent of each other so a cheap in-memory scan is never
  throttled by a budget meant for expensive round trips.
• No new race class. A pre-search-selected subset is locked through the exact same
  TryLockBatch call and existing win/loss/blacklist bookkeeping as any other window; a
  concurrent selector finding and racing for the same subset loses exactly the way any other
  contended-token race is lost today.
• Precision. The equality checks (amount == remaining, cache[lo]+cache[hi] == remaining)
  reuse the same token2.Quantity type and arithmetic the existing sum logic already uses
  (fixed-precision, NUMERIC(78,0)-backed, not floats), scoped to the same wallet+currency+type
  candidate set as the surrounding query — no new precision or multi-currency concern.

Fetcher/cache interaction

cachedFetcher.update() loads the entire spendable-token set (unfiltered by wallet/currency)
and shards it into per-wallet+currency slices with no truncation, and UnspentTokensIteratorBy
hands back a fresh permutation of that per-key slice. The pre-search reads that same per-key
slice directly — a plain sorted-by-amount view, not the shuffled permutation the greedy walk
uses for iteration order — so it sees the wallet's complete cached candidate set for the
requested currency and never disturbs the iterator the greedy fallback will use if the
pre-search misses. This requires one small, additive fetcher API surface: a way to read the
current cached slice for a wallet+currency without minting a new permutation or advancing any
cursor (e.g. a PeekSortedCandidates accessor alongside UnspentTokensIteratorBy), which does
not change the existing iterator's public behaviour.

One asymmetry is worth stating plainly: the point-query backstop reads live from the database,
bypassing the Ristretto cache layer entirely, while the cache the pre-search normally uses may
be serving data up to fetcherCacheRefresh stale. The backstop is therefore more current than
the common path, not less — intentional, since it exists specifically to catch what the cache
missed, but worth calling out rather than leaving implicit.

Cost of running the pre-search itself. Because it operates on an in-memory, already-fetched
slice, k = 1's binary search and k = 2's two-pointer scan are cheap relative to any DB
round trip — but not free for a pathological wallet holding a very large number of tokens for
one currency (e.g. many dust outputs), especially since selectInternal's outer loop can re-run
the pre-search on every immediate retry (up to maxImmediateRetries = 5). Bound this with a
maxScanCandidates cap: if the cached slice exceeds the cap, the pre-search only considers its
maxScanCandidates smallest entries (the slice is already sorted ascending, so this is a
deterministic truncation, not a random subset) — this only reduces the feature's recall for
unusually large per-currency candidate sets, it never affects correctness, since the fallback
greedy walk still runs when the pre-search doesn't find anything.

Interaction with existing mechanisms

• Anti-join / already-locked exclusion (fix(selector): anti-join locked tokens and order candidates by amount #2399): unchanged and reused — cached candidates
  were already filtered by the anti-join at fetch time, and the point-query backstop uses the
  same NOT EXISTS predicate, so the pre-search never proposes a candidate someone else already
  holds.
• Bucket-shuffle within same amount (fix(selector): anti-join locked tokens and order candidates by amount #2399): unchanged for the fallback greedy walk. The
  pre-search has its own, analogous shuffle for k = 2 tie-breaking (see Algorithm above); k = 1 has no ties to break (only one candidate can equal remaining).
• Immediate-retry / blacklist (fix(selector): sherdlock per-attempt lock blacklist (Phase 3 of #2395) #2398): a pre-search-selected candidate that loses its lock
  race is added to the same per-Select() blacklist as any other candidate, so a refetch during
  the immediate-retry layer, and the pre-search that runs again after it, won't immediately
  re-attempt it.
• Backoff / StubbornSelector layer: untouched — a full retry still re-runs
  selectInternal from scratch, re-deriving the pre-search fresh each time.
• Release on settlement (fix(selector): release token locks on transaction settlement #2400): unaffected; this proposal only changes which candidates are
  chosen among already-available ones, not the lock lifecycle.
• Lock-acquisition strategy (feat(selector): configurable Postgres lock-acquisition strategies (Phase 6, #2395) #2402): orthogonal — a pre-search-selected window is claimed
  through whichever lockStrategy is configured, exactly like any other window, and the
  round-trip analysis above is specifically about not undoing what feat(selector): configurable Postgres lock-acquisition strategies (Phase 6, #2395) #2402 achieves.

Configuration

Extend the existing token.selector YAML block (all new, all optional, defaulting to
preserving today's behaviour when unset):

token:
  selector:
    exactMatch:
      enabled: false                # default off until benchmarked (see Rollout)
      maxLookaheadInputs: 1         # k: 1 (single completing token) or 2 (completing pair)
      maxScanCandidates: 256        # cheap in-memory cap on cache entries the pre-search considers
      maxTieBreakCandidates: 4      # k=2 only: how many equal-sum pairs to collect before shuffling
      maxPointQueries: 1            # k=1 only: DB round-trip budget per Select() call, separate
                                     # from the free in-memory scan above

simple reads and ignores this block, same pattern as lockStrategy today.

Observability

Add counters alongside the contention metrics already introduced in #2397
(LockConflicts, DistinctTokensAttempted): ExactMatchAttempts, ExactMatchCacheHits,
ExactMatchPointQueryHits, ExactMatchMisses. Splitting cache hits from point-query hits (not
just an aggregate "hit") is what lets the round-trip cost claim above be checked against real
traffic instead of assumed.

Testing

• Unit: table-driven cases over synthetic candidate sets, explicitly including the
  [30,40,70,200]/100 example from Problem (must select {30,70}, not {30,40,70}), plus:
  no completion needed (single candidate already exact), k=1 cache hit, k=1 point-query
  backstop hit, k=2 pair found with multiple ties (verify shuffling occurs across repeated
  runs), a contended pre-search subset that partially loses its race (falls through to a
  second, smaller pre-search / greedy pass), no completion exists at any depth (unchanged
  greedy result, and identical to today's output), maxScanCandidates/maxPointQueries
  exhausted (falls back without hanging or an extra round trip).
• A dedicated test asserting the pre-search path issues exactly one TryLockBatch call (not
  two) for a cache-hit window — the concrete, testable form of the "zero additional round trips
  in the common case" claim.
• Extend TestHotTokenContention-style benchmarks (added in test(selector): close lock-contention testable gaps + extend sherdlock benchmarks (Phases 7-8, #2395) #2403) with an "exact-match enabled"
  variant, measuring RoundTrips() and cache-hit vs. point-query-hit rates, to confirm the added
  cost stays within the claims above under realistic contention.
• Fuzz: none required — this logic never parses untrusted bytes; it operates on amounts already
  validated by the token store.

Rollout

1. Land behind exactMatch.enabled: false (default off), scoped to sherdlock only, including
   the new PeekSortedCandidates-style fetcher accessor and the (owner_wallet_id, token_type, amount) index.
2. Benchmark with the extended TestHotTokenContention variant above, plus a "change-output
   rate" measurement against a synthetic wallet population (the metric this feature is meant to
   move), measuring both the read-side round-trip cost and the write-side index-maintenance cost,
   before flipping the default.
3. Only after that data is in, consider flipping the default to true and revisit whether k = 2 is worth enabling by default given its extra scan/tie-break cost — a separate, data-driven
   decision, not part of this initial rollout.

[AkramBitar]

Two items before merge.

1. itest (dvp-fabtoken) failed
Confirm it is a flake (re-run or check the log) before merging.

2. releasePreSearchLocks calls UnlockAll — fragile across multiple pair attempts
lockExactMatchPair calls UnlockAll to release a partial pair lock. The comment says 'only the pre-search's own partial acquisition' — that holds today because pairs are tried sequentially and each call enters with a clean lock set. But if two pairs are tried and both fail their second token, UnlockAll is called twice. The invariant is correct now but breaks if the pair loop is ever parallelised or if the call order changes.

Switch to unlocking by token ID instead of UnlockAll, or add a test that asserts UnlockAllCallCount() equals exactly the number of failed second-lock attempts — so any future change that breaks the invariant is caught immediately.

[Effi-S]

Two items before merge.

1. itest (dvp-fabtoken) failed Confirm it is a flake (re-run or check the log) before merging.

2. releasePreSearchLocks calls UnlockAll — fragile across multiple pair attempts lockExactMatchPair calls UnlockAll to release a partial pair lock. The comment says 'only the pre-search's own partial acquisition' — that holds today because pairs are tried sequentially and each call enters with a clean lock set. But if two pairs are tried and both fail their second token, UnlockAll is called twice. The invariant is correct now but breaks if the pair loop is ever parallelised or if the call order changes.

Switch to unlocking by token ID instead of UnlockAll, or add a test that asserts UnlockAllCallCount() equals exactly the number of failed second-lock attempts — so any future change that breaks the invariant is caught immediately.

1. Seems like a flake to me - rerunning to be sure..
2. Added the unit test as you suggested

ready for re review

[AkramBitar]

Bottom line: finding 1 is a correctness bug and should block merge. The rest are a same-pass fix (2) and polish (3-5).

The design doc and the MultiplePartialPairLocksUnwindOncePerFailure test pin down the intra-selection half of the unwind invariant nicely — the problem is that the invariant does not survive a second Select on the same consumer tx.

Verification: go test ./token/services/selector/... is green at this head and go vet is clean; findings 1 and 2 were each reproduced with a throwaway unit test (not included here). The branch was force-pushed from 7c9d0a658 to ff43de0ff during the review — I checked it was a pure rebase (git diff across the two on the touched files is empty), so all line anchors below are against the current head.

[AkramBitar]

MEDIUM — pre-search lock attempts are invisible to the contention metrics added in #2397.

selectInternal threads an attempted set through the greedy walk and records every TryLock target into it, and increments LockConflicts on driver.ErrTokenAlreadyLocked. tryExactMatch is never passed attempted, and no pre-search path touches LockConflicts.

With k=2 enabled, one selection can issue up to 1 + 4x2 = 9 lock attempts and lose up to 8 lock races while DistinctTokensAttempted reports 0 extra tokens and LockConflicts stays flat. A pre-search hit is worse: it lands as "no attempt at all", because observeDistinctTokensAttempted skips the empty set. That is under-reporting in precisely the high-contention scenario those metrics were added to diagnose.

[Effi-S]

@AkramBitar,
I fundamentally disagree here,
#2397 metrics measure the greedy walk .
This pre-search is not a walk.
It's a speculative probe designed to lose cleanly and fall through.