client: Recover expired VTXOs through normal refresh - #1000
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 a selective expired-VTXO redemption and reissue mechanism, allowing clients to reconcile locally-expired VTXOs with the operator via a new CheckVTXORedeemability RPC. It adds a RedemptionCoordinator to manage the polling and finalization of redemptions, updates the database schema to track claims and redemptions, and enhances the fraud watcher to recognize operator sweeps. The review feedback highlights several critical improvement opportunities, including potential memory leaks in slice operations within the round actor, potential nil pointer dereferences across multiple files, database schema inconsistencies regarding outpoint index types, and a potential wedging of the redemption coordinator loop on persistent validation errors. Addressing these issues will significantly improve the robustness and reliability of the redemption flow.
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.
| slog.String( | ||
| "outpoint", req.VTXOOutpoint.String(), | ||
| ), | ||
| ) |
There was a problem hiding this comment.
Slicing a.deferredRefreshes without nil-ing out the first element causes a memory leak because the underlying array still holds a reference to the popped pointer, preventing it from being garbage collected. Nil out the pointer before slicing.
a.deferredRefreshes[0] = nil
a.deferredRefreshes = a.deferredRefreshes[1:]|
|
||
| return | ||
| } | ||
| a.deferredBoardings = a.deferredBoardings[1:] |
There was a problem hiding this comment.
Slicing a.deferredBoardings without nil-ing out the first element causes a memory leak because the underlying array still holds a reference to the popped pointer, preventing it from being garbage collected. Nil out the pointer before slicing.
a.deferredBoardings[0] = nil
a.deferredBoardings = a.deferredBoardings[1:]| continue | ||
| } | ||
|
|
||
| leaves := clientTree.Root.GetLeafNodes() |
There was a problem hiding this comment.
While clientTree is checked for nil, clientTree.Root is not. If clientTree.Root is nil, calling clientTree.Root.GetLeafNodes() will cause a nil pointer dereference panic. Add a check for clientTree.Root == nil.
| leaves := clientTree.Root.GetLeafNodes() | |
| if clientTree.Root == nil { | |
| log.WarnS( | |
| ctx, | |
| "Claim replacement tree root is missing", | |
| nil, | |
| slog.Int("claim_index", i), | |
| ) | |
| continue | |
| } | |
| leaves := clientTree.Root.GetLeafNodes() |
| heightResp, ok := heightRespAny.(*chainsource.BestHeightResponse) | ||
| if !ok { | ||
| return fmt.Errorf("query startup best height: unexpected "+ | ||
| "response %T", heightRespAny) | ||
| } |
There was a problem hiding this comment.
The type assertion heightRespAny.(*chainsource.BestHeightResponse) can succeed even if the interface value contains a nil pointer of that type. Accessing heightResp.Height would then cause a nil pointer dereference panic. Add a check to ensure heightResp is not nil.
| heightResp, ok := heightRespAny.(*chainsource.BestHeightResponse) | |
| if !ok { | |
| return fmt.Errorf("query startup best height: unexpected "+ | |
| "response %T", heightRespAny) | |
| } | |
| heightResp, ok := heightRespAny.(*chainsource.BestHeightResponse) | |
| if !ok || heightResp == nil { | |
| return fmt.Errorf("query startup best height: unexpected or nil "+ | |
| "response %T", heightRespAny) | |
| } |
| } | ||
| reconcileResp, ok := reconcileRespAny.(*vtxo.ReconcileExpiryResponse) | ||
| if !ok { | ||
| return fmt.Errorf("reconcile VTXOs: unexpected response %T", | ||
| reconcileRespAny) |
There was a problem hiding this comment.
The type assertion reconcileRespAny.(*vtxo.ReconcileExpiryResponse) can succeed even if the interface value contains a nil pointer of that type. Accessing reconcileResp.Checked would then cause a nil pointer dereference panic. Add a check to ensure reconcileResp is not nil.
| } | |
| reconcileResp, ok := reconcileRespAny.(*vtxo.ReconcileExpiryResponse) | |
| if !ok { | |
| return fmt.Errorf("reconcile VTXOs: unexpected response %T", | |
| reconcileRespAny) | |
| reconcileResp, ok := reconcileRespAny.(*vtxo.ReconcileExpiryResponse) | |
| if !ok || reconcileResp == nil { | |
| return fmt.Errorf("reconcile VTXOs: unexpected or nil response %T", | |
| reconcileRespAny) | |
| } |
| func pendingRedemptionFromDB(sourceHash []byte, sourceIndex int32, | ||
| replacementHash []byte, replacementIndex int32, |
There was a problem hiding this comment.
The database schema defines source_index and replacement_index as INTEGER (mapped to int32 in Go) in the vtxo_redemption_outbox table, whereas round_vtxo_claims defines source_index as BIGINT (mapped to int64 in Go). Since wire.OutPoint.Index is a uint32, any index greater than 2,147,483,647 will overflow to a negative value when cast to int32, triggering the negative index validation error. It is highly recommended to use BIGINT (int64) consistently for all outpoint indices across the database schema.
|
|
||
| const errMsg = "Failed to handle VTXO claim reissue" | ||
|
|
||
| if msg.AmountSat <= 0 { |
There was a problem hiding this comment.
The pointer msg is dereferenced directly without a nil check. If msg is nil, this will cause a panic. Add a nil check at the beginning of the handler.
if msg == nil {
return a.fail(
ctx, errMsg, fmt.Errorf("%w: VTXOClaimReissuedMsg is nil",
ErrInvalidMessage),
)
}
if msg.AmountSat <= 0 {| if err := c.replayPendingRedemption( | ||
| ctx, redemption, | ||
| ); err != nil { | ||
| return outcome, err | ||
| } |
There was a problem hiding this comment.
If c.replayPendingRedemption encounters a persistent validation error (such as a status mismatch or a conflicting replacement), returning the error here will cause the entire coordinator loop to back off and retry indefinitely. This wedges the coordinator and blocks all other pending redemptions from being processed. Consider logging persistent validation errors and skipping or acknowledging them rather than returning an error that halts the entire loop.
865b856 to
2fb14e9
Compare
|
@claude please review |
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Claude finished @sputn1ck's task in 4m 32s —— View job PR Review: client — Reissue swept expired VTXOsI reviewed the full PR across
🔴 Should fix before merge1. CI is red — Since 2. Redemption coordinator can wedge on a poisoned pending record ( 🟡 Minor / nice-to-have3. Unsynchronized access to 4. In-memory 5. Deferred-slice pops don't nil the popped element ( 6. Missing nil guard in ✅ Gemini flags that don't hold up (verified)
❓ Worth a maintainer confirm (not bugs)
Things that looked good
Nice work overall — the two 🔴 items are the main blockers (CI + the coordinator wedge). |
There was a problem hiding this comment.
Pull request overview
This PR adds a client-side “expired VTXO reissue” flow so wallets that were offline during an operator sweep can deterministically (and durably) detect locally-expired VTXOs, selectively reconcile them with the operator, and reissue the swept value at full amount with the same policy/pkScript—while tracking the lifecycle in RPC, persistence, and accounting.
Changes:
- Extend VTXO lifecycle with
EXPIRED/REDEEMING/REDEEMED, plus replacement linkage, and expose pending-reissue balance viaGetBalance. - Add claim-only round participation: claim inputs/quotes, join-auth binding, and durable recovery/ledger semantics for net-zero reissues.
- Add selective operator reconciliation RPC (
CheckVTXORedeemability) and durable DB schema/migrations to persist claim/redemption state.
Reviewed changes
Copilot reviewed 69 out of 76 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| waverpc/daemon.proto | Adds pending-reissue balance + new VTXO statuses/fields |
| waverpc/daemon.pb.go | Regenerated RPC bindings for new fields/statuses |
| waved/vtxo_startup_reconcile_test.go | Tests startup reconcile gating of wallet readiness |
| waved/rpc_server.go | Computes vtxo_pending_reissue_sat; maps new statuses; sets replaced_by |
| waved/config.go | Adds redemption coordinator integration hooks to daemon config |
| vtxo/transitions.go | Transitions active states to durable Expired terminal state |
| vtxo/transitions_test.go | Tests local expiry transitions across active states |
| vtxo/states.go | Adds terminal Expired/Redeeming/Redeemed states |
| vtxo/messages.go | Adds manager startup reconcile request/response messages |
| vtxo/manager.go | Adds block-trigger observer + startup expiry reconciliation + expired observer hook |
| vtxo/manager_observer_test.go | Tests redemption block observer and legacy-expired recovery path |
| vtxo/interfaces.go | Adds new VTXO statuses + replacement/redemption metadata + legacy store interface |
| vtxo/harness_test.go | Extends mock store with legacy-expired recovery method |
| vtxo/actor.go | Recovers new redemption lifecycle statuses into terminal states |
| vtxo/actor_test.go | Tests recovery of redemption lifecycle statuses |
| rpc/roundpb/round.proto | Adds claim inputs and claim quotes to round RPCs |
| round/transitions_test.go | Ensures claim reissue preserves custom policy metadata in outputs |
| round/states.go | Extends client quote state with claim quote entries |
| round/outbox_messages.go | Serializes claim inputs into JoinRoundRequest protobuf |
| round/outbox_messages_test.go | Tests claim input protobuf serialization |
| round/ledger_emit_test.go | Ensures claim reissue doesn’t double-emit generic received ledger events |
| round/join_auth.go | Includes claim inputs in join auth, supports claim-only joins |
| round/join_auth_test.go | Adds claim-only join-auth test and updates existing tests |
| round/interfaces.go | Adds claim intents and claim-only intent helpers |
| round/from_proto.go | Decodes claim quotes and claim inputs from protobuf |
| round/from_proto_test.go | Tests claim input decode round-trip |
| round/events.go | Adds claim logging/emptiness checks in intent packages |
| round/events_test.go | Updates log-attribute expectations to include claims |
| round/claim_quote_test.go | Adds quote validation tests for claim-only flows |
| round/claim_lifecycle_test.go | Tests claim replacement mapping + finalization hooks |
| round/claim_isolation_test.go | Tests that claim rounds remain isolated from ordinary registrations |
| round/actor_test.go | Tests recovery + concurrency gating updates related to registration sealing |
| lib/types/vtxo_claim.go | Adds tagged Schnorr claim authorization message/digest/verification |
| lib/types/vtxo_claim_test.go | Tests claim digest binding and signature exclusion properties |
| lib/types/codec.go | Bumps join-auth TLV version; adds claim input encoding/decoding |
| lib/types/codec_test.go | Tests join-auth claim binding + updated versioned TLV expectations |
| lib/types/boarding.go | Adds JoinRoundRequest claim inputs + round ID; adds claim-reissue origin |
| ledger/messages.go | Adds durable ledger message type for atomic claim reissue pair |
| ledger/handlers.go | Implements claim reissue handler + idempotency key derivation |
| ledger/handlers_test.go | Tests claim reissue atomic-pair and malformed rejection |
| ledger/actor.go | Routes new claim-reissue ledger message; adds source label |
| indexer/client.go | Adds selective redeemability request builder + RPC method |
| indexer/client_test.go | Tests selective redeemability request building + limits |
| fraud/watch_model.go | Tracks batch expiry and sweep tapscript root in watch model |
| fraud/messages.go | Extends spend-observed message with tx and input index |
| fraud/actor.go | Detects operator sweep leaf spends and suppresses fraud escalation after local expiry reconcile |
| fraud/actor_test.go | Tests operator sweep classification triggers expiry reconcile, not unroll |
| db/vtxo_store_test.go | Tests legacy-expired recovery and full redemption lifecycle persistence/outbox |
| db/sqlc/schemas/generated_schema.sql | Updates generated schema for claims, sweep_delay, redemption outbox, new fields |
| db/sqlc/queries/vtxo.sql | Adds queries for legacy recovery + redemption lifecycle + redemption outbox |
| db/sqlc/queries/round.sql | Persists sweep_delay, request origins, and claim rows |
| db/sqlc/querier.go | Adds generated querier methods for new queries/tables |
| db/sqlc/models.go | Adds models for claim rows, sweep_delay, redemption outbox, origins |
| db/sqlc/migrations/000016_vtxo_redemption_round.up.sql | Adds redemption round id, claim tables, sweep_delay, redemption outbox |
| db/sqlc/migrations/000016_vtxo_redemption_round.down.sql | Down migration for new redemption/claim schema |
| db/sqlc/migrations/000004_vtxos.up.sql | Documents new VTXO status enum values in base schema |
| db/round_store_test.go | Tests checkpoint recovery of claim authorization + origin/sweep_delay |
| db/migrations.go | Bumps latest migration version to 16 |
| chainbackends/lnd.go | Improves spend registration forwarder to handle reorg+replacement spends safely |
| chainbackends/lnd_test.go | Adds test ensuring replacement spend is forwarded after reorg |
| arkrpc/indexer.proto | Adds CheckVTXORedeemability RPC + response messages |
| arkrpc/indexer_mailboxrpc.pb.go | Regenerated mailbox RPC bindings for new indexer method |
| arkrpc/indexer_grpc.pb.go | Regenerated gRPC bindings for new indexer method |
Files not reviewed (7)
- arkrpc/indexer.pb.go: Generated file
- arkrpc/indexer_grpc.pb.go: Generated file
- arkrpc/indexer_mailboxrpc.pb.go: Generated file
- db/sqlc/models.go: Generated file
- db/sqlc/querier.go: Generated file
- db/sqlc/round.sql.go: Generated file
- db/sqlc/vtxo.sql.go: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ci := &roundpb.VTXOClaimInput{ | ||
| SourceOutpoint: roundpb.OutpointToProto( | ||
| claim.SourceOutpoint, | ||
| ), | ||
| Nonce: bytes.Clone(claim.Nonce[:]), | ||
| ValidFrom: claim.ValidFrom, | ||
| ValidUntil: claim.ValidUntil, | ||
| Signature: bytes.Clone(claim.Signature), | ||
| } | ||
| if claim.ParticipantPubKey != nil { | ||
| ci.ParticipantPubkey = claim.ParticipantPubKey. | ||
| SerializeCompressed() | ||
| } | ||
| if claim.ReplacementSigningKey.PubKey != nil { | ||
| ci.ReplacementSigningPubkey = | ||
| claim.ReplacementSigningKey.PubKey. | ||
| SerializeCompressed() | ||
| } |
d63a35b to
a556d84
Compare
|
@claude rereview |
|
Claude finished @sputn1ck's task in 1m 50s —— View job Re-review: client — Reissue swept expired VTXOsRe-reviewed the three follow-up commits ( 🔴 Blockers — both fixed ✅1. 2. Coordinator wedge ( 🟡 Minor items — all addressed ✅3. Data race on 4. 5. Deferred-slice pops ( 6. Startup typed-nil derefs ( ✅ Reviewer-comment items resolved
🟢 Remaining non-blocking notes (unchanged from last review)
New in this round:
|
7197b61 to
1aad4fc
Compare
|
@claude please rereview the new approach where we reuse the refresh/forfeit mechanics |
|
Claude finished @sputn1ck's task in 8m 22s —— View job Re-review: client — Reissue swept expired VTXOs (refresh/forfeit reuse approach)Re-reviewed against the reworked design. This is a much simpler and cleaner approach than the prior claim-round protocol: the whole CI: last run green; current 🟢 The reuse design is sound
🔵 Core semantic change — please confirm intentExpiry now retires to a terminal
This is defensible within Ark's trust model (post-expiry the operator's sweep path is spendable anyway, so racing it is unreliable). Worth an explicit confirm: is there any intended fallback if the operator never reports the source as redeemable? Such value sits in 🟡 Worth fixing1. if len(suppressed) > 0 {
if err := w.reconcileExpiry(ctx, msg.Height); err != nil {
return nil, err // ← also skips the escalation loop below
}
}
for target := range targets { if suppressed ... else ensureUnroll(...) }
🟢 Minor / nice-to-have2. Shared custom-refresh path now unconditionally populates output 3. 4. Doc nit: 5. Cosmetic: Verdict: The refresh/forfeit reuse is a solid simplification and the lifecycle/persistence is carefully guarded. One worth-fixing correctness gap (fraud escalation drop, item 1) and one design point to confirm (post-expiry terminal state, above). Everything else is minor. Nice rework. 👍 |
ab3e596 to
7826d5f
Compare
7826d5f to
be5a6d3
Compare
sputn1ck
changed the title
Every expiry decision is derived from blocksRemaining = BatchExpiry - currentHeight and BatchExpiry was trusted unconditionally. It is copied verbatim from the wire by the incoming-VTXO handler, so a zero arriving from the operator reads back as "expired by the entire height of the chain" and classifies a brand-new VTXO as expired. Nothing validated it at ingress and nothing guarded the arithmetic. Add HasUsableBatchExpiry and reject three shapes: a non-positive expiry, and an expiry earlier than the height the VTXO was created at, since a VTXO cannot expire before it existed. CheckExpiry now returns the new ExpiryStatusUnknown for those rather than ExpiryStatusExpired. Unknown is deliberately its own status rather than folding into either extreme. Reporting "expired" surrenders live funds on the strength of a corrupt field; reporting "safe" silently skips the refresh a real deadline needs. LiveState holds the VTXO live and warns, so a data fault neither retires the coin nor wedges the actor on every block. The status is appended last so the existing numeric values are unchanged, and the remaining call sites already treat anything that is not Critical/Expired/NeedsRefresh as inaction. The incoming handler now drops an event carrying an unusable expiry instead of materializing it. Dropping is the safer failure: the wallet still re-derives the VTXO from ListVTXOsByScripts, which reads the authoritative expiry off the server's round row, whereas a poisoned expiry persists locally and is never rewritten.
CalculateCriticalThreshold sized the unilateral-exit window from the commitment-tree depth and the CSV delay alone. It ignored ChainDepth, the number of OOR checkpoint hops between the commitment and the VTXO. Those hops are not free. Each is a recovery transaction that must confirm before the exit's final CSV even starts, and they are strictly sequential because each checkpoint spends the previous one. unroll already budgets fees this way, one recovery tx per hop, so the time budget disagreed with the fee budget. The threshold exists precisely so a client never has to race the operator's sweep, and it was under-sized for exactly the deep OOR chains that need the most room. Factor the sequential transaction count into exitTxDepth: the deepest tree path, since parallel ancestry fragments confirm concurrently and the worst branch sets the pace, plus one transaction per OOR hop. A negative hop count is treated as zero rather than being allowed to shorten the budget.
Identify the witness path that actually spent each watched VTXO instead of inferring intent from chain height. Operator batch sweeps can then retire expired watches without being escalated as client fraud.
A round is checkpointed at input_sig_sent, the point of no return, and can confirm long afterwards. The confirmation handler derives each new VTXO's absolute batch expiry as confirmation_height + sweep_delay but the delay lived only in the in-memory FSM state. A daemon restart between checkpoint and confirmation rebuilt InputSigSentState without it, so the resumed round computed an expiry of confirmation_height + 0 and stamped every VTXO it created with BatchExpiry == CreatedHeight. The wallet reads that back as already expired, which retires a VTXO that was created seconds earlier. Add a sweep_delay column to the rounds table, carry the value on round.Round, and restore it onto both the round record and the FSM state. The upsert only adopts an incoming delay when it is non-zero, since the value is fixed for the life of a round and a later checkpoint must not clear what an earlier one recorded. Rounds checkpointed before this migration have no recorded delay. For those the confirmation path now leaves the expiry unstamped rather than stamping a wrong one, and logs at error level. An unstamped expiry classifies as ExpiryStatusUnknown, so the VTXO stays live and spendable with only expiry monitoring disabled, and the authoritative expiry is still recoverable from the operator's indexer.
5c0874f to
b31e43b
Compare
Keep expired value recoverable but outside the spendable set. After chain catch-up, replay the synchronized tip through the ordinary refresh and forfeit flow while preserving in-flight locks and restart recovery. Co-authored-by: sputn1ck <kon@kon.ninja>
vtxoStatusToProto had no case for the new expired status, so an expired VTXO was reported to clients as VTXO_STATUS_UNSPECIFIED. Expiry is not terminal — the value is recovered by forfeiting the VTXO in an ordinary round — so a wallet UI has to be able to tell "expired, recoverable, not counted in spendable balance" apart from "the daemon does not know what this is". Add VTXO_STATUS_EXPIRED and map it in both directions.
Allow explicit and automatic expired refresh intents through the daemon. Count quarantined value against the boarding limit and preview the exact one-for-one recovery as fee-free. Co-authored-by: sputn1ck <kon@kon.ninja>
b31e43b to
4171deb
Compare
Summary
Design
There is no claim RPC, server sweep journal, or sweep-confirmation gate. The client decides that a VTXO is expired from BatchExpiry, then uses the normal refresh protocol. The ordinary forfeit protects the operator if the old lineage remains spendable.
This intentionally drops the one-participant override for multi-party policies. Expired custom policies use the same authorization and signing rules as any other refresh.
Validation
Companion server PR: https://github.com/lightninglabs/lumos/pull/691