OOR client FSM (grand summary) - #52
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) transfers by introducing fundamental building blocks. It establishes the necessary cryptographic primitives for constructing and validating Ark transactions and their associated checkpoints using PSBTs. Furthermore, it defines state machines and an actor model to manage the lifecycle of both sending and receiving OOR transfers, ensuring a structured and resilient approach to these operations. The changes also include enhancements to VTXO management to support the new OOR functionalities. 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
|
![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 comprehensive set of features for client-side Out-of-Round (OOR) transfers. It adds low-level primitives for constructing and validating Ark and checkpoint PSBTs, including canonical ordering rules and tap tree encoding. On top of these primitives, it implements state machines (FSMs) for both outgoing and incoming OOR transfers, wrapped in an actor for clear lifecycle management and handling of side effects. The changes are well-structured, with a clear separation between transaction-level logic in the lib/ directory and protocol orchestration in the oor/ directory. The code is of high quality, well-tested, and includes thoughtful refactoring of existing vtxo and round components to integrate the new functionality. My review found one minor issue related to defensive programming in state recovery, which I've commented on. Overall, this is an excellent contribution that significantly advances the OOR transfer capabilities.
| default: | ||
| return &LiveState{VTXO: vtxo, LastCheckedHeight: vtxo.CreatedHeight} | ||
| return &LiveState{ | ||
| VTXO: vtxo, | ||
| LastCheckedHeight: vtxo.CreatedHeight, | ||
| } |
There was a problem hiding this comment.
The default case in this switch statement currently falls back to creating a LiveState. This could be risky if an unknown or corrupted VTXOStatus value is encountered, as it might cause the VTXO to be treated as live when it should not be. To make the state recovery more robust and fail-safe, it would be better to transition to a FailedState for any unhandled status. This ensures that any unexpected state is explicitly marked as an error condition, preventing potential misuse of the VTXO.
| default: | |
| return &LiveState{VTXO: vtxo, LastCheckedHeight: vtxo.CreatedHeight} | |
| return &LiveState{ | |
| VTXO: vtxo, | |
| LastCheckedHeight: vtxo.CreatedHeight, | |
| } | |
| default: | |
| return &FailedState{ | |
| VTXO: vtxo, | |
| Reason: fmt.Sprintf("unknown vtxo status %d", vtxo.Status), | |
| } |
There was a problem hiding this comment.
Fixed by changing the default case to return FailedState with an explicit "unknown vtxo status" reason, so we fail closed on unexpected statuses.
828eae0 to
6ef8b82
Compare
bhandras
changed the title
824cd1a to
5dd9cf7
Compare
| // caller has already constructed an Ark tx that includes exactly one anchor | ||
| // output of value 0. If the anchor output is missing or invalid, this returns | ||
| // an error rather than guessing what to do. | ||
| func CanonicalizeOrdering(tx *wire.MsgTx) error { |
There was a problem hiding this comment.
Seems we can use the existing BIP 69 sort here? We use a slight variant elsewhere in lnd.
There was a problem hiding this comment.
Updated canonical output ordering to BIP69 (amount then pkScript) and aligned the builders/validators and design doc; anchor output remains last.
| // This encoding is part of the PSBT profile for OOR transfers. If we ever need | ||
| // to support richer trees (multiple depths), this function must become | ||
| // versioned rather than changing behavior silently. | ||
| func EncodeTapTree(leaves [][]byte) ([]byte, error) { |
There was a problem hiding this comment.
Any reason to not use TLV here?
You could also in theory pass in txscript.IndexTapscriptTree here.
There was a problem hiding this comment.
My round persistence PR has TLV encoding for waddrmgr.Tapscript, which is used elsewhere and is a super set of just the leaves.
There was a problem hiding this comment.
Switched the tap tree encoding to the TLV leaf format used by waddrmgr.Tapscript, so we share the same durable representation while still encoding the full leaf set.
| // | ||
| // A future version should consider namespacing this (for example, | ||
| // `ark/taptree`) to reduce collision risk with other PSBT extensions. | ||
| TapTreePSBTKey = []byte("taptree") |
There was a problem hiding this comment.
Is it critical that we encode this within the PSBT, or is it fine to just transmit the tapscript leaf information along side?
There was a problem hiding this comment.
Not strictly required, but embedding it in the PSBT keeps the transfer package self-contained and deterministic across the client/server boundary and restarts; the same bytes being signed also carry the tap tree metadata, which avoids out-of-band mismatches during finalize/restore. The tradeoff is a slightly larger PSBT and tighter coupling to the PSBT profile/unknown-key namespace; passing it alongside is lighter-weight but adds coordination/state to keep metadata and signed bytes in sync.
| if err != nil { | ||
| return err | ||
| } | ||
|
|
There was a problem hiding this comment.
Does this handle fault tolerance w.r.t persistence + delivery of these outbox messages the way #48 does?
There was a problem hiding this comment.
Like let's say you're sending a message to the chain resolver sub-system that things failed and we need to go on chain. It gets this message but doesn't actually process it. This is now critical on restart that we start the un roll process, does this pattern have a cut out to handle such patterns?
There was a problem hiding this comment.
Also depending on the fault tolerance properties of the OutboxHandler (eg: does it handle all the messages in a single db transaction?), we may end up with inconsistent state on disk. Still getting through things atm, so persistence isn't super clear quite yet.
There was a problem hiding this comment.
Thank you for pointing this out 🙏 The current actor only persists the session snapshot and re-derives the outbox from state on resume, but it does not persist the outbox itself. That means correctness depends on the OutboxHandler providing durable, idempotent delivery (ideally a transactional outbox: persist snapshot + enqueue outbox in one DB tx), which is exactly what the durable actor layer in #48 provides. Without that, there’s a window where state can advance but the side effect isn’t durably queued, or a side effect is delivered and the follow-up transition isn’t persisted. So the intent here is to keep the boundary compatible with #48’s transactional outbox semantics; the handler implementation is where we must enforce atomicity/dedup for critical effects.
|
|
||
| // StartTransferEvent requests starting an OOR transfer by building a submit | ||
| // package (checkpoint PSBTs + Ark PSBT). | ||
| type StartTransferEvent struct { |
There was a problem hiding this comment.
What about the receiver side? Is your idea to have another state machine for receiving, or it'll be bundled into this state machine?
There was a problem hiding this comment.
Plan is a separate receive-side FSM. The incoming transfer path is modeled independently (see IncomingTransferEvent and receive states), so it’s not bundled into the outgoing FSM; this keeps notification/ack flows and failure handling separate.
| // The incoming transfer FSM is intentionally separate from the outgoing FSM so | ||
| // applications can handle notifications and acknowledgements independently of | ||
| // initiating transfers. | ||
| type IncomingTransferEvent struct { |
There was a problem hiding this comment.
Ah scratch my other comment, I see this now for incoming transfers.
| // Extract recipients and surface the notification to the | ||
| // application layer. | ||
| // | ||
| // The outbox is intentionally ordered: |
There was a problem hiding this comment.
Could move this comment down below re when the outbox fields are being set.
There was a problem hiding this comment.
Moved the explanatory comment to sit next to the outbox emission so it’s aligned with where the fields are set.
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
There was a problem hiding this comment.
Receive-time signing isn’t required in the current model. The incoming path only validates the canonical Ark PSBT, surfaces the transfer, and materializes VTXOs; the VTXO descriptor is built so the recipient can sign later when spending (collab or timeout). If you want an explicit receipt signature or other policy (e.g., cryptographic ack), we can add a dedicated outbox step, but that would be a protocol change beyond the current design.
5dd9cf7 to
e5fe62f
Compare
b3f6c56 to
426355e
Compare
426355e to
2ac0033
Compare
bhandras
changed the title
d4fd6c0 to
7d7f9bf
Compare
2ac0033 to
b73cdef
Compare
b73cdef to
7c06b56
Compare
(cherry picked from commit eadff97)
Add a v0 structural validator for OOR submit packages. The validator enforces canonical Ark PSBT ordering, ensures each Ark input spends a provided checkpoint tx output (vout=0), checks that Ark PSBT witness UTXOs match the referenced checkpoint output, and requires per-input `taptree` metadata for later finalization. Unit tests cover a happy path and common failure cases. (cherry picked from commit 1834eea)
Add a v0 structural validator for OOR finalize packages. The validator checks that the provided checkpoint PSBT set matches the Ark tx input checkpoint set (txid:vout=0), and requires each checkpoint PSBT to include some signature material (final witness/script or taproot sig fields). Unit tests cover a happy path and common failure cases. (cherry picked from commit 9f97816)
Add draft OOR checkpoint script helpers. This introduces a minimal CheckpointPolicy and helpers to deterministically construct a two-leaf checkpoint taproot tree (operator CSV unroll leaf plus caller-provided owner leaf) and derive the corresponding P2TR pkScript. The implementation lives in new files to minimize overlap with ongoing closure/vtxo refactors, and is intended to be swapped to closure-based building later. Unit tests assert the result is a valid P2TR pkScript and that the tapscript root hash and output key are computed consistently. (cherry picked from commit f6e2018)
7c06b56 to
aee4ce1
Compare
|
Closing this as this is replaced by the splits. |
2 participants
Summary
This is the grand roll-up for the full OOR client FSM work.
The reviewable changes are split across smaller PRs for incremental review; this PR tracks the consolidated branch for end-to-end context and CI only.
Split PRs (review targets)
Notes
main.