OOR client 3/4: durable actor, TLV codecs, signing context - #79
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 significantly improves the robustness and reliability of the OOR client by enhancing its test coverage. It focuses on critical areas such as crash-resume capabilities, ensuring that client sessions can be seamlessly restored even after unexpected interruptions. Additionally, the changes extend the testing of the client's state machine to cover various edge cases, including how it reacts to out-of-band events and handles communication errors, thereby making the client more resilient to failures and unexpected scenarios. 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 significantly improves the test coverage for the OOR client, focusing on crucial crash-resume scenarios and FSM edge cases. The new tests in oor/actor_drive_event_test.go, oor/coverage_misc_test.go, and oor/outbox_error_test.go are well-structured and cover important functionality like external event injection, terminal state behavior, and outbox error handling. The addition of TestOORClientActorResumeAfterServerCoSignedFromStore in oor/actor_resume_test.go is particularly valuable as it verifies the client's ability to recover from a persisted state after a crash. My main suggestion is to refactor some duplicated test setup code to improve maintainability. Overall, these changes greatly enhance the robustness and reliability of the client actor.
99ceb4a to
182ae84
Compare
ad6f25a to
989cbdf
Compare
989cbdf to
5c2be65
Compare
182ae84 to
af84021
Compare
5c2be65 to
a770497
Compare
af84021 to
0973b3e
Compare
a770497 to
ddd06f7
Compare
0973b3e to
61ee8b5
Compare
|
@bhandras, remember to re-request review from reviewers when ready |
| @@ -0,0 +1,215 @@ | |||
| package arktx | |||
There was a problem hiding this comment.
The existing
lib/tx/oorpackage keeps its public API stable via thin
wrappers and aliases, so server/client call sites (and tests) continue
to work while we migrate incrementally.
just checking (maybe answered later): but think we dont have to keep things backwards compatible rn and can just do big refactors (i know the agents like to do this bw compat thing)
There was a problem hiding this comment.
Updated: transitional OOR canonical wrappers were removed and call sites now use lib/tx/arktx directly, so there is no extra compatibility layer left here.
|
|
||
| // AwaitingArkConfirmation indicates the server accepted finalize and the | ||
| // client is optionally waiting for the Ark tx to confirm on-chain. | ||
| type AwaitingArkConfirmation struct { |
There was a problem hiding this comment.
still making way through diff but at this point not entirely sure i follow why we need this - does that mean the client is expected to unroll?
There was a problem hiding this comment.
in my mind, stronger semantics would perhaps mean "send via batch swap"
There was a problem hiding this comment.
This is mainly for durability/unilateral-exit readiness: client needs enough artifacts to recover path construction after restart.
There was a problem hiding this comment.
You were right to question this — that was a design mistake on our side.
We initially introduced an AwaitingArkConfirmation phase (WaitForArkConfirmation / ArkConfirmDepth) while shaping restart semantics, but this does not belong in the normal collaborative OOR flow. It made the state model noisier without a clear protocol requirement, so we removed it from split 3.
Current model no longer includes that phase or fields.
Ref: d897a4b
ddd06f7 to
be7ba2b
Compare
61ee8b5 to
e0c6502
Compare
be7ba2b to
d1b9baf
Compare
3d1da3a to
d37031c
Compare
203c2cb to
bb493dd
Compare
d37031c to
0ccb7e4
Compare
54a56c4 to
7935c29
Compare
b32b529 to
b017cd1
Compare
7935c29 to
ad25141
Compare
b017cd1 to
4dbd28b
Compare
5c293ea to
4735dfd
Compare
b1e870b to
8bca2e0
Compare
| submitPackageVersionRecordType tlv.Type = 1 | ||
| submitPackageArkPSBTRecordType tlv.Type = 3 | ||
| submitPackageCheckpointsRecordType tlv.Type = 5 |
There was a problem hiding this comment.
just noting that we dont do this skipping of numbers on server side
There was a problem hiding this comment.
Fixed: TLV record types now use sequential numbering (1, 2, 3) matching the server-side convention.
| // Each checkpoint PSBT is expected to spend exactly one VTXO (input index 0). | ||
| // The TransferInput slice is expected to match the checkpoint PSBT slice | ||
| // 1:1. | ||
| func SignCheckpointPSBTs(signer input.Signer, inputs []TransferInput, |
There was a problem hiding this comment.
can the server not also make use of this rather than re-defining it?
There was a problem hiding this comment.
Good call. The helpers (AddTapLeafScript, AddTaprootScriptSpendSig) are now in lib/tx/psbtutil so both client and server can use them. The server-side dedup (replacing its local copies with imports from psbtutil) is tracked in darepo#91.
|
|
||
| // TransferInputs are the vtxo descriptors and scripts needed later on | ||
| // to sign the checkpoint PSBTs. | ||
| TransferInputs []TransferInput |
There was a problem hiding this comment.
does the FSM itself actually need this? isnt it only used for signing (ie at the actor level)?
There was a problem hiding this comment.
You're right — the FSM transitions don't use it. It's threaded through the states so the actor-level outbox events (which need the signing context) and checkpoint snapshots (which capture it for crash-resume) can access it without a separate lookup path. Added a comment on both states explaining this rationale.
| ) | ||
|
|
||
| const ( | ||
| oorDurableCommandTLVType tlv.Type = 42001 |
There was a problem hiding this comment.
reason behind the large type number?
There was a problem hiding this comment.
Fixed: renamed to 0x7003 with a doc comment explaining the high range avoids collisions with the actor framework's reserved types (e.g. actor.RestartTLVType). The old 42001 was arbitrary.
| // deterministically, without serializing Go wallet types directly. | ||
| type TransferInputSnapshot struct { | ||
| // Outpoint is the VTXO outpoint being transferred. | ||
| Outpoint wire.OutPoint `json:"outpoint"` |
There was a problem hiding this comment.
why do we need the json tags if we are using tlv?
There was a problem hiding this comment.
Fixed: JSON tags removed. They were leftover from an earlier iteration before the TLV encoding was in place.
| @@ -3,11 +3,19 @@ package oor | |||
| import ( | |||
There was a problem hiding this comment.
code here could use some docs!
There was a problem hiding this comment.
Fixed: added godoc comments to oorDurableBehavior, Receive, handleRestart, restoreFromCheckpoint, resumeRestoredSessions, and persistCheckpoint.
|
|
||
| actorID string | ||
|
|
||
| sessions map[SessionID]*sessionHandle |
There was a problem hiding this comment.
Good catch — removed. actorID was redundant with cfg.ActorID. All references now go through b.cfg.ActorID.
| "github.com/stretchr/testify/require" | ||
| "google.golang.org/protobuf/types/known/anypb" | ||
| ) | ||
|
|
There was a problem hiding this comment.
godocs missing for this commit and before
There was a problem hiding this comment.
Fixed: added godoc comments to all test functions in this file.
8bca2e0 to
c28fdfc
Compare
Move Ark tx canonicalization and checkpoint-building helpers out of `lib/tx/oor` into dedicated `lib/tx/arktx` and `lib/tx/checkpoint` packages. Keep compatibility wrappers in `lib/tx/oor` so existing call sites and current tests continue to compile while the stack migrates.
Add `lib/tx/psbtutil` helpers for serializing and parsing PSBT packets. Introduce a versioned submit-package encoding in `lib/tx/oor` so OOR payloads have a stable, transport-independent format for persistence and replay.
Refactor outgoing transfer inputs to carry full VTXO signing descriptors (keys, scripts, and expiry) instead of raw checkpoint-only inputs. Add checkpoint-signing helpers that consume this context and add incoming materialization helpers so receive-side flow can derive local VTXO artifacts from Ark outputs.
Replace JSON command payload serialization with typed TLV records and length-prefixed vectors for start and session durability envelopes. Add transfer-input snapshot payload encoders/decoders and focused round-trip tests for durable actor command paths.
Add explicit overflow checks when converting decoded TLV numeric values to signed and narrower integer types. Fail fast on malformed payloads instead of silently wrapping values, with coverage for actor messages and outgoing snapshot codecs.
Bind outgoing sessions to a canonical identity derived from the proposed spend and keep the start-session contract consistent at actor boundaries. Reject duplicate session IDs during restore to prevent malformed durable payloads from replacing an existing in-memory session.
Replace placeholder `ToProto` implementations for submit, finalize, mark-inputs-spent, and incoming-ack outbox events. Emit typed `Any` envelopes with deterministic binary payloads and explicit `*.error` envelopes, then add tests asserting type URLs and non-empty payload bytes.
Set restart-envelope `available_at` to the epoch in `PrependRestartMessage` instead of using `time.Now()`. This preserves restart priority while avoiding ordering drift when the delivery store clock differs from wall clock in tests or runtime.
Persist DriveEventRequest messages through the durable actor command codec so externally driven transitions survive restarts. Harden SubmitAcceptedEvent handling by requiring the request session, event session, and Ark txid-derived session identity to match.
Add focused unit coverage for OOR primitive helpers: checkpoint build artifacts, submit-package wrapper methods, PSBT utility codecs, outgoing snapshot transitions, and transfer-input snapshot round-trips. This expands confidence in the low-level tx/persistence helpers that the actor durability flow depends on.
Decode each tap tree leaf from its length-scoped reader so unknown TLV records do not consume bytes from adjacent records. Add a regression test that appends an extra odd TLV record to verify known fields still decode correctly.
Add package-level docs and exported type/field comments across the OOR transfer stack and tx helper packages. This aligns public API documentation with repo style requirements and keeps durability-related types easier to review.
Add design comments explaining why TransferInputs are carried on FSM states rather than managed at the actor boundary. The signing context is threaded through states so the FSM can emit complete outbox events and checkpoint snapshots can capture it for crash-resume.
c28fdfc to
1d659be
Compare
Context
Third client PR in the OOR stack. Split 2 (#78) landed the outgoing/incoming FSMs and the actor wrapper. This split makes the actor durable: every command, FSM state, and signing artifact survives a restart so in-flight transfers resume exactly where they left off.
What changes
Tx primitive reorganization
lib/tx/ooris split into focused packages (arktx,checkpoint,psbtutil) and a versioned submit-package encoding is added so OOR payloads have a stable binary format for persistence and replay.VTXO signing context
Transfer inputs now carry full VTXO descriptors (keys, scripts, expiry) instead of raw checkpoint-only data. This context is threaded through FSM states so checkpoint signing, outbox events, and crash-resume snapshots all have access to it without a separate lookup path.
TLV durability codecs
JSON command/snapshot serialization is replaced with typed TLV records. Start, session, restore, and drive-event payloads each get their own encode/decode path with overflow-checked numeric conversions and length-scoped leaf decoding.
Durable actor plumbing
DriveEventRequestis persisted through the command codec.Anyenvelopes with explicit error variants.Hardening and docs
Client durability model
Two persistence scopes with distinct ownership:
1) Runtime durability (generic actor delivery)
Owned by
db/actordeliveryandbaselib/actor.DurableActor:mailbox_messages)processed_messages)ask_results)fsm_checkpoints)dead_letters)2) OOR session snapshot durability (checkpoint payload)
Owned by OOR durable behavior in
oor/actor.go:OutgoingSnapshotPersistence map
mailbox_messages.payloadReceivedispatchfsm_checkpoints.state_dataCommits
Not included
Stack
Parent: #78 · Next: #81