Skip to content

darepod: support custom vHTLC refresh signatures - #745

Merged
bhandras merged 23 commits into
mainfrom
codex/vhtlc-refresh-client
Jun 24, 2026
Merged

darepod: support custom vHTLC refresh signatures#745
bhandras merged 23 commits into
mainfrom
codex/vhtlc-refresh-client

Conversation

@bhandras

@bhandras bhandras commented Jun 16, 2026

Copy link
Copy Markdown
Member

What this PR does

This PR adds the darepod and SDK support needed for cooperative custom vHTLC refresh.

A vHTLC refresh is the path where swapdk-server keeps a live swap vTXO alive by moving it into a fresh Ark round before the old batch expires. That keeps the swap on the normal Lightning path instead of immediately escalating to recovery.

The important detail is that a vHTLC is not a normal single-party wallet vTXO. Its future forfeit transaction can require signatures from more than one participant. The daemon that queues the refresh must therefore be able to collect a participant signature after the round builder has assigned the connector and built the exact forfeit transaction.

This branch teaches darepod to do that.

What changed

The PR adds four pieces of plumbing.

  1. Round and vTXO code can carry multiple participant signatures for one forfeit transaction.
  2. Custom vTXO refresh requests can include explicit forfeit signing context, so darepod knows how a later participant-signature request should be answered.
  3. The daemon RPC exposes pending connector-bound forfeit-signature requests and accepts submitted participant signatures for those requests.
  4. The bundled swap runtime signs the correct side of the request:
    • if darepod has a local participant signer, it signs inside the daemon;
    • if another participant must sign, darepod publishes a pending request and waits for the external signature to be submitted.

There is also a small cleanup path for failed custom refresh admission. If darepod queued temporary custom refresh actors but the later trigger step fails, the RPC asks the wallet to drop those temporary actors so stale signer contexts do not linger.

The RPC contract

The protocol is intentionally route-based, not swap-direction-based. A custom vHTLC may be used by swaps today, but the daemon RPC does not need to know whether the caller thinks of the vHTLC as a swap in or a swap out.

RefreshCustomVTXOs takes the old custom vTXO, the replacement output, and a ForfeitSigningContext:

  • payment_hash is an opaque 32-byte correlation id. Swap users set it to the swap hash, but darepod only copies it into later pending requests so the external coordinator can find its own state.
  • signing_route tells darepod how to answer the future connector-bound participant-signature request.

There are two routes:

  • LOCAL_SIGNER: when the round later supplies the exact forfeit transaction, darepod asks its configured local signer to sign the vTXO input immediately.
  • PENDING_REQUEST: darepod stores the exact transcript, returns it from ListPendingForfeitParticipantSignatureRequests, blocks the temporary VTXO actor, and resumes only after SubmitForfeitParticipantSignatures supplies the participant signature for that request_id.

That split is necessary because RefreshCustomVTXOs runs before a round has assigned a connector. At that time the final forfeit transaction does not exist, so the participant signature cannot be collected yet. The later pending request includes the connector outpoint, connector amount, connector pkScript, selected forfeit spend path, unsigned forfeit transaction, and server forfeit output script. The external participant signs that exact VTXO input transcript.

Function-level flow

RefreshCustomVTXOs validates the caller-supplied custom input and queues a temporary refresh actor. That actor stores the old vTXO, the replacement output, and the requested signing route, but it cannot ask anyone to sign yet because the connector has not been assigned.

When the round actor later builds the exact connector-bound forfeit transaction, it calls into the temporary actor with the full transcript. The actor then chooses the route:

  • for LOCAL_SIGNER, it calls the local forfeit signer and returns the participant signature directly to the round path;
  • for PENDING_REQUEST, it registers the transcript in the in-process broker and waits on SubmitForfeitParticipantSignatures.

ListPendingForfeitParticipantSignatureRequests is the polling API for external coordinators. It returns only unanswered requests and uses next_sequence as a cursor. Answered later requests no longer advance that cursor past an earlier pending request, so an external signer cannot accidentally skip a still-unanswered transcript.

SubmitForfeitParticipantSignatures matches the request_id, wakes the blocked actor, and removes the answered request from the broker. Repeated submits for an already-answered request are treated as idempotent success only when the submitted signatures match the stored answer. A different answer for the same request id is rejected as AlreadyExists.

Why this is safe

The participant signs an exact forfeit transaction transcript. That keeps the signature scoped to the connector-bound transaction that the operator will use for the refreshed round.

The pending-request path is durable at the coordinator layer: the request contains the opaque payment hash and a stable request id, so swapdk-server can recover its own state and resubmit the participant signature if it restarts in the middle.

Daemon-local signing contexts are removed after a successful external submit or synchronous local signer response, so successful refresh signing does not leave stale contexts behind.

Ordinary wallet-managed VTXOs keep using the existing behavior. The new signing path is only used for custom refresh inputs that carry a forfeit signing context.

Stack

This is the darepo-client side of the active vHTLC refresh stack. The matching darepo PR carries the round-wire fields, and the swapdk-server PR uses these APIs when a funded vHTLC is approaching batch expiry.

Validation

  • make fmt-changed-check base=origin/main
  • make commitmsg-lint range=origin/main..HEAD
  • go test ./darepod ./db -count=1
  • Earlier on this branch: go test ./darepod ./wallet ./daemonrpc ./sdk/swaps -count=1

