Use forfeit pubkey in batch out sweep closure and checkpoint tapscript

[altafan]

This makes sure that when any output is swept onchain, funds go back to the liquidity provider and not to the signer, which is an entity that is not expected to own onchain funds; rather, it just cosigns vtxos.

Summary by CodeRabbit

• Bug Fixes

  • Corrected key selection for forfeit and sweep flows so transactions sign reliably and finalization succeeds.
  • Ensured Liquidity Provider mode always uses the designated forfeit key even if the primary signer is unavailable.

• Refactor

  • Consolidated and simplified signing key selection for more consistent transaction handling.

• Chores

  • Updated SDK dependency to the latest pre-release version for compatibility.

[coderabbitai]

Walkthrough

Rewires forfeit vs signer key usage across core, builder, wallet, tests and bumps github.com/arkade-os/go-sdk version in two go.mod files; key retrieval and signing now use wallet/GetForfeitPubkey and forfeit private key in LP mode where applicable.

Changes

Cohort / File(s)	Summary
Dependency bump go.mod, pkg/ark-cli/go.mod	Bumped github.com/arkade-os/go-sdk from v0.7.2-0.20251004010022-39eb0fd18e10 → v0.7.2-0.20251004011707-e3e48806e23d.
Core service forfeit wiring internal/core/application/service.go	Replaced signerPubkey with forfeitPubkey in CSVMultisigClosure constructions and when building commitment/forfeit-related transactions during finalization.
Tx builder: wallet-based forfeit key & signing internal/infrastructure/tx-builder/covenantless/builder.go	BuildSweepTx now calls wallet.SignTransactionTapscript to sign; extraction uses wallet.GetForfeitPubkey(...) for the sweeper pubkey instead of signer-based pubkey.
Wallet signing selection logic pkg/arkd-wallet/core/application/wallet/service.go	In SignTransaction (Taproot leaf path) always initialize signingKey from w.SignerKey and override with w.keyMgr.forfeitPrvkey when SignModeLiquidityProvider is active (consolidated key-selection logic).
Tests: delegate key rename & usage test/e2e/e2e_test.go, test/e2e/utils_test.go	Replaced signerPubKey usages with forfeitPubKey in JoinBatchSession call and renamed struct field signerPubKey → forfeitPubKey; adjusted MultisigClosure to use the forfeit pubkey in test handlers.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Core as Core Service
  participant Builder as Tx Builder
  participant Wallet as Wallet
  participant Chain as Bitcoin Network

  Note over Core,Builder: Finalization and sweep now use forfeit key paths

  Core->>Builder: BuildCommitmentTx(forfeitPubkey, ...)
  Builder-->>Core: PSBT (commitment)

  Core->>Builder: BuildSweepTx(PSBT)
  Builder->>Wallet: GetForfeitPubkey()
  Wallet-->>Builder: forfeitPubkey
  Builder->>Wallet: SignTransactionTapscript(PSBT, forfeit context)
  Wallet-->>Builder: Signed PSBT
  Builder-->>Core: Sweep TX

  Core->>Wallet: SignTransaction(PSBT, mode)
  alt SignModeLiquidityProvider
    Wallet->>Wallet: signingKey = keyMgr.forfeitPrvkey
  else other modes
    Wallet->>Wallet: signingKey = SignerKey
  end
  Wallet-->>Core: Signed PSBT

  Core->>Chain: Broadcast TX
  Chain-->>Core: TXID / result

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• Allow any sighash type to be used in forfeit transactions + TestDelegateRefresh #677 — Modifies forfeit-related signing and TestDelegateRefresh flow; closely related to forfeit vs signer changes.
• Tidy up wallet & Introduce Signer #702 — Changes wallet/signer APIs and wiring (GetForfeitPubkey / forfeit key usage); strongly related.
• Batch session: TreeNoncesEvent to let clients to aggregate nonces #730 — Alters finalization/event emission and interacts with startFinalization signing paths; relevant to core service changes.

Suggested reviewers

• Kukks

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The title precisely describes the primary update of using the forfeit public key in both the batch-out sweep closure and the checkpoint tapscript as described in the change summary, making it clear and specific without extraneous detail.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch use-forfeit-pubkey

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d538c13 and 4d9b7d3.

📒 Files selected for processing (2)

• test/e2e/e2e_test.go (1 hunks)
• test/e2e/utils_test.go (2 hunks)

🧰 Additional context used

🧠 Learnings (1)

📓 Common learnings

Learnt from: louisinger
PR: arkade-os/arkd#691
File: internal/core/application/service.go:557-562
Timestamp: 2025-08-19T10:58:41.042Z
Learning: In the arkd SubmitOffchainTx method, using the checkpoint PSBT input's tapscript (forfeit path) for the VtxoInput.Tapscript field is the correct behavior, not a bug as initially thought. The system correctly handles the relationship between checkpoint inputs and Ark transaction inputs.

🧬 Code graph analysis (1)

test/e2e/utils_test.go (1)

pkg/ark-lib/script/closure.go (1)

• MultisigClosure (26-29)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)

• GitHub Check: integration tests
• GitHub Check: Build and Scan
• GitHub Check: unit tests

🔇 Additional comments (2)

test/e2e/e2e_test.go (1)

2027-2027: LGTM! Correctly uses forfeit pubkey for delegation.

The change from signerPubKey to aliceConfig.ForfeitPubKey correctly aligns with the PR objective to ensure swept funds return to the liquidity provider. This is consistent with the field rename in delegateBatchEventsHandler (utils_test.go:462) and the updated sweep closure construction that now uses the forfeit public key.

test/e2e/utils_test.go (1)

462-462: LGTM! Correctly implements forfeit pubkey in sweep closure.

The field rename from signerPubKey to forfeitPubKey (line 462) and its usage in the sweep closure construction (line 530) correctly implement the PR's objective. The sweep closure now uses the liquidity provider's forfeit public key, ensuring that expired batch outputs are swept back to the LP rather than the signer. This aligns with the intended behavior described in the PR where the signer should not hold onchain funds.

Also applies to: 530-530

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[Kukks]

ACK but would be good to have e2e test where we explictly verify this is correct in all locations during operation