OOR client 2/4: outgoing/incoming FSM + actor wrapper - #78
Conversation
Summary of ChangesHello @bhandras, 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 lays the groundwork for client-side Out-of-Round (OOR) Ark transfers by introducing a robust FSM and actor architecture. The primary goal is to enable reliable and resilient transfer operations, particularly for environments prone to interruptions, by ensuring deterministic transaction construction, seamless recovery from crashes, and a clear interface for all external interactions. The changes cover the full lifecycle of both sending and receiving OOR transfers, from initial setup and signing to finalization and on-chain confirmation monitoring. 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
|
There was a problem hiding this comment.
Code Review
This pull request introduces a well-designed client-side finite state machine (FSM) and an actor for handling out-of-round (OOR) transfers. The architecture is robust, with a clear separation between the pure FSM logic and side effects, which are managed through an outbox/handler pattern. This design choice, combined with the snapshot-based persistence and resume logic, provides a solid foundation for reliable transfers, especially in environments like mobile where applications can be suspended or terminated unexpectedly.
The test coverage is comprehensive, including happy paths, various failure scenarios, and critical resume/retry logic, which gives high confidence in the implementation's correctness.
I have one minor suggestion regarding code organization for improved maintainability, but overall, this is an excellent contribution.
57e967a to
7c870f8
Compare
ad6f25a to
989cbdf
Compare
f6e2018 to
5af35c8
Compare
5c2be65 to
a770497
Compare
a770497 to
ddd06f7
Compare
5af35c8 to
aee4ce1
Compare
|
@bhandras, remember to re-request review from reviewers when ready |
| // Outpoint is the outpoint of the VTXO output being spent. | ||
| Outpoint wire.OutPoint | ||
|
|
||
| // WitnessUtxo is the previous output being spent (value + pkScript). |
There was a problem hiding this comment.
should we change the name a bit since ideally this never becomes a Utxo?
also wondering if we should like group the wire.outpoint & wire.TxOut together to represent the VTXO we are spending?
There was a problem hiding this comment.
Updated after latest force-push: we dropped the SpentVTXO wrapper and now reuse checkpoint.Input directly (outpoint + witness txout), so OOR uses one canonical checkpoint input shape.
There was a problem hiding this comment.
not sure this answers my question. all that has been done is that the type was moved and is now aliased here. but my question still stands
There was a problem hiding this comment.
fair. to address this, the canonical checkpoint shape is now checkpoint.Input with SpentVTXORef{Outpoint, Output} so identity+witness material stay grouped in one type.
There was a problem hiding this comment.
okkkk doesnt look addressed in this pr but looks like it is later on somwhere
| // TapTreeEncoded is the v0 tap tree encoding for the checkpoint output. | ||
| // | ||
| // This is intended to be attached to the Ark tx PSBT inputs under the | ||
| // `taptree` unknown key so finalization can later copy it onto the | ||
| // checkpoint output metadata. |
There was a problem hiding this comment.
similar question on previous PR: when is it encoded in PSBT vs explicitly communicated like this?
perhaps worth keeping explicit? 🤷♀️ if not, then perhaps worth having a type that hides the data but that returns a result that defs always has it encoded? then have methods to extract taptree on that type?
There was a problem hiding this comment.
For now we keep this explicit across the boundary for clarity; once wire types stabilize we can compress payload shape.
There was a problem hiding this comment.
Updated with latest changes: we no longer expose a CheckpointOutput() helper. Callers build typed CheckpointOutput directly from Result{PSBT, TapTreeEncoded}, keeping the primitive API flat while preserving metadata.
There was a problem hiding this comment.
i dont get it. the argument on the previous PR was to keep spending metadata in the PSBT but to make session data explicit. not clear to me 🤷♀️
There was a problem hiding this comment.
personally i think there should be a new type that wraps the two to make it clear that the psbt does or doesnt include the data.
There was a problem hiding this comment.
agree and implemented: we now return a wrapper artifact (CheckpointArtifact) that carries {PSBT, TapTreeEncoded} explicitly so it's clear when metadata is sidecar vs embedded.
There was a problem hiding this comment.
almost there: the agent is trying to remain backwards compatible but doenst need to:
// CheckpointResult is a backwards-compatible alias for CheckpointArtifact.
type CheckpointResult = CheckpointArtifact`
There was a problem hiding this comment.
Good call — removed.
Done in e32d939
CheckpointResult no longer carries a backward-compat alias; we keep the explicit result shape only.
5a2ff9e to
3393d5f
Compare
ddd06f7 to
be7ba2b
Compare
bb60fb7 to
85e0814
Compare
be7ba2b to
d1b9baf
Compare
85e0814 to
8b38cd8
Compare
7935c29 to
ad25141
Compare
acc8c5c to
2b7d0fa
Compare
ad25141 to
f77db9a
Compare
2b7d0fa to
24cbc6e
Compare
a215161 to
f01992d
Compare
24cbc6e to
9ffa9bb
Compare
| // TapTreeEncoded is the v0 tap tree encoding for the checkpoint output. | ||
| // | ||
| // This is intended to be attached to the Ark tx PSBT inputs under the | ||
| // `taptree` unknown key so finalization can later copy it onto the | ||
| // checkpoint output metadata. |
There was a problem hiding this comment.
i dont get it. the argument on the previous PR was to keep spending metadata in the PSBT but to make session data explicit. not clear to me 🤷♀️
| // TapTreeEncoded is the v0 tap tree encoding for the checkpoint output. | ||
| // | ||
| // This is intended to be attached to the Ark tx PSBT inputs under the | ||
| // `taptree` unknown key so finalization can later copy it onto the | ||
| // checkpoint output metadata. |
There was a problem hiding this comment.
personally i think there should be a new type that wraps the two to make it clear that the psbt does or doesnt include the data.
| type RecipientOutput struct { | ||
| // PkScript is the destination script. | ||
| PkScript []byte | ||
|
|
||
| // Value is the amount to send in satoshis. | ||
| Value btcutil.Amount | ||
| } |
There was a problem hiding this comment.
also fixed: we now use wire.TxOut in the checkpoint/spent-vtxo shapes (checkpoint.SpentVTXORef.Output, CheckpointOutput.Output).
| // | ||
| // This function does not attempt to sign the checkpoint tx. It also does not | ||
| // validate that the owner leaf is a canonical Ark closure (draft phase). | ||
| func BuildCheckpointPSBT(policy scripts.CheckpointPolicy, |
There was a problem hiding this comment.
btw do we check somewher that the policy value used by the operator for the checkpoint's CSV is reasonable?
we dont want it to be too short. ie, we should have a min acceptable value for this.
also: the server should check that the user has used its advertised values. just want to make sure we are doing that
There was a problem hiding this comment.
good call. we now enforce a minimum csv delay in checkpoint builder (checkpoint.MinCheckpointCSVDelay). server-side 'matches advertised policy' enforcement is still a follow-up in server OOR hardening.
There was a problem hiding this comment.
Yes, tracked here: https://github.com/lightninglabs/darepo/issues/91
That issue covers the server-side follow-up to enforce that submitted checkpoint policy values match what the server advertises.
| // ArkPSBT is echoed back for convenience and to allow stateless | ||
| // finalization (tap tree metadata is bound to it). | ||
| ArkPSBT *psbt.Packet |
There was a problem hiding this comment.
it's not just echoed back for convenience though right? the operator has also just sent us their sig for the ark tx iiuc?
There was a problem hiding this comment.
ie, this is co-signed now too
There was a problem hiding this comment.
not co-signed ark here; operator co-signs checkpoints. ark psbt in SubmitAcceptedEvent is the canonical session artifact + consistency check, while co-signed material is in CoSignedCheckpointPSBTs.
| } | ||
|
|
||
| // SendSubmitPackageRequest asks the transport layer to send the submit package | ||
| // (Ark PSBT + checkpoint PSBTs) to the server. |
There was a problem hiding this comment.
and to sign the ark psbt yeah?
There was a problem hiding this comment.
yes exactly. RequestArkSignatures is specifically the boundary for attaching client ark-input signatures before submit.
There was a problem hiding this comment.
what i mean is that the comment should be expanded to say this
There was a problem hiding this comment.
Agreed — expanded the comments in this pass to make the signing boundary explicit in the outbox types:
e32d939
SendSubmitPackageRequest now documents submit payload semantics, and RequestCheckpointSignatures documents where checkpoint signature material is attached.
|
|
||
| ark, checkpoints, err := buildSubmitPackage(policy, inputs, outputs) | ||
| if err != nil { | ||
| return nil, nil, err | ||
| } |
There was a problem hiding this comment.
agree, and this is now in-fsm: StartTransferEvent is handled by Idle.ProcessEvent which builds deterministic submit package internally.
| sm.Start(ctx) | ||
|
|
||
| outbox := []OutboxEvent{ | ||
| &SendSubmitPackageRequest{ |
There was a problem hiding this comment.
first the ark tx needs to be signed
There was a problem hiding this comment.
yep, and that's now enforced in flow: submit outbox is emitted only after ArkSignedEvent.
| @@ -0,0 +1,79 @@ | |||
| package oor | |||
There was a problem hiding this comment.
cant find original comment but i still feel like this package is a bit messy: we have an states.go file and a receive_states.go file but then an events.go file that merges types from both...
the package is very large.. so again feel like maybe cleanly separating the two FSMs could be good... at the very least at a file naming level
There was a problem hiding this comment.
fair point. package split is still imperfect; i kept behavior changes separate in this stack, but i'm happy to do a focused file-structure cleanup pass right after this lands.
f01992d to
f67b8f4
Compare
9ffa9bb to
7a8ef31
Compare
f67b8f4 to
5b21fb8
Compare
| // TapTreeEncoded is the v0 tap tree encoding for the checkpoint output. | ||
| // | ||
| // This is intended to be attached to the Ark tx PSBT inputs under the | ||
| // `taptree` unknown key so finalization can later copy it onto the | ||
| // checkpoint output metadata. |
There was a problem hiding this comment.
almost there: the agent is trying to remain backwards compatible but doenst need to:
// CheckpointResult is a backwards-compatible alias for CheckpointArtifact.
type CheckpointResult = CheckpointArtifact`
| // | ||
| // This function does not attempt to sign the checkpoint tx. It also does not | ||
| // validate that the owner leaf is a canonical Ark closure (draft phase). | ||
| func BuildCheckpointPSBT(policy scripts.CheckpointPolicy, |
| // StartTransferEvent is the one-and-only "kick off a session" event. | ||
| // After this, the FSM should have a deterministic submit package so | ||
| // retries produce a stable session id (Ark txid). |
|
|
||
| // CheckpointInputs is the set of VTXO inputs to convert into | ||
| // checkpoints. | ||
| CheckpointInputs []oortx.CheckpointInput |
There was a problem hiding this comment.
perhaps the name can be improved then. VTXOInputs?
| // AnchorAmount is reserved for future extensions. v0 uses a fixed P2A | ||
| // anchor output with 0 sats. | ||
| AnchorAmount btcutil.Amount |
There was a problem hiding this comment.
the field is still there though? also it is completely unused now... let's just remove it?
| // SubmitAcceptedEvent is the client's view of the "point-of-no-return". | ||
| // | ||
| // Once the operator co-signs the checkpoint PSBTs, the client must be | ||
| // able to resume and obtain the same co-signed artifacts even if it | ||
| // did not receive the response due to a crash or transport loss. |
There was a problem hiding this comment.
doesnt look like this has been addressed
| } | ||
|
|
||
| // SendSubmitPackageRequest asks the transport layer to send the submit package | ||
| // (Ark PSBT + checkpoint PSBTs) to the server. |
There was a problem hiding this comment.
what i mean is that the comment should be expanded to say this
| // AnchorAmount is reserved for future extensions. v0 uses a fixed P2A | ||
| // anchor output with 0 sats. | ||
| AnchorAmount btcutil.Amount | ||
| } |
There was a problem hiding this comment.
later in this series seems to add PrebuiltArkPSBT and PrebuiltCheckpointPSBTs here but they are unused. can we remove them?
There was a problem hiding this comment.
Agreed.
Those prebuilt fields are not in StartTransferEvent anymore; the event now carries only inputs/recipients/policy:
e32d939
| ArkPSBT *psbt.Packet | ||
|
|
||
| FinalCheckpointPSBTs []*psbt.Packet |
There was a problem hiding this comment.
is this tracked in the new issue?
e32d939 to
3bd47c1
Compare
ellemouton
left a comment
There was a problem hiding this comment.
incoming actor stuff looks good!
a question about receive-flow but that can also be addressed in later PRs in the series since the actor-wrapping for that isn't in this pr
| // Outpoint is the outpoint of the VTXO output being spent. | ||
| Outpoint wire.OutPoint | ||
|
|
||
| // WitnessUtxo is the previous output being spent (value + pkScript). |
There was a problem hiding this comment.
okkkk doesnt look addressed in this pr but looks like it is later on somwhere
| // IncomingTransferNotification is an application-facing notification. | ||
| // It carries the canonical Ark PSBT and the derived recipients so the | ||
| // UI can display a stable summary of the transfer. |
There was a problem hiding this comment.
Ack — cleaned up the comment placement / formatting in this file so the struct docs read cleanly.
| // 1) notify the app/UI so it can show the transfer; | ||
| // 2) materialize incoming VTXOs into local state; and |
There was a problem hiding this comment.
not quite understanding the differnence between thtse two. Also, should we send the app/UI notifications about a transfer before weve actually checked that we are a recipient?
looks like the "check if we are recipient & extraction of our new received vtxos" is done outside of the FSM...wondering if that is the correct boundary? as is, this FSM doesnt seem to do much
There was a problem hiding this comment.
i think this receive flow/fsm could use some docs/diagrams just to understand what is happening & when
There was a problem hiding this comment.
Good point. The intent is that these two outbox events serve different consumers:
IncomingTransferNotification: application/UI-facing “you received an OOR transfer” signal so the app can surface a summary and decide how/when to present it.MaterializeIncomingVTXOsRequest: wallet/state-facing work item that filters the transfer outputs down to “ours” and persists the resulting VTXOs/checkpoints.
On the “notify before recipient check” question: the receive flow assumes IncomingTransferEvent is only delivered by the transport layer when the server believes we are a recipient. The FSM still does the structural/canonical validation (SessionID/txid match + canonical Ark PSBT validation), and the wallet-ownership filtering is intentionally behind the outbox boundary because it depends on wallet keys/policy.
To make this boundary clearer, I added a human-readable diagram + notes in code:
(See the new comment block in oor/receive_transitions.go + the NOTE on IncomingTransferEvent in oor/events.go.)
| // This event is intended to be delivered by some higher layer (RPC push, | ||
| // polling, or push-notification wakeup) once the server has accepted and | ||
| // finalized the transfer. |
| // outboxSealed marks this as implementing the sealed OutboxEvent interface. | ||
| func (m *IncomingTransferNotification) outboxSealed() {} | ||
|
|
||
| // MaterializeIncomingVTXOsRequest asks the application/wallet layer to |
There was a problem hiding this comment.
both this and IncomingTransferNotification mention application layer communication. isnt super clear to me what the line between these two outbox messages are
There was a problem hiding this comment.
feels like there should be a "Verify/ExtractOurVTXO" step somewhere
There was a problem hiding this comment.
Yep, agreed the boundary wasn’t clear.
The intended split is:
IncomingTransferNotification: UI/app-facing “show/notify” semantics (surface a summary / notification). It should not persist wallet state.MaterializeIncomingVTXOsRequest: wallet/state-facing “filter + persist” semantics (decide which outputs are ours, then persist/track VTXOs).
I clarified the doc comments to make this explicit here:
| // outboxSealed marks this as implementing the sealed OutboxEvent interface. | ||
| func (m *MaterializeIncomingVTXOsRequest) outboxSealed() {} | ||
|
|
||
| // SendIncomingAckRequest requests the transport layer to ack receipt of the |
There was a problem hiding this comment.
should we do what we do for rounds and make these explicit serverconn.ServerConnMsgs that we send to server via
// ServerConn is a reference to the ServerConnectionActor for sending
// messages to the Ark server.
ServerConn actor.TellOnlyRef[serverconn.ServerConnMsg]
?
There was a problem hiding this comment.
Good question.
My intent with the OOR outbox types is to keep them “protocol-level” (submit/finalize/ack/sign/persist) so the FSM doesn’t depend on a specific transport actor shape. A concrete OutboxHandler can still be a thin adapter that sends serverconn.ServerConnMsg internally.
I agree it’s worth converging the patterns across subsystems; I added a follow-up tracker item for this in darepo#91 so we can make an explicit call and keep it consistent with the DurableActor story:
https://github.com/lightninglabs/darepo/issues/91
| session, outbox, err := NewSession( | ||
| ctx, req.Policy, req.Inputs, req.Recipients, | ||
| ) | ||
| if err != nil { |
There was a problem hiding this comment.
where do we protect against same transfer req being submitted twice?
There was a problem hiding this comment.
Good catch.
I made StartTransferRequest idempotent by session ID: if the same deterministic transfer is submitted twice (retries/replay), we keep the existing session FSM and just return the existing SessionID.
Applied in:
3388d4d
|
|
||
| // driveOutbox executes outbox work using the configured handler and feeds any | ||
| // follow-up events back into the FSM. | ||
| func (a *OORClientActor) driveOutbox(ctx context.Context, sessionID SessionID, |
There was a problem hiding this comment.
somewhere (not in this PR) we should probs unify how this outbox driving happens as we do something completely different in rounds... but then again perhaps it is system specific 🤷♀️
There was a problem hiding this comment.
Agreed.
The current OOR actor outbox driving is intentionally small/simple so we could iterate on the OOR protocol boundary quickly.
Now that DurableActor primitives are landing, I think it makes sense to converge on one standard pattern for “drive outbox / persist / retry / replay” across rounds and OOR. I tracked this as a follow-up in darepo#91 so we can do it in one place without reshaping this PR again:
https://github.com/lightninglabs/darepo/issues/91
Add deterministic builders for v0 OOR checkpoint and Ark PSBTs. BuildCheckpointPSBT constructs an unsigned checkpoint tx that pays the full input value to a checkpoint P2TR output derived from the draft checkpoint script helpers. BuildArkPSBT constructs a fee-less Ark tx spending checkpoint outputs, sorts inputs/outputs canonically, appends the anchor output last, and attaches per-input `taptree` metadata. Unit tests assert the resulting submit package passes ValidateSubmitPackage. (cherry picked from commit f3615c5)
Add a client-side protofsm workflow for out-of-round transfers. The FSM builds a canonical submit package via lib/tx/oor, emits transport outbox requests, and validates finalize packages after client signature attachment and tap-tree metadata binding. (cherry picked from commit e850abe)
Extract non-anchor recipient outputs from incoming Ark PSBTs and include them in the incoming notification. Also emit a dedicated outbox request that higher layers can implement to materialize/persist received VTXOs once closure/script semantics are finalized. This keeps the receive path restart-friendly and allows end-to-end harnesses to wire in mock materialization before durable storage exists. (cherry picked from commit ca19187)
Add a minimal recipient-side OOR transfer FSM that validates an incoming Ark PSBT structurally, emits an application notification, and requests an ack to the server via an outbox boundary. This keeps sender/receiver flows symmetric and provides a place to later attach full VTXO materialization once scripts/closures and the notification payload are finalized. (cherry picked from commit a9ca5f7)
Introduce an OORClientActor that owns per-session protofsm instances and drives side effects through an explicit outbox handler interface. This mirrors the server-side coordinator pattern and enables fully in-process end-to-end tests without requiring RPC or durable actors yet. A new unit test exercises the happy path by stubbing out server/wallet behavior in an in-memory outbox handler. (cherry picked from commit 989cbdf)
Simplify StartTransferEvent by removing the unused AnchorAmount field and renaming CheckpointInputs to VTXOInputs so the input role is explicit. This keeps the event contract aligned with actual v0 behavior and avoids carrying dead data. Also clean up SubmitAcceptedEvent documentation to describe point-of-no-return semantics and Ark artifact usage more clearly. Transitions are updated to use the renamed field without behavioral changes.
Treat StartTransferRequest as idempotent by returning the existing session ID when the same deterministic transfer is submitted twice. This keeps the actor resilient to retries/replay and avoids accidentally replacing an in-flight session FSM.
3388d4d to
23dfdae
Compare
Add a human-readable receive FSM diagram and clarify the intended responsibilities for incoming transfers. The FSM performs structural/canonical validation and emits outbox requests in a fixed order: - notify app/UI - materialize wallet state - ack the server Wallet-ownership filtering happens behind the outbox boundary.
Make the boundary between incoming-transfer outbox messages explicit. IncomingTransferNotification is UI/app-facing (display/notify). MaterializeIncomingVTXOsRequest is wallet/state-facing (filter + persist).
Context
Companion client stack for OOR epic: lightninglabs/darepo#89.
This split introduces client-side transfer orchestration (outgoing + incoming)
using FSM + actor abstractions.
Scope
Included
Not Included
Testing
Stack