Add swapserver-backed vHTLC recovery - #759
Conversation
Add owner-proof fields, recoverable swap listing RPCs, identity Schnorr signing, and wallet recovery counters to the public API surface. Regenerate the protobuf, gateway, mailbox, and devrpc stubs.
Register owner proofs when creating swap rows and seal out-swap preimages with daemon-derived recovery key material before requesting route hints. Restored clients can ask swapd for owned rows without storing plaintext preimages server-side.
Call the swapserver recovery listing during wallet restore, validate live vHTLC scripts with the daemon and indexer, and arm daemon-owned refund or claim recovery rows. Surface vHTLC recovery counters through walletdk and swapwallet responses.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces vHTLC recovery functionality during seed restore, allowing the discovery of swapserver-owned recoverable swaps and the arming of daemon-owned recovery jobs. It adds the SignIdentitySchnorr RPC to the daemon service and the ListRecoverableSwaps RPC to the swap service, along with client SDK updates, encryption/decryption of recovery blobs, and corresponding tests. The review feedback highlights a critical compilation error in recovery_auth.go due to incorrect arguments passed to chainhash.TaggedHash. Additionally, potential nil pointer dereferences were flagged when allocating receive scripts, and an improvement was suggested to make the recovery loop more robust by skipping individual malformed rows instead of aborting the entire process.
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.
| key := chainhash.TaggedHash( | ||
| []byte(swapRecoveryBlobTag), | ||
| sharedSecret[:], | ||
| paymentHash[:], | ||
| clientKey.SerializeCompressed(), | ||
| ) |
There was a problem hiding this comment.
chainhash.TaggedHash from github.com/btcsuite/btcd/chaincfg/chainhash expects exactly two arguments: tag []byte and msg []byte. Passing four arguments here will cause a compilation error.
To fix this, concatenate the message fields into a single byte slice before passing it to chainhash.TaggedHash.
var msg []byte
msg = append(msg, sharedSecret[:]...)
msg = append(msg, paymentHash[:]...)
msg = append(msg, clientKey.SerializeCompressed()...)
key := chainhash.TaggedHash(
[]byte(swapRecoveryBlobTag),
msg,
)| if err != nil { | ||
| return fmt.Errorf("allocate refund destination: %w", err) | ||
| } |
There was a problem hiding this comment.
If AllocateReceiveScript returns a nil destination without an error, accessing destination.PkScript on line 172 will cause a nil pointer dereference panic. It is safer to add a nil check for destination.
if err != nil {
return fmt.Errorf("allocate refund destination: %w", err)
}
if destination == nil {
return fmt.Errorf("allocated refund destination is nil")
}| if err != nil { | ||
| return fmt.Errorf("allocate claim destination: %w", err) | ||
| } |
There was a problem hiding this comment.
If AllocateReceiveScript returns a nil destination without an error, accessing destination.PkScript on line 238 will cause a nil pointer dereference panic. It is safer to add a nil check for destination.
if err != nil {
return fmt.Errorf("allocate claim destination: %w", err)
}
if destination == nil {
return fmt.Errorf("allocated claim destination is nil")
}| for _, row := range rows { | ||
| recovered, err := c.recoverSwapserverVHTLC( | ||
| ctx, clientKey, row, | ||
| ) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
During wallet recovery, if a single row returned by the swapserver is malformed or fails validation (e.g., recoverSwapserverVHTLC returns an error), aborting the entire loop will prevent the recovery of all other valid swaps.
Consider logging the error and continuing with the remaining rows to make the recovery process more robust against isolated server-side data corruption or compatibility issues.
| for _, row := range rows { | |
| recovered, err := c.recoverSwapserverVHTLC( | |
| ctx, clientKey, row, | |
| ) | |
| if err != nil { | |
| return nil, err | |
| } | |
| for _, row := range rows { | |
| recovered, err := c.recoverSwapserverVHTLC( | |
| ctx, clientKey, row, | |
| ) | |
| if err != nil { | |
| continue | |
| } |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ff2a97452e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| destination, err := c.daemon.AllocateReceiveScript( | ||
| ctx, "vhtlc-recovery-refund", | ||
| ) |
There was a problem hiding this comment.
When seed recovery is retried after this swap was already armed, this fresh AllocateReceiveScript returns a different destination, but the RequestId below is deterministic for the same payment hash/action. ArmRecovery treats the same request id with different durable parameters (including DestinationScript) as an idempotency conflict, so a second recovery pass or a retry after a later step fails instead of resuming the existing job. Reuse a deterministic/stored destination or load the existing recovery before allocating; the claim path below has the same pattern.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Adds swapserver-backed recovery for vHTLC-based swaps by extending swap RPCs with owner-proof authentication and encrypted recovery metadata, wiring daemon identity Schnorr signing, and invoking the new recovery flow during wallet seed restore.
Changes:
- Introduces
ListRecoverableSwapsplusSwapOwnerProof/recovery metadata fields in swap RPCs and REST/gateway/mailbox stubs. - Adds daemon RPC
SignIdentitySchnorrand SDK plumbing to produce tagged BIP-340 Schnorr signatures for recovery ownership proofs. - Implements SDK swap recovery discovery/arming (
RecoverSwapserverVHTLCs) and integrates recovery counters into wallet creation/recovery responses.
Reviewed changes
Copilot reviewed 28 out of 36 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| swapwallet/register_test.go | Updates fakes to satisfy the expanded swap backend interface (RecoverVHTLCs). |
| swapwallet/admin.go | Surfaces new vHTLC recovery counters in wallet create response. |
| swaprpc/swap.yaml | Exposes ListRecoverableSwaps via grpc-gateway HTTP mapping. |
| swaprpc/swap.proto | Adds ListRecoverableSwaps, SwapOwnerProof, recovery blob fields, and recoverable swap metadata. |
| swaprpc/swap.pb.gw.go | Regenerates gateway handlers for the new swap RPC. |
| swaprpc/swap.pb.go | Regenerates protobuf Go types for new swap recovery messages/enums/RPCs. |
| swaprpc/swap_mailboxrpc.pb.go | Regenerates mailbox RPC adapter to include ListRecoverableSwaps. |
| swaprpc/swap_grpc.pb.go | Regenerates gRPC client/server stubs to include ListRecoverableSwaps. |
| swapclientserver/service.go | Implements RecoverVHTLCs to bridge daemon recovery with sdk/swaps recovery. |
| swapclientserver/service_test.go | Updates call sites for new RequestChannelID signature and fake runtime recovery method. |
| sdk/walletdk/types.go | Adds recovered vHTLC counters to SDK wallet result type. |
| sdk/walletdk/client.go | Maps new daemon recovery counters into SDK wallet create response. |
| sdk/swaps/server_recovery.go | Implements swapserver-driven vHTLC discovery + daemon recovery arming logic. |
| sdk/swaps/server_recovery_test.go | Adds tests for recovery blob round-trip and recovery arming behavior. |
| sdk/swaps/rest_conn_test.go | Updates REST swap server conn tests for new request parameters. |
| sdk/swaps/recovery_auth.go | Implements owner-proof signing and out-swap recovery blob sealing/opening. |
| sdk/swaps/out_swap.go | Registers owner proof + sealed recovery blob during out-swap setup. |
| sdk/swaps/out_swap_test.go | Extends test server/daemon fakes for owner proofs, recoverable listing, and identity Schnorr signing. |
| sdk/swaps/in_swap.go | Registers owner proof during in-swap creation. |
| sdk/swaps/in_swap_test.go | Updates in-swap test server fake interface for new parameters/methods. |
| sdk/swaps/grpc_conn.go | Extends swap server conn with owner proof fields and ListRecoverableSwaps. |
| sdk/swaps/client.go | Extends SwapServerConn and DaemonConn interfaces for recovery/signing. |
| sdk/ark/client.go | Implements SignIdentitySchnorr via daemon RPC for SDK consumers. |
| rpc/walletdkrpc/wallet.proto | Adds recovered vHTLC counters to wallet create response schema. |
| rpc/walletdkrpc/wallet.pb.go | Regenerates walletdk protobuf Go types/accessors with new counters. |
| rpc/restclient/clients.go | Adds REST client methods for ListRecoverableSwaps and SignIdentitySchnorr. |
| darepod/wallet_recovery.go | Invokes swap vHTLC recovery during wallet seed restore and records counters. |
| darepod/rpc_wallet.go | Adds daemon RPC handler SignIdentitySchnorr. |
| darepod/config.go | Extends swap backend interface with RecoverVHTLCs and defines SwapRecoveryResult. |
| daemonrpc/daemon.yaml | Exposes SignIdentitySchnorr via grpc-gateway HTTP mapping. |
| daemonrpc/daemon.proto | Adds daemon RPC SignIdentitySchnorr and new recovery counters in InitWalletResponse. |
| daemonrpc/daemon.pb.gw.go | Regenerates gateway handlers for SignIdentitySchnorr. |
| daemonrpc/daemon_mailboxrpc.pb.go | Regenerates mailbox RPC adapter to include SignIdentitySchnorr. |
| daemonrpc/daemon_grpc.pb.go | Regenerates gRPC client/server stubs to include SignIdentitySchnorr. |
| cmd/darepocli/darepoclicommands/devrpc/registry_generated.go | Regenerates CLI dev RPC registry to include SignIdentitySchnorr. |
Files not reviewed (7)
- cmd/darepocli/darepoclicommands/devrpc/registry_generated.go: Generated file
- daemonrpc/daemon.pb.gw.go: Generated file
- daemonrpc/daemon_grpc.pb.go: Generated file
- daemonrpc/daemon_mailboxrpc.pb.go: Generated file
- rpc/walletdkrpc/wallet.pb.go: Generated file
- swaprpc/swap.pb.go: Generated file
- swaprpc/swap.pb.gw.go: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if daemon == nil { | ||
| return nil, fmt.Errorf("daemon connection is required") | ||
| } | ||
| if clientKey == nil { | ||
| return nil, fmt.Errorf("client identity key is required") | ||
| } |
| if len(blob) < 1+24 { | ||
| return nil, fmt.Errorf("recovery blob is too short") | ||
| } | ||
| if blob[0] != swapRecoveryBlobVersion { | ||
| return nil, fmt.Errorf("unsupported recovery blob version %d", | ||
| blob[0]) | ||
| } | ||
|
|
||
| key, err := outSwapRecoveryBlobKey(ctx, daemon, clientKey, paymentHash) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| var nonce [24]byte | ||
| copy(nonce[:], blob[1:25]) | ||
|
|
||
| plaintext, ok := secretbox.Open(nil, blob[25:], &nonce, &key) | ||
| if !ok { | ||
| return nil, fmt.Errorf("open recovery blob") | ||
| } |
| _, err = c.daemon.ArmVHTLCRecovery( | ||
| ctx, &daemonrpc.ArmVHTLCRecoveryRequest{ | ||
| RequestId: recoveryRequestID( | ||
| string(SwapDirectionPay), paymentHash, | ||
| recoveryActionRefundWithoutReceiver, | ||
| ), | ||
| SwapId: append([]byte(nil), paymentHash[:]...), | ||
| Direction: recoveryDirectionPay, | ||
| Action: recoveryActionRefundWithoutReceiver, | ||
| VtxoOutpoint: live.Outpoint, | ||
| VtxoAmountSat: live.AmountSat, | ||
| SenderPubkey: append([]byte(nil), row.GetSenderPubkey()...), | ||
| ReceiverPubkey: append([]byte(nil), row.GetReceiverPubkey()...), | ||
| ServerPubkey: append([]byte(nil), row.GetOperatorPubkey()...), | ||
| RefundLocktime: int32(row.GetRefundLocktime()), | ||
| UnilateralClaimDelay: int32( | ||
| row.GetUnilateralClaimDelay(), | ||
| ), | ||
| UnilateralRefundDelay: int32( | ||
| row.GetUnilateralRefundDelay(), | ||
| ), | ||
| UnilateralRefundWithoutReceiverDelay: int32( | ||
| row.GetUnilateralRefundWithoutReceiverDelay(), | ||
| ), | ||
| PreimageHash: append([]byte(nil), paymentHash[:]...), |
| RefundLocktime: int32(row.GetRefundLocktime()), | ||
| UnilateralClaimDelay: int32( | ||
| row.GetUnilateralClaimDelay(), | ||
| ), | ||
| UnilateralRefundDelay: int32( | ||
| row.GetUnilateralRefundDelay(), | ||
| ), | ||
| UnilateralRefundWithoutReceiverDelay: int32( | ||
| row.GetUnilateralRefundWithoutReceiverDelay(), | ||
| ), |
|
@claude please review, focus on overcomplication of the process |
|
Claude finished @sputn1ck's task in 3m 31s —— View job Review: swapserver-backed vHTLC recovery — focus on overcomplication
I focused on process complexity as requested. The feature is sound, but there are a few places where the recovery path carries more machinery than it needs. Ranked by how much they'd simplify things. 1. The recovery arming path is a near-verbatim copy of the live arming path 🔴
This is the biggest simplification opportunity: extract one builder, e.g. func buildArmRequest(p armParams) *daemonrpc.ArmVHTLCRecoveryRequestand have both the live sessions and the recovery path populate 2. Is the sealed recovery blob necessary at all? 🟡 (worth a design discussion)The entire But the blob key is already derived deterministically from daemon seed material ( The one wrinkle: the current key is bound to If random preimages are a hard requirement, the blob is fine — just flagging the tradeoff. 3. Recovery arming isn't idempotent across retries 🟡
4. Smaller redundancies
5. Note on the other bots' findings
Overall: the feature works, but the recovery path would shrink meaningfully by collapsing the duplicated arm-request construction (#1) and reconsidering whether the preimage blob is needed (#2). #3 is a real retry bug. |
|
Addressed the review follow-ups in 3cc192e:
Deterministic preimages are intentionally deferred as a larger protocol redesign. The Gemini TaggedHash compile comment is a false positive: chainhash.TaggedHash is variadic, and the existing call shape compiles. |
|
@sputn1ck, remember to re-request review from reviewers when ready |
Summary
Tests