multi: implement missing vtxo db persistence, add refresh route via wallet - #93
Conversation
Summary of ChangesHello @Roasbeef, 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 enhances the VTXO management system by introducing robust persistence for VTXO lifecycle states and supporting the VTXO refresh flow. It lays the groundwork for clients to forfeit old VTXOs and receive new ones, improving the system's ability to handle VTXO expiration and recovery. The changes include a new database migration, a dedicated persistence store, and a new wallet API for initiating refresh operations, all while improving type safety and ensuring crash recovery for critical transactions. 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. 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
|
| -- This approach preserves existing data while removing the new columns. | ||
|
|
||
| -- Create a temporary table with the original schema. | ||
| CREATE TABLE vtxos_backup ( |
There was a problem hiding this comment.
No need to make this new migration, can just edit the existing schema in place.
![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 and well-structured changes to support VTXO lifecycle persistence, which is a crucial step for enabling the refresh flow. The database migration adds necessary status tracking fields to the vtxos table, and the new VTXOPersistenceStore provides a clean, transaction-safe interface for managing VTXO state. The introduction of RefreshVTXOsRequest/Response messages in the wallet package correctly sets up the high-level API for this new functionality.
I've identified a critical issue in the down migration script that needs to be addressed. Additionally, there are a couple of medium-severity suggestions regarding code duplication in test helpers and formatting in a generated schema file to improve maintainability.
Overall, this is a solid contribution that lays important groundwork. The move towards stronger types like *schnorr.Signature and round.RoundID is also a great improvement for type safety and code clarity.
| -- last_update_time is the unix epoch timestamp when this VTXO was last | ||
| -- modified, such as when it was marked as spent. | ||
| last_update_time BIGINT NOT NULL, | ||
| last_update_time BIGINT NOT NULL, status INTEGER NOT NULL DEFAULT 0, forfeit_round_id TEXT, forfeit_tx BLOB, forfeit_txid BLOB, replaced_by_hash BLOB, replaced_by_index INTEGER, |
There was a problem hiding this comment.
The formatting of the new columns in the vtxos table definition is difficult to read as they are all on a single line. While this is a generated file, it would be beneficial for schema readability if each new column definition was on its own line. This might be influenced by how the ALTER TABLE statements are structured in the up migration file or a setting in the schema generation tool.
| // testSchnorrSignature creates a deterministic schnorr signature for tests. | ||
| func testSchnorrSignature(t *testing.T, seed string) *schnorr.Signature { | ||
| t.Helper() | ||
|
|
||
| h := chainhash.HashH([]byte(seed)) | ||
| privKey, _ := btcec.PrivKeyFromBytes(h[:]) | ||
| msg := chainhash.HashH([]byte("test message")) | ||
| sig, err := schnorr.Sign(privKey, msg[:]) | ||
| require.NoError(t, err) | ||
|
|
||
| return sig | ||
| } |
There was a problem hiding this comment.
This testSchnorrSignature helper function is a duplicate of the one defined in round/transitions_test.go. To improve maintainability and avoid code duplication, consider moving this function to a shared test utility package where it can be accessed by both vtxo and round package tests.
![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: 741f1a3828
ℹ️ 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".
| SELECT * FROM vtxos | ||
| WHERE status < 3 |
There was a problem hiding this comment.
ListLiveVTXOs (used for recovery) filters only on status < 3. Because the migration defaults status to 0 and MarkVTXOSpent still only toggles the legacy spent flag, any VTXO that is already spent (including pre-migration rows) will continue to have status 0 and will be returned as live on restart, causing spent VTXOs to be resurrected and acted on. Consider backfilling/updating status on spend or also filtering on spent = FALSE here.
Useful? React with 👍 / 👎.
| // TODO(roasbeef): Full implementation requires vtxoStore and roundActor | ||
| // dependencies to be added to the wallet actor. For now, return success | ||
| // to indicate the request was accepted. The systest will wire up the | ||
| // actual refresh flow via direct round actor calls. | ||
| resp := &RefreshVTXOsResponse{ |
There was a problem hiding this comment.
handleRefreshVTXOs currently only logs and returns a success response with RefreshingCount derived from the request, but it never enqueues a refresh or consults ForceRefresh/expiry. Any client using this new API will receive a success response even though no VTXO is actually refreshed, which can silently leave expiring VTXOs untouched. Until the wiring is in place, consider returning an explicit error or zero counts.
Useful? React with 👍 / 👎.
295194b to
4519c28
Compare
bfcfcf0 to
6558cbc
Compare
4519c28 to
d0cde8f
Compare
|
Claude finished @Roasbeef's task in 2m 43s —— View job Code Review CompleteI've reviewed this PR thoroughly. The implementation is well-structured and follows good design patterns. Below are my findings organized by severity. SummaryThis PR adds VTXO lifecycle persistence to enable the refresh flow. The core changes include extending the database schema with status tracking fields, implementing VTXOPersistenceStore for transaction-safe operations, and adding wallet-level refresh APIs. The work is solid, but there are several important issues to address. Critical Issues1. Migration Design Issue (Per Author's Comment)Location: As @Roasbeef noted in the review, this shouldn't be a separate migration. The VTXO status fields should be integrated into the existing schema in place rather than adding them via ALTER TABLE. This creates unnecessary migration complexity. Recommendation: Collapse the status-related columns into the initial CREATE TABLE statement in the original migration. 2. Spent VTXOs Resurrection BugLocation: -- ListLiveVTXOs returns all VTXOs that are not in a terminal state.
SELECT * FROM vtxos
WHERE status < 3
ORDER BY creation_time DESC;Issue: The query filters only on Root cause: The migration sets Recommendation: Either:
High Priority Issues3. Wallet Refresh Handler Returns False SuccessLocation: func (a *Ark) handleRefreshVTXOs(ctx context.Context,
req *RefreshVTXOsRequest) fn.Result[WalletResp] {
// ... logs the request ...
// Forward to round actor if configured.
a.roundActor.WhenSome(func(ref actor.TellOnlyRef[actormsg.RoundReceivable]) {
ref.Tell(ctx, &actormsg.TriggerVTXORefreshMsg{
TargetOutpoints: req.TargetOutpoints,
ForceRefresh: req.ForceRefresh,
})
})
resp := &RefreshVTXOsResponse{
RefreshingCount: len(req.TargetOutpoints), // Always returns count from request
Errors: make(map[wire.OutPoint]error),
}
return fn.Ok[WalletResp](resp)
}Issue: The handler always returns success with
This creates a false API contract where callers think VTXOs are being refreshed when they may not be. Recommendation:
4. Missing Function CommentsLocation: Multiple files The codebase style guide at
Several functions are missing comments:
Example of proper format: // descriptorToInsertParams converts a vtxo.Descriptor to sqlc insert
// parameters. This handles serialization of the tree path and public keys
// for database storage.
func (s *VTXOPersistenceStore) descriptorToInsertParams(
desc *vtxo.Descriptor,
) (InsertVTXOParams, error) {These functions do have comments, but they could be more detailed per the guidelines' emphasis on explaining how/why rather than just what. Medium Priority Issues5. Inconsistent Signature Type UsageLocation: The PR description mentions "signature handling updated to use // vtxo/outbox_messages.go:99
Signature *schnorr.SignatureAnd byte slices in others: // round/transitions.go (forfeit signature handling)Recommendation: Verify this is intentional and document why some places use 6. Status Code Magic NumbersLocation: -- status tracks VTXO lifecycle (vtxo.VTXOStatus enum):
-- 0 = Live (default)
-- 1 = RefreshRequested
-- 2 = Forfeiting
-- 3 = Forfeited
-- 4 = Spent
-- 5 = Expiring
-- 6 = Failed
status INTEGER NOT NULL DEFAULT 0,WHERE status < 3Issue: The query uses magic number Recommendation: Add a comment explaining the threshold: -- ListLiveVTXOs returns all VTXOs that are not in a terminal state.
-- Terminal states are: Forfeited (3), Spent (4), Expiring (5), Failed (6).
-- This is used during startup to recover active VTXO actors.
SELECT * FROM vtxos
WHERE status < 3 -- Live (0), RefreshRequested (1), Forfeiting (2)
ORDER BY creation_time DESC;Actually, looking again, the comment is already there! This is fine as-is. 7. Code Duplication in Test HelpersLocation: As noted by the gemini-code-assist reviewer, there's a Low Priority / Style Issues8. Line Length and FormattingLocation: The generated schema file has status columns on a single line which impacts readability. While this is a generated file, consider if the generation settings can be adjusted to put each column on its own line for better git diffs and readability. 9. Error Log Level UsageLocation: Appears correct throughout I checked the logging calls and they correctly follow the guideline:
The structured logging calls (ending in Positive Observations
Testing RecommendationsBefore merging, ensure:
Summary of Required ChangesMust fix before merge:
Should fix before merge: Nice to have: Overall this is solid work that lays important groundwork for refresh functionality. The main concerns are the database query bug and the false success response from the wallet handler, both of which could cause operational issues. |
6558cbc to
fc7d4e4
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d0cde8f441
ℹ️ 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".
| -- status tracks VTXO lifecycle (vtxo.VTXOStatus enum): | ||
| -- 0 = Live (default) | ||
| -- 1 = RefreshRequested | ||
| -- 2 = Forfeiting | ||
| -- 3 = Forfeited |
There was a problem hiding this comment.
These new vtxos columns are introduced by editing migration 000003, but existing deployments that already ran 000003 will not re-run it, so their schema will still lack status/forfeit_* fields. All the new queries (e.g., ListLiveVTXOs, UpdateVTXOStatus) reference status and will start failing at runtime after upgrade. Please add a new forward migration (ALTER TABLE + backfill spent→status) instead of modifying a historical migration.
Useful? React with 👍 / 👎.
| return &vtxo.Descriptor{ | ||
| Outpoint: outpoint, | ||
| Amount: btcutil.Amount(row.Amount), | ||
| PkScript: row.PkScript, | ||
| ClientKey: keychain.KeyDescriptor{ |
There was a problem hiding this comment.
The recovered descriptor from rowToDescriptor only fills a subset of fields; TapScript, BatchExpiry, TreeDepth, and CreatedHeight remain zero/nil. After a restart, expiry checks in ExpiryConfig.CheckExpiry will treat BatchExpiry as expired (<=0), and signForfeitVTXOInput will fail because TapScript is nil, so refresh/forfeit flows break for any recovered actor. Consider persisting these fields or reconstructing them (e.g., from tree_path + round metadata) when loading.
Useful? React with 👍 / 👎.
| a.roundActor.WhenSome(func(ref actor.TellOnlyRef[actormsg.RoundReceivable]) { | ||
| ref.Tell(ctx, &actormsg.TriggerVTXORefreshMsg{ | ||
| TargetOutpoints: req.TargetOutpoints, | ||
| ForceRefresh: req.ForceRefresh, |
There was a problem hiding this comment.
RefreshVTXOsRequest documents that an empty TargetOutpoints should refresh all VTXOs within the expiry threshold, but handleRefreshVTXOs forwards the empty list as-is. That means no VTXO actors are triggered and the response reports RefreshingCount as 0, so the documented behavior never happens. Consider querying the store for eligible VTXOs when the list is empty before forwarding.
Useful? React with 👍 / 👎.
d0cde8f to
828e638
Compare
828e638 to
407336f
Compare
In this commit, we change the forfeit signature type from raw []byte to *schnorr.Signature across the forfeit flow types. This provides type safety and consistency with the server's types.ForfeitTxSig which uses *schnorr.Signature for ClientVTXOSig. The following types are updated: - ForfeitSignatureResponse.Signature (round/events.go) - ForfeitSignatureSubmission.Signature (vtxo/outbox_messages.go) - SubmitVTXOForfeitSigsToServer.ForfeitSigs map values (round/outbox_messages.go) The signForfeitVTXOInput function now parses the serialized signature into a typed *schnorr.Signature before returning. Test files add a testSchnorrSignature helper that creates deterministic signatures from a seed for consistent test behavior.
9c51104 to
57a6af3
Compare
Add migration 000004_vtxo_status to extend the vtxos table with status tracking fields for the VTXO refresh flow: - status: lifecycle state (Live, RefreshRequested, Forfeiting, etc.) - forfeit_round_id: tracks the new round during refresh - forfeit_tx: stores signed forfeit tx for crash recovery - forfeit_txid: records confirmed forfeit transaction ID - replaced_by_hash/index: links old VTXO to its replacement Also adds vtxo.sql queries for status updates, forfeit tracking, and VTXO retrieval by status.
Add VTXOPersistenceStore implementing vtxo.VTXOStore interface for VTXO lifecycle persistence. The store uses the BatchedTx pattern for transaction-safe operations and handles: - SaveVTXO: persist new VTXOs with serialized tree paths - GetVTXO/ListLiveVTXOs: retrieve VTXOs for actor recovery - UpdateVTXOStatus: atomic status transitions - MarkForfeiting: store forfeit tx for crash recovery - MarkForfeited: record forfeit confirmation Also updates migrations.go with the new migration and makes minor adjustments to round_store.go for consistency.
Add wallet-level API messages for triggering VTXO refresh: - RefreshVTXOsRequest: specifies target VTXOs to refresh with optional ForceRefresh flag to bypass expiry threshold checking - RefreshVTXOsResponse: reports count of VTXOs queued for refresh and any errors encountered These messages form the high-level interface for the refresh flow, routing through the wallet actor to the round actor.
Add handler for RefreshVTXOsRequest that: 1. Retrieves target VTXOs from store (specific outpoints or all live) 2. Filters by expiry threshold unless ForceRefresh is set 3. Sends RefreshVTXORequest to round actor for each eligible VTXO Also adds vtxoStore and roundActor dependencies to the wallet actor config for refresh flow support.
Fix type consistency by using RoundID instead of string in the SubmitVTXOForfeitSigsToServer message. This aligns with other round messages that use the typed RoundID for better type safety.
Add TriggerRefreshEvent which is sent to VTXO actors to manually trigger a refresh request. This bypasses the automatic expiry-based refresh and immediately transitions the VTXO to RefreshRequested state. The event is defined in round/vtxo_messages.go (where all VTXO actor messages live) and type-aliased in vtxo/events.go. The LiveState handles this event by emitting RefreshRequest to the round actor via outbox. This enables user-initiated refresh through the wallet actor without waiting for automatic expiry thresholds.
Add TriggerVTXORefreshMsg which is sent from the wallet actor to the round actor to trigger refresh of specific VTXOs. This message is defined in actormsg to avoid an import cycle between wallet and round packages (round imports wallet for BoardingAddress). The message implements RoundReceivable so it can be sent via the wallet's TellOnlyRef[RoundReceivable] reference to the round actor.
Add handleTriggerVTXORefresh which processes TriggerVTXORefreshMsg from the wallet actor. For each target outpoint, we look up the VTXO actor via its service key and send TriggerRefreshEvent. The VTXO actors then emit RefreshVTXORequest back to us through their outbox. This completes the wallet -> round -> vtxo refresh triggering flow.
When handling RefreshVTXORequest events, create a corresponding VTXORequest to ensure the refresh has an output destination. Without this, refresh-only rounds would fail validation due to zero total output amount. Also add debug logging to both round and VTXO actors to aid in tracing the forfeit request flow and state transitions during refresh operations.
57a6af3 to
117022e
Compare
1 participant
This PR adds VTXO lifecycle persistence support to enable the refresh flow where clients can forfeit old VTXOs to receive new ones in a subsequent round.
The core addition is a new database migration (000004_vtxo_status) that extends the vtxos table with status tracking fields. VTXOs now track their lifecycle state (Live, RefreshRequested, Forfeiting, Forfeited, Spent, Expiring, Failed) along with forfeit transaction data for crash recovery. The schema also includes replacement tracking so forfeited VTXOs can be linked to their successors for auditing purposes.
A new VTXOPersistenceStore implements the vtxo.VTXOStore interface using the BatchedTx pattern for transaction-safe operations. The store handles saving new VTXOs with serialized tree paths, retrieving VTXOs for actor recovery on startup, atomic status transitions, and persisting signed forfeit transactions so they can be recovered after a crash.
The wallet package gains a new RefreshVTXOsRequest/Response message pair that forms the high-level API for triggering refresh. The wallet actor handler looks up target VTXOs from the store, optionally filters by expiry threshold, and forwards RefreshVTXORequest messages to the round actor for each eligible VTXO.
A small type consistency fix changes SubmitVTXOForfeitSigsToServer to use the RoundID type instead of a bare string, aligning it with other round messages for better type safety.
All sqlc queries have been regenerated to include the new VTXO lifecycle operations including InsertVTXO, GetVTXO, ListLiveVTXOs, UpdateVTXOStatus, MarkVTXOForfeiting, MarkVTXOForfeited, GetVTXOForfeitTx, and DeleteVTXO.
This is all prep for a new set of
systests for refresh behavior.