Sweep fallback across primary and fallback arkd-wallets

[bitcoin-coder-bob]

Why

Phase-2 core of letting a single arkd front more than one LP. #1099 dials and validates primary + fallback arkd-wallets but nothing uses the fallbacks yet. This PR makes sweeping use them: a batch (or checkpoint, or admin) sweep is now signed by whichever of the primary/fallback wallets can sign it, so arkd can sweep batches belonging to any of its LPs.

Stacked PRs (merge top-down):

• add Settings domain with DB persistence and admin CRUD API #939 — ✅ merged into master
• Require arkd-wallet to be initialized and unlocked out of band #1098 — base master
• Dial primary plus fallback arkd-wallets #1099 — dial primary + fallback wallets (base bob/arkd-require-unlocked-wallet)
• this PR — base bob/arkd-multi-wallet-dial; merges after Dial primary plus fallback arkd-wallets #1099
• Persist primary/fallback wallet addresses in the settings domain #1102 — wallet addresses in settings (draft), stacked on this branch

What changes

Build/sign split (tx-builder/covenantless/sweep.go, ports/tx_builder.go)

• sweepTransaction is split into:
  • buildSweepTransaction(ctx, wallet, inputs) -> (unsignedTx, txid) — destination address, fee estimate and dust limit all come from the primary wallet, so every signing candidate sweeps to the same output and the txid is stable regardless of who signs.
  • signSweepTransaction(ctx, wallet, unsignedTx) -> signedTx — signs with an arbitrary wallet; the tapscript inputs that need signing are re-derived from the psbt (inputs carrying a TaprootLeafScript), so no index list has to be threaded through the API.
• TxBuilder.BuildSweepTx now returns the unsigned tx; new TxBuilder.SignSweepTx(wallet, unsignedTx).

Fallback iteration (application/sweeper.go, application/admin.go)

• New buildAndSignSweepTx(builder, wallets, inputs) builds once, then tries signing with each wallet in order, returning on first success and aggregating errors (errors.Join) if none can sign.
• sweeper and adminService gained a walletFallbacks field and a signingWallets() helper returning [primary, ...fallbacks]. The three sweeper BuildSweepTx call sites (batch, event-rebuild, checkpoint) and admin.Sweep all use the new helper.
• Broadcasting stays on the primary wallet. BroadcastTransaction only relays raw bytes to the wallet daemon, so a fallback-signed (fully valid) tx broadcasts fine via the primary; the existing BIP68 non-final retry loop is untouched.
• Graceful degradation on the event-rebuild path. In createBatchSweepTask, when a batch's outputs are already spent and no primary/fallback wallet can sign the rebuilt sweep, it no longer errors out: it logs a warning and reconciles using the unsigned tx (re-deriving the txid) so the sweep event still completes.

Wiring (config.go)

• c.fallbackWalletServices() is threaded into NewService (→ sweeper) and NewAdminService. With no fallbacks configured, signingWallets() returns just [primary], so single-wallet behavior is unchanged.

Tests

• TestBuildAndSignSweepTx (application): first-success short-circuit, ordered fallback, all-fail aggregated error, build-error short-circuit.
• TestSignSweepTransaction_DerivesTapscriptIndexes / _NoTapscriptInputs (covenantless): the re-derived tapscript indexes match the inputs carrying a leaf script, and the tapscript-signed psbt is forwarded to the final sign.
• go build ./..., make lint (0 issues), go test on the touched non-infra packages pass with -race.

Notes

• The fallback path only exercises the second wallet end to end once it's actually used; Dial primary plus fallback arkd-wallets #1099 already wired arkd-wallet-2 into docker-compose.regtest.yml and the e2e harness, so the regtest suite now runs with a fallback present.
• Selecting which LP a wallet serves (vs. just "whoever can sign") and moving the designation into the settings domain are follow-ups.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

🗂️ Base branches to auto review (1)

• next-version

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 989efc30-5f66-4897-bd78-8cdc516e61b3

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch bob/arkd-sweep-fallback

---

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.}

[ghost]

Arkana Code Review — arkd#1101

Reviewer: Arkana (aggressive review mode)
Verdict: Request changes — one correctness concern + protocol-critical flag

---

Overall Assessment

Well-structured PR. The build/sign split is clean, the fallback iteration logic is correct, and the test coverage is solid. The tapscript-index re-derivation from the PSBT (signSweepTransaction) is the right design — it avoids threading index lists across API boundaries.

That said, I have one medium-severity finding and several observations.

---

🔴 M1 — "All-spent" rebuild path now fails if no wallet can sign

File: internal/core/application/sweeper.go — the call site at the old line 669 (now buildAndSignSweepTx(s.builder, s.signingWallets(), outputsToSweep))

Context: When all batch outputs are already spent, the sweeper builds a sweep tx without broadcasting it — solely to reconstruct the sweepEvent for the projection store. The comment on the old line 667-668 says exactly this:

"if all outputs are spent, it means we missed to mark the batch as swept, build a sweep transaction without broadcasting it. we'll use it rebuild sweepEvent."

Problem: Before this PR, BuildSweepTx both built and signed with the primary wallet. If the primary wallet couldn't sign, this path failed — but that was already a latent issue. Now buildAndSignSweepTx iterates all wallets and returns a hard error ("no wallet could sign sweep tx") if none can sign.

