multi: harden directed send and add receiver notification - #225
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 significantly enhances the security, robustness, and client-side experience of directed sends within the system. It introduces a more secure and data-driven approach to VTXO ownership, moving away from a simple boolean flag to a persistent script-based check. Furthermore, it streamlines the process of VTXO materialization for recipients by providing comprehensive event notifications, reducing the need for additional queries and improving overall efficiency and reliability. The changes also include important hardening measures to prevent overflow issues and ensure proper resource cleanup. 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. 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 new IncomingVTXOEvent protobuf message with extended fields and a corresponding IncomingVTXOHandler to process these events, materialize VTXOs, and persist them. It also replaces the IsOwner flag with OwnedScriptChecker and OwnedScriptRegistrar interfaces for managing script ownership. The SendVTXO RPC now includes validation for recipient count and amounts, and dry-run error handling in wallet/wallet.go has been updated. Review comments highlight several areas in the IncomingVTXOHandler where critical failures (e.g., VTXO persistence, script store initialization, tapscript derivation, or data inconsistencies) might be silently ignored, suggesting that these should either return errors or be logged with a more severe level to ensure data integrity and prevent silent failures.
| if h.cfg.VTXOStore != nil { | ||
| saveErr := h.cfg.VTXOStore.SaveVTXO( | ||
| lookupCtx, desc, | ||
| ) | ||
| if saveErr != nil { | ||
| h.log.Warnf("Failed to save incoming "+ | ||
| "VTXO %s: %v", outpoint, saveErr) | ||
|
|
||
| return fn.Ok[IncomingVTXOResp](nil) | ||
| } |
There was a problem hiding this comment.
Similar to previous points, if h.cfg.VTXOStore.SaveVTXO fails, a warning is logged, and the function returns fn.Ok[IncomingVTXOResp](nil). This means that if the VTXO cannot be persisted, it will be silently dropped from the client's state. This is a critical operation, and its failure should likely result in an error being returned to the caller, or at least a more severe log level, to ensure data integrity.
There was a problem hiding this comment.
~cc: This is intentional best-effort design. The IncomingVTXOHandler processes server push notifications — returning an error here would only surface to the actor framework, not to any user-facing RPC. The handler will receive the event again on next connection/restart. A warning log is sufficient for operator visibility.
There was a problem hiding this comment.
I'd say this more signals a database error or schema inconsistency which we should surface.
| if h.cfg.ScriptStore == nil { | ||
| return fn.Ok[IncomingVTXOResp](nil) | ||
| } |
There was a problem hiding this comment.
The ScriptStore is checked for nil and if it is, the function returns fn.Ok[IncomingVTXOResp](nil). This means that if the ScriptStore is not initialized, the VTXO will not be materialized, but no error will be reported. While this might be intended for optionality, it could lead to silent failures if the store is expected to be present. Consider logging a warning or returning an error if ScriptStore is a mandatory dependency for VTXO materialization.
There was a problem hiding this comment.
~cc: This nil check is for optionality during startup — the handler can be registered before all stores are wired. It's a defensive guard, not a normal code path. Adding a warning here would be noisy since it fires for every event during early init.
| if err != nil { | ||
| // Not our script — ignore. | ||
| return fn.Ok[IncomingVTXOResp](nil) |
There was a problem hiding this comment.
The comment "Not our script — ignore." indicates that if LookupOwnedReceiveScript returns an error (meaning the script is not found), the function silently returns fn.Ok[IncomingVTXOResp](nil). This is a valid design choice for filtering, but it's important to ensure that this behavior is explicitly desired and doesn't mask potential misconfigurations or unexpected errors from the ScriptStore itself. If LookupOwnedReceiveScript can return errors for reasons other than "not found," those might be silently ignored.
There was a problem hiding this comment.
~cc: Agreed this is a design choice. In practice, LookupOwnedReceiveScript returns a 'not found' error for scripts we don't own — which is the common case since the server pushes events for ALL round leaves, not just ours. A follow-up could distinguish 'not found' from unexpected DB errors, but the current behavior is correct for the happy path.
| if rec.ClientKey.PubKey == nil { | ||
| return fn.Ok[IncomingVTXOResp](nil) | ||
| } |
There was a problem hiding this comment.
If rec.ClientKey.PubKey is nil, the function returns fn.Ok[IncomingVTXOResp](nil). This implies that a record might exist for the pkScript, but it's considered invalid if the client's public key is missing. This could indicate an incomplete or corrupted record in the ScriptStore. It might be beneficial to log a warning here to aid in debugging such data inconsistencies.
There was a problem hiding this comment.
~cc: Good callout. A nil PubKey here would indicate a corrupted DB record. Adding a warning log for this case is reasonable — will note as a follow-up improvement.
| if err != nil { | ||
| h.log.Warnf("Failed to derive tapscript for "+ | ||
| "incoming VTXO %s: %v", outpoint, err) | ||
|
|
||
| return fn.Ok[IncomingVTXOResp](nil) | ||
| } |
There was a problem hiding this comment.
If scripts.VTXOTapScript fails, a warning is logged, and the function returns fn.Ok[IncomingVTXOResp](nil). This means that if the tapscript cannot be derived, the VTXO will not be materialized, but the process will continue without an explicit error. Depending on the criticality of VTXO materialization, this might warrant returning an error to the caller or escalating the issue, rather than just logging a warning and proceeding.
There was a problem hiding this comment.
~cc: This is already logging a warning with the outpoint and error details. The handler is best-effort — the VTXO event will be re-delivered on reconnect. Returning an error to the actor framework wouldn't add recovery capability here.
| // Ensure reserved VTXOs are released if we don't reach the | ||
| // successful registration at the end. Use a background | ||
| // context so cleanup survives client disconnection. | ||
| committed := false | ||
| defer func() { | ||
| if committed { | ||
| return | ||
| } | ||
|
|
||
| return fn.Err[WalletResp](primary) | ||
| } | ||
| releaseCtx := context.WithoutCancel(ctx) | ||
| _ = a.releaseManagerForfeitStrict( | ||
| releaseCtx, reservedOutpoints, | ||
| ) | ||
| }() |
There was a problem hiding this comment.
The defer statement with context.WithoutCancel(ctx) for releasing reserved VTXOs is a significant improvement. It ensures that cleanup occurs even if the client disconnects or the function panics, making the system more robust. This pattern correctly addresses the need for durable cleanup actions.
There was a problem hiding this comment.
~cc: Agreed — this pattern ensures VTXOs are always released even on context cancellation or panic.
074ce93 to
359ef64
Compare
359ef64 to
0e1db22
Compare
|
@claude review this |
|
Claude finished @ellemouton's task in 3m 57s —— View job Code Review
Overall this is a well-structured follow-up with clear separation of concerns across the four commits. The 1. Logging style violations —
|
0e1db22 to
0beffff
Compare
|
~cc: Addressed the review feedback: Fixed in this push:
Acknowledged as follow-ups:
|
| return | ||
| } | ||
|
|
||
| releaseCtx := context.WithoutCancel(ctx) |
|
|
||
| var _ round.OwnedScriptChecker = (*ownedScriptCheckerAdapter)(nil) | ||
|
|
||
| func (a *ownedScriptCheckerAdapter) IsOwnedScript(ctx context.Context, |
| // (e.g., during round confirmation in a shutting-down FSM). | ||
| lookupCtx := context.WithoutCancel(ctx) | ||
|
|
||
| _, err := a.store.LookupOwnedReceiveScript(lookupCtx, pkScript) |
There was a problem hiding this comment.
I don't think this is correct as is, we should instead surface the error and a bool flag if it returns the receive script?
|
|
||
| var _ round.OwnedScriptRegistrar = (*ownedScriptRegistrarAdapter)(nil) | ||
|
|
||
| func (a *ownedScriptRegistrarAdapter) RegisterOwnedScript( |
| oorStore := db.NewStore( | ||
| s.db.DB, s.db.Queries, s.db.Backend(), | ||
| s.log, | ||
| ).NewOORArtifactStore(clock.NewDefaultClock()) |
There was a problem hiding this comment.
nit: perhaps we could store the clock in the server and inject it on creation of the server?
| if evt == nil || | ||
| evt.Type != arkrpc.VTXOEventType_VTXO_EVENT_TYPE_CREATED { | ||
|
|
||
| return fn.Ok[IncomingVTXOResp](nil) |
There was a problem hiding this comment.
IIUC this always need to be VTXOEventType_VTXO_EVENT_TYPE_CREATED right? Maybe we could add some debug logging in case something else is coming through.
| } | ||
|
|
||
| // Look up the pkScript in owned receive scripts. | ||
| lookupCtx := context.WithoutCancel(ctx) |
There was a problem hiding this comment.
Do we strictly need withoutcancel here?
| rec, err := h.cfg.ScriptStore.LookupOwnedReceiveScript( | ||
| lookupCtx, pkScript, | ||
| ) | ||
| if err != nil { |
There was a problem hiding this comment.
Perhaps we could specifically only skip not-found errors (or even just log) otherwise surface the error.
| if h.cfg.VTXOStore != nil { | ||
| saveErr := h.cfg.VTXOStore.SaveVTXO( | ||
| lookupCtx, desc, | ||
| ) | ||
| if saveErr != nil { | ||
| h.log.Warnf("Failed to save incoming "+ | ||
| "VTXO %s: %v", outpoint, saveErr) | ||
|
|
||
| return fn.Ok[IncomingVTXOResp](nil) | ||
| } |
There was a problem hiding this comment.
I'd say this more signals a database error or schema inconsistency which we should surface.
| } | ||
|
|
||
| if out.AmountSat <= 0 { | ||
| const maxAmountSat int64 = 21_000_000 * 1e8 |
| OperatorKey: operatorKey, | ||
| TapScript: tapscript, | ||
| RoundID: evt.RoundId, | ||
| CommitmentTxID: commitTxID, |
There was a problem hiding this comment.
codex: outpoint.Hash here is the leaf txid, not the round commitment txid. GetNonAnchorOutpoint derives the outpoint from the leaf transaction, while the rest of the client uses CommitmentTxID for commitment-lineage metadata. Storing the leaf txid under CommitmentTxID will leave these descriptors with inconsistent ancestry information.
| TapScript: tapscript, | ||
| RoundID: evt.RoundId, | ||
| CommitmentTxID: commitTxID, | ||
| BatchExpiry: evt.BatchExpiryHeight, |
There was a problem hiding this comment.
codex: This assumes evt.BatchExpiryHeight is already an absolute height, but the companion server PR currently populates it from Terms.SweepDelay when publishing the event. The rest of the client treats BatchExpiry as an absolute block height, so a received VTXO will look immediately critical/expired on any chain height greater than the sweep delay. Can we either send confirmation_height + sweep_delay from the server or recompute it here?
| rec, err := h.cfg.ScriptStore.LookupOwnedReceiveScript( | ||
| lookupCtx, pkScript, | ||
| ) | ||
| if err != nil { |
There was a problem hiding this comment.
codex: Returning Ok on store/materialization failures makes this mailbox event effectively lossy. IncomingVTXOEvent is delivered via Tell, so once we ack it there is no retry path here; a transient DB issue during lookup/save would silently drop the receive. Could we distinguish not found from real store errors and surface the latter instead of swallowing them?
|
@ellemouton, remember to re-request review from reviewers when ready |
| return nil, err | ||
| } | ||
|
|
||
| const maxRecipients = 256 |
There was a problem hiding this comment.
Should make an issue to tune this param later.
| } | ||
|
|
||
| if r.Amount <= 0 { | ||
| const maxAmount btcutil.Amount = 21_000_000 * 1e8 |
There was a problem hiding this comment.
Can use btcutil.MaxSatoshi here again.
| "1 leaf for signing key, got %d", | ||
| len(leaves)) | ||
| } | ||
| leaf := leaves[0] |
There was a problem hiding this comment.
We now assume a single leaf here?
|
|
||
| // pk_script is the VTXO output script. Present for CREATED events | ||
| // so the client can match against registered receive scripts. | ||
| bytes pk_script = 5; |
There was a problem hiding this comment.
Perhaps we should use an enum/oneof here to add more structure to the proto? So we can more easily distinguiish if this is a new VTXO from an in round send, or an oor.
| // IncomingVTXOHandler materializes VTXOs from IncomingVTXOEvent | ||
| // notifications pushed by the server's indexer after round | ||
| // confirmation. | ||
| type IncomingVTXOHandler struct { |
There was a problem hiding this comment.
Let's move this to a new package? Similar to the other actor sub-ssytems, then we can write some dierct unit tests against it.
There was a problem hiding this comment.
Can go in the vtxo package perhaps
| // route. When the server publishes a VTXO_CREATED event for a round | ||
| // leaf matching a registered receive script, this route dispatches it | ||
| // to the incoming VTXO handler actor for materialization. | ||
| func (s *Server) registerIncomingVTXOEventRoute( |
| Service: arkServiceName, | ||
| Method: MethodIncomingVTXO, | ||
| NewEvent: func() proto.Message { | ||
| return &arkrpc.IncomingVTXOEvent{} |
There was a problem hiding this comment.
Ah so we added this, but it wasn't actuall used for the oor flow?
- Validate recipient amounts against 21M BTC cap with overflow-safe addition in both RPC and wallet layers - Replace releaseAndFail with defer+committed pattern so panics don't leak PendingForfeitState VTXOs - Use context.WithoutCancel for deferred forfeit release so cleanup survives client disconnection - Cap recipients at 256 in RPC validation - Dry-run release is now best-effort via defer (errors don't propagate)
Remove the IsOwner boolean from VTXOIntent and VTXORequest in favor of a data-driven OwnedScriptChecker interface. Instead of tagging each VTXO with an ownership flag at construction time, the round FSM now queries the owned receive scripts store at confirmation time to determine which VTXOs to persist locally. This decouples VTXO construction from ownership tracking and is required for OOR receive scripts to work correctly with round VTXOs. Key changes: - Add OwnedScriptChecker and OwnedScriptRegistrar interfaces to round package - Wire OwnedScriptChecker into ClientEnvironment and all FSM construction sites - Register owned scripts in buildVTXOIntent and handleRegisterIntent via OwnedScriptRegistrar - Create ownedScriptCheckerAdapter and ownedScriptRegistrarAdapter in darepod backed by OORArtifactPersistenceStore - Use context.WithoutCancel in IsOwnedScript for shutdown safety - Update all tests to use mockOwnedScriptChecker instead of IsOwner
Extend IncomingVTXOEvent with pk_script, value_sat, round_id, batch_expiry_height, and relative_expiry fields. For VTXO_CREATED events from confirmed rounds, these carry enough data for the client to materialize the VTXO without a follow-up indexer query.
0beffff to
6a38835
Compare
Review comment resolutionsPushed a force-update with all fixups squashed into their target commits. Here's what changed: Commit 1:
|
Add a lightweight actor that handles IncomingVTXOEvent push notifications from the server's indexer. When the server publishes a VTXO_CREATED event for a confirmed round leaf matching a registered receive script, the handler: 1. Looks up the pkScript in owned_receive_scripts 2. Derives the tapscript from the owner key + operator key 3. Builds a vtxo.Descriptor from the event's metadata 4. Persists the VTXO via VTXOPersistenceStore 5. Notifies the VTXO manager via VTXOsMaterializedNotification This enables in-round VTXO receipt via the same receive-script mechanism as OOR — the recipient calls NewOORReceiveScript once and receives VTXOs from both OOR and in-round sends. The event route is registered for (arkrpc.ArkService, IncomingVTXO) alongside the existing IncomingOOR route.
6a38835 to
6c2fefa
Compare
Three landed PRs since the last broad gardening sweep touch subsystems whose CLAUDE.md/AGENTS.md did not yet reflect the new shape: PR #225 (send-in-round-followup), PR #238 (fee-estimator fix, no doc impact), and PR #239 (partial-unroll-harness-accessor). The most significant structural changes are the introduction of a data-driven pkScript ownership path and a dedicated actor that materializes round-produced VTXOs from indexer push notifications. The round FSM no longer relies on a per-intent IsOwner flag. Instead, buildOwnedClientVTXOs resolves ownership at round confirmation time by asking a round.OwnedScriptChecker whether each VTXO's pkScript is recognized by the local wallet, and round.OwnedScriptRegistrar persists locally-owned scripts at intent-build time and inside handleRegisterIntent for incoming intents whose owner key has a non-zero KeyLocator. The darepod package exposes both interfaces as thin adapters over the OOR owned-receive-scripts store, which means the same table now backs three abstractions (OwnedScriptChecker, OwnedScriptRegistrar, and vtxo.OwnedScriptLookup). The per-package docs pick up these new interfaces, the adapter types, and the invariants they enforce; ARCHITECTURE.md gains a new "Data-Driven Script Ownership" pattern section so agents encountering the code can follow the flow without re-deriving it from commits. The vtxo package now hosts an IncomingVTXOHandler actor that decodes arkrpc.IncomingVTXOEvent push notifications, materializes the VTXO descriptor via lib/scripts.VTXOTapScript, persists it, and notifies the VTXO manager via VTXOsMaterializedNotification. darepod registers the actor under vtxo.IncomingVTXOServiceKey and wires a new MethodIncomingVTXO route into the EventRouter. The vtxo CLAUDE.md now documents the handler's inputs, validation rules (only VTXO_CREATED events, bounds-checked values, nil-safe pkScripts), and the fact that CommitmentTxID comes from the event rather than the leaf txid. The darepod doc picks up the route registration and the service-key bootstrap; ARCHITECTURE.md mentions the new route under the RPC-over-Mailbox pattern. Smaller updates round out the sweep. wallet/CLAUDE.md describes the hardened handleSendVTXOs path: recipient amounts are now bounded by MaxSatoshi, the running total uses overflow-safe accumulation, and the old releaseAndFail helper is replaced by a deferred release that uses context.WithoutCancel so cleanup survives caller disconnect. The darepod doc records the matching RPC-side validation cap (maxRecipients = 256) and notes that s.clk is now a cached clock instance so sub-stores share a single Clock for deterministic tests, plus the new GetStoredVTXO harness accessor used by partial unroll itests. Round, darepod, vtxo, and wallet docs all cross-reference the new ownership interfaces where relevant so navigating the per-package graph reaches the same explanation from any entry point. make doc-check is clean after these changes.
5 participants
Summary
Follow-up to #176. Addresses security hardening, ownership model
improvements, and receiver-side VTXO materialization for in-round
directed sends.
Closes #223.
Changes
1. Hardening (
74fc7c7)releaseAndFailwithdefer+committedpattern so panics don't leakPendingForfeitStateVTXOscontext.WithoutCancelfor deferred forfeit release (survives client disconnection)2. Replace
IsOwnerwithOwnedScriptChecker(e4f5aa9)OwnedScriptCheckerinterface to round FSM'sClientEnvironmentowned_receive_scriptsDB storeIsOwnerfromVTXOIntentandVTXORequestOwnedScriptRegistrarinterface on round actor — registers pkScripts for boarding, refresh, and change VTXOs at intent timeOORArtifactPersistenceStore3. Extend
IncomingVTXOEventproto (1d2ead4)pk_script,value_sat,round_id,batch_expiry_height,relative_expiryfieldsVTXO_CREATEDevents from confirmed rounds, carries enough data for the client to materialize without a follow-up indexer query4. Receiver-side VTXO materialization (
074ce93)IncomingVTXOHandleractor processesIncomingVTXOEventpush notificationsVTXO_CREATEDevent for a round leaf matching a registered receive script:owned_receive_scriptsvtxo.Descriptorfrom event metadataVTXOPersistenceStoreVTXOsMaterializedNotification(arkrpc.ArkService, IncomingVTXO)NewOORReceiveScriptonce and receives VTXOs from both OOR and in-round sendsTesting
TestDirectedSendIntegrationverifies bob receives his VTXO viaListVTXOs,TestDirectedSendSelfSendverifies both VTXOsCompanion PR
Server-side itest + VTXOEventPublisher: lightninglabs/darepo#205