The matching darepo and swapdk-server PRs add full integration coverage for the multi-participant refresh path. Those tests prove that the extra participant signature is requested, submitted, carried into the round input artifacts, accepted by the operator, and confirmed in the replacement round.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces support for custom-policy VTXO refreshes (such as vHTLCs) and multi-participant forfeit signing in the Ark client and daemon. It adds new RPC endpoints (SignVTXOForfeit, RefreshCustomVTXOs, ListPendingForfeitParticipantSignatureRequests, and SubmitForfeitParticipantSignatures) along with a forfeitSignatureBroker to coordinate external participant signatures. Feedback on these changes highlights a memory leak in the forfeit signature broker's sign function due to uncleaned waiter channels on cancellation and unremoved contexts on success, as well as a potential nil-pointer panic in the swap client's forfeit-refresh handler.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +95 to +164
func (b *forfeitSignatureBroker) sign(ctx context.Context,
req *vtxo.ForfeitParticipantSignRequest) (
[]*types.ForfeitParticipantSig, error) {

if b == nil || req == nil || req.VTXO == nil {
return nil, nil
}

outpoint := req.VTXO.Outpoint.String()

b.mu.Lock()
correlation, ok := b.contexts[outpoint]
b.mu.Unlock()
if !ok {
return nil, nil
}

if correlation.direction == daemonrpc.
ForfeitSigningDirection_FORFEIT_SIGNING_DIRECTION_IN_SWAP {

b.mu.Lock()
signer := b.inSwapSigner
b.mu.Unlock()
if signer != nil {
return signer(ctx, req)
}
}

pending, err := pendingForfeitSignatureRequest(correlation, req)
if err != nil {
return nil, err
}

requestID := string(pending.GetRequestId())
waiter := make(chan []*types.ForfeitParticipantSig, 1)

b.mu.Lock()
stored, ok := b.requests[requestID]
if ok {
if !samePendingForfeitSignatureRequest(stored.proto, pending) {
b.mu.Unlock()
return nil, fmt.Errorf(
"forfeit signature request id conflict",
)
}
} else {
b.nextSequence++
pending.Sequence = b.nextSequence
stored = &forfeitSignatureRequest{proto: pending}
b.requests[requestID] = stored
b.order = append(b.order, requestID)
}

if len(stored.signatures) != 0 {
sigs := cloneParticipantSigs(stored.signatures)
b.mu.Unlock()
return sigs, nil
}

stored.waiters = append(stored.waiters, waiter)
b.mu.Unlock()

select {
case sigs := <-waiter:
return sigs, nil

case <-ctx.Done():
return nil, ctx.Err()
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Memory Leak and Resource Cleanup Issues in sign

There are two distinct resource/memory leaks in the sign function:

  1. Waiter Channel Leak on Cancellation: If ctx.Done() is selected, the waiter channel is left inside stored.waiters. Since stored is persisted in the b.requests map, these cancelled waiter channels will accumulate over time, causing a memory leak. Additionally, when submit is called later, it will iterate over and try to notify/close these dead channels.
  2. Context Map Leak on Success: Once a forfeit signature is successfully obtained (either immediately or via the waiter), the outpoint's context is no longer needed. However, the returned unregister function from registerContext is discarded on success in RefreshCustomVTXOs, meaning the context is never deleted from b.contexts and will leak indefinitely.

We can resolve both issues by:

  • Cleaning up the waiter from stored.waiters under lock when ctx.Done() is triggered.
  • Deleting the outpoint from b.contexts under lock once the signatures are successfully returned.
func (b *forfeitSignatureBroker) sign(ctx context.Context,
	req *vtxo.ForfeitParticipantSignRequest) (
	[]*types.ForfeitParticipantSig, error) {

	if b == nil || req == nil || req.VTXO == nil {
		return nil, nil
	}

	outpoint := req.VTXO.Outpoint.String()

	b.mu.Lock()
	correlation, ok := b.contexts[outpoint]
	b.mu.Unlock()
	if !ok {
		return nil, nil
	}

	if correlation.direction == daemonrpc.
		ForfeitSigningDirection_FORFEIT_SIGNING_DIRECTION_IN_SWAP {

		b.mu.Lock()
		signer := b.inSwapSigner
		b.mu.Unlock()
		if signer != nil {
			sigs, err := signer(ctx, req)
			if err == nil {
				b.mu.Lock()
				delete(b.contexts, outpoint)
				b.mu.Unlock()
			}
			return sigs, err
		}
	}

	pending, err := pendingForfeitSignatureRequest(correlation, req)
	if err != nil {
		return nil, err
	}

	requestID := string(pending.GetRequestId())
	waiter := make(chan []*types.ForfeitParticipantSig, 1)

	b.mu.Lock()
	stored, ok := b.requests[requestID]
	if ok {
		if !samePendingForfeitSignatureRequest(stored.proto, pending) {
			b.mu.Unlock()
			return nil, fmt.Errorf(
				"forfeit signature request id conflict",
			)
		}
	} else {
		b.nextSequence++
		pending.Sequence = b.nextSequence
		stored = &forfeitSignatureRequest{proto: pending}
		b.requests[requestID] = stored
		b.order = append(b.order, requestID)
	}

	if len(stored.signatures) != 0 {
		sigs := cloneParticipantSigs(stored.signatures)
		delete(b.contexts, outpoint)
		b.mu.Unlock()
		return sigs, nil
	}

	stored.waiters = append(stored.waiters, waiter)
	b.mu.Unlock()

	select {
	case sigs := <-waiter:
		b.mu.Lock()
		delete(b.contexts, outpoint)
		b.mu.Unlock()
		return sigs, nil

	case <-ctx.Done():
		b.mu.Lock()
		for i, w := range stored.waiters {
			if w == waiter {
				stored.waiters = append(stored.waiters[:i], stored.waiters[i+1:]...)
				break
			}
		}
		b.mu.Unlock()
		return nil, ctx.Err()
	}
}

Comment on lines +130 to +133
resp, err := s.client.daemon.SignVTXOForfeit(ctx, req)
if err != nil {
return fmt.Errorf("sign out-swap forfeit payload: %w", err)
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Defensive Programming: Nil Check for Daemon Response

If SignVTXOForfeit returns a nil response with a nil error (which can happen in mock or custom implementations), accessing resp.GetPubkey() will panic. Adding a nil check for resp prevents potential runtime panics.

	resp, err := s.client.daemon.SignVTXOForfeit(ctx, req)
	if err != nil {
		return fmt.Errorf("sign out-swap forfeit payload: %w", err)
	}
	if resp == nil {
		return fmt.Errorf("sign out-swap forfeit payload: empty response")
	}

@bhandras
bhandras force-pushed the codex/vhtlc-refresh-client branch 2 times, most recently from 7c18d19 to e20c0df Compare June 16, 2026 14:01
@bhandras

Copy link
Copy Markdown
Member Author

@claude pls review this refreshed custom vHTLC refresh support. Context: the latest force-push folds the lint fixes and the PR body now calls out the multi-participant refresh integration coverage in the companion darepo and swapdk-server PRs. Please focus on durable signing/ack ordering, participant forfeit signature plumbing, and whether the daemon RPC surface is sufficient for swapdk-server active refresh.

@claude

claude Bot commented Jun 16, 2026

Copy link
Copy Markdown

Claude finished @bhandras's task in 5m 20s —— View job


Review: custom vHTLC refresh support

I focused on the three areas you called out: durable signing/ack ordering, participant forfeit-signature plumbing, and whether the daemon RPC surface is sufficient for swapdk-server active refresh. Overall the design is sound — the request transcript is bound to the exact connector + unsigned forfeit tx, the out-swap ack is correctly gated on server acceptance, and the daemon-side validation in SignVTXOForfeit is genuinely strict (policy↔pkScript, spend-path binding, identity-key membership, forfeit-tx structure). A few correctness and durability issues are worth addressing before this is leaned on for active refresh.

🔴 1. forfeitSignatureBroker.sign blocks the VTXO actor turn on an unbounded context

handleForfeitRequest (vtxo/transitions.go:281) calls externalForfeitParticipantSigsbroker.sign inline inside the FSM transition, which runs on the VTXO actor's single turn. For the out-swap direction sign blocks on <-waiter until swapdk-server polls list, forwards over the mailbox, the receiver signs, and the signature is submitted back (darepod/forfeit_signature_broker.go:157).

Two problems compound here:

  • The forfeit event is Tell'd from the round with context.WithoutCancel(ctx) (round/actor.go:2395), so the ctx reaching sign is not bounded by the round's defaultForfeitCollectionTimeout (2 min). When the round's forfeit-collection window expires and the round fails the refresh, the VTXO actor is still parked inside sign waiting on the waiter — wedged until daemon shutdown (or a late submit that nobody is waiting for anymore). That's a per-VTXO actor liveness leak.
  • Even within the happy path, the actor is head-of-line blocked for the whole network round-trip (block epochs, spend/forfeit-release events for that VTXO queue behind it). Blocking an actor turn on a multi-party network call is the pattern the context-lifecycle guidance warns against.

Recommendation: derive a bounded context for the wait (aligned with the forfeit-collection window) and ensure the waiter is removed on ctx.Done() (see #2). Longer term, consider returning a "pending forfeit" transition and resuming via an inbound event rather than blocking the turn.

🔴 2. Memory leaks in the broker (confirms the Gemini finding, partially)

In darepod/forfeit_signature_broker.go:

  • Waiter leak on cancellation (sign, line 161): the <-ctx.Done() branch returns without removing waiter from stored.waiters. Since stored lives forever in b.requests, cancelled waiters accumulate. Gemini's fix (splice the waiter out under lock) is correct and pairs directly with lib: start adding lib helpers #1.
  • contexts map never cleared: RefreshCustomVTXOs intentionally discards unregister on success (the round needs the context later), but sign never deletes the entry afterwards — for either direction (the in-swap early return at line 117-119 also leaks). Every refresh leaks one contexts entry permanently. Deleting on success is right, but guard against the round retrying sign for the same outpoint (e.g. a reseal pass): if you delete eagerly and the FSM re-enters handleForfeitRequest, the second sign finds no context and returns nil, nil, silently dropping the participant signature. Prefer clearing on a terminal signal (forfeit confirmed / round failed) rather than on first successful sign.
  • requests / order never pruned: even after submit, entries stay forever and list walks the entire history on every poll → O(all-time-requests) per poll for a long-lived daemon. Worth a bounded retention / pruning-on-terminal policy.

Fix waiter + context leaks →

🟠 3. Broker state is in-memory only — operator-side restart loses the refresh

The broker holds contexts, requests, and order purely in memory; there is no store. The PR body's durability argument covers the receiver's mailbox cursor, but the operator's signing-context registration (created by the RefreshCustomVTXOs RPC, which is not persisted) is volatile. If the operator daemon restarts mid-refresh:

  • the round FSM resumes and re-issues the forfeit request, but sign finds no contexts[outpoint] and returns nil, nil → the forfeit is built without the required participant signature → invalid multi-party forfeit.
  • swapdk-server polling list sees nothing and has no signal that the context was lost.

This is largely a swapdk-server-resilience concern (it must re-drive RefreshCustomVTXOs after detecting an operator restart), but the RPC surface gives no explicit way to detect it. Worth documenting as an assumption, or persisting the signing context alongside the queued refresh.

🟠 4. list cursor semantics can strand an unsigned request

list (forfeit_signature_broker.go:166) advances next past every request it returns, and ListPendingForfeitParticipantSignatureRequests echoes it as NextSequence. A consumer that treats NextSequence as a monotonic high-water cursor (the natural reading of after_sequence) will see each pending request exactly once and never re-list one it failed to fulfill. The only safe drain pattern is to poll from after_sequence = 0 every time and rely on signed-request filtering — which makes the watermark misleading. Please document the intended polling contract (and note it's O(history) per poll given #2). This directly affects "is the RPC surface sufficient for active refresh."

🟡 5. sameParticipantSigSet is order-sensitive

submit idempotency (forfeit_signature_broker.go:439) compares sigs positionally. If swapdk-server resubmits the same set in a different order (or Schnorr signing is non-deterministic across a re-sign on ack-retry), submit returns AlreadyExists, which in the out-swap path propagates back as a SubmitOutSwapForfeitSignature error → the receiver never acks → retry loop. Consider matching by pubkey-keyed set, and confirm whether the daemon's SignOutputRaw forfeit signature is deterministic across retries.

🟡 6. Gemini's nil-resp finding (forfeit_refresh.go:132) — not an actual panic

resp.GetPubkey() on a nil *SignVTXOForfeitResponse does not panic: generated proto getters are nil-receiver-safe and return nil. The empty pubkey then fails forfeitParticipantSignatureToProto with a clean error. The explicit nil check is harmless defensive polish but not a bug fix — I'd not block on it.

✅ What looks good

  • Out-swap ack ordering in handleOutSwapForfeitSignatureRequest (sdk/swaps/forfeit_refresh.go:100): sign → submit-to-server → then ack. Failure anywhere short-circuits before the ack, so the mailbox event is re-pulled. Correct.
  • SignVTXOForfeit validation is thorough and the right trust boundary for the receiver — it signs only when its identity key is genuinely required by the spend path and the policy binds to the pkScript.
  • The deterministic RequestId over the full transcript (excluding the broker-assigned Sequence) correctly binds the signature to the connector + unsigned tx.

Notes: I could not compute git diff origin/main...HEAD (shallow checkout has no merge-base) or run the build/tests in this environment, so findings are from static reading of the changed files. The in-memory-broker and actor-blocking points (#1#3) are the ones I'd most want a second look at before active refresh depends on this path.
· branch codex/vhtlc-refresh-client

@bhandras
bhandras force-pushed the codex/vhtlc-refresh-client branch from e20c0df to 6d00788 Compare June 16, 2026 14:24
@bhandras

Copy link
Copy Markdown
Member Author

@claude pls re-review with context: since the last request I folded the local review finding that successful forfeit signing left daemon-local signing contexts behind. The broker now clears the context after external signatures are accepted and after synchronous in-swap signing succeeds, with tests for both paths. I also updated the PR body/validation to reflect the folded tests. Please re-check durable signing/ack ordering and context lifecycle.

@claude

claude Bot commented Jun 16, 2026

Copy link
Copy Markdown

Claude finished @bhandras's task in 2m 33s —— View job


Re-review: custom vHTLC refresh — context lifecycle & durable ordering

I re-read the broker against the folded fix and re-checked the three areas you called out. The context-clearing fold is correct on the two paths it targets, and both tests genuinely assert it. But the fold only closes the success paths — the cancellation/error paths (the ones that matter most for restart + timeout durability) still leak, and the prior 🔴#1/🔴#2 findings are untouched.


✅ The folded context-clearing fix is correct (verified)

  • In-swap synchronous signing clears the context: sign deletes b.contexts[outpoint] only after signer(ctx, req) returns no error (forfeit_signature_broker.go:118-127). TestForfeitSignatureBrokerDelegatesInSwapRequests asserts contexts is empty afterward (:154-157). Good — and correctly not deleted on signer error, so a retry can still find the context.
  • External submit clears the context inside submit under the same lock that stores the signatures (:269-270), and TestForfeitSignatureBrokerSurfacesAndCompletesRequest asserts the entry is gone after submit (:114-117). Clearing in submit (rather than eagerly in sign) is the right call — it avoids the "delete-then-reseal-finds-nothing" race the previous review warned about, because a later sign for the same outpoint now correctly short-circuits at :107-109.
  • Out-swap ack ordering is unchanged and still correct: sign daemon → SubmitOutSwapForfeitSignature to server → then Ack (sdk/swaps/forfeit_refresh.go:129-154). Any failure short-circuits before the ack, so the mailbox event is re-pulled.

🔴 1. Context still leaks on every non-success path (fold is success-only)

sign only deletes the context on the two happy paths. On all of these the b.contexts[outpoint] entry survives indefinitely:

  • ctx cancellation — :170-171 returns ctx.Err() without deleting;
  • in-swap signer error — :119-121;
  • transcript build error — :132-134;
  • request-id conflict — :142-146.

For a refresh that is cancelled or times out (precisely the restart/timeout durability case you asked about), the daemon-local context is never reclaimed. So the fold improved the steady state but the unhappy-path lifecycle — the one that actually exercises durability — is still open.

🔴 2. Waiter leak on ctx.Done — still present (prior #2 / Gemini, not yet folded)

sign appends waiter to stored.waiters at :163, but the ctx.Done branch at :170-171 returns without splicing it out:

case <-ctx.Done():
        return nil, ctx.Err()   // waiter left in stored.waiters

Since stored lives forever in b.requests, a request that is cancelled and then never submitted accumulates dead waiters. (submit does drain+close them, so the leak is specifically for requests abandoned before any submit.) This pairs directly with #1 — both want a cleanup block on ctx.Done that removes the waiter and deletes the context.

🔴 3. sign still blocks the VTXO actor turn on an unbounded context (prior #1, untouched)

VTXOActor.Receive runs the FSM transition inline on the actor turn (vtxo/actor.go:324), so handleForfeitRequest → externalForfeitParticipantSigs → broker.sign blocks on <-waiter inside the turn. The ctx that reaches sign is the round's context.WithoutCancel(ctx) (round/actor.go:2395), so it is not bounded by defaultForfeitCollectionTimeout (2 min, round/actor.go:39).

Consequences when active refresh leans on this:

  • When the round's forfeit-collection window expires and the round fails the refresh, the VTXO actor is still parked in sign on the unbounded ctx — wedged until a late submit nobody awaits, or daemon shutdown. Combined with lib: start adding lib helpers #1/multI: add initial repo scaffolding #2 that abandoned request also leaks its context + waiter.
  • Even on the happy path the actor is head-of-line blocked for the full multi-party round-trip; per-VTXO block-epoch / spend / forfeit-release events queue behind it. This is the actor-turn-blocking pattern the context-lifecycle guidance warns against.

Suggested shape (closes #1, #2, #3 together): derive a bounded ctx for the wait aligned with the forfeit-collection window, and on ctx.Done splice the waiter out of stored.waiters and delete b.contexts[outpoint], both under b.mu. Longer term, return a "pending forfeit" transition and resume via an inbound event instead of blocking the turn.

Fix #1/#2 →


Re your specific question — "is the daemon RPC surface sufficient for active refresh?"

The signing transcript binding, the synchronous in-swap path, and the out-swap submit→ack ordering are sound. The gap is operator-side durability/observability, unchanged by this fold:

  • the broker is in-memory only (contexts/requests/order), so an operator restart mid-refresh loses the signing context; the resumed round re-issues the forfeit request, sign finds no context and returns nil, nil (:107-109) → forfeit built without the participant signature. swapdk-server must re-drive RefreshCustomVTXOs on restart, and the RPC gives it no signal that the context was lost.
  • list advancing NextSequence past every returned request (:193-209) still makes the watermark unsafe as a monotonic cursor — the only safe drain is polling from after_sequence = 0 each time. Worth documenting the intended polling contract.

These are the same 🟠#3/🟠#4 points from last round and remain assumptions swapdk-server has to honor; flagging that the fold did not change them.


Summary: the context-clearing fold is correct and well-tested for the success paths, and ack ordering is solid. The remaining work for "leaned on for active refresh" is the cancellation/timeout lifecycle: context+waiter cleanup on ctx.Done (#1/#2) and unblocking the actor turn / bounding the wait (#3). I'd want those before active refresh depends on this path. (Static read only — I could not build/test in this environment.)
· branch codex/vhtlc-refresh-client

@bhandras
bhandras force-pushed the codex/vhtlc-refresh-client branch from 6d00788 to a35102a Compare June 16, 2026 14:28
@bhandras

Copy link
Copy Markdown
Member Author

@claude small extra context: I force-pushed once more after the prior re-review request to fold the static-check fix. The new head adds a tiny generated-schema comment refresh and gofmt/import formatting; local make fmt-changed-check base=origin/main, make sqlc-check, commitmsg-lint, and smoke compile are now green. Please review the current head.

@claude

claude Bot commented Jun 16, 2026

Copy link
Copy Markdown

Claude finished @bhandras's task in 1m 59s —— View job


Re-review (current head a35102a): delta + standing findings

I diffed the current head against the prior re-reviewed state and re-read the broker and the actor call path. The force-push delta is as you described plus two small functional changes that are worth calling out as improvements — but the three durability/lifecycle findings from the last round are still present unchanged.

Delta since the last re-review (verified)

  • db/sqlc/schemas/generated_schema.sql — generated comment refresh only (HEAD commit a35102a).
  • sdk/swaps/out_swap.go — the out-swap responder goroutine now captures receiver/paymentHash/clientPubKey before go ... instead of reading s.* fields inside the goroutine. Good — removes a concurrent-field-read on the session and aligns with the context-lifecycle guidance. ✅
  • sdk/swaps/forfeit_refresh.gorespondToOutSwapForfeitSignatureRequests now takes those captured values as params; pure refactor of the above. ✅
  • darepod/rpc_server.goRefreshCustomVTXOs now calls TriggerRoundRegistration and, on trigger failure, runs every unregister fn to drop the just-registered signing contexts before returning Internal. This is a real improvement: it closes the context leak on the RPC-level failure path. ✅

Still open — the cancellation/timeout lifecycle (unchanged from last round)

The fold improved RPC-failure and success paths, but the in-broker unhappy paths the last review flagged are byte-for-byte the same in darepod/forfeit_signature_broker.go:

🔴 1. Waiter leak on ctx.Done (:170-171). The <-ctx.Done() branch still returns ctx.Err() without splicing waiter out of stored.waiters (appended at :163). stored lives forever in b.requests, so a request cancelled before any submit accumulates dead waiters.

🔴 2. Context leak on every non-success path. b.contexts[outpoint] is deleted only on in-swap signer success (:124) and inside submit (:270). It is not deleted on: ctx cancellation (:170), in-swap signer error (:119-121), transcript-build error (:131-134), or request-id conflict (:142-147). A cancelled/timed-out refresh — the exact restart/timeout case — never reclaims its context. (Note the new RPC-failure unregister does not cover these: it only fires when TriggerRoundRegistration itself fails, after which sign is never reached.)

🔴 3. sign blocks the VTXO actor turn on an unbounded ctx. handleForfeitRequest calls externalForfeitParticipantSigsbroker.sign inline on the FSM transition (vtxo/transitions.go:281), and sign blocks on <-waiter (:166-167). The ctx reaching it is the round's context.WithoutCancel(ctx) (round/actor.go:2395), so it is not bounded by defaultForfeitCollectionTimeout (2 min, round/actor.go:39). When the round fails the refresh after its collection window, the VTXO actor stays parked in sign until a late submit nobody awaits or daemon shutdown; on the happy path the actor is head-of-line blocked for the whole multi-party round-trip.

Suggested shape (closes #1#3 together): derive a bounded ctx for the wait aligned with the forfeit-collection window, and in the ctx.Done branch, under b.mu, both splice waiter out of stored.waiters and delete(b.contexts, outpoint); also delete the context on the in-swap-error and transcript-build-error returns. Longer term, return a "pending forfeit" transition and resume on an inbound event rather than blocking the turn.

Fix #1/#2 →

RPC-surface durability (unchanged assumptions for swapdk-server)

  • Broker state (contexts/requests/order) is still in-memory only. An operator restart mid-refresh loses the signing context; the resumed round re-issues the forfeit request, sign finds no context and returns nil, nil (:107-109) → forfeit built without the participant signature. swapdk-server must re-drive RefreshCustomVTXOs on restart, and the RPC gives no signal that the context was lost. Worth documenting (or persisting the context alongside the queued refresh).
  • list still advances next past every returned request (:193-209), so NextSequence is unsafe as a monotonic high-water cursor — the only correct drain is polling from after_sequence = 0 each time and relying on signed-request filtering (which is O(history) per poll given no pruning). Please document the intended polling contract.

Verdict

The success/RPC-failure context handling and the out-swap ack ordering (sign → submit-to-server → ack, sdk/swaps/forfeit_refresh.go) are solid, and the responder field-capture is a nice tidy-up. The remaining blockers for "leaned on for active refresh" are the same three: waiter+context cleanup on ctx.Done/error (#1/#2) and unblocking/bounding the actor wait (#3). I'd want those before active refresh depends on this path.

Static read only — I confirmed the delta with git diff but did not build/run tests in this environment.
· branch codex/vhtlc-refresh-client

@bhandras
bhandras force-pushed the codex/vhtlc-refresh-client branch from a09b9fd to 5d5a26d Compare June 16, 2026 15:06
@bhandras

Copy link
Copy Markdown
Member Author

@claude pls re-review with context: I folded the custom refresh signing API away from swap-direction terminology and into route terminology. The daemon proto now documents the full flow: RefreshCustomVTXOs queues the custom vTXO before a connector exists, LOCAL_SIGNER signs inside darepod when the connector-bound ForfeitRequest arrives, and PENDING_REQUEST publishes the exact transcript through ListPendingForfeitParticipantSignatureRequests until SubmitForfeitParticipantSignatures answers it. Client head is 5d5a26d. Local validation: make fmt-changed-check base=origin/main; make commitmsg-lint range=origin/main..HEAD; go test ./darepod ./wallet ./daemonrpc ./sdk/swaps -count=1.

@claude

claude Bot commented Jun 16, 2026

Copy link
Copy Markdown

Claude finished @bhandras's task in 2m 46s —— View job


Re-review (head 5d5a26d2): route-terminology fold

I diffed the fold commit (daemonrpc: route custom refresh forfeit signing) against the prior re-reviewed head and re-read the broker, the actor call path, and the new wallet cleanup message. The rename to route terminology is a real improvement to the RPC contract, and one of the standing 🔴 findings (the waiter leak) is now fixed. Two lifecycle items remain, and they are interlocked.

Delta since last re-review — all verified, all improvements

  • Route terminology + proto docs. ForfeitSigningRoute{LOCAL_SIGNER, PENDING_REQUEST} replaces the swap-direction enum, and the proto comments now document the full lifecycle (queue → connector assigned → sign callback → route resolution) accurately. The "opaque correlation id" framing of payment_hash matches the code (pendingForfeitSignatureRequest just copies it through). ✅
  • LOCAL_SIGNER fails closed. sign now returns an error when no local signer is configured (forfeit_signature_broker.go:117-120) instead of silently degrading to an external pending request. Good — that was a latent foot-gun. ✅
  • 🔴 Prior lib: start adding lib helpers #1 (waiter leak on ctx.Done) is fixed. The ctx.Done branch now calls removeWaiter, which splices the waiter out of stored.waiters under b.mu (:178-181, :185-207). Correct. ✅
  • request_id binds the connector amount. forfeitSignatureRequestID now folds ConnectorAmountSat into the digest (:381-385). Correct — the connector value is part of the exact prevout being signed, so it belongs in the transcript identity. ✅
  • Trigger-failure cleanup is now complete. RefreshCustomVTXOs on trigger failure runs every unregister (drops broker contexts) and dropCustomRefreshVTXOsDropCustomRefreshVTXOsRequest (drops the temporary wallet/manager PendingForfeit signer actors), under a context.WithoutCancel+10s bound (rpc_server.go, wallet/wallet.go:235-242). This closes the RPC-level actor/context leak end-to-end. ✅

🟠 1. The actor-turn block is still unbounded — and the removeWaiter fix barely runs because of it

This is the prior 🔴#3 and it's the one I'd still want addressed before active refresh leans on the path. The chain is unchanged:

  • round/actor.go:2395 Tells ForfeitRequestEvent with reqCtx := context.WithoutCancel(ctx).
  • The VTXO actor processes that Tell inline on its turn; handleForfeitRequest → externalForfeitParticipantSigs → env.ForfeitParticipantSigner = broker.sign, which blocks on <-waiter (forfeit_signature_broker.go:174-176).

Because the ctx that reaches sign is WithoutCancel, the <-waiter select has no deadline and no cancellation source: it is not bounded by the round's defaultForfeitCollectionTimeout (2 min, round/actor.go:39), and it's not bounded by ForfeitVTXOActorAskTimeout either (that bounds manager→child Asks, but the forfeit request arrives as a direct Tell to the actor's service key, not through that Ask path). So:

  • If the external participant never submits (or the round fails its collection window and moves on), the VTXO actor stays parked in sign until daemon shutdown.
  • A useful corollary: the new removeWaiter cleanup only fires on ctx.Done, but with WithoutCancel the only thing that ever cancels is the actor's own lifecycle ctx at shutdown. So the leak fix is correct but rarely exercised in practice — the underlying liveness problem (unbounded park + head-of-line blocking of block-epoch / spend / forfeit-release events for that VTXO) is what dominates.

Suggested shape: derive a bounded ctx for the wait aligned with the forfeit-collection window (so a stalled/abandoned refresh frees the actor and triggers the now-correct removeWaiter), and longer term return a "pending forfeit" transition that resumes on an inbound submit event instead of blocking the turn.

Bound the forfeit wait →

🟡 2. PENDING_REQUEST context leaks on the remaining non-success returns

b.contexts[outpoint] is deleted on LOCAL_SIGNER success (:127-129) and inside submit (:304), but not on: ctx.Done (:178), transcript-build error (:139-142), or request-id conflict (:149-155). Low severity in practice — given #1 the ctx.Done path effectively only runs at daemon shutdown — but the transcript-build / id-conflict returns can leak a context for a VTXO that then never reaches submit. Cheap to fold the delete into those returns (the in-swap signer-error case at :122-125 is correctly left intact for retry).

RPC-surface durability — your specific question, unchanged assumptions

The route fold makes the contract cleaner and the transcript binding is sound (now including the connector amount). Two operator-side gaps remain for swapdk-server to honor:

  • In-memory broker, no persistence. contexts/requests/order are memory-only. On operator restart mid-refresh the resumed round re-issues the forfeit request, sign finds no context and returns nil, nil (:108-110) → forfeit built without the participant signature. swapdk-server must re-drive RefreshCustomVTXOs after detecting an operator restart, and the RPC gives it no explicit signal that the context was lost. Worth documenting as an assumption (or persisting the signing context alongside the queued refresh).
  • list cursor is not a safe high-water mark. list advances next past every returned request, signed or not (:226-245), and echoes it as NextSequence. A consumer that fails to fulfill a returned request and then polls with after_sequence = NextSequence will never see that request again — the only correct drain is to poll from after_sequence = 0 every time and rely on signed-request filtering (which is O(history) per poll, since entries are never pruned). The new proto comment calls sequence "a monotonically increasing in-memory cursor" but doesn't warn against using it as a watermark for unfulfilled requests; please document the intended polling contract.

Also still open (minor, prior 🟡)

  • sameParticipantSigSet (:488-510) compares positionally, so a resubmit of the same signatures in a different order returns AlreadyExists rather than the idempotent nil. Consider matching by pubkey-keyed set.

Verdict

The fold is a clean contract improvement and genuinely fixes the waiter leak (prior 🔴#1) and the connector-amount transcript binding. The remaining blocker for "leaned on for active refresh" is #1 — the unbounded actor-turn wait — with #2 as a cheap companion cleanup; the in-memory-broker and list-cursor points are swapdk-server-facing assumptions worth documenting. The out-swap sign→submit→ack ordering and the strict SignVTXOForfeit validation remain solid.

Static read only — I confirmed the delta with git show 5d5a26d2 but did not build/run tests in this environment; your local go test ./darepod ./wallet ./daemonrpc ./sdk/swaps and fmt/commitmsg validation are the source of truth there.
· branch codex/vhtlc-refresh-client

@bhandras
bhandras force-pushed the codex/vhtlc-refresh-client branch from 5d5a26d to 7cb5bb6 Compare June 16, 2026 15:33
@bhandras

Copy link
Copy Markdown
Member Author

@claude pls re-review. Context since the last review: I expanded the daemon proto/RPC comments and PR body to describe the route-based signing flow in detail; fixed the broker cursor bug so answered later requests do not advance next_sequence past an earlier pending request; added TestForfeitSignatureBrokerListDoesNotAdvancePastEarlierPendingRequest; and documented that custom policy refresh outputs may fail standard policy decode while still persisting policy_template and pk_script as authoritative script data. Current head is 7cb5bb6.

@claude

claude Bot commented Jun 16, 2026

Copy link
Copy Markdown

Claude finished @bhandras's task in 2m 25s —— View job


Re-review (head 7cb5bb65): cursor fix + doc fold

I diffed the current head, re-read the broker, the list cursor path, the new test, the proto docs, and the round_store decode comment, and re-checked the standing findings. The cursor fix is correct and well-tested, and the documentation folds are accurate. The two lifecycle items from the last round are unchanged.

Delta since last re-review — all verified

  • 🟠→✅ list cursor bug fixed. list now advances next only to the sequence of a returned (unsigned) request; signed requests are skipped with continue and no longer move the cursor (forfeit_signature_broker.go:227-244). The concrete guarantee this buys: every pending request with sequence ∈ (after_sequence, next_sequence] is included in the response, so a later answered request can no longer strand an earlier unanswered one. TestForfeitSignatureBrokerListDoesNotAdvancePastEarlierPendingRequest asserts exactly this — seq 1 pending + seq 2 signed → list(0,10) returns only seq 1 with next == 1 (:288-291). Good, and the prior 🟠multi: stage boarding actor client  #4 is now substantively addressed. ✅
  • Proto/RPC route docs expanded. The lifecycle (queue → connector assigned → sign callback → route resolution) reads accurately against the code, and after_sequence/sequence are now documented (daemon.proto:1136-1138, :1180-1182). ✅
  • Custom-policy decode comment. vtxoRequestToRoundParams now documents that DecodeStandardPolicyTemplate failing is expected for custom refresh replacements, with policy_template/pk_script remaining authoritative and the legacy client/operator/expiry columns populated only for the standard shape (db/round_store.go:1217-1228). Matches the behavior — the decode error is swallowed (err == nil gate) rather than propagated. ✅

One small doc gap left on the cursor contract

The fix makes next_sequence safe as a high-water cursor only if the caller fulfills every returned request before advancing. If a consumer gets seq 1, fails to submit it, then polls with after_sequence = next_sequence (= 1), seq 1 is now <= after and is skipped — the only way to re-see it is to poll from after_sequence = 0. That's a reasonable contract, but the next_sequence field (daemon.proto:1190) has no comment stating it. Worth one line: "advance to next_sequence only after fulfilling every returned request; otherwise re-poll from 0." Also still O(history) per poll since requests/order are never pruned.

Still open — unchanged from last round (not in scope of this fold, flagging for completeness)

🟠 1. sign blocks the VTXO actor turn on an unbounded ctx. round/actor.go:2395 still Tells ForfeitRequestEvent with reqCtx := context.WithoutCancel(ctx); the VTXO actor runs the FSM transition inline (vtxo/transitions.go:281externalForfeitParticipantSigsbroker.sign), which blocks on <-waiter (forfeit_signature_broker.go:174-176). Because the ctx is WithoutCancel, the wait is bounded by neither defaultForfeitCollectionTimeout (2 min) nor ForfeitVTXOActorAskTimeout. If the external participant never submits (or the round fails its collection window and moves on), the actor stays parked until daemon shutdown, and on the happy path it is head-of-line blocked for the full multi-party round-trip. Corollary: the now-correct removeWaiter cleanup only fires on ctx.Done, which under WithoutCancel effectively means daemon shutdown — so the leak fix is right but rarely exercised; the liveness problem dominates.

Suggested shape: derive a bounded ctx for the wait aligned with the forfeit-collection window (so an abandoned refresh frees the actor and triggers removeWaiter), and longer term return a "pending forfeit" transition that resumes on an inbound submit event rather than blocking the turn.

Bound the forfeit wait →

🟡 2. PENDING_REQUEST context leaks on the non-success returns. b.contexts[outpoint] is deleted on LOCAL_SIGNER success (:127-129) and inside submit (:303), but not on ctx.Done (:178-181), transcript-build error (:139-142), or request-id conflict (:149-155). Low severity (the in-swap signer-error path at :122-125 is correctly left intact for retry), but the transcript-build / id-conflict returns can strand a context for a VTXO that never reaches submit. Cheap to fold a delete(b.contexts, outpoint) into those returns.

Standing RPC-surface assumptions for swapdk-server (unchanged)

  • In-memory broker, no persistence. On operator restart mid-refresh the resumed round re-issues the forfeit request, sign finds no context and returns nil, nil (:108-110) → forfeit built without the participant signature. swapdk-server must re-drive RefreshCustomVTXOs after a detected restart, and the RPC gives no explicit "context lost" signal. Worth documenting (or persisting the signing context alongside the queued refresh).
  • sameParticipantSigSet positional compare (:487-509) — a resubmit of the same sigs in a different order returns AlreadyExists rather than idempotent nil. Consider pubkey-keyed matching.

Verdict

This fold is clean: the cursor bug is genuinely fixed and tested, and the proto/decode docs are accurate. The remaining blocker for "leaned on for active refresh" is still #1 (unbounded actor-turn wait), with #2 as a cheap companion; the in-memory-broker and next_sequence-contract points are swapdk-server-facing and worth a doc line. The out-swap sign→submit→ack ordering and the strict SignVTXOForfeit validation remain solid.

Static read only — I confirmed the broker/proto/round_store state on 7cb5bb65 but did not build/run tests in this environment; your local go test ./darepod ./wallet ./daemonrpc ./sdk/swaps and fmt/commitmsg/sqlc-check validation are the source of truth.
· branch codex/vhtlc-refresh-client

@bhandras
bhandras force-pushed the codex/vhtlc-refresh-client branch from 7cb5bb6 to 6b0ebfd Compare June 16, 2026 15:52
@bhandras

Copy link
Copy Markdown
Member Author

@claude pls re-review with context.

Folded the last local/CI cleanup on top of the expanded proto/RPC documentation:

  • wrapped the long ForfeitSigningRoute_PENDING_REQUEST case in darepod/forfeit_signature_broker.go so lint should no longer fail on the 81-character line;
  • kept the broker cursor fix and test from the prior push intact;
  • PR body already reflects the clarified idempotency contract: repeated submit with matching signatures is idempotent, different signatures return AlreadyExists.

Latest pushed client head: 6b0ebfdab8df5477947ae982095de5774121f81d.

Local validation after the fold:

  • make fmt-changed-check base=origin/main
  • make commitmsg-lint range=origin/main..HEAD
  • go test ./darepod ./db -count=1

@claude

claude Bot commented Jun 16, 2026

Copy link
Copy Markdown

Claude finished @bhandras's task in 1m 28s —— View job


Re-review (head 6b0ebfd): lint-wrap fold + standing findings

I re-read the broker, the list cursor path, the submit idempotency contract, and the actor call chain on the current head, and diffed the fold commit. The lint wrap is cosmetic and correct, the cursor fix + its test are intact, and the idempotency contract matches the PR body. No substantive behavior changed since the last re-review, so the two lifecycle items and the swapdk-server-facing assumptions stand unchanged.

Delta since last re-review — verified

  • Lint wrap. The PENDING_REQUEST case is now split across two lines (forfeit_signature_broker.go:133-134), purely to clear the 80-col limit. No logic change. ✅
  • Cursor fix intact. list still advances next only to the sequence of a returned (unsigned) request and skips signed ones with continue (:228-243), so a later answered request can't strand an earlier unanswered one. The guard test from the prior push is still present. ✅
  • Idempotency contract matches the PR body. submit returns nil (idempotent) when the request is already signed and sameParticipantSigSet matches, else codes.AlreadyExists (:292-301). This is exactly "repeated submit with matching signatures is idempotent; a different answer is AlreadyExists." ✅ — with the one caveat below.

Still open — unchanged, flagging for completeness (not in scope of this fold)

🟠 1. sign blocks the VTXO actor turn on an unbounded ctx. round/actor.go:2395 still Tells ForfeitRequestEvent with reqCtx := context.WithoutCancel(ctx). The VTXO actor runs the FSM transition inline (vtxo/transitions.go:281externalForfeitParticipantSigsbroker.sign), which blocks on <-waiter (forfeit_signature_broker.go:175-183). Because the ctx is WithoutCancel, the wait is bounded by neither defaultForfeitCollectionTimeout (2 min) nor ForfeitVTXOActorAskTimeout. If the external participant never submits (or the round fails its collection window and moves on), the actor stays parked until daemon shutdown; on the happy path it is head-of-line blocked for the full multi-party round-trip (block-epoch / spend / forfeit-release events for that VTXO queue behind it). Corollary: the now-correct removeWaiter (:186-208) only fires on ctx.Done, which under WithoutCancel effectively means daemon shutdown — so the leak fix is right but rarely exercised; the liveness problem dominates. This remains the one blocker I'd want before active refresh leans on the path.

Suggested shape: derive a bounded ctx for the wait aligned with the forfeit-collection window (so an abandoned refresh frees the actor and triggers removeWaiter), and longer term return a "pending forfeit" transition that resumes on an inbound submit event rather than blocking the turn.

Bound the forfeit wait →

🟡 2. PENDING_REQUEST context leaks on the non-success returns. b.contexts[outpoint] is deleted on LOCAL_SIGNER success (:127-129) and inside submit (:304), but not on ctx.Done (:179-182), transcript-build error (:140-142), or request-id conflict (:151-156). Low severity (the LOCAL_SIGNER signer-error path at :122-125 is correctly left intact for retry), but the transcript-build / id-conflict returns can strand a context for a VTXO that never reaches submit. Cheap to fold a delete(b.contexts, outpoint) into those two returns.

🟡 3. sameParticipantSigSet is positional (:488-510). A resubmit of the same signatures in a different order compares unequal and returns AlreadyExists rather than the idempotent nil — so the documented idempotency only holds when the external signer resubmits in the same order. Given Schnorr sig bytes can differ across a re-sign, this is the more likely trigger than ordering, but pubkey-keyed set matching would make the contract robust to both.

Standing RPC-surface assumptions for swapdk-server (unchanged)

  • In-memory broker, no persistence. contexts/requests/order are memory-only. On operator restart mid-refresh the resumed round re-issues the forfeit request, sign finds no context and returns nil, nil (:108-110) → forfeit built without the participant signature. swapdk-server must re-drive RefreshCustomVTXOs after a detected restart, and the RPC gives no explicit "context lost" signal. Worth documenting as an assumption (or persisting the signing context alongside the queued refresh).
  • next_sequence is a "fulfill-before-advance" cursor. Safe as a high-water mark only if the caller fulfills every returned request before advancing; an unfulfilled returned request is only re-seen by polling from after_sequence = 0. The cursor fix makes this correct, but the contract is still worth one line on the next_sequence proto field. Also O(history) per poll since requests/order are never pruned.

Verdict

This fold is cosmetic (lint wrap) over an already-reviewed state; the cursor fix and idempotency contract verify out as described. The out-swap sign→submit→ack ordering and the strict SignVTXOForfeit validation remain solid. The remaining blocker for "leaned on for active refresh" is still #1 (unbounded actor-turn wait), with #2 as a cheap companion and #3 worth tightening for true idempotency; the in-memory-broker and cursor-contract points are swapdk-server-facing assumptions worth a doc line.

Static read only — I confirmed the broker/round/transitions state on 6b0ebfd and the fold diff via git show, but did not build/run tests in this environment; your local go test ./darepod ./db and fmt/commitmsg validation are the source of truth there.
· branch codex/vhtlc-refresh-client

@bhandras

Copy link
Copy Markdown
Member Author

@claude pls re-review with context. I folded the remaining review concerns on the client side:

  • bounded pending external forfeit-signature waits using the existing forfeit collection timeout, so a missing receiver-side response cannot block the VTXO actor indefinitely;
  • cleaned up temporary broker signing contexts on transcript-build failure, request-id conflict, and wait cancellation;
  • made duplicate signature submission idempotence order-insensitive for the same participant signature set;
  • added a defensive nil-response guard around SignVTXOForfeit in the SDK handler.

Focused local checks passed:

  • go test ./darepod -run 'TestForfeitSignatureBroker|TestRefreshCustomVTXOs|TestSignVTXOForfeit' -count=1
  • go test ./sdk/swaps -run 'Test.*Forfeit|Test.*Refresh' -count=1
  • make fmt-changed-check base=origin/main
  • make commitmsg-lint range=origin/main..HEAD

@claude

claude Bot commented Jun 16, 2026

Copy link
Copy Markdown

Claude finished @bhandras's task in 2m 7s —— View job


Re-review (head 7ce9cd7): bounded waits + lifecycle cleanup

I diffed the fold commit (daemonrpc: bound refresh signer waits) against the prior reviewed head and re-read the broker, its new tests, the actor call path, and the SDK handler. This fold closes the standing blocker (#1, the unbounded actor-turn wait) and the remaining context-leak items, and the order-insensitive idempotency + nil guard are correct and tested. One minor wiring nit and the two swapdk-server-facing assumptions remain.

Delta since last re-review — all verified

  • 🟠→✅ lib: start adding lib helpers #1 The forfeit wait is now bounded. sign derives waitCtx, cancel := b.waitContext(ctx) and selects on <-waitCtx.Done() instead of the raw (WithoutCancel) parent ctx (forfeit_signature_broker.go:179-191). waitContext wraps the parent in context.WithTimeout(ctx, b.waitTimeout) with waitTimeout defaulting to DefaultForfeitCollectionTimeout (2 min, config.go:64). So even though round/actor.go:2395 still Tells with context.WithoutCancel(ctx), the wait can no longer outlive the collection window — a missing receiver response frees the actor after the timeout and triggers the removeWaiter/deleteContext cleanup. TestForfeitSignatureBrokerTimesOutPendingRequest (:249-276) asserts exactly this: DeadlineExceeded, empty waiters, and the outpoint context dropped. This was the blocker I'd flagged across the last several rounds — it's resolved. ✅
  • 🟡→✅ multI: add initial repo scaffolding #2 Context cleanup on the non-success paths. b.contexts[outpoint] is now deleted on transcript-build error (:143), request-id conflict (:156), and wait timeout (:188), in addition to the prior LOCAL_SIGNER-success (:128-130) and submit (:328) paths. The LOCAL_SIGNER signer-error path (:124-125) is correctly left intact so a retry can still find the context. TestForfeitSignatureBrokerClearsContextOnTranscriptError (:282-305) covers the transcript path. ✅
  • 🟡→✅ chainbackend+chainsource: add chainsource actor and backend impl #3 Order-insensitive idempotency. sameParticipantSigSet now compares via participantSigMap (pubkey-keyed map[string][]byte), so a resubmit of the same signatures in any order returns the idempotent nil rather than AlreadyExists (:513-552). participantSigMap also fails closed on nil pubkey/sig and rejects duplicate pubkeys, so a malformed or duplicate-key set can't masquerade as a match. TestForfeitSignatureBrokerSubmitIsOrderInsensitive (:406-438) asserts the swapped-order resubmit succeeds. ✅
  • multi: stage boarding actor client  #4 nil-response guard. handleOutSwapForfeitSignatureRequest now returns a clean error on a nil SignVTXOForfeit response before dereferencing it (sdk/swaps/forfeit_refresh.go:133-136), and the sign→submit→ack ordering is unchanged and still correct. ✅

🟡 Minor — broker wait timeout isn't wired to the configured round timeout

newForfeitSignatureBroker() hardcodes waitTimeout: DefaultForfeitCollectionTimeout (forfeit_signature_broker.go:63), and server.go:385 constructs it with no config. But the round actor takes the operator-configurable cfg.ForfeitCollectionTimeout (server.go:3496-3497). So if an operator sets a non-default forfeitcollectiontimeout, the round and the broker wait on different durations — the liveness fix still holds (the wait is always bounded), but the "same timeout used for round forfeit collection" intent only holds at the default. Cheap to thread cfg.ForfeitCollectionTimeout into the broker so the two stay aligned when configured.

Align broker wait with config →

Residual (acknowledged, not blocking)

  • Actor-turn head-of-line blocking on the happy path. sign still runs inline on the VTXO actor's turn, so block-epoch / spend / forfeit-release events for that VTXO queue behind the (now bounded) multi-party round-trip. The prior "return a pending-forfeit transition and resume on an inbound submit event" remains the longer-term shape, but bounding the wait was the correctness blocker and that's done.
  • Request-id-conflict context cleanup is untested. The delete(b.contexts, outpoint) at :156 has no direct test (the transcript-error sibling does). Low risk; worth a one-liner if you touch the file again.

Standing RPC-surface assumptions for swapdk-server (unchanged)

  • In-memory broker, no persistence. On operator restart mid-refresh the resumed round re-issues the forfeit request, sign finds no context and returns nil, nil (:109-111) → forfeit built without the participant signature. swapdk-server must re-drive RefreshCustomVTXOs after a detected restart; the RPC still gives no explicit "context lost" signal. Worth documenting (or persisting the signing context alongside the queued refresh).
  • next_sequence is a fulfill-before-advance cursor. Safe as a high-water mark only if the caller fulfills every returned request before advancing; an unfulfilled returned request is only re-seen by polling from after_sequence = 0. The cursor fix makes this correct, but the contract is still worth one line on the next_sequence proto field. Also O(history) per poll since requests/order are never pruned.

Verdict

This fold lands the work the last several reviews were waiting on: the forfeit wait is bounded (#1 resolved), the cancellation/error paths no longer leak contexts (#2 resolved), idempotency is now order-insensitive (#3 resolved), and the nil guard is in (#4). The out-swap sign→submit→ack ordering and the strict SignVTXOForfeit validation remain solid. The only items left are the cosmetic timeout-config alignment, the acknowledged longer-term actor-turn refactor, and the two swapdk-server-facing assumptions worth a doc line. With the integration coverage the companion PRs add, I'd be comfortable with active refresh leaning on this path.

Static read only — I confirmed the fold diff via git show 7ce9cd7 and re-read the broker/tests/SDK/actor path on 7ce9cd7, but did not build/run tests in this environment; your local go test ./darepod ./sdk/swaps runs and fmt/commitmsg checks are the source of truth.
· branch codex/vhtlc-refresh-client

@bhandras
bhandras force-pushed the codex/vhtlc-refresh-client branch from 7ce9cd7 to df4dec7 Compare June 17, 2026 14:05
@bhandras

Copy link
Copy Markdown
Member Author

Folded the remaining client-side review items and force-pushed the branch.

What changed:

  • SignVTXOForfeit now requires the requested VTXO outpoint to exist in the daemon's local VTXO store, and it rejects transcripts whose amount / pkScript / policy template do not match that local state.
  • forfeitSignatureBroker.submit now verifies submitted participant signatures against the exact pending connector-bound transcript before storing them or waking the blocked VTXO actor.
  • broker tests now use real forfeit signatures over the actual VTXO-input sighash, plus coverage for invalid-signature rejection and idempotent resubmission of the same valid participant set.

Focused validation rerun:

  • go test ./darepod -run 'TestForfeitSignatureBroker|TestSignVTXOForfeit' -count=1
  • go test ./sdk/swaps -run 'Test.*Forfeit' -count=1
  • make lint-changed-local

@claude please re-review when you get a chance. The two substantive client blockers from the last review round should now be addressed.

@claude

claude Bot commented Jun 17, 2026

Copy link
Copy Markdown

Claude finished @bhandras's task in 2m 50s —— View job


Re-review (head df4dec7): local-VTXO authorization + submit transcript verification

I re-read SignVTXOForfeit + validateSignVTXOForfeitLocalVTXO, the broker's submitverifyForfeitParticipantSignatures path, and the new/updated tests. Both substantive client blockers from the last round are addressed, and both are verified by real-signature tests. Details below.

✅ Blocker 1 — SignVTXOForfeit is now anchored to local VTXO state (verified)

SignVTXOForfeit calls validateSignVTXOForfeitLocalVTXO before any signing (rpc_server.go:3294-3298). That helper (:3422-3456):

  • requires the outpoint to exist in r.server.vtxoStorecodes.NotFound if missing/nil (:3430-3438);
  • rejects on desc.Amount != VtxoAmountSat, desc.PkScript != VtxoPkScript, and desc.PolicyTemplate != VtxoPolicyTemplatecodes.InvalidArgument (:3440-3453).

This closes the prior gap where the RPC trusted the caller-supplied transcript for amount/script/policy. It composes correctly with the existing strict checks that follow (policy↔pkScript via MatchesPkScript, spend-path binding via VerifyBindsToPkScript, identity-key membership, ValidateForfeitTx). The fixture seeds the store via SaveVTXO (rpc_vtxo_forfeit_test.go:147-162) and TestSignVTXOForfeitRejectsMalformedRequests exercises the "local vtxo mismatch" case (:261-267). ✅

✅ Blocker 2 — submit verifies sigs against the exact transcript before storing/waking (verified)

sign now stashes the live request on the stored entry (signReq: req, forfeit_signature_broker.go:169), and submit runs verifyForfeitParticipantSignatures(req.signReq, participantSigs) before it stores signatures or wakes any waiter (:332-349). The verifier (:431-510) is strict in the right ways:

  • resolves the required non-operator signing keys from the policy template + spend path, excluding the operator key (:459-477);
  • rejects an unexpected key, a duplicate key, or an incomplete sig (:485-492);
  • verifies each signature against the VTXO-input tapscript sighash of the exact pending forfeit tx (verifyForfeitParticipantSignature, :514-550), recomputed with the same prevout fetcher / leaf / SigHashDefault as SignVTXOForfeit and the test helper;
  • requires every required key to be present, else "missing participant signature" (:503-507).

So a parseable-but-wrong signature, a wrong-key signature, or a partial set can no longer be stored or used to wake the blocked VTXO actor. TestForfeitSignatureBrokerSubmitRejectsInvalidSignature flips a byte and asserts InvalidArgument and that req.signatures stays empty (:400-435); the broker tests sign over the real VTXO-input sighash (:472-518); idempotent resubmit of the same valid set returns nil (:439-468). ✅

Minor observations (non-blocking)

  • submit verifies under b.mu. The whole function holds the broker lock (:312-313) and runs Schnorr verification inside it, so list/sign/other submit calls serialize behind the crypto. Verification is sub-millisecond, so this is negligible in practice — just noting it's on the hot lock.
  • Full-participant-set requirement. verifyForfeitParticipantSignatures requires every non-operator signing key for the leaf to be present in one submission. That matches the PENDING_REQUEST model where swapdk-server coordinates and submits the complete client-side participant set; the companion integration PRs are the source of truth that the operator accepts exactly this set. Worth keeping in mind that a submission carrying only a strict subset is (correctly) rejected as "missing participant signature".

Standing items — unchanged, all previously classed non-blocking

  • 🟡 Broker wait timeout still hardcoded. newForfeitSignatureBroker() pins waitTimeout = DefaultForfeitCollectionTimeout (forfeit_signature_broker.go:68) and server.go:385 constructs it with no config, while the round actor uses the operator-configurable cfg.ForfeitCollectionTimeout (server.go:3496-3497). The bounded-wait liveness fix still holds at any value; the two just diverge if an operator sets a non-default forfeitcollectiontimeout. Cheap to thread through. Align broker wait with config →
  • In-memory broker / operator-restart durability and next_sequence is a fulfill-before-advance cursor remain swapdk-server-facing assumptions worth a doc line on the proto field; the actor-turn happy-path head-of-line block is the acknowledged longer-term "pending-forfeit transition" refactor. None are blockers given the bounded wait.

Verdict

The two blockers from the last round are genuinely resolved: SignVTXOForfeit now refuses to sign anything that doesn't match locally persisted VTXO state, and submit cryptographically binds every accepted participant signature to the exact connector-bound forfeit transcript before it can wake the actor. Combined with the previously-landed bounded wait, order-insensitive idempotency, and context cleanup, the durable signing / ack-ordering / participant-plumbing surface looks sound for active refresh. The remaining items are the cosmetic timeout-config alignment and the documented swapdk-server-facing assumptions.

Static read only — the sandbox blocked go test here, so your local go test ./darepod -run 'TestForfeitSignatureBroker|TestSignVTXOForfeit' / ./sdk/swaps runs and make lint-changed-local are the source of truth. I confirmed the implementation and test bodies on df4dec7.
· branch codex/vhtlc-refresh-client

@bhandras
bhandras force-pushed the codex/vhtlc-refresh-client branch 5 times, most recently from 82d678d to 6fc0a90 Compare June 17, 2026 17:02
@bhandras
bhandras force-pushed the codex/vhtlc-refresh-client branch 4 times, most recently from 5def4c5 to 74834f1 Compare June 23, 2026 16:36
@litbot-9000

Copy link
Copy Markdown
Collaborator

@bhandras, remember to re-request review from reviewers when ready

bhandras added 23 commits June 24, 2026 19:18
In this commit we add the arkscript helpers needed by custom vHTLC
refresh flows.  Callers can recover the CHECKSIG public keys from the
policy-template leaf selected by an encoded spend path.  The helper
keeps the semantic key order from the policy AST, which lets
multi-signer refund paths build and verify witnesses consistently.  The
vHTLC policy also exposes named unilateral claim and sender-only refund
spend paths.  These accessors let higher layers choose auth and forfeit
paths explicitly instead of rebuilding them from private knowledge of
the template shape.  The tests cover both the signing-key order and the
new named accessors.
In this commit we extend the round wire and durable message types with
the metadata required by custom refresh inputs.  Join requests can now
carry the auth spend path, the connector-bound forfeit spend path, keyed
participant signatures, and fixed-amount output markers.  The local
boarding and codec paths preserve the same fields so a queued custom
refresh can survive actor persistence and retry.  Proto conversion
rejects malformed spend paths early and keeps legacy single-signer
requests compatible.  The roundpb generated output is included with its
source proto so reviewers can audit one schema change.  The tests cover
codec round trips and proto conversion for the new fields.
In this commit the round actor starts consuming the custom forfeit
metadata that was added to the request envelope. The transition logic
carries fixed outputs through the registration path without treating
them as wallet change. Outbox messages can now request participant
signatures for connector-bound forfeits instead of assuming the daemon
identity key is the only non-operator signer. Rollback and timeout paths
also drop temporary custom admissions when a quote is rejected or the
round cannot complete. This keeps the state-machine behavior separate
from the test-only coverage that exercises the new cases.
In this commit the round tests pin the custom forfeit signing behavior
added by the runtime change. The actor coverage checks that custom
refresh requests keep their fixed marker and signing context through
admission. Outbox serialization tests assert that the participant
signature request carries the custom forfeit transcript instead of
dropping it at the boundary. Quote, change-marker, and timeout tests
cover the rollback side of rejected or stalled custom admissions.
Keeping the assertions in their own commit makes the protocol behavior
easy to inspect without mixing it with the state-machine rewrite.
In this commit the wallet layer learns how to accept caller-supplied
custom refresh inputs and outputs.  The wallet messages carry the old
VTXO proof material, the replacement output policy, and the signing
route that should be used after the round assigns a connector.
Admission checks keep ordinary wallet-managed refreshes on their legacy
path while allowing custom-policy VTXOs to be queued intentionally.  The
replacement output can be marked fixed amount so the round cannot pay
fees by shrinking a contract output.  Tests cover the wallet-facing
admission rules and make sure invalid custom refresh requests fail
before they reach the VTXO manager.  This commit deliberately stops at
the wallet boundary; actor activation follows next.
In this commit the VTXO manager can materialize temporary actors for
custom refresh inputs that are not ordinary wallet-managed live rows.
The manager validates amount, pkScript, policy template, auth path, and
forfeit path before queueing the old VTXO for a refresh round. It
preserves pre-existing custom rows and rolls back partial activations
when quote rejection or admission failure prevents the round from
completing. The VTXO messages carry the custom signing context through
to the actor that later receives the exact forfeit request. The
implementation stays focused on manager state so the admission coverage
can land separately.
In this commit the manager admission tests cover the custom refresh
actor path. The tests assert that duplicate rows are preserved, custom
rows can be activated, and failed admissions roll their temporary state
back. They also check that fixed replacement outputs survive the manager
path without being mistaken for ordinary change. The coverage exercises
the message payloads that carry custom signing context toward the VTXO
actor. Splitting these assertions out keeps the production manager
change smaller while documenting the safety cases around activation and
rollback.
In this commit VTXO actors use the custom signing context once a round
has assigned the connector-bound forfeit transaction.  Standard
refreshes still use the local wallet signer and legacy single-signature
path.  Custom-policy actors ask the daemon environment for participant
signatures selected by the stored forfeit spend path.  The environment
boundary carries enough transcript material for either a local signer or
an external pending request to answer safely.  Transition tests cover
the pending participant-signature handoff and ensure the actor waits for
the requested signatures before completing the forfeit.  This is the
runtime counterpart to the manager activation commit.
In this commit OOR receive flows preserve the policy template attached
to a custom recipient output.  The indexer RPC exposes the template
recorded by the server, and the receive adapter copies it into the
incoming VTXO material. Durable receive messages and snapshots carry the
template so a restart does not downgrade a custom vHTLC into an opaque
standard output.  Materialization validates the template against the
pkScript and falls back to standard VTXO construction only when older
servers omit the field.  Later refresh and forfeit signing code needs
this semantic policy to choose the right spend path.  Existing
non-custom OOR flows continue to use the same receive state machine with
an empty policy template.
In this commit vHTLC recovery jobs become durable across refreshed
output generations. A refreshed swap output can keep the same policy
semantics while moving to a new VTXO outpoint, so the recovery store
keys jobs by the concrete target outpoint as well as the swap/action
pair. This lets the daemon retain one row per active recovery target
instead of treating the first row as the only row the old schema could
represent. The migration updates recovery-job uniqueness and makes the
downgrade path collapse multiple generations back to the newest
representable job. The recovery target builder reconstructs the vHTLC
policy from stored participants, delays, hash, and policy template. It
then verifies that the rebuilt policy still matches the VTXO pkScript
before handing the target to the sweeper. The tests cover round-store
persistence, recovery-store generation changes, lossy downgrade
behavior, and idempotent cancellation for missing or advanced jobs.
In this commit the daemon RPC surface grows the primitives needed by
active custom refresh callers.  The schema adds VTXO expiry lookup,
exact VTXO forfeit signing, custom refresh admission, pending
participant-signature listing, and participant-signature submission.
Generated grpc, gateway, mailbox, REST, and CLI registry output is kept
with the proto so the API movement is explicit. The SDK Ark client wraps
the same methods for higher-level callers that should not know about raw
protobuf types.  The generated size is large, but the source schema is
the actual review target.  Proto and client tests cover the new enum and
request surfaces.
In this commit darepod gains the in-memory broker used to publish and
resolve connector-bound participant signature requests. The broker
stores pending requests by request id and exposes sequence-based listing
so external coordinators can poll without missing entries. Submitters
can answer a request with the participant signatures required by the
selected spend path. A blocked VTXO actor is woken only for the exact
request id it emitted, which prevents one connector transcript from
satisfying another. The broker remains independent from the RPC handlers
so the service primitive can be reviewed on its own.
In this commit the forfeit signature broker receives focused unit
coverage. The tests cover request delivery, duplicate handling,
sequence-based listing, response submission, and cancellation behavior.
They assert that a waiter only wakes for the matching request id and
that unrelated responses do not leak across pending requests. The
coverage also pins the broker's copy behavior around signature payloads
so later RPC handlers can rely on stable transcripts. Keeping the tests
separate makes the broker contract visible before the daemon RPC surface
starts using it.
In this commit darepod can classify the expiry posture of a VTXO by
outpoint or indexed pkScript.  The handler asks the wallet and VTXO
layers for the same threshold data used internally by refresh
monitoring.  Callers receive the current height, batch expiry, remaining
blocks, refresh threshold, critical threshold, relative expiry, and
tree-depth context.  This lets swapdk-server make direction-specific
refresh decisions without guessing the daemon's wallet policy.  Missing
or unregistered VTXOs are reported as not found instead of being treated
as safe.  Tests cover local and indexed lookup paths along with status
conversion.
In this commit darepod gains the metadata helpers and server wiring
needed by the custom refresh RPC surface. The daemon environment now
owns the forfeit signature broker and carries the payment-hash routing
metadata used by swapdk-server. Helper code converts caller-supplied
swap metadata into the internal lookup shape without forcing the
handlers to know every storage detail. The VTXO expiry and refresh
handlers added later can share this plumbing instead of open-coding
route extraction. This keeps the structural daemon wiring separate from
the RPC methods that execute refresh work.
In this commit darepod wires the refresh RPC methods into the wallet and
VTXO runtime. RefreshCustomVTXOs validates caller-supplied custom
inputs, fixed replacement outputs, auth spend paths, forfeit spend
paths, and signing route metadata before queueing the refresh.
SignVTXOForfeit exposes the local daemon identity signer for exact
connector-bound forfeit transactions after swap-level authorization has
happened elsewhere. Pending participant-signature requests can be listed
and submitted through the broker added earlier in the stack. The
handlers use the metadata plumbing from the previous commit so the RPC
boundary stays focused on validation and orchestration.
In this commit the daemon RPC tests cover the custom refresh handlers
and signing boundary. The custom refresh coverage checks validation of
custom inputs, fixed replacement outputs, auth paths, forfeit paths, and
swap routing metadata. The VTXO forfeit tests assert that the daemon
signs only the exact connector-bound transcript it can verify from local
VTXO context. Broker-facing tests cover pending request listing and
participant-signature submission through the RPC surface. Splitting the
tests from the handlers makes the service behavior easier to audit while
preserving the same end-state coverage.
In this commit the swap RPC surface gains the messages used to exchange
custom vHTLC refresh signatures.  The schema carries the connector-bound
forfeit transcript, payment-hash routing key, participant signature, and
the RPCs used by in-swap and out-swap coordinators.  Generated grpc,
gateway, and mailbox bindings are included with the proto so downstream
callers can use the new methods immediately.  The swap client server
glue converts the swap-level messages into daemon-level participant
signature operations.  This commit only moves the wire surface and
service bridge; the higher-level SDK orchestration lands next.  Keeping
the proto separate makes the API review much easier.
In this commit the swap SDK exposes the refresh-signing methods
through its gRPC transport. The connection wrapper maps the new daemon
RPCs onto the same client path used by the rest of the SDK. Callers can
queue custom refreshes, list pending forfeit-signature requests, submit
participant signatures, and ask for exact local forfeit signatures
without handling raw protobuf plumbing. The transport layer keeps
protobuf conversion at the edge so orchestration code can work with
swap-domain request and response shapes. The tests cover request and
response conversion across that boundary. Keeping this layer first gives
the later orchestration code a stable SDK surface to call.
In this commit the swap SDK coordinates the refresh-signing flow
exposed by swaprpc and darepod. Helper code builds custom refresh inputs
from vHTLC material and selects the correct auth and forfeit paths for
each swap side. The SDK registers a mailbox-driven responder that reacts
to receiver-signature requests and feeds the synchronous daemon callback
when the local process owns the participant key. Out-swap and in-swap
callers keep the payment hash as the routing key so each connector-bound
request maps to the right swap state machine. Mailbox helpers then
publish and consume receiver-side signatures without leaking daemon
internals. The orchestration code lands separately from its tests so the
flow can be reviewed as a compact runtime change.
In this commit the swap SDK tests cover the refresh-signing
orchestration path. The out-swap coverage checks custom refresh input
construction, pending request polling, participant-signature submission,
and refreshed receive tracking. Mailbox tests assert that receiver-side
signature messages are published and consumed without exposing daemon
internals to the swap state machine. In-swap coverage pins the small
routing hook that shares the payment hash with the refresh flow. Keeping
these assertions separate from the runtime helpers makes the SDK
behavior easier to inspect at the end of the stack.
Add unit coverage for the OOR recipient policy template invariant that
previously had no client-side tests:

- BuildIncomingVTXODescriptor preserves a server-supplied template that
  binds to the recipient pkScript, and rejects one that decodes cleanly
  but does not bind (no silent downgrade to the standard template).
- The recipient template survives an IncomingSnapshot encode/decode
  cycle, so a custom policy is not lost across a restart between notify
  and materialization.
PR745 added migration 000020_vhtlc_recovery_job_generations but did not
document it. Add a migration note describing the widened recovery-job
uniqueness key and the de-duplicating down migration.

Also bring db/AGENTS.md back in sync with db/CLAUDE.md: the mirror had
drifted (it was missing the pending-intents entries and the schema
version), so this makes the pair byte-identical again.
@bhandras
bhandras force-pushed the codex/vhtlc-refresh-client branch from 4ce67e7 to 0442333 Compare June 24, 2026 17:24
@bhandras
bhandras merged commit 238cb70 into main Jun 24, 2026
47 of 50 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P1 Priority 1 — high

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants