multi: cut VTXO listing and coin selection CPU - #728
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces significant performance optimizations to the VTXO listing and coin selection paths. It adds options to exclude checkpoint PSBTs, introduces lightweight listing and selection projection queries to skip ancestry joins, and implements an in-memory descriptor cache to memoize expensive public key parsing and taproot script reconstruction. Additionally, it transitions spend reservations to a detached, asynchronous model to prevent blocking the manager. The review feedback highlights two critical issues: first, the new vtxoDescriptorCache is not thread-safe and requires mutex protection for concurrent access; second, the asynchronous detachedReserve implementation in vtxo/manager.go incorrectly uses a timed-out context for its outcome watcher and failure reporting, which could cause failed reservations to go unreported and permanently lock liquidity.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| import ( | ||
| "errors" | ||
| "fmt" | ||
|
|
||
| "github.com/btcsuite/btcd/btcec/v2" | ||
| "github.com/btcsuite/btcd/wire" | ||
| "github.com/btcsuite/btcwallet/waddrmgr" | ||
| "github.com/lightninglabs/neutrino/cache" | ||
| "github.com/lightninglabs/neutrino/cache/lru" | ||
| ) |
There was a problem hiding this comment.
The vtxoDescriptorCache is accessed concurrently by multiple goroutines (e.g., during concurrent RPC requests). Since lru.Cache is not thread-safe, concurrent access to get and put will cause data races and potential memory corruption. We should add a sync.Mutex to protect the cache operations.
| import ( | |
| "errors" | |
| "fmt" | |
| "github.com/btcsuite/btcd/btcec/v2" | |
| "github.com/btcsuite/btcd/wire" | |
| "github.com/btcsuite/btcwallet/waddrmgr" | |
| "github.com/lightninglabs/neutrino/cache" | |
| "github.com/lightninglabs/neutrino/cache/lru" | |
| ) | |
| import ( | |
| "errors" | |
| "fmt" | |
| "sync" | |
| "github.com/btcsuite/btcd/btcec/v2" | |
| "github.com/btcsuite/btcd/wire" | |
| "github.com/btcsuite/btcwallet/waddrmgr" | |
| "github.com/lightninglabs/neutrino/cache" | |
| "github.com/lightninglabs/neutrino/cache/lru" | |
| ) |
| type vtxoDescriptorCache struct { | ||
| entries *lru.Cache[vtxoDescriptorCacheKey, *vtxoDescriptorCacheValue] | ||
| } |
There was a problem hiding this comment.
Add a sync.Mutex to the vtxoDescriptorCache struct to ensure thread-safe access to the underlying lru.Cache.
| type vtxoDescriptorCache struct { | |
| entries *lru.Cache[vtxoDescriptorCacheKey, *vtxoDescriptorCacheValue] | |
| } | |
| type vtxoDescriptorCache struct { | |
| mu sync.Mutex | |
| entries *lru.Cache[vtxoDescriptorCacheKey, *vtxoDescriptorCacheValue] | |
| } |
| func (c *vtxoDescriptorCache) get(op wire.OutPoint) (*vtxoDescriptorCacheValue, | ||
| bool) { | ||
|
|
||
| if c == nil || c.entries == nil { | ||
| return nil, false | ||
| } | ||
|
|
||
| cached, err := c.entries.Get(keyForOutpoint(op)) | ||
| if err != nil { | ||
| return nil, false | ||
| } | ||
|
|
||
| return cached, true | ||
| } |
There was a problem hiding this comment.
Lock the mutex during cache retrieval to prevent concurrent read/write races on the LRU list.
| func (c *vtxoDescriptorCache) get(op wire.OutPoint) (*vtxoDescriptorCacheValue, | |
| bool) { | |
| if c == nil || c.entries == nil { | |
| return nil, false | |
| } | |
| cached, err := c.entries.Get(keyForOutpoint(op)) | |
| if err != nil { | |
| return nil, false | |
| } | |
| return cached, true | |
| } | |
| func (c *vtxoDescriptorCache) get(op wire.OutPoint) (*vtxoDescriptorCacheValue, | |
| bool) { | |
| if c == nil || c.entries == nil { | |
| return nil, false | |
| } | |
| c.mu.Lock() | |
| defer c.mu.Unlock() | |
| cached, err := c.entries.Get(keyForOutpoint(op)) | |
| if err != nil { | |
| return nil, false | |
| } | |
| return cached, true | |
| } |
| func (c *vtxoDescriptorCache) put(op wire.OutPoint, | ||
| value *vtxoDescriptorCacheValue) error { | ||
|
|
||
| if c == nil || c.entries == nil { | ||
| return nil | ||
| } | ||
|
|
||
| if _, err := c.entries.Put(keyForOutpoint(op), value); err != nil && | ||
| !errors.Is(err, cache.ErrElementNotFound) { | ||
| return fmt.Errorf("put descriptor cache: %w", err) | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
Lock the mutex during cache insertion to prevent concurrent write races on the LRU list.
| func (c *vtxoDescriptorCache) put(op wire.OutPoint, | |
| value *vtxoDescriptorCacheValue) error { | |
| if c == nil || c.entries == nil { | |
| return nil | |
| } | |
| if _, err := c.entries.Put(keyForOutpoint(op), value); err != nil && | |
| !errors.Is(err, cache.ErrElementNotFound) { | |
| return fmt.Errorf("put descriptor cache: %w", err) | |
| } | |
| return nil | |
| } | |
| func (c *vtxoDescriptorCache) put(op wire.OutPoint, | |
| value *vtxoDescriptorCacheValue) error { | |
| if c == nil || c.entries == nil { | |
| return nil | |
| } | |
| c.mu.Lock() | |
| defer c.mu.Unlock() | |
| if _, err := c.entries.Put(keyForOutpoint(op), value); err != nil && | |
| !errors.Is(err, cache.ErrElementNotFound) { | |
| return fmt.Errorf("put descriptor cache: %w", err) | |
| } | |
| return nil | |
| } |
| func (m *Manager) detachedReserve(ctx context.Context, ref VTXOActorRef, | ||
| op wire.OutPoint, event actormsg.VTXOActorMsg, label string) { | ||
|
|
||
| log := m.logger(ctx) | ||
| managerRef := m.managerRef | ||
|
|
||
| askCtx, cancel := context.WithTimeout( | ||
| context.WithoutCancel(ctx), detachedReserveTimeout, | ||
| ) | ||
|
|
||
| future := ref.Ask(askCtx, event) | ||
| future.OnComplete( | ||
| askCtx, func(res fn.Result[actormsg.VTXOActorResp]) { | ||
| defer cancel() | ||
|
|
||
| _, err := res.Unpack() | ||
| if err == nil { | ||
| return | ||
| } | ||
|
|
||
| log.WarnS(askCtx, label+" detached reserve failed", | ||
| err, | ||
| slog.String("outpoint", op.String()), | ||
| ) | ||
|
|
||
| if managerRef == nil { | ||
| return | ||
| } | ||
|
|
||
| tellErr := managerRef.Tell( | ||
| askCtx, &spendReservationFailedMsg{ | ||
| Outpoint: op, | ||
| }, | ||
| ) | ||
| if tellErr != nil { | ||
| log.WarnS(askCtx, "Failed to report detached "+ | ||
| "reserve failure", tellErr, | ||
| slog.String("outpoint", op.String()), | ||
| ) | ||
| } | ||
| }, | ||
| ) | ||
| } |
There was a problem hiding this comment.
Using askCtx (which has a 30-second timeout) for both OnComplete and managerRef.Tell introduces a critical bug. If the reservation times out, askCtx is canceled. This can cause OnComplete to skip executing the callback (if it de-registers on context cancellation), or cause managerRef.Tell to fail immediately because its context is already canceled. As a result, the failure is never reported back to the manager, leaving the VTXO permanently locked in the in-memory reserved map. Using a detached context (context.WithoutCancel(ctx)) for OnComplete and Tell ensures the failure is always reliably reported.
func (m *Manager) detachedReserve(ctx context.Context, ref VTXOActorRef,
op wire.OutPoint, event actormsg.VTXOActorMsg, label string) {
log := m.logger(ctx)
managerRef := m.managerRef
detachedCtx := context.WithoutCancel(ctx)
askCtx, cancel := context.WithTimeout(
detachedCtx, detachedReserveTimeout,
)
future := ref.Ask(askCtx, event)
future.OnComplete(
detachedCtx, func(res fn.Result[actormsg.VTXOActorResp]) {
defer cancel()
_, err := res.Unpack()
if err == nil {
return
}
log.WarnS(detachedCtx, label+" detached reserve failed",
err,
slog.String("outpoint", op.String()),
)
if managerRef == nil {
return
}
tellErr := managerRef.Tell(
detachedCtx, &spendReservationFailedMsg{
Outpoint: op,
},
)
if tellErr != nil {
log.WarnS(detachedCtx, "Failed to report detached "+
"reserve failure", tellErr,
slog.String("outpoint", op.String()),
)
}
},
)
}![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 897b8f41f3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Reverse direction: a reservation row whose VTXO row is NOT in | ||
| // SpendingState means the owning session checkpointed but the | ||
| // detached Spending status write never landed before the shutdown. | ||
| // The session resumes on this boot and still owns the input, so | ||
| // re-mark the in-memory reservation and re-drive the reserve event | ||
| // to converge the durable status. Without this, a restarted daemon | ||
| // could select an input an in-flight session owns. |
There was a problem hiding this comment.
When a daemon restarts after an OOR session checkpoints its reservation row but before the detached SpendReserveEvent writes SpendingState, the reserved input is still Live, so spending is empty. The early len(spending) == 0 return above this new reverse-direction recovery means ListReservedOutpoints is never read and this block never re-marks or re-drives the reservation, leaving the live row selectable by another spend even though the resumed session still owns it. Move the reservation-list/read and redrive path before that early return, or only skip the orphan-release half when there are no Spending rows.
Useful? React with 👍 / 👎.
897b8f4 to
bb9bb3b
Compare
5a88bd8 to
dcf9293
Compare
b9242ad to
5a4b20d
Compare
dcf9293 to
9e6f144
Compare
5a4b20d to
811adab
Compare
6be5a1d to
8e93cfb
Compare
1e1ebf7 to
e0b941e
Compare
Profiling at ~9.5 payments/s showed ListVTXOsByStatus consuming 32% of all CPU samples, with 26% in the per-row descriptor decode. Coin selection runs that listing on every payment but only consumes the outpoint, amount, and pkScript columns, so this adds a projection query returning exactly those three fields. The Go-side switch to the projection lands separately.
Profiling at ~9.5 payments/s showed coin selection responsible for a large slice of the hottest CPU path: selectAndReserveVTXOs listed all live VTXOs through the full descriptor decode on every payment, paying pubkey parsing, taproot script reconstruction, policy template decode, and the batched ancestry query for fields it never reads. Selection consumes exactly the outpoint, the amount, and the pkScript. VTXOStore gains ListSelectionCandidatesByStatus, returning SelectedVTXO projections straight from the new sqlc query. The manager wraps the rows as minimal descriptors at its single call site so the shared largest-first machinery (dust handling, liquidity classification) stays unchanged; the partial descriptors never escape the selection function, whose response already carries the same three fields.
Profiling at ~9.5 payments/s put ListVTXOsByStatus at 32% of all CPU samples, 26% of it in rowToDescriptor: every listing re-parsed both public keys, re-decoded the policy template (twice), and re-derived the taproot output script with live secp256k1 point math for every row, on every call. All of that material is immutable for a given outpoint, since a VTXO's script is bound to its on-chain output. rowToDescriptor now resolves the derived parts through a process-local outpoint-keyed LRU, following the same read-only sharing contract as the existing ancestry tree cache. Mutable row state (status and friends) is read fresh on every call and stamped onto a new Descriptor per row, so listings observe status transitions immediately while the expensive derivation runs once per outpoint. The cache needs no invalidation for the same reason it is correct: an outpoint can never map to different script material. This also makes the ListVTXOs RPC cheap on repeat calls, since the handler decodes through the same store.
The materialized-VTXO notification that spawns a new VTXO's actor is delivered asynchronously after the producing OOR session's commit, so a coin selection racing that window observes the committed Live row before the actor exists and fails the payment with "no actor for outpoint". A 1000-payment stress run at ~9.3 payments/s hit the window twice; a notification lost outright (e.g. to a crash) used to strand the liquidity until restart. The store row is the source of truth, so a missing resident actor must never make committed liquidity unusable. The reserve loop now respawns the actor from the persisted descriptor on a map miss, on the manager goroutine where the actors map is owned. The respawn fails closed: a store miss or a no-longer-live row surfaces as a normal reservation error, and no actor is registered for liquidity that is not spendable.
811adab to
89c147f
Compare
|
@claude review this |
|
Claude finished @Roasbeef's task in 5m 44s —— View job Review: PR #728 — VTXO listing & coin selection CPU cutsRead the full diff against 1.
|
89c147f to
0fdac36
Compare
|
@claude review this |
|
Claude finished @Roasbeef's task in 4m 27s —— View job Re-review: PR #728 — VTXO listing & coin selection CPU cutsRe-reviewed the current tip of Status of the earlier findings1. 2. 3. Fresh observations (all non-blocking)
TestingStatic review only —
Summary: the two correctness blockers from the prior review are resolved in HEAD; the remaining items are test-coverage and invariant-hardening suggestions, none blocking.
|
Profiling after the listing levers showed the system wait-bound, not CPU-bound: 9.5 payments/s with 16 workers works out to an effective parallelism of ~1.4 by Little's law, with zero pacing and zero liquidity waits. The serializer is the manager goroutine, which held every payment's selection turn across a synchronous Ask per selected input, each one a mailbox hop plus an FSM turn plus a write transaction committing VTXOStatusSpending. Spend reservations are now detached. The manager marks the outpoint in a goroutine-owned in-memory reservation map, issues the Ask, and observes the child's future via OnComplete on a detached goroutine instead of awaiting it, so the selection turn ends in microseconds. A failed reservation (the candidate raced out of LiveState) hops back as a manager-internal message that drops the mark; the owning session then fails at signing or submit and retries through the existing machinery. The map gates both spend and forfeit admission ahead of the durable status, since a detached Spending write may still be in flight when the next selection lists candidates. Crash safety rests where it already rested: the spending_reservations index is written by the OOR session's checkpoint, and the startup sweep releases Spending rows with no reservation row. The sweep gains the reverse reconcile for the new write ordering: a reservation row whose VTXO row is still Live means the owner checkpointed before the detached status write landed, so the boot re-marks the reservation and re-drives the reserve event. The forfeit path keeps its synchronous ask: round participation wants the durable state settled before proceeding, and it is not on the payment hot path. Admission tests are adapted to the detached contract, including the asynchronous failure hop-back and the post-restart actor-level rejection arm.
Profiling the OOR stress workload at its post-optimization rate shows ListVTXOs at 31% of daemon CPU, and 21% of that is attaching the finalized OOR checkpoint PSBTs to every returned VTXO: each one costs a package lookup in the artifact store, which re-reads the PSBT blobs from disk on every call. Listing-only consumers such as balance views and coin selection never inspect those PSBTs, yet they pay the full price on every poll. In this commit, we add an exclude_checkpoint_psbts field to ListVTXOsRequest so those callers can opt out of the package loads. The default keeps the full response, so existing consumers (the SDK VTXO model and the swap lookup path) are unaffected.
In this commit, we honor the new exclude_checkpoint_psbts request field in the ListVTXOs handler: when set, the handler never opens the OOR artifact store, so no per-VTXO package reads happen at all. This removes the dominant CPU cost of high-frequency live-VTXO polling during the stress workload while leaving the default response shape untouched.
Profiling the stress workload at ~11.6 payments/s shows the ListVTXOs RPC still at ~40% of daemon CPU after the checkpoint-PSBT opt-out, now inside ListVTXOAncestryPathsByStatus: the batched ancestry join drags every VTXO's TLV tree fragments through SQLite's external merge sorter, and those blobs grow with OOR chain depth, so the sorter spills PMA files to disk on every listing call (~61% of CPU samples were the resulting pwrite syscalls). The listing response never carries ancestry: only the unroller walks lineage, which is exactly the split migration 000009 introduced the side table for. In this commit, we add ListVTXOsByStatusLight and ListLiveVTXOsLight to the VTXO persistence store, identical to their full counterparts except the ancestry side table is never touched, and switch the ListVTXOs RPC handler to them unconditionally. The VTXO manager's recovery and exit paths keep the full listings since their descriptors do feed unroll.
A manual unroll of a VTXO mid-OOR-spend escalates SpendingState to UnilateralExitState, but the emitted VTXOStatusUpdate omitted the ReleaseSpendReservation flag the sibling critical-expiry branch sets, so the durable spending-reservation row was left behind even though its comment claimed the same outbox shape. A stale row was previously benign, overwritten by the next legitimate spend. With the startup reservation sweep's reverse direction, a restart that rolls a recoverable exit back to LiveState now sees the orphan row, re-marks the recovered VTXO reserved, and re-drives SpendReserveEvent on an input no session owns, stranding it in SpendingState. Set the flag so the row is deleted atomically with the status change, and assert it in the SpendingState ForceUnroll transition test.
0fdac36 to
54e8a77
Compare
2 participants
In this PR, we take the CPU cost out of the VTXO listing and coin
selection paths. Profiling the stress workload at ~10/s showed
ListVTXOsByStatusalone eating 32% of daemon CPU, almost all of itdecode work and ancestry loads on rows the caller never needed in full.
Coin selection now runs on a lightweight selection-candidate projection
instead of fully materialized VTXOs: a new sqlc query returns just the
fields selection needs, and the manager consumes that shape directly. The
immutable descriptor decode is memoized by outpoint, so the expensive
parse happens once per VTXO lifetime rather than once per listing. RPC
listings skip the ancestry load outright, and
ListVTXOscallers can optout of checkpoint PSBT population via a new
exclude_checkpoint_psbtsflag (the checkpoint PSBT package loads were the dominant share of the
listing cost). Together these took
ListVTXOsByStatusfrom 32% to 4.8% ofCPU and killed the sqlite external-sorter temp-file spill that accounted
for 61% of raw syscalls on the profile.
Two changes here are correctness hardening that the faster listing path
made visible: live VTXOs whose session actor is gone are self-healed
during coin selection (respawned through the registry rather than failing
the selection), and spend reservations are delivered tell-style off the
manager turn so reservation delivery can't deadlock against the manager's
own mailbox.
This PR builds on #727 and is part of the OOR optimization train.