arkscript/oor: Prepare channel-backed OOR funding - #1189
Conversation
Represent cooperative channel funding, delayed channel materialization, and the longer funder refund as one validated VTXO policy.
Persist deterministic OOR packages before signatures or transport are released so channel negotiation can commit or abort safely.
Keep application-owned channel VTXOs out of wallet balances while retaining enough policy and expiry state to resume materialization.
There was a problem hiding this comment.
Pull request overview
This PR adds infrastructure for Ark-backed Lightning channel flows by introducing application-owned “recovery-only” VTXOs, a two-phase (prepare/commit) OOR transfer mode, and non-standard unroll policy handling (including recognizing a peer-published pre-signed final spend as successful completion). It also adds DB and codec support so these new states and artifacts survive restarts and actor reaping.
Changes:
- Add
VTXOStatusRecoveryOnlyplus store/manager logic to keep application-owned outputs out of wallet balance/selection/actors, while still allowing deterministic recovery and spend completion. - Extend unroll to support policy-kind dispatch and “pre-signed final spend” policies, treating the exact expected transaction as success even if published by the peer.
- Add prepared OOR sessions (prepare → commit/abort), persist richer outgoing snapshots (recipients + pre-PONR failure marker), and allow registry state reads after terminal children are reaped.
Reviewed changes
Copilot reviewed 34 out of 36 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| vtxo/manager.go | Allows actorless completion for recovery-only spends and confirms recovery-only exits gated by non-standard policy kind. |
| vtxo/manager_admission_test.go | Adds tests for actorless recovery-only completion and missing-actor rejection for normal VTXOs. |
| vtxo/interfaces.go | Introduces VTXOStatusRecoveryOnly and string form. |
| vtxo/filter.go | Excludes recovery-only from pending-balance accounting. |
| vtxo/filter_test.go | Verifies pending-balance exclusion for recovery-only descriptors. |
| unroll/policy_resolvers.go | Adds a composite resolver that dispatches exit spend policy by kind. |
| unroll/interfaces.go | Adds PreSignedExitSpendPolicy interface for immutable final spends. |
| unroll/actor.go | Recognizes the exact pre-signed final spend as a successful completion path. |
| unroll/actor_test.go | Adds harness seam + test verifying peer-published pre-signed spend completes the actor. |
| oor/transitions.go | Adds Prepared state handling, commit/abort events, and PrePONR tracking for failures. |
| oor/states.go | Introduces Prepared state and extends Failed with PrePONR. |
| oor/session.go | Adds APIs for prepared sessions and plumbs PrepareOnly through session creation. |
| oor/session_test.go | Tests prepared commit behavior and abort releasing inputs (pre-PONR). |
| oor/session_actor_handlers.go | Forwards PrepareOnly into session creation from actor handler. |
| oor/resume.go | Ensures Prepared state has no implied outbox side effects on resume. |
| oor/registry.go | Allows GetState to read persisted snapshots when a terminal child has been reaped. |
| oor/registry_test.go | Tests GetState reads persisted snapshot without spawning a child. |
| oor/outgoing_snapshot.go | Adds Prepared snapshot phase, recipient metadata persistence, and pre-PONR failure marker; adds recipient extraction helper. |
| oor/outgoing_snapshot_codec.go | Adds TLV records for recipient metadata + pre-PONR failure flag; decodes them with limits. |
| oor/outgoing_snapshot_codec_test.go | Extends TLV round-trip test coverage for new snapshot fields. |
| oor/events.go | Adds prepare-only start flag and commit/abort prepared events. |
| oor/actor_messages.go | Adds PrepareOnly to start-transfer request encode/decode. |
| oor/actor_durable_message.go | Adds durable mailbox encoding for prepare-only + prepared commit/abort events; refactors incoming transfer decode helper. |
| oor/actor_durable_message_test.go | Adds tests for prepare-only payload round-trip and prepared commit/abort durable round-trip. |
| lib/arkscript/channel_vtxo.go | Adds compiled policy construction/validation helpers for Ark-backed channel VTXO scripts. |
| lib/arkscript/channel_vtxo_test.go | Tests channel policy paths, reaction-window enforcement, and template validation. |
| db/vtxo_store.go | Adds SaveRecoveryOnlyVTXO and SetRecoveryOnlyVTXORelativeExpiry plus descriptor replay validation. |
| db/vtxo_store_test.go | Tests recovery-only descriptors are excluded from wallet sets and validates replay rules. |
| db/sqlc/vtxo.sql.go | Adds generated SQL for setting recovery-only relative expiry. |
| db/sqlc/schemas/generated_schema.sql | Documents new numeric status mapping for Expired/RecoveryOnly. |
| db/sqlc/queries/vtxo.sql | Adds SetRecoveryOnlyVTXORelativeExpiry query. |
| db/sqlc/querier.go | Exposes new query in the sqlc querier interface. |
| db/sqlc/migrations/000004_vtxos.up.sql | Documents new numeric status mapping for Expired/RecoveryOnly. |
| db/round_store.go | Plumbs the new query into the RoundStore interface. |
| db/oor_unroll_resolver.go | Adds session-id-based package resolution path and shares traversal logic. |
| db/oor_unroll_resolver_test.go | Tests resolving unroll packages by session ID before an outpoint binding exists. |
Files not reviewed (2)
- db/sqlc/querier.go: Generated file
- db/sqlc/vtxo.sql.go: Generated file
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| IdempotencyKey: string(idKey), | ||
| DispatchRequestData: dispatchRequestData, | ||
| AdmissionDeadlineUnixNanos: int64(deadline), | ||
| PrepareOnly: prepareOnly != 0, | ||
| }, nil |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 20dcd1c701
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if evt.PrepareOnly { | ||
| return &StateTransition{ | ||
| NextState: &Prepared{ |
There was a problem hiding this comment.
Restore keyed prepared sessions into retry dedup
When the daemon restarts while a keyed PrepareOnly session is in Prepared, no oor_dispatch_attempts row exists because the first submit has not been queued, and restoreNonTerminal restores the child without rebuilding pendingOutgoingKeys. A lookup or retry by that key therefore reports a miss; if the wallet selects different inputs, the resulting session has a different ID and its prepared-row commit repeatedly collides with the existing active idempotency-key index instead of returning the original session. Restore prepared keys into the retry lookup or consult the mutable prepared row before admitting another session.
AGENTS.md reference: oor/AGENTS.md:L82-L86
Useful? React with 👍 / 👎.
|
/gateway review |
There was a problem hiding this comment.
Gateway review — 5 findings
🔴 0 Blocker · 🟠 3 Major · 🟡 2 Minor · 🔵 0 Nit
Summary
This is layer 1 of the Ark channels stack: a RecoveryOnly VTXO status that keeps application-owned outputs out of wallet balance/selection/actors, a prepare/commit two-phase OOR mode so a channel coordinator can use a deterministic outpoint before any signature is released, and unroll support for recognizing a peer-published pre-signed final spend as success. The layering is clean and the new states are threaded through the snapshot codec, the registry, and the DB consistently.
Three things need attention before merge. The prepare-only mode creates a durable session that carries an idempotency key but no dispatch-attempt row, and validateOutgoingDispatchIdentity treats exactly that shape as a keyless admission — so a keyed prepared transfer becomes unretriable after a restart. SaveRecoveryOnlyVTXO's replay validation treats expiry as immutable while SetRecoveryOnlyVTXORelativeExpiry, ten lines above it, exists to mutate that column. And ValidateChannelVTXOTemplate compares an untrusted template only by its set of compiled leaf scripts, never by the output script that template actually derives.
The new channel-policy delay arithmetic (validate) is carefully bounded and well tested; the recovery-only exclusion from ListLiveVTXOs / ListRecoverableVTXOs / selection candidates is covered end to end.
Bot commands
/gateway re-review— re-run after pushing changes (maintainers)/gateway dismiss <id>— silence a finding (maintainers)/gateway explain <id>— elaborate on a finding (anyone)
| session, _, err := newSessionWithDispatchRequest( | ||
| ctx, req.Policy, req.Inputs, req.Recipients, req.IdempotencyKey, | ||
| req.DispatchRequestData, r.envConfig(), | ||
| req.DispatchRequestData, req.PrepareOnly, r.envConfig(), |
There was a problem hiding this comment.
🟠 F1 (Major) — Keyed prepared transfer cannot be retried after restart · oor/registry.go:650
A PrepareOnly transfer admitted with an idempotency key becomes permanently unretriable under that key once the daemon restarts: the caller gets ErrIdempotencyKeyConflict instead of the existing session, and there is no other way to reach the prepared session by key.
Why this matters
The mechanism is the interaction between the new prepare-only path and the existing dedup ladder in handleStartTransfer / validateOutgoingDispatchIdentity. A prepared session never enqueues submit transport, so no oor_dispatch_attempts row is written — the first dedup lookup (GetDispatchAttemptByIdempotencyKey) misses. The second witness, pendingOutgoingKeys, is documented as a same-boot-only map and is not rebuilt by restoreNonTerminal, so it also misses after a restart. The registry then rebuilds the deterministic session (same inputs → same Ark txid → same session id) and calls validateOutgoingDispatchIdentity, which finds a live durable row with no attempt and hits case err == nil: return ErrIdempotencyKeyConflict. That branch's comment — "A live durable session without an attempt was admitted keylessly" — is exactly the assumption prepare-only breaks: OORSessionRegistryRecord.IdempotencyKey is populated for these rows, but the function never reads it.
Two candidate fixes: compare record.IdempotencyKey == key in validateOutgoingDispatchIdentity and treat a match as the same operation (returning the existing session), or repopulate pendingOutgoingKeys from the restored non-terminal rows in restoreNonTerminal. The first is the durable one, since the in-memory map is explicitly documented as a same-boot witness.
| if row.RoundID != desc.RoundID || row.Amount != int64(desc.Amount) || | ||
| !bytes.Equal(row.PkScript, desc.PkScript) || | ||
| !bytes.Equal(row.PolicyTemplate, desc.PolicyTemplate) || | ||
| row.Expiry != int32(desc.RelativeExpiry) || |
There was a problem hiding this comment.
🟠 F2 (Major) — Replay of SaveRecoveryOnlyVTXO fails after the expiry is set · db/vtxo_store.go:231
Once SetRecoveryOnlyVTXORelativeExpiry has written the channel final-spend delay, any replay of SaveRecoveryOnlyVTXO with the original descriptor fails with "recovery-only VTXO descriptor mismatch" — the exact idempotent replay the method's own doc comment promises to accept.
Why this matters
validateRecoveryOnlyVTXORow compares row.Expiry != int32(desc.RelativeExpiry) as part of the "immutable descriptor" set, but expiry is precisely the column the sibling method mutates (SET expiry = $3 ... WHERE ... status = 9). The existence of a separate setter implies the delay is not known at save time; if it were, the caller would stamp desc.RelativeExpiry before saving and the setter would be unnecessary. So the ordering SaveRecoveryOnlyVTXO → SetRecoveryOnlyVTXORelativeExpiry → replay of the durable close action → SaveRecoveryOnlyVTXO is the expected flow, and its last step errors. The comment on the test that exercises this ("Durable close actions may replay source preparation after the row has already been committed. Identical terms are idempotent") documents the intent, but the test only replays before the expiry is ever set, so it passes.
Either drop expiry from the immutable comparison set (it is mutable by construction), or accept a row whose expiry differs from the descriptor's when the row is already RecoveryOnly. Whichever you choose, add a test that runs the setter between the two SaveRecoveryOnlyVTXO calls, since that is the sequence the durable replay actually produces.
| actualScripts[string(script)] = struct{}{} | ||
| } | ||
|
|
||
| for i := range expected.Template.Leaves { |
There was a problem hiding this comment.
🟠 F3 (Major) — Channel template validation never checks the derived output script · lib/arkscript/channel_vtxo.go:191
ValidateChannelVTXOTemplate can accept an untrusted template that does not fund the P2TR output the local side expects, because it compares only the set of compiled leaf scripts and never compares the script the template derives.
Why this matters
EncodeChannelVTXOArtifacts shows the output script is a function of the whole template (policy.Template.PkScript()), not of its leaves alone. The validator builds actualScripts as an unordered map[string]struct{} and asserts each expected leaf is present, so any template property that feeds PkScript() but is not a leaf script — taptree ordering, an internal key, per-leaf version — is unconstrained. I cannot tell from the diff whether PolicyTemplate carries an internal key or whether Compile() canonicalizes leaf order, so I cannot say how far this gap goes; if leaves are sorted and there is no caller-controlled internal key, the current check may be equivalent in practice. If either is caller-controlled, a counterparty can supply a template that passes validation while committing to a different output than the one the local side is about to fund.
The cheap, complete fix is to compare the derived scripts directly: template.PkScript() against expected.Template.PkScript(), one comparison that subsumes leaf contents, leaf count, ordering, and any internal-key/tree field. Keeping the per-leaf loop afterward is still useful for producing a specific error message. Since this function is the gate on an untrusted channel contract and has no callers yet, it is worth getting exact now rather than in layer 2.
| continue | ||
| } | ||
| support, ok := resolver.(ResolverKindSupport) | ||
| if !ok || !support.SupportsKind(req.Kind) { |
There was a problem hiding this comment.
🟡 F4 (Minor) — PolicyResolvers silently ignores resolvers that don't advertise kinds · unroll/policy_resolvers.go:36
A resolver added to a PolicyResolvers list that implements ExitSpendPolicyResolver but not the optional ResolverKindSupport is silently skipped — ResolveExitSpendPolicy never consults it and the caller gets "no exit spend policy resolver for kind" as if the resolver were absent. The element type is ExitSpendPolicyResolver, so nothing at the type level tells a caller that the extra interface is mandatory; the type's doc comment ("Each child resolver remains responsible for validating its own reference") reads as if plain resolvers are fine. The test double added in this PR, testExitPolicyResolver, is itself an example of a resolver that would be dropped. Either narrow the slice element type to an interface that embeds ResolverKindSupport, or fall back to calling a non-advertising resolver and letting it reject the kind itself.
| require.NoError(t, err) | ||
| require.Empty(t, recoverable) | ||
| require.ErrorContains( | ||
| t, vtxoStore.SetRecoveryOnlyVTXORelativeExpiry( |
There was a problem hiding this comment.
🟡 F5 (Minor) — Recovery-only expiry setter is only tested on the overflow path · db/vtxo_store_test.go:2050
The only assertion on SetRecoveryOnlyVTXORelativeExpiry is the ^uint32(0) overflow rejection, so neither the success path nor the status = 9 guard is covered. That guard is the security property the query exists for — its comment claims "Ordinary wallet VTXOs can never be modified through this query" — and the rows != 1 check that enforces it is untested. Two more assertions in this test would close it: call the setter with a valid delay and confirm the row's RelativeExpiry changed, and call it against a Live descriptor and confirm it returns "not found" and leaves the row untouched.
|
🤖 gateway audit metadata for this PR — auto-generated, please don't edit. |
Summary
This is layer 1 of 5 in the Ark channels stack. The next layer is #1190.
Testing
go test ./lib/arkscript ./oor ./unroll ./vtxo ./dbmake buildmake lint-changed-local