vtxo+wallet: implement VTXO coin selection with atomic locking - #168
vtxo+wallet: implement VTXO coin selection with atomic locking#168ellemouton wants to merge 7 commits into
Conversation
Define message types for VTXO locking in the actormsg package so both the wallet and vtxo packages can use them without import cycles. This includes ListAvailableVTXOs, LockVTXOs, and UnlockVTXOs request/response pairs, plus a VTXOManagerResp marker interface and a well-known service key for looking up the VTXO manager actor.
b17fbe3 to
77ea637
Compare
Add an in-memory locked outpoints set to the VTXO manager and wire up three new message handlers: ListAvailableVTXOs (returns live minus locked), LockVTXOs, and UnlockVTXOs. These enable the wallet actor to perform coin selection against available VTXOs and atomically lock selected ones for OOR transfers. The ManagerResp interface is updated to embed actormsg.VTXOManagerResp so response types defined in actormsg can satisfy it without import cycles. Terminated VTXOs are also automatically unlocked.
Add SelectAndLockVTXOs and UnlockVTXOs handlers to the wallet actor. The wallet queries the VTXO manager for available (live, unlocked) VTXOs via service key lookup, runs largest-first coin selection to cover the target amount, then asks the manager to lock the selected outpoints. Unlock forwards directly to the manager. The coin selection algorithm sorts VTXOs by descending amount and greedily picks until the target is met, minimizing input count.
Add tests for the selectCoinsLargestFirst pure function covering exact match, multiple inputs, all inputs needed, insufficient balance, empty input, PkScript preservation, and input slice immutability. Add tests for the VTXO manager lock/unlock handlers covering list available with exclusion, lock counting, unlock, unlock nonexistent, automatic unlock on termination, field mapping, and backward compatibility of the GetActiveVTXOCount message.
Update comments on SelectAndLockVTXOsRequest, SelectedVTXO, UnlockVTXOsRequest, and the RPC unlockVTXOs helper to reflect that VTXO coin selection and locking applies to both OOR transfers and in-round directed sends, not just OOR.
Wire the VTXO manager into the daemon's actor system so it is discoverable via the well-known VTXOManagerServiceKey. The new initVTXOManager method creates the manager with all required dependencies (store, wallet signer, chain source, round actor), registers it under the service key, starts it to recover persisted VTXOs, and returns a MapInputRef adapter for the round actor's VTXOManager field. The vtxo.ManagerMsg and ManagerResp types are converted from embedding interfaces to type aliases for actormsg.VTXOManagerMsg and actormsg.VTXOManagerResp respectively. This is required because Go generics treat interface embedding as a distinct type, so RegisterWithSystem would fail with a type mismatch when using the actormsg-typed service key.
Add integration tests in internal/coinselect that exercise the full coin selection flow: wallet actor → service key lookup → VTXO manager Ask → coin selection → lock → response. This covers the orchestration glue that unit tests in wallet/ and vtxo/ cannot reach due to the import cycle (wallet → vtxo → round → wallet). Tests cover: - Basic select and lock via real actor system - Locked VTXOs excluded from subsequent selections - Insufficient balance after locking - Unlock restores availability for re-selection - Clear error when no actor system is configured The stubVTXOStore uses a ready flag so Manager.Start sees an empty store (avoiding VTXO actor spawning), then the test VTXOs become visible for coin selection queries.
77ea637 to
2fae7f2
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 introduces a critical enhancement to the system's VTXO handling by implementing a robust and secure coin selection and locking mechanism. The primary goal is to ensure that VTXOs are atomically selected and locked, effectively preventing race conditions and double-spending in a concurrent environment. By centralizing lock management within the VTXO manager, the system gains improved architectural clarity and leverages the benefits of the actor model for reliable state management. 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
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 implements atomic VTXO coin selection and locking within the VTXO manager, which is a crucial step for preventing double-spends during concurrent transfers. The architecture is well-designed, leveraging the actor model and service keys to decouple the wallet and VTXO manager, avoiding import cycles. The changes are extensive, touching actor wiring, message definitions, and adding new logic to the VTXO manager. The accompanying integration and unit tests are thorough.
My review has identified a couple of areas for improvement:
- There is a duplicated and unused coin selection function in the wallet actor that should be removed to eliminate dead code.
- A minor bug was found in one of the new test files where function return values were handled incorrectly.
Overall, this is a great contribution that significantly enhances the wallet's capabilities.
I am having trouble creating individual review comments. Click here to see my feedback.
wallet/wallet.go (831-835)
This selectCoinsLargestFirst function seems to be a duplicate of the one in vtxo/manager.go and appears to be unused. The coin selection logic is now handled entirely by the VTXO manager, as seen in vtxo/manager.go:handleSelectAndLockVTXOs. To avoid code duplication and reduce confusion, this function should be removed. The corresponding test file wallet/coin_selection_test.go should also be removed.
vtxo/manager_lock_test.go (49-50)
The return values from result.Unpack() are (ManagerResp, error), but they are being assigned to resp, ok. The second return value is an error, not a boolean. This should be resp, err := result.Unpack() followed by require.NoError(t, err).
resp, err := result.Unpack()
require.NoError(t, err)
vtxo/manager_lock_test.go (65-66)
Similar to the comment above, the return values from result.Unpack() are (ManagerResp, error), but they are being assigned to resp, ok. The second return value is an error, not a boolean. This should be resp, err = result.Unpack() followed by require.NoError(t, err).
resp, err = result.Unpack()
require.NoError(t, err)
| // handleListAvailable returns all live VTXOs that are not currently locked. | ||
| // This queries the store for live VTXOs and filters out any that appear in | ||
| // the locked outpoints set. | ||
| func (m *Manager) handleListAvailable( |
There was a problem hiding this comment.
Should add tests for these new methods.
Also do the vtxos themselves need to know that they're locked?
| mgrKey := actormsg.VTXOManagerServiceKey() | ||
| mgrRef := mgrKey.Ref(a.actorSystem) | ||
|
|
||
| future := mgrRef.Ask(ctx, &actormsg.SelectAndLockVTXOsRequest{ |
There was a problem hiding this comment.
We could prob make a helper function (type param'd w/ the message type) for the 10 or so lines below this (actor call boiler plate).
| mgrKey := actormsg.VTXOManagerServiceKey() | ||
| mgrRef := mgrKey.Ref(a.actorSystem) | ||
|
|
||
| future := mgrRef.Ask( |
There was a problem hiding this comment.
Same here re the boiler plate: req, await, call, type cast.
| // find it via service key lookup for lock/unlock operations. | ||
| func (s *Server) initVTXOManager(ctx context.Context, | ||
| clientWallet vtxo.VTXOWallet, | ||
| chainSourceRef actor.ActorRef[ |
There was a problem hiding this comment.
Non-blocking, but can add some type aliases to the respective packages to cut down on the type decl length here.
| @@ -0,0 +1,332 @@ | |||
| package coinselect | |||
There was a problem hiding this comment.
Any reason to put put these in systest? I guess it doesn't necessarily need access to any of the backends?
|
|
||
| resp, err := result.Unpack() | ||
| if err != nil { | ||
| return fn.Err[WalletResp]( |
There was a problem hiding this comment.
These errors paths should unlock the locked vtxos. Also we may want to consider a sort of auto unlock timeout here as well.
Also I think the vtxo actors themelves need to also know about this locking, as right now the path for refresh is wallet -> round <-> vtxo. Otherwise we could try an oor, but then it races with a refresh attempt.
|
|
||
| var locked int | ||
| for _, op := range req.Outpoints { | ||
| if !m.lockedOutpoints.Contains(op) { |
There was a problem hiding this comment.
Only add them if they're actually vtxo outpoints? The manager has a map of all the actual outpoints.
2 participants
VTXO coin selection + locking for transfers
Closes #150
Implements full VTXO coin selection with in-memory locking to prevent
double-spends across concurrent OOR sends and in-round directed
transfers. The wallet actor selects VTXOs using a largest-first
strategy and delegates lock management to the VTXO manager via the
actor system's service key pattern.
Architecture
Locking lives in the VTXO manager rather than the wallet because:
Receive) eliminates explicit synchronizationwallet → vtxo → round → walletimport cycleby discovering the manager through the well-known
VTXOManagerServiceKeySelect & Lock Flow
Selection and locking happen in a single atomic manager message
(
SelectAndLockVTXOsRequest) — no race window between list and lock.Unlock Flow (failure / cancellation)
Auto-Unlock on Termination
Double-Spend Prevention
Locked VTXOs are invisible to subsequent selections. Two concurrent
callers requesting 50k each will never receive the same VTXO:
All access is serialized through the manager's
Receive— no explicitmutex needed.
Design Notes
fn.Set): cleared on daemonrestart. After restart there are no in-flight transfers, so nothing
needs to stay locked.
sorts a copy to avoid mutating the caller's slice.
ManagerMsg/ManagerRespare type aliases (not embeddinginterfaces) so Go generics treat them as identical to
actormsg.VTXOManagerMsg/actormsg.VTXOManagerResp— required forRegisterWithSystemto accept the service key's type parameters.on termination + transient locks cover the practical cases.