vtxo+wallet: FSM-based admission gating for VTXO spend and forfeit - #175
Conversation
1856bd7 to
30d3d49
Compare
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors VTXO management by introducing a state machine-based admission gating system. Instead of relying on external in-memory locks, the availability of a VTXO is now intrinsically tied to its FSM state, which is persisted across restarts. This change enhances the system's resilience against crashes and ensures a single, consistent source of truth for VTXO status, streamlining both out-of-round spend and cooperative forfeit processes. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 significant architectural improvement by using the VTXO's FSM state as a locking mechanism for spend and forfeit operations. This fundamentally addresses potential race conditions and double-spending issues, especially across restarts, by creating a single source of truth for VTXO availability.
The changes are well-structured and thoughtfully implemented:
- A new
VTXOManageractor serves as a single admission gate for all VTXO operations, centralizing control and simplifying logic in other parts of the system. - New FSM states (
SpendingState,PendingForfeitState) are introduced to represent the locked states, and these are persisted to the database to survive restarts. - The wallet and OOR modules are cleanly refactored to use this new admission gating system, forwarding requests to the manager instead of handling locking logic themselves.
- The two-phase commit-like pattern for cooperative forfeits (reserve, then register intent, with rollback on failure) is a robust solution for coordinating state changes across multiple actors.
The code is clear, well-documented, and accompanied by an extensive and thorough set of tests that cover numerous success, failure, and recovery scenarios. This is an excellent contribution that significantly enhances the robustness and reliability of the VTXO lifecycle management.
30d3d49 to
dde85aa
Compare
|
@claude review this |
|
Claude finished @ellemouton's task in 3m 43s —— View job Code Review
SummaryThis PR replaces an in-memory lock set with FSM-state-as-lock, a fundamentally stronger design. The architecture is sound: the manager is the single serialization point, persistence through Issues1. Stale // PR 2 may add reservation checks here, but the default must always be
// to relay forfeit requests promptly.This PR is PR 2. The comment should either be removed or updated to reflect the current design decision (that auto-expiry forfeits bypass admission on purpose). 2. Partial release failure leaves VTXOs permanently locked
The rollback helpers // Current (problematic):
for _, op := range outpoints {
result := ref.Ask(ctx, &SpendReleasedEvent{}).Await(ctx)
if _, err := result.Unpack(); err != nil {
return fn.Err[ManagerResp](fmt.Errorf("release %s: %w", op, err))
// ^ remaining VTXOs are never released
}
}3. In NextState: &LiveState{
VTXO: s.VTXO,
LastCheckedHeight: 0, // ← loses expiry context
},
Consider storing NextState: &LiveState{
VTXO: s.VTXO,
LastCheckedHeight: s.RequestedAtHeight,
},4. In // NOTE: Placed after VTXOStatusFailed to preserve the numeric
// values of existing statuses used in SQL queries.
VTXOStatusSpendingThe comment is correct but easy to miss. 5. In // Still safe, stay in SpendingState.
return &VTXOStateTransition{NextState: s}, nilThe VTXO may not be safe — it may need refresh — but the comment says it's safe. This is intentional (you can't forfeit while spending), but the comment should reflect the actual reasoning: "Refresh is blocked while spending; if the spend completes promptly, the VTXO will return to LiveState and handle refresh from there." 6. In 7. Lines 1096, 1139, and 1167 each suppress mgrResp, ok := resp.(*actormsg.SelectAndReserveSpendResponse)
if !ok {
return fn.Err[WalletResp](fmt.Errorf("unexpected response type: %T", resp))
}Observations / Non-Issues
|
dde85aa to
f553a3f
Compare
13f9daf to
61f031b
Compare
f553a3f to
b3b518b
Compare
a15ea97 to
696ae8b
Compare
cdb91b3 to
17fffd2
Compare
17fffd2 to
01ba349
Compare
Add explicit spend-related states to the VTXO FSM so that availability is represented directly in actor state rather than a separate reservation layer. New states: - SpendingState (non-terminal, persisted as VTXOStatusSpending) - SpentState (terminal, maps to existing VTXOStatusSpent) New events: - SpendReserveEvent: Live -> Spending - SpendReleasedEvent: Spending -> Live - SpendCompletedEvent: Spending -> Spent - ForfeitReleasedEvent: PendingForfeit -> Live Key design decisions: - SpendingState preserves expiry monitoring via LastCheckedHeight so critical expiry still triggers UnilateralExitState - VTXOStatusSpending is placed at iota value 7 (after Failed) to preserve existing SQL hardcoded status values - ListLiveVTXOs query updated to include status=7 for recovery - Conflict rules: Spending rejects PendingForfeit, PendingForfeit rejects SpendReserve This is commit 1 of the VTXO coin selection PR, laying the FSM foundation for manager-coordinated admission in subsequent commits.
Add manager message handlers that coordinate VTXO admission for both OOR spend and cooperative forfeit operations. The manager is the admission orchestrator, but actor state owns the lock. New manager messages: - SelectAndReserveSpendRequest: largest-first coin selection + atomic reservation via SpendReserveEvent on each actor - ReleaseSpendRequest: release spend claims back to LiveState - CompleteSpendRequest: finalize OOR spend to SpentState - ReserveForfeitRequest: reserve specific outpoints for forfeit - ReleaseForfeitRequest: release forfeit claims back to LiveState Key design: - Atomic admission: all requested VTXOs enter claimed state or all are rolled back on any failure - Unknown outpoints rejected eagerly before reservation - Rollback is best-effort (logged but non-fatal) - Coin selection uses largest-first with store-level filtering Tests cover: successful selection, insufficient funds, double exclusion, spend release/completion, forfeit reserve/release, cross-direction conflicts, partial failure rollback, and unknown outpoint rejection.
The FSM extension in the prior commit added VTXOStatusSpending as a persisted lifecycle state, but the RPC proto enum and conversion helper were not updated. Without this, ListVTXOs would serialize Spending VTXOs as VTXO_STATUS_UNSPECIFIED, hiding the state from clients. Add VTXO_STATUS_SPENDING = 8 to the proto enum and handle the new case in vtxoStatusToProto.
Move admission request/response types (SelectAndReserveSpend, ReleaseSpend, CompleteSpend, ReserveForfeit, ReleaseForfeit) from vtxo/messages.go to actormsg/vtxo_admission.go so both the wallet and vtxo packages can reference them without an import cycle (wallet → vtxo → round → wallet). Add VTXOManagerResp marker interface and VTXOManagerServiceKey to actormsg, following the same service key pattern used for the round actor. The vtxo.ManagerResp type becomes an alias for the actormsg marker, and vtxo/messages.go re-exports the moved types as aliases for backwards compatibility. Add wallet forwarding handlers (handleSelectAndLockVTXOs, handleUnlockVTXOs, handleCompleteSpendVTXOs) that look up the VTXO manager via service key and translate between wallet messages and actormsg admission types. A shared askManager helper reduces Ask/Await boilerplate.
683e6c2 to
3715fdd
Compare
Replace the direct VTXOStatusSpent store write in the OOR local persistence handler with a SpendCompleter callback that routes completion through the VTXO manager. Each consumed VTXO now transitions to SpentState via its own FSM, keeping the VTXO actor as the single source of truth for availability state. The handler retains a fallback to direct store writes when the callback is nil, for backwards compatibility during migration. Production wiring in darepod sends CompleteSpendRequest to the VTXO manager via the actor system service key.
Reserve forfeit inputs through the VTXO manager before sending RegisterIntentMsg to the round actor. If round registration fails, the wallet releases the reservations so VTXOs return to LiveState. This removes the round actor's post-registration PendingForfeitEvent notification loop. VTXOs are now already in PendingForfeitState by the time the round receives the intent, preventing split-brain where the round has an intent for a VTXO that is still Live or claimed for OOR spend.
Wire the VTXO manager into the daemon startup sequence as step 10, between the wallet and round actors. The manager recovers persisted VTXOs on startup and gates all admission operations (OOR spend, cooperative forfeit) via its service key. Also wire VTXOManager into the round actor config via MapInputRef so the round can forward VTXOCreatedNotification to spawn VTXO actors for newly created VTXOs. The ManagerMsg type is changed from an interface embedding to a type alias for actormsg.VTXOManagerMsg so the manager can be registered directly with the actormsg service key without generic type mismatch.
Update vtxo, wallet, and round package CLAUDE.md files to reflect the new admission model: manager-gated VTXO operations, wallet-driven cooperative admission before round registration, and removal of round-side PendingForfeit marking.
Sync AGENTS.md with CLAUDE.md for round, vtxo, and wallet packages after the admission model doc updates. Add bin/* to .gitignore so compiled binaries are not tracked.
These were scaffolding docs for the VTXO refactor PR series and are no longer needed.
3715fdd to
0943444
Compare
A node merging several boarded asset inputs failed roughly half its commits because the SDK's commit verifier folded alternate-leaf order into a byte comparison. Repin once tap-sdk #175 merges.
A node merging several boarded asset inputs failed roughly half its commits because the SDK's commit verifier folded alternate-leaf order into a byte comparison. Repin once tap-sdk #175 merges.
A node merging several boarded asset inputs failed roughly half its commits because the SDK's commit verifier folded alternate-leaf order into a byte comparison. Repin once tap-sdk #175 merges.
A node merging several boarded asset inputs failed roughly half its commits because the SDK's commit verifier folded alternate-leaf order into a byte comparison. Repin once tap-sdk #175 merges.
2 participants
Summary
Replaces #168 with a fundamentally different approach: instead of an in-memory lock set outside the FSM, VTXO lifecycle state is the lock. The manager acts as a single admission gate for all VTXO operations — OOR spend, cooperative forfeit, and completion — using purpose-specific FSM states (
SpendingState,PendingForfeitState) that survive restarts.Depends on: #172 (
vtxo-fsm-refactor)Replaces: #168 (
vtxo-coin-select)Why This Approach Is Better
PR #168 used an in-memory
lockedVTXOs map[wire.OutPoint]struct{}that:This PR eliminates all three problems:
Admission Flow — OOR Spend
Admission Flow — Cooperative Forfeit
Mutual Exclusion
PR #168 Review Comments Addressed
askManagerhelper eliminates repetitionReleaseSpend/ReleaseForfeit; timeout deferred to follow-upm.actorsmap