In the "all-spent" path, the signed tx hex is never broadcast. Only sweepTxId is used (for event reconstruction at line 676). The signing is entirely wasted work. Worse: if the batch was originally swept by a wallet that's no longer configured as a fallback, this path will now error out and prevent the sweeper from reconciling its state.

Fix: For the all-spent path, call builder.BuildSweepTx(inputs) directly (which returns (unsignedTx, txid, err)) and use only the txid. No signing needed.

// all outputs already spent — only need the txid for event reconciliation
unsignedTx, sweepTxId, err := s.builder.BuildSweepTx(outputsToSweep)
_ = unsignedTx
if err != nil {
    return err
}

Or better yet, extract a BuildSweepTxID helper that doesn't sign at all.

---

🟡 O1 — Positional struct initialization in newSweeper is fragile

File: internal/core/application/sweeper.go:52-54

return &sweeper{
    wallet, walletFallbacks, repoManager, builder, scheduler,
    &sync.Mutex{}, make(map[string]struct{}), nil,
}

8 positional fields. If anyone inserts a field between scheduler and locker, this silently misassigns values (and the compiler won't catch it if the types happen to align). Use named fields:

return &sweeper{
    wallet:          wallet,
    walletFallbacks: walletFallbacks,
    repoManager:     repoManager,
    builder:         builder,
    scheduler:       scheduler,
    locker:          &sync.Mutex{},
    scheduledTasks:  make(map[string]struct{}),
}

This is pre-existing tech debt (it was positional before), but since you're already modifying the constructor and adding a field, now is the time to fix it.

---

🟡 O2 — signingWallets() is duplicated between sweeper and adminService

Files: sweeper.go and admin.go — identical implementations:

func (s *sweeper) signingWallets() []ports.WalletService {
    return append([]ports.WalletService{s.wallet}, s.walletFallbacks...)
}

Consider extracting to a shared free function or embedding a common struct. Minor, but this is the kind of duplication that drifts.

---

🟡 O3 — context.Background() in SignSweepTx

File: internal/infrastructure/tx-builder/covenantless/builder.go:283-286

func (b *txBuilder) SignSweepTx(
    wallet ports.WalletService, unsignedTx string,
) (signedTx string, err error) {
    ctx := context.Background()
    return signSweepTransaction(ctx, wallet, unsignedTx)
}

Both BuildSweepTx and SignSweepTx use context.Background(). The sweeper checks s.ctx.Done() in its BIP68 retry loop, but the actual wallet RPC calls inside signSweepTransaction are not cancellable. If arkd is shutting down and a wallet RPC hangs, these calls block indefinitely. This is pre-existing (the old sweepTransaction also used context.Background() via BuildSweepTx), but since you're refactoring the interface, consider threading context through:

SignSweepTx(ctx context.Context, wallet WalletService, unsignedTx string) (string, error)

Not blocking, but worth a follow-up.

---

🟢 Positive observations

1. Return-value swap is handled correctly. The dangerous (txid, signed, err) → (unsigned, txid, err) reorder in BuildSweepTx is safely encapsulated inside buildAndSignSweepTx, which maps it back to (txid, signed, err) for all callers. I verified all 4 call sites.

2. Tapscript index re-derivation (signSweepTransaction scanning for TaprootLeafScript) is correct and well-tested. Eliminates a fragile index-threading dependency.

3. Broadcast stays on primary wallet — correct, since BroadcastTransaction just relays raw bytes.

4. Test coverage is good. TestBuildAndSignSweepTx covers: primary-first short-circuit, ordered fallback, all-fail aggregation, no-wallets, build-error. TestSignSweepTransaction_DerivesTapscriptIndexes verifies the index derivation. Edge cases are handled.

5. errors.Join for aggregated sign errors is the right pattern — gives operators visibility into which wallet rejected.

---

⚠️ Protocol-Critical Flag

This PR modifies sweep transaction signing — a protocol-critical path. Sweep transactions recover expired VTXO funds back to the operator. A bug here means stuck funds or failed sweeps.

The code looks correct to me, but per protocol: this requires explicit human sign-off before merge.

---

Summary: Fix M1 (all-spent path shouldn't require signing), consider O1-O3 as improvements. Requesting human review for protocol-critical sweep signing changes.

[ghost]

Arkana Follow-Up Review — arkd#1101 (commit b9db0bc)

Focus: Incremental changes since my last review (ded6d98 → b9db0bc).

---

✅ O1 and O2 — Fixed

• newSweeper now uses named struct fields — good. No more positional foot-gun.
• primaryThenFallbacks is extracted as a shared free function used by both sweeper.signingWallets() and adminService.signingWallets() — clean.

Both changes are correct. No issues.

---

🔴 M1 — Still open: all-spent path still signs unnecessarily

File: internal/core/application/sweeper.go:722-729

// if all outputs are spent, it means we missed to mark the batch as swept,
// build a sweep transaction without broadcasting it. we'll use it rebuild sweepEvent.
sweepTxId, sweepTx, err = buildAndSignSweepTx(
    s.builder, s.signingWallets(), outputsToSweep,
)

This path only needs sweepTxId for event reconciliation — the signed tx is never broadcast. Signing is wasted work. More importantly: if the batch was originally swept by a wallet that's since been decommissioned (no longer primary or fallback), this path hard-fails with "no wallet could sign sweep tx", preventing the sweeper from reconciling that batch.

Fix: Call s.builder.BuildSweepTx(outputsToSweep) directly — it returns (unsignedTx, txid, err) now, which is all this code path needs:

_, sweepTxId, err = s.builder.BuildSweepTx(outputsToSweep)
if err != nil {
    return err
}

Or if sweepTx is actually used downstream for the event (I checked — it is passed to NewBatchSweptEvent at line 735), then this path truly does need a signed tx. In that case the current behavior is acceptable, but please confirm the intent and add a comment explaining why signing is required even though the tx isn't broadcast.

---

⚠️ Protocol-critical flag remains

Sweep signing path — requires human sign-off before merge.

Summary: O1/O2 look good. M1 still needs attention — either skip signing in the all-spent path or document why it's needed.

[ghost]

Arkana Follow-Up Review — arkd#1101 (rebase: 48fbf22..44379b3)

Focus: Branch was rebased/squashed. Verified the rebase is clean — O1 (named struct fields) and O2 (primaryThenFallbacks extraction) are correctly incorporated. README update (44379b3) is accurate. No new issues introduced.

---

🔴 M1 — Still open (third review): all-spent reconciliation path signs unnecessarily

File: internal/core/application/sweeper.go:721-725 (in PR diff)

// if all outputs are spent, it means we missed to mark the batch as swept,
// build a sweep transaction without broadcasting it. we'll use it rebuild sweepEvent.
sweepTxId, sweepTx, err = buildAndSignSweepTx(
    s.builder, s.signingWallets(), outputsToSweep,
)

Why this matters: I checked — round.Sweep(leafVtxos, preconfirmedVtxos, sweepTxId, sweepTx) stores sweepTx in r.SweepTxs[txid] via the BatchSwept event handler. So sweepTx isn't discarded. However:

1. The stored hex is wrong anyway. The all-spent path means this batch was already swept on-chain by some wallet. The tx we sign here has different witness data than the actual on-chain tx (segwit txid stays the same, but the raw hex differs). We're storing a "fake" signed version.

2. Hard failure when the original wallet is decommissioned. If the batch was swept by a wallet that's no longer configured as primary or fallback, buildAndSignSweepTx returns "no wallet could sign sweep tx", and the sweeper permanently fails to reconcile this batch. It retries every cycle, logs errors, never succeeds — a livelock.

Suggested fix options (pick one):

(A) Skip signing, store the unsigned PSBT (if downstream consumers of SweepTxs only need the txid for lookups):

unsignedTx, sweepTxId, err = s.builder.BuildSweepTx(outputsToSweep)
// pass unsignedTx as sweepTx — never broadcast, only used for event storage

(B) Best-effort signing, fall through on failure:

sweepTxId, sweepTx, err = buildAndSignSweepTx(
    s.builder, s.signingWallets(), outputsToSweep,
)
if err != nil {
    // All outputs already spent — signing failure is non-fatal.
    // Use unsigned tx for event reconciliation.
    var unsignedTx string
    unsignedTx, sweepTxId, err = s.builder.BuildSweepTx(outputsToSweep)
    if err != nil {
        return err
    }
    sweepTx = unsignedTx
}

Option B is more defensive — it tries to sign (preserving current behavior when possible) but doesn't block reconciliation when it can't.

---

⚠️ Protocol-critical flag remains

Sweep signing path — requires human sign-off before merge.

Summary: Rebase is clean. M1 is the only remaining blocker — the all-spent reconciliation path can livelock when a wallet is decommissioned. Everything else looks correct.

[ghost]

Arkana Follow-Up Review — arkd#1101 (rebase: ef4ac4d)

Verified: pure rebase onto updated base, no content changes to PR files. No new issues introduced by the rebase.

🔴 M1 — Still open (4th review): all-spent reconciliation path signs unnecessarily

File: internal/core/application/sweeper.go:722-729

This remains the only blocker. The all-spent code path calls buildAndSignSweepTx when it only needs a txid for event reconciliation. When the original signing wallet is decommissioned (no longer primary or fallback), this path hard-fails with "no wallet could sign sweep tx" every cycle — a permanent livelock that prevents batch reconciliation.

Suggested fix (option B from my previous review — best-effort signing, graceful fallback):

sweepTxId, sweepTx, err = buildAndSignSweepTx(
    s.builder, s.signingWallets(), outputsToSweep,
)
if err != nil {
    // All outputs already spent on-chain — signing failure is non-fatal.
    // Fall back to the unsigned tx for event reconciliation.
    var unsignedTx string
    unsignedTx, sweepTxId, err = s.builder.BuildSweepTx(outputsToSweep)
    if err != nil {
        return err
    }
    sweepTx = unsignedTx
}

⚠️ Protocol-critical flag

Sweep signing path — requires human sign-off before merge.

[ghost]

Arkana Follow-Up Review — arkd#1101 (commit 89271cc)

Focus: New commit addressing M1 — all-spent reconciliation path.

✅ M1 — Resolved

The all-spent path now implements option B (best-effort signing with graceful fallback):

sweepTxId, sweepTx, err = buildAndSignSweepTx(...)
if err != nil {
    log.WithError(err).Warnf(...)
    sweepTx, sweepTxId, err = s.builder.BuildSweepTx(outputsToSweep)
    if err != nil {
        return err
    }
}

Verified:

• Return value mapping is correct: BuildSweepTx returns (unsignedTx, txid, err) → assigned to (sweepTx, sweepTxId, err) ✓
• Signing failure no longer blocks reconciliation — livelock eliminated ✓
• Warning log includes batch ID and error context for operator visibility ✓
• Comment clearly explains why signing failure is non-fatal here ✓

All previous findings (O1, O2, M1) are now addressed. Code is correct.

⚠️ Protocol-critical flag remains

This PR modifies sweep transaction signing — a protocol-critical path. The code looks correct to me, but this requires explicit human sign-off before merge.

[ghost]

Arkana Follow-Up Review — arkd#1101 (rebase: ccb8a46)

Verified: pure rebase onto updated base, no content changes from previously approved commit 89271cce. All prior findings (O1, O2, M1) remain addressed. Approval carries forward.

⚠️ Protocol-critical flag remains — sweep signing path requires human sign-off before merge.

[ghost]

Arkana Follow-Up Review — arkd#1101 (merge: 45b1aa8)

Focus: Base branch merge — 3 new commits from bob/arkd-multi-wallet-dial (#1099).

Changes from base branch (not this PR's code)

• FallbackWallet{Addr, Service} struct replaces bare []ports.WalletService
• Duplicate and primary-equals-fallback validation at startup
• Error messages now use host:port instead of positional index

Integration verified ✅

• fallbackWalletServices() correctly extracts Service fields for the application layer
• gRPC shutdown/readiness uses FallbackWallets() (needs Addr for error context) — correct
• appService() / adminService() wiring unchanged — still passes []ports.WalletService to application code
• No merge conflict artifacts, no logic changes to this PR's sweep fallback code

All previously reviewed PR logic (build/sign split, fallback iteration, M1 reconciliation fix) is untouched. Approval carries forward.

⚠️ Protocol-critical flag remains — sweep signing path requires human sign-off before merge.

[ghost]

Arkana Follow-Up Review — arkd#1101 (merge: 45b1aa8)

Focus: Base branch merge — 3 new commits from bob/arkd-multi-wallet-dial (#1099).

Changes from base branch (not this PR's code)

• FallbackWallet{Addr, Service} struct replaces bare []ports.WalletService
• Duplicate and primary-equals-fallback validation at startup
• Error messages now use host:port instead of positional index

Integration verified ✅

• fallbackWalletServices() correctly extracts Service fields for the application layer
• gRPC shutdown/readiness uses FallbackWallets() (needs Addr for error context) — correct
• appService() / adminService() wiring unchanged — still passes []ports.WalletService to application code
• No merge conflict artifacts, no logic changes to this PR's sweep fallback code

All previously reviewed PR logic (build/sign split, fallback iteration, M1 reconciliation fix) is untouched. Approval carries forward.

⚠️ Protocol-critical flag remains — sweep signing path requires human sign-off before merge.

[ghost]

Arkana Follow-Up Review — arkd#1101 (commit be5ee75)

Diff since last review (45b1aa8d..be5ee751): Pure refactor/comment cleanup. No logic changes.

Changes reviewed

1. primaryThenFallbacks inlined (sweeper.go, admin.go) — the helper was a one-liner wrapping append([]WalletService{primary}, fallbacks...). Now inlined into both signingWallets() methods. Functionally identical; removes one level of indirection. ✅
2. Test file renamed — sweep_fallback_internal_test.go → sweeper_test.go. Git reports similarity index 100%. Content-preserving rename to match package convention. ✅
3. Comment trimming (config.go:856, sweep.go:21-23,189-190, tx_builder.go:63-68, sweeper.go:715-719) — shortened verbose doc comments. No semantic change. ✅

Verdict

No protocol, security, or correctness concerns. Clean cosmetic pass addressing prior review feedback.

Approving the refactor commit. Prior reviews' requirement for human sign-off on the protocol-critical sweep-fallback logic still applies to the PR as a whole.

🤖 Arkana — automated reviewer · aggressive mode

[ghost]

🔍 Arkana Code Review — arkd#1101

Reviewer: Arkana (aggressive review, protocol-critical)
Verdict: Request changes (1 bug, 2 design concerns, protocol-critical → needs human sign-off)

---

⚠️ PROTOCOL-CRITICAL

This PR modifies sweep transaction building and signing — the mechanism that recovers expired VTXO batch outputs on-chain. Bugs here mean funds stuck forever or swept to wrong addresses. Human review required regardless of code quality.

---

🐛 BUG — Return-value order swap on BuildSweepTx is a silent foot-gun

internal/core/ports/tx_builder.go:63 (old) → new interface

// OLD: BuildSweepTx(inputs []TxInput) (txid string, signedSweepTx string, err error)
// NEW: BuildSweepTx(inputs []TxInput) (unsignedTx string, txid string, err error)

The first two returns swapped AND the semantics changed (signed → unsigned). Both are string, so the Go compiler will not catch any caller that destructures in the old order. All callers in this PR are updated correctly — I verified each one:

• sweeper.go broadcast path: sweepTxId, sweepTx, err = buildAndSignSweepTx(...) ✓ (buildAndSignSweepTx returns txid, signed)
• sweeper.go graceful-degradation fallback: sweepTx, sweepTxId, err = s.builder.BuildSweepTx(...) ✓ (reversed order matches new unsignedTx, txid)
• admin.go Sweep: txid, txhex, err = buildAndSignSweepTx(...) ✓
• sweeper.go checkpoint: _, sweepTx, err := buildAndSignSweepTx(...) ✓

No external consumers exist (confirmed by cross-repo search of all 140+ repos).

However: the asymmetry between buildAndSignSweepTx returning (txid, signedTx, err) and BuildSweepTx returning (unsignedTx, txid, err) is a maintenance hazard. A future developer calling BuildSweepTx and destructuring as txid, tx, err := (the natural reading) will silently get an unsigned PSBT in txid and the real txid in tx. This PR literally has both orderings in the same function (createBatchSweepTask).

Recommendation: Make the return order consistent. Either:

1. Change BuildSweepTx to return (txid, unsignedTx, err) to match buildAndSignSweepTx, or
2. Return a struct: type SweepResult struct { UnsignedTx, Txid string } — makes it impossible to confuse.

At minimum, if keeping the current order, add a loud comment at the interface definition warning about the order.

---

🟡 DESIGN — Graceful degradation stores unsigned PSBT in SweepTxs

internal/core/application/sweeper.go (fallback in createBatchSweepTask)

When all outputs are already spent and no wallet can sign the rebuilt sweep, the fallback calls BuildSweepTx which now returns an unsigned PSBT base64 string. This gets stored in round.SweepTxs[txid] via round.Sweep() → BatchSwept.Tx → persisted to sqlite/postgres/badger.

Previously, SweepTxs always contained signed raw tx hex. Now it may contain unsigned PSBT base64. While I confirmed GetBatchSweepTxs (indexer) only reads the map keys (txids), the values are persisted and could be consumed by future code that assumes signed tx hex.

Not blocking, but worth documenting the invariant change, or storing an empty string for Tx when unsigned (since it's never broadcast in this path anyway).

---

🟡 DESIGN — Double BuildSweepTx in graceful-degradation path

internal/core/application/sweeper.go (fallback path)

sweepTxId, sweepTx, err = buildAndSignSweepTx(s.builder, s.signingWallets(), outputsToSweep)
if err != nil {
    // ...
    sweepTx, sweepTxId, err = s.builder.BuildSweepTx(outputsToSweep)  // builds AGAIN

buildAndSignSweepTx internally calls builder.BuildSweepTx to build the unsigned tx, then tries signing. If signing fails, the outer code calls BuildSweepTx a second time. Both calls go through buildSweepTransaction → wallet.DeriveAddresses → NBXplorer GetNewUnusedAddress.

I verified that GetNewUnusedAddress returns the next unused address (not a freshly-derived one), so the two builds should produce the same txid. Still, this is wasteful and fragile — if the address derivation behavior ever changes, the txids would diverge silently.

Recommendation: Have buildAndSignSweepTx return the unsigned tx and txid on signing failure (alongside the error), so the caller can reuse them without rebuilding. Something like:

func buildAndSignSweepTx(...) (txid, signedOrUnsigned string, signed bool, err error)

Or split the call in the fallback path: build once, then try signing, then use the already-built unsigned tx if signing fails.

---

✅ What looks good

1. Build/sign split is clean. buildSweepTransaction + signSweepTransaction is a solid separation. The tapscript index re-derivation from the PSBT (len(in.TaprootLeafScript) > 0) in signSweepTransaction is correct and eliminates index-threading through the API.

2. Fallback iteration in buildAndSignSweepTx is well-structured: build once, try each wallet, short-circuit on first success, aggregate errors with wallet index for debuggability.

3. Test coverage is solid. TestBuildAndSignSweepTx covers all branches (primary succeeds, fallback succeeds, all fail, no wallets, build error). TestSignSweepTransaction_DerivesTapscriptIndexes validates the re-derivation logic.

4. Broadcasting stays on primary wallet — correct, since BroadcastTransaction only relays raw bytes.

5. No cross-repo breakage. TxBuilder interface is internal to arkd. Confirmed no external implementors or consumers across all 140+ repos.

6. No race conditions. signingWallets() creates a fresh slice per call. Wallet list is read-only after init.

7. NewService / NewAdminService signature changes are correctly updated at all call sites (config.go and tests).

---

📋 Summary

#	Severity	File	Issue
1	🟠 Medium	ports/tx_builder.go	Return-value swap is a maintenance trap — make order consistent or use struct
2	🟡 Low	sweeper.go (fallback)	Unsigned PSBT stored in SweepTxs — document or use empty string
3	🟡 Low	sweeper.go (fallback)	Double BuildSweepTx build — reuse unsigned tx from first call
4	🔴 Required	—	Protocol-critical: human must review and approve

The architecture is sound. The build/sign split and fallback iteration are well-designed. Requesting changes for the return-order consistency issue (#1) which is a real hazard in protocol-critical code — the rest are suggestions. Once #1 is addressed and a human has reviewed, this is good to merge.

[ghost]

Arkana Follow-Up Review — arkd#1101 (commit e39ff38)

All three issues from my last review are addressed:

#	Issue	Status
1	Return-value order swap (BuildSweepTx vs buildAndSignSweepTx)	✅ Fixed — both now return (txid, tx, err)
2	Unsigned PSBT stored in SweepTxs	✅ Fixed — sweepTx = "" on signing failure
3	Double BuildSweepTx in graceful-degradation path	✅ Fixed — buildAndSignSweepTx returns txid on signing failure; no rebuild needed

Verified all four call sites (createBatchSweepTask broadcast path, already-spent reconcile path, checkpoint sweep, admin.Sweep) destructure consistently as (txid, tx, err).

The reconcile path correctly distinguishes build failure (sweepTxId == "" → fatal) from signing failure (sweepTxId != "" → log warning, continue with txid-only reconciliation). Clean.

⚠️ PROTOCOL-CRITICAL

This PR modifies sweep transaction building and signing. Human review and approval still required before merge.

🤖 Arkana — automated reviewer · aggressive mode

[arkana-ai-bot]

PROTOCOL-CRITICAL: human review required.

This PR reshapes the sweep pipeline (VTXO-tree expiration + checkpoint + admin sweep) into a build-once / try-each-wallet-in-order model and adds a "reconcile with unsigned txid only" branch that changes what is written into Round.SweepTxs. That is protocol-facing state; please get a human protocol reviewer to sign off before merging even if this comment finds nothing.

What I looked at

• internal/infrastructure/tx-builder/covenantless/sweep.go (build/sign split, tapscript index re-derivation)
• internal/infrastructure/tx-builder/covenantless/builder.go, internal/core/ports/tx_builder.go (renamed contract + new SignSweepTx)
• internal/core/application/sweeper.go (buildAndSignSweepTx, signingWallets, batch + checkpoint + already-spent branch)
• internal/core/application/admin.go, service.go, config.go (fallback wiring)
• Cross-checked consumers of BatchSwept.Tx / SweepTxs[txid] in internal/core/domain/round.go, internal/core/application/{service.go,indexer.go}, internal/infrastructure/db/service.go, internal/interface/grpc/handlers/{arkservice.go,parser.go}, and the go-sdk handleSweepTx.

Correctness of the mechanical split (destination/fee/dust from primary → stable txid; per-wallet SignSweepTx re-parses unsignedTx fresh so no state bleeds between attempts; broadcast stays on the primary because it's just relaying bytes) looks right to me. Comments below are the things a human protocol reviewer should push on.

Findings

1. SweepTxs[txid] = "" is a new, previously-impossible state (sweeper.go:719-741)

In the "already-spent" branch, on any signing failure the code now proceeds with sweepTx = "" and calls round.Sweep(..., sweepTxId, ""). That propagates through Round.apply (internal/core/domain/round.go:311) as r.SweepTxs[e.Txid] = "", and gets emitted to subscribers via TransactionEvent.TxData.Tx = "" (internal/core/application/service.go:257-262, then parser.go:236 → arkv1.TxNotification).

Consumers I checked:

• internal/infrastructure/db/service.go:676-678 treats a batch as swept when len(sweepTxs) > 0; an empty-string entry still counts, which is the intent here. Good.
• internal/core/application/indexer.go:566 returns the raw []string from GetSweepTxs; an operator hitting GetBatchSweepTxs will now see [""] mixed with real hexes. Not a bug per se, but worth documenting on the indexer API — external tooling may not expect empty entries.
• go-sdk handleSweepTx (/srv/arkana/repos/go-sdk/wallet.go:1663) only touches SweptVtxos, so it's unaffected. I didn't grep every downstream SDK — a rust/ts consumer that reads sweep_tx.tx and assumes non-empty when txid is non-empty could break. Please double-check the SDKs before shipping.

Also: this branch relies on buildSweepTransaction(outputsToSweep) deterministically reproducing the same txid that actually landed on-chain. outputsToSweep here is the full expired set, not the unspentOutputsToSweep filter used at broadcast time, and wallet.DeriveAddresses(ctx, 1) on the primary can hand back a fresh address on every call. If either the input set or the destination address differs from what was originally broadcast, the reconciled sweepTxId is a made-up txid that does not exist on-chain. This is pre-existing behavior (old code also called BuildSweepTx(outputsToSweep)), but the PR narrows the surface where you'd notice: previously a signing error blew up the task loudly; now it silently reconciles with whatever BuildSweepTx returns. A test that pins down "we only take this branch when we can prove the derived txid matches the on-chain spend" would be worth adding.

2. Admin sweep across multiple batches now requires one wallet to hold every batch's key (admin.go:596)

adminService.Sweep builds a single sweep tx spanning all commitmentTxids passed in (line 517-580) and then does buildAndSignSweepTx(a.txBuilder, a.signingWallets(), inputs). buildAndSignSweepTx picks the first wallet that can sign the whole tx — it does not attempt per-input wallet mixing. So if an operator asks the admin API to sweep two batches signed by different LP wallets, none of the configured signing wallets can produce a fully-signed tx and the sweep fails.

The batched sweeper path is fine (each createBatchSweepTask invocation is scoped to one batch). Only the admin Sweep RPC has this cross-batch limitation. Not a regression vs. pre-PR (before, only the primary could sign at all), but worth surfacing in the admin API docs or splitting per-batch inside Sweep.

3. Tapscript index re-derivation relies on TaprootLeafScript being the sole discriminator (sweep.go:196-204)

signSweepTransaction walks ptx.Inputs and treats any input with a non-empty TaprootLeafScript as tapscript-signable. That's true for the sweep PSBTs buildSweepTransaction produces today, but the contract is now: any future edit that attaches a TaprootLeafScript to a non-signable input (e.g. an anchor, a witness-utxo-only entry) will silently pull it into the wallet's tapscript sign list. The old code carried the index list explicitly from the build site, which was harder to accidentally break. Consider a comment on SignSweepTx in ports/tx_builder.go calling out this invariant, or asserting on it inside signSweepTransaction.

4. Fee estimation is done on the primary but signing may be by a fallback (sweep.go:154-176)

EstimateFees runs on the primary before the winning signer is known. For the current sweep script (CSV multisig w/ schnorr sigs) witness sizes are deterministic per closure and don't vary between wallets, so this is safe today. If a future sweep closure gains a variable-sized witness (e.g. musig aggregation with per-signer control blocks), this assumption breaks and the primary's estimate could underpay relative to the fallback-produced witness. Worth an inline comment so nobody moves fee estimation into signSweepTransaction thinking it doesn't matter.

5. Test coverage gaps

The unit tests exercise buildAndSignSweepTx and the tapscript-index re-derivation well. What's missing:

• No test that the "already-spent, all-wallets-refuse-signing" branch actually reconciles (i.e. that Round.Sweep is called with sweepTx == "" and the event fires). This is the highest-risk new behavior; a table test in sweeper_test.go against a mocked repoManager.Events() would catch a regression.
• No test that a fallback-signed sweep tx broadcasts successfully via the primary (mocked WalletService.BroadcastTransaction). The PR notes explicitly defer real end-to-end coverage to when a second wallet is used in anger; that's fine, but at minimum a mock-level test that the primary is what gets BroadcastTransaction while a fallback is what got SignSweepTx would lock in the split.
• TestSignSweepTransaction_DerivesTapscriptIndexes uses a control block that is <leaf-version> || <32-byte internal key> with no merkle path. Consider extending it with a real multi-leaf tap tree so a future refactor that mishandles merkle-path bytes fails loudly.

6. Minor / nits

• sweeper.go:106-107 — s.scheduledTasks = make(...) is initialized again inside start, but newSweeper already sets it. Not new to this PR but the constructor change made the redundancy more visible.
• buildAndSignSweepTx doc says "A build failure returns an empty txid" — matches the code, but the "callers that broadcast must treat a non-nil error as fatal" clause is only enforced by convention; consider returning a distinct sentinel error type for the "signed-with-nobody-but-txid-is-valid" case so a caller can't accidentally read signedTx on error and try to relay an empty string.
• admin_test.go: the two callers now pass nil for walletFallbacks. Consider a settings-serialization test with a non-nil fallbacks list to make sure nothing captures the slice.

Nothing here is a hard blocker on the mechanical change, but items 1, 2, and 5 need protocol-reviewer eyes before this ships to a network that has real value at stake.

[arkana-ai-bot]

This PR has been open for 3+ days without review. @bitcoin-coder-bob is anyone looking at this?

[arkana-ai-bot]

This PR has been open for 30+ days without review. @bitcoin-coder-bob is anyone looking at this?

[arkana-ai-bot]

This PR has been open for 6+ days without review. @bitcoin-coder-bob is anyone looking at this?

[arkana-ai-bot]

Review — Sweep fallback across primary and fallback arkd-wallets

⚠️ PROTOCOL-CRITICAL — changes sweep signing path. Flagging for human sign-off.

What changed:
Both and now carry . On sweep, the primary wallet is tried first; if it fails (key mismatch for a pre-rotation batch), each fallback is tried in order. Swept funds always go to the primary wallet's address.

Key question for reviewers: What exactly triggers the fallback? Is it any signing error from the primary, or specifically a key-mismatch error? If it's any error (network timeout, wallet locked, etc.), a transient primary failure would silently shift sweep signing to a fallback wallet, which might not be the intended behaviour. A locked primary should probably surface as a hard error rather than transparently falling back. Please confirm that the error discrimination is correct.

Security note: The fallback path must not allow a fallback wallet to sign batches it doesn't own. The README says "a batch created before this wallet became the primary" — so ownership is determined by the signing key in the batch's tapscript. If the fallback-wallet filter is the same "does my key appear in the leaf" check from #1121, this is sound; if it's more permissive, it could let a fallback wallet sign arbitrary batches. Please confirm.

Overall structure looks correct. Pending confirmation of the two points above.

[arkana-ai-bot]

This PR (sweep fallback across wallets) has been open 74 days without review. @bitcoin-coder-bob is anyone looking at this stack (#1098-#1101)?

[arkana-ai-bot]

This PR has been open for 74 days without review. @bitcoin-coder-bob — is anyone looking at this?

[arkana-ai-bot]

PROTOCOL-CRITICAL: human review required.

Follow-up pass — e39ff38 → a7588bb. This diff wires walletFallbacks into adminService, adds buildAndSignSweepTx/signingWallets() to both services, and ships new unit tests for both. Prior-issue scorecard: M1 Fixed, O1 Fixed, O2 Still open, O3 Still open. No new blocking findings.

---

Prior findings — closure

✅ M1 — All-spent reconciliation path (Fixed in e39ff38)

The fix landed in the base of this diff. In sweeper.go at the all-spent branch (≈line 735), the code calls buildAndSignSweepTx and discriminates two failure modes:

• sweepTxId == "" → build itself failed, nothing to reconcile with → return err (fatal).
• sweepTxId != "" with err != nil → signing failed but the txid is stable → logs a warning, sets sweepTx = "", continues with txid-only reconciliation.

The buildAndSignSweepTx doc comment spells this contract out explicitly ("Callers that broadcast must treat a non-nil error as fatal"). Implementation matches comment. M1 is closed.

✅ O1 — Positional struct literal in newSweeper (Fixed in this diff)

newSweeper now uses named fields throughout. All 8 fields are named. Confirmed in the diff at internal/core/application/sweeper.go.

🟡 O2 — signingWallets() duplication (Still open)

The new diff adds a second identical method on adminService:

func (a *adminService) signingWallets() []ports.WalletService {
    return append([]ports.WalletService{a.walletSvc}, a.walletFallbacks...)
}

sweeper.signingWallets() is byte-for-byte identical. Both are correct and tested. This remains a minor housekeeping item — not a correctness concern, but worth a follow-up consolidation.

🟡 O3 — context.Background() in SignSweepTx (Still open)

internal/infrastructure/tx-builder/covenantless/builder.go:~285 still uses context.Background() internally. Wallet RPC calls inside signSweepTransaction remain uncancellable on shutdown. Pre-existing, non-blocking.

---

Incremental diff — new findings

🟢 signSweepTransaction index re-derivation is correct

The new implementation scans ptx.Inputs[i].TaprootLeafScript to identify tapscript inputs. This is faithful to what buildSweepTransaction writes (TaprootLeafScript is set for exactly the inputs where input.TapscriptLeaf != nil). TaprootInternalKey is also preserved in the serialised PSBT and is available to SignTransactionTapscript via the full PSBT, so the signing contract is intact.

🟢 TestSignSweepTransaction_DerivesTapscriptIndexes (sweep_test.go)

The control-block construction (BaseLeafVersion || internalKeyBytes) is the correct minimal format for PSBT round-tripping. The test verifies that only inputs 1 and 2 trigger SignTransactionTapscript (not input 0), and that the tapscript-signed PSBT — not the original unsigned one — is forwarded to SignTransaction. Solid coverage for the re-derivation path.

🟢 TestBuildAndSignSweepTx (sweeper_test.go)

Covers: primary signs on first try, later fallback wins, all-wallets-fail returns aggregated error with txid preserved, zero-wallet short-circuits, and build error short-circuits before any sign call. All five cases are correct and complete.

🔍 Minor: buildAndSignSweepTx returns non-empty txid on signing error

When signing fails, buildAndSignSweepTx returns (txid, "", signingError) — an unusual Go convention. The admin.Sweep path (internal/core/application/admin.go:~696) does the right thing (if err != nil { return }) and stops before broadcast. The doc comment documents the intent. Risk is low, but any future caller that inspects the first return value without checking the error could silently use an unbroadcast txid. Worth a follow-up comment at the call site in admin.Sweep, something like:

// err != nil here means signing failed; txid is the build txid, never broadcast.
txid, txhex, err = buildAndSignSweepTx(a.txBuilder, a.signingWallets(), inputs)
if err != nil {
    return
}

(The inline comment is the guard, not the if. Currently no such comment exists at that call site.)

🟢 Config plumbing (config.go)

fallbackWalletServices() correctly extracts WalletService from the already-dialed FallbackWallet slice. Both appService() and adminService() now receive the fallback slice. No issues.

---

Summary

The core sweep-fallback mechanism is implemented correctly end-to-end: build once (destination and fees from primary), sign with each wallet in turn, broadcast only on success, reconcile gracefully when all outputs are already spent. The two outstanding items (O2, O3) are housekeeping and non-blocking. No new correctness or security findings in the incremental diff.

[arkana-ai-bot]

This PR has been open for 4-5+ days without a review. @bitcoin-coder-bob is anyone looking at this? (Sweep fallback across primary and fallback arkd-wallets; 4 days without review.)

[arkana-ai-bot]

This PR has been open for 11+ weeks without a review. @bitcoin-coder-bob — any update on this?

[arkana-ai-bot]

This PR has been open for 83+ days without a review. @bitcoin-coder-bob — sweep fallback across wallets: same question, is this stacked and waiting on #1099?

[arkana-ai-bot]

This PR has been open for 3+ months without a review. @bitcoin-coder-bob is anyone looking at this? Let us know if it needs a rebase or if there's a blocker.

[arkana-ai-bot]

This PR has been open for 86+ days without review. @bitcoin-coder-bob is the sweep fallback work (primary + fallback arkd-wallets) still active?

[arkana-ai-bot]

This PR has been open for 87+ days without review. @bitcoin-coder-bob is anyone looking at this?