multi: implement in-round directed VTXO sends - #176
Conversation
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 integrates in-round directed VTXO sending functionality, enabling users to transfer virtual transaction outputs to other participants within a cooperative round. The changes focus on robust and atomic handling of coin selection, VTXO reservation, and round registration, ensuring secure and efficient transfers while leveraging existing admission controls. This significantly expands the system's capabilities for cooperative 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. 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
|
|
@claude review this |
|
Claude finished @ellemouton's task in 5m 53s —— View job Code Review
The overall design is sound and the architecture is clean — using the cooperative admission gate (PR 2) as the foundation for directed sends is the right call. The flow is straightforward to follow and the test coverage is solid. A few issues to work through: Critical1. Sender's wallet tracks unowned recipient VTXOs
Result:
For self-refresh and leave this was never an issue because 2.
|
There was a problem hiding this comment.
Code Review
This pull request introduces a significant feature: in-round directed VTXO sends. The implementation is well-structured, spanning the RPC layer, wallet actor, and VTXO manager. I appreciate the focus on atomicity with the new SelectAndReserveForfeitRequest, and the robust error handling in the wallet actor which ensures reservations are cleaned up correctly on failure. The fix for VTXO ownership in buildClientVTXOs is a critical detail that has been handled well. The test coverage is comprehensive, with new unit, integration, and a full end-to-end system test. I have one minor suggestion to improve the accuracy of the new system test's setup.
| OperatorKey: operatorKey, | ||
| TreePath: treePath, | ||
| RoundID: roundID.String(), | ||
| CommitmentTxID: commitmentTxID, |
There was a problem hiding this comment.
The CommitmentTxID used when saving the seeded VTXO is inconsistent with the transaction hash used to finalize the round. commitmentTxID is a hardcoded hash, while the round is finalized with commitmentTx.TxHash(). The VTXO descriptor should store the actual transaction hash of the commitment transaction that created it. While this might not affect the current test flow, it could cause issues in the future if any logic relies on this field to match the on-chain transaction, and it makes the test setup less accurate.
| CommitmentTxID: commitmentTxID, | |
| CommitmentTxID: commitmentTx.TxHash(), |
ecc4cad to
457728e
Compare
8af9fb1 to
cdb91b3
Compare
457728e to
99a76af
Compare
cdb91b3 to
17fffd2
Compare
8d30ba3 to
8dc7b24
Compare
8dc7b24 to
b6eb9ec
Compare
17fffd2 to
01ba349
Compare
b6eb9ec to
a252139
Compare
a252139 to
920326c
Compare
3715fdd to
0943444
Compare
920326c to
55e1825
Compare
Roasbeef
left a comment
There was a problem hiding this comment.
LGTM 🫐
Solid PR, no direct blocking comments, see some of the inline comments.
| actor.BaseMessage | ||
|
|
||
| // Status is "submitted" for real sends or "preview" for dry-run. | ||
| Status string |
There was a problem hiding this comment.
Should this return a round ID or something else that can be used to query the state of the ongoing send?
| ) | ||
| } | ||
|
|
||
| pkScript, err := txscript.PayToAddrScript(addr) |
There was a problem hiding this comment.
Should we actually be making an actual script here? In that we only really want a pubkey from them, as that'll be used as the pubkey in the newly created VTXO.
|
|
||
| vtxoRequests = append(vtxoRequests, types.VTXORequest{ | ||
| Amount: r.Amount, | ||
| PkScript: r.PkScript, |
There was a problem hiding this comment.
Is this actually used? Given we haven't finished the AST feature yet (not merged in).
| OperatorFee: terms.MinOperatorFee, | ||
| DustLimit: terms.DustLimit, | ||
| OperatorKey: terms.PubKey, | ||
| VTXOExitDelay: terms.VTXOExitDelay, |
There was a problem hiding this comment.
Since I had the scope of this PR, we should also have the client validate that the exit delay is actually saying otherwise if it's zero, then that means that there's no actual safety for the user.
|
One other thing that we should consider here is the receiver side. For example, for OOR, we actually have the indexer and then the receiver can hit the indexer to basically know when something is sent to it and to get all the checkpoints, etc. Here we don't actually have anything like that. How is the receiver supposed to know that they have a new Inron VTXO? I think we should make a follow-up here to put the proper plumbing to make sure that the receiver can also know whether there's a new and run VTXO because, for example, the server would also do a similar push notification as it did for O or R to the receiver so they can actually know that they have a new VTXO to get all the information and it's imported and recognized as its own. |
|
thanks for review 🙏 working on addressing things locally. realised i think we first need to fix this bug: #210 |
79e8722 to
fecbdb0
Compare
Add the atomic cooperative select-and-reserve message pair for directed send. This is the cooperative counterpart of SelectAndReserveSpendRequest: it selects VTXOs covering a target amount and drives each into PendingForfeitState rather than SpendingState. Without this atomic API, a split select-then-reserve flow would re-open the race that PR 2's admission model closes.
Add handleSelectAndReserveForfeit to the VTXO manager. This is the directed-send counterpart of handleSelectAndReserveSpend: it selects VTXOs covering a target amount using largest-first coin selection, then atomically reserves each via PendingForfeitEvent. On partial failure, already-reserved VTXOs are rolled back. Five tests cover success, multi-VTXO selection, insufficient funds, non-live exclusion, and partial rollback.
Add SendVTXOsRequest, SendVTXOsResponse, and SendRecipient types for in-round directed send. The request carries resolved recipient pkScripts, operator terms, and a dry-run flag. The response returns selection details and change amount.
Add the core wallet handler for in-round directed send. The flow: 1. Validate recipients (non-empty pkScript, positive amounts) 2. Atomic select-and-reserve via SelectAndReserveForfeitRequest 3. Compute change and reject below-dust change 4. Build IntentPackage: forfeits + recipient VTXOs + change VTXO 5. Register with round actor via RegisterIntentMsg 6. On failure: release forfeit reservation Dry-run exercises the real admission path then immediately releases, surfacing release failures explicitly. Seven tests cover success, no-change, dust rejection, dry-run, dry-run release failure, round rejection with release, and insufficient funds.
Extend the SendVTXOResponse proto message with two new fields that the RPC handler will populate after coin selection: - change_amount_sat: change returned to sender (zero if exact match) - selected_count: number of VTXOs selected as inputs
Replace the SendVTXO stub with a full implementation that delegates to the wallet actor's SendVTXOsRequest. The RPC handler resolves recipient destinations (taproot address or x-only pubkey) into both a pkScript and a client public key, fetches operator terms, and forwards the request to the wallet for atomic coin selection, reservation, and round registration. A new resolveRecipientOutput helper extracts the client key from taproot addresses (witness program) or raw pubkeys. Raw pk_script destinations are rejected since they lack the public key needed for VTXO descriptor construction and MuSig2 signing.
Cover all five destination resolution paths for directed sends: - Pubkey: valid x-only key → pkScript + clientKey - Address: taproot bech32m → pkScript + clientKey extraction - PkScript: rejected (no public key for VTXO construction) - Non-taproot address: rejected (segwit v0 lacks x-only key) - Invalid pubkey: wrong length rejected
buildClientVTXOs was assigning req.SigningKey (the sender's MuSig2 co-signing key) as the ClientKey on persisted ClientVTXO records. For self-refresh this is harmless since sender and recipient are the same, but for directed sends the resulting VTXO would appear owned by the sender's derived key instead of the recipient's declared public key. Use req.ClientKey (the declared VTXO owner) wrapped in a KeyDescriptor so directed send recipients get VTXOs they actually control.
Add a full-daemon SendVTXO system test that runs darepod against the regtest harness and a fake operator mailbox edge. The test preseeds a live VTXO, exercises the public gRPC API, and asserts that directed send admission moves the input into pending forfeit and creates a temp round. This gives stronger integration coverage for PR 3 than the existing wallet and RPC seam tests without requiring the full operator round protocol in the systest harness.
Add documentation for the new SelectAndReserveForfeitRequest admission type, SendVTXO RPC handler, wallet directed send flow, and the OwnerKey vs SigningKey distinction for VTXO ownership persistence.
|
Rebased on top of master after the recent changes. |
6b4e8f8 to
7b4bc72
Compare
Follow-up items (separate PR)The following changes are planned for a follow-up PR on top of this one: Hardening
Ownership model (replace
|
- Fix bare log reference in SendVTXO RPC handler (use r.server.log) - Fix systest: update NewVTXODescriptor call to 5-arg signature and rename ClientKey → OwnerKey per #210 - Fix gofmt alignment in wallet send handler
7b4bc72 to
406678e
Compare
Part 1: real-daemon integration tests (partial coverage)
Summary
Replaces #169
Fixes #156.
This PR implements first-class in-round directed sends through
SendVTXOontop of the admission model introduced in #175.
Instead of reusing the out-of-round spend path, directed send is treated as the
cooperative flow it actually is:
cooperative use
PendingForfeitchange outputs
round handling
That makes this the PR 3 replacement for #169, but with the post-PR-1/PR-2
architecture rather than the older send-specific trigger flow.
Why This Replaces #169
#169 was built before the current wallet/round boundary and before actor-owned
VTXO admission existed. The main problem with reviving it directly was that it
mixed directed send with the old spend-locking path.
This PR replaces that design with the same single-source-of-truth model used by
PR 2:
Live -> Spending -> SpentLive -> PendingForfeit -> Forfeiting -> ForfeitedSo directed send now reuses cooperative admission rather than creating a second
locking path.
Flow
Main Changes
SelectAndReserveForfeitRequestso directed send can do atomiccooperative coin selection and reservation in one step.
handleSendVTXOswith strict cleanup on every post-reservationfailure path.
SendVTXOin the daemon RPC server, including recipientresolution for taproot addresses and x-only pubkeys.
change amount.
ClientKey, not the sender signing key.Dependency
This PR depends on #175 (
vtxo-spend-state).It is opened against
vtxo-spend-statebecause PR 3 relies on PR 2's actor-owned admission model and manager-side cooperative select-and-reserve API.
Test Plan
go test ./darepod ./wallet ./vtxo ./round ./oorgo test -tags=systest ./systest -run TestSendVTXOEndToEnd -count=1