staticaddr: dyn-conf-tracker

[hieblmi]

This PR surfaces static-address deposits as soon as they appear in the wallet, including mempool deposits, instead of waiting for the old confirmation readiness
threshold.

It also updates the static-address flows around those low-confirmation deposits:

• Track, reconcile, and list unconfirmed static-address deposits.
• Preserve logical deposit identity across RBF/replacement while hiding replaced deposits from normal listings.
• Allow static loop-ins to select low-confirmation deposits, while keeping withdrawals and channel opens restricted to confirmed inputs.
• Add CLI/RPC warnings and metadata so users can see when low-confirmation deposits may delay swap execution.
• Harden static loop-in lifecycle handling around orphan invoices, replaced inputs, failed swaps, and server confirmation-risk decisions.
• Persist confirmation-risk decisions so restart recovery preserves accepted/rejected risk state and payment deadline timing.
• Prevent notification-manager stalls by queueing blocking notification fan-out paths.
• Add regression coverage for deposit tracking, selection, replacement, recovery, notification delivery, and risk-decision persistence.

[gemini-code-assist]

Summary of Changes

Hello, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly improves the reliability and responsiveness of static-address deposits. By allowing immediate visibility of mempool deposits while enforcing strict confirmation requirements for high-risk operations like channel opens and withdrawals, the system balances user experience with security. The changes also include robust handling of deposit replacement, durable persistence of risk decisions for recovery, and comprehensive regression testing to ensure stable behavior across the deposit lifecycle.

Highlights

• Immediate Deposit Visibility: Surfaces static-address deposits as soon as they appear in the wallet, including those currently in the mempool, removing the need to wait for the old confirmation threshold.
• Enhanced Deposit Lifecycle: Introduces robust tracking and reconciliation for unconfirmed deposits, including logic to handle RBF (Replace-by-Fee) scenarios by marking replaced outpoints as 'Replaced' to keep the database clean.
• Confirmation Enforcement: Restricts sensitive operations such as withdrawals and channel opens to confirmed inputs, ensuring that only finalized funds are used for these actions.
• Risk Decision Persistence: Persists confirmation-risk decisions to the database, allowing for consistent swap execution and payment deadline timing even after service restarts.
• User Feedback: Adds CLI and RPC warnings to inform users when low-confirmation deposits might delay swap execution.

New Features

🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request introduces support for unconfirmed deposits in static address loop-ins, enabling faster swap initiation while managing confirmation risk. It includes client-side warnings for low-confirmation outputs, server-side persistence of risk decisions, and logic to handle deposits that vanish from the wallet view due to mempool replacement or reorgs. The notification manager was also enhanced to support asynchronous risk acceptance/rejection events. Feedback was provided regarding the duplication of coin selection logic between the CLI warning path and the loop-in manager, recommending a refactor into a shared utility for better maintainability.

[hieblmi]

@claude please review

[claude]

Code Review — PR #1141: staticaddr: dyn-conf-tracker

This PR is a substantial addition: it surfaces mempool deposits immediately (vs. waiting for MinConfs), adds RBF/replacement tracking, persist-and-replay for the server's confirmation-risk decision, notification manager queueing, and regression coverage across all the new flows. The overall design is sound and the test coverage is good. A few correctness issues and edge cases worth addressing before merge:

---

Bug (High) — Payment deadline reset to full duration on legacy MinConfs fallback

File: staticaddr/loopin/actions.go

The new startPaymentDeadline helper has a branching contract on its startedAt parameter: if non-zero it subtracts elapsed time; if zero it uses the full PaymentTimeoutDuration(). The risk-accepted notification path correctly passes the persisted ConfirmationRiskDecisionTime, so the remaining window is computed accurately. But the legacy MinConfs fallback call passes time.Time{}:

// Legacy path
startPaymentDeadline(
    "legacy confirmation fallback",
    time.Time{},  // <-- zero time → full timeout granted from NOW
)

This means a swap initiated 55 seconds ago with a 60-second payment window grants the server a fresh 60-second deadline from the moment deposits confirm, rather than the remaining 5 seconds. The server could use this discrepancy to delay payment well beyond the negotiated window. The fix is to pass f.loopIn.InitiationTime (or whichever timestamp records when the swap was started) so startPaymentDeadline can compute the true remaining budget.

---

Bug (Medium) — unlockDeposits failure at deadline fires silently and is unretried until htlc timeout

File: staticaddr/loopin/actions.go

When deadlineChan fires:

case <-deadlineChan:
    deadlineChan = nil   // won't fire again

    cancelInvoice()

    err = f.unlockDeposits(ctx)
    if err != nil {
        f.Errorf("unable to unlock deposits after payment deadline: %v", err)
        continue   // depositsUnlocked stays false
    }
    depositsUnlocked = true

After this continue, deadlineChan is nil and the only retry path is the !depositsUnlocked guard in the htlc-timeout branch — which only fires when the htlc actually times out on-chain. If the htlc is never published (or both unlock attempts fail), the deposits remain stuck in LoopingIn until daemon restart. Consider logging with a stronger warning and, at minimum, scheduling a bounded retry rather than relying solely on the htlc-timeout coincidence.

---

Bug (Medium) — TransitionDeposits "deposit in final state" error silently swallowed in background paths

Files: staticaddr/withdraw/manager.go, staticaddr/openchannel/manager.go

The PR adds a new final-state guard in TransitionDeposits that returns an error when a deposit is already in a final state (including the new Replaced state). The user-facing gRPC handlers propagate this correctly. However, several background goroutines only log.Errorf it:

• handleConfirmedWithdrawal — withdrawal confirmed but TransitionDeposits fails: deposits silently remain in Withdrawing state.
• openChannelPsbt on confirmation — deposits stay stuck in OpeningChannel instead of advancing to ChannelPublished.
• OpenChannel rollback path — rollback TransitionDeposits failure is only logged; deposits remain stuck in OpeningChannel rather than being returned to Deposited.

In all these cases the error is caused by a deposit being concurrently Replaced (a legitimate race this PR introduces). The user has no visibility that their deposits are stuck. Consider incrementing a metric, surfacing the state via ListStaticAddressDeposits, or adding a recovery path in reconcileDeposits.

---

Maintainability — autoSelectedWarningOutpoints duplicates server-side SelectDeposits logic

File: cmd/loop/staticaddr.go

The PR itself acknowledges this with a // KEEP IN SYNC with staticaddr/loopin.SelectDeopits comment. The duplication is structurally necessary (CLI uses RPC wire types; server uses internal types), so this isn't a blocking issue — but the "KEEP IN SYNC" pattern is fragile. If the server-side tie-break criteria change (e.g. adding a third criterion), the warning could silently diverge, showing spurious or missing confirmations warnings. Consider at least a unit test that runs both selection functions against the same simulated deposit set and asserts they agree on which deposits are selected, or document the exact shared invariants the comment refers to.

---

Good work on the test coverage — the reconciliation, replacement, recovery, and risk-decision-persistence tests are exactly what's needed here. The depositBlocksUntilExpiry helper correctly guards the unconfirmed case (returns int64(expiry) when confirmationHeight <= 0), which avoids the large-negative-number issue the old inline formula would have produced for mempool deposits.

[hieblmi]

@claude please re-review

[claude]

Code Review — PR #1141: staticaddr: dyn-conf-tracker

This PR surfaces static-address deposits from the mempool (without waiting for 6 confirmations), adds RBF/replacement tracking, introduces confirmation-risk notifications from the server, hardens the loop-in lifecycle, and persists risk decisions across restarts. It's a substantial, well-structured change. The findings below are ranked most-severe first.

---

Findings

1. deadlineChan case omits invoiceCanceledForNonPayment = true — deposit re-use window after HTLC confirms
staticaddr/loopin/actions.go — new MonitorInvoiceAndHtlcTxAction event loop

When the payment deadline fires, cancelInvoice() is called but invoiceCanceledForNonPayment is never set to true. That flag drives the htlcConfChan case:

case <-htlcConfChan:
    htlcConfirmed = true
    if invoiceCanceledForNonPayment {          // ← always false after deadlineChan fires
        transitionDepositsToHtlcTimeout(...)  // ← skipped
    }

The consequence: if the server publishes the HTLC on-chain after the deadline has already expired, deposits are left in Deposited state (they were unlocked by unlockDepositsAfterInvoiceCancel) until the HTLC-timeout block fires. In that window the deposit manager may select them for a new loop-in. When the original FSM's blockChan htlc-timeout path eventually fires and calls transitionDepositsToHtlcTimeout, the deposits may now be in LoopingIn state for the new swap — OnSweepingHtlcTimeout is not a valid transition from LoopingIn, producing an FSM error that leaves the original HTLC un-swept.

Fix: add invoiceCanceledForNonPayment = true after cancelInvoice() inside the deadlineChan case.

---

2. Legacy MinConfs fallback grants the server the full payment timeout instead of the remaining time
staticaddr/loopin/actions.go — blockChan case

startPaymentDeadline(
    "legacy confirmation fallback",
    time.Time{},        // ← zero value
)

Inside startPaymentDeadline:

timeout := f.loopIn.PaymentTimeoutDuration()
if !startedAt.IsZero() {
    timeout -= time.Since(startedAt)  // ← never executed for zero value
    ...
}

When deposits confirm 30 minutes after swap initiation with a 60-minute PaymentTimeoutSeconds, the server receives 60 minutes from the legacy-MinConfs block instead of the ~30 remaining minutes the contract specifies. The old code used RemainingPaymentTimeSeconds() which measured from InitiationTime. Fix: pass f.loopIn.InitiationTime as startedAt for the legacy path so elapsed time is always deducted.

---

3. AfterFunc in SubscribeStaticLoopInRiskAccepted/Rejected can evict a notification the FSM never consumed
notifications/manager.go — SubscribeStaticLoopInRiskAccepted / SubscribeStaticLoopInRiskRejected

context.AfterFunc(ctx, func() {
    m.removeSubscriber(NotificationTypeStaticLoopInRiskAccepted, sub)
    m.Lock()
    delete(m.staticLoopInRiskAccepted, swapHash)   // ← unconditional
    m.Unlock()
    close(notifChan)
})

Problematic sequence:

1. FSM subscribes for swap H (no notification yet in cache).
2. FSM's notificationCtx is cancelled (transient error before the for/select reads from riskAcceptedChan).
3. Server sends risk-accepted — handleStaticLoopInRiskDecision persists to DB (best-effort), caches in m.staticLoopInRiskAccepted[H], and calls dropNotification. Because sub.subCtx.Done() is already closed, dropNotification's select takes the ctx.Done() branch — notification is NOT delivered to notifChan.
4. AfterFunc fires → deletes m.staticLoopInRiskAccepted[H], the only in-memory copy.
5. If the DB persist also failed, an FSM restart finds neither a cache entry nor a persisted decision. The payment deadline never starts; the swap is stuck until the legacy MinConfs fallback fires (which itself has the zero-time bug from finding Update README.md #2).

Consider not deleting the cache entry in AfterFunc, or only deleting it after verifying the notification was actually delivered to the subscriber.

---

4. ListUnspentDeposits becomes blind to freshly-arrived mempool deposits
loopd/swapclient_server.go — ListUnspentDeposits

Old code added any confirmed UTXO that had no DB record to isUnspent via the confirmedToCheck map, guaranteeing immediate visibility. New code:

ignoreUnknownOutpoints := true   // was false
deposits, err := s.depositManager.DepositsForOutpoints(...)
for _, d := range deposits {
    if d == nil { continue }      // unknown outpoints silently skipped
    if d.IsInState(deposit.Deposited) {
        isUnspent[d.OutPoint] = struct{}{}
    }
}

A deposit that arrived between two reconcile ticks (up to PollInterval = 10 s) has no DB record yet and returns nil from DepositsForOutpoints, so it is silently excluded from the response. This contradicts the PR's stated goal of surfacing deposits "as soon as they appear in the wallet." Consider keeping the confirmedToCheck fallback, or running a reconcile synchronously before returning results.

---

5. autoSelectedWarningOutpoints re-implements SelectDeposits with a // KEEP IN SYNC comment
cmd/loop/staticaddr.go — autoSelectedWarningOutpoints

The function manually reproduces the sort/filter logic from staticaddr/loopin.SelectDeposits, acknowledged with a comment. Future changes to SelectDeposits (e.g. a new deposit state, a tie-breaking rule) will silently diverge the CLI warning, causing it to warn about different deposits than those the server will actually wait on. The existing TestWarningDepositSelectionMatchesLoopInSelection helps, but only if that test is kept in sync too. Consider factoring the pure selection logic into a shared helper that both the CLI and the FSM import.

---

6. selectedDepositConfirmationHeights allocates and acquires locks on every block after the deadline has started
staticaddr/loopin/actions.go — blockChan case

After deadlineStarted = true, legacyMinConfsReached will immediately return true on every subsequent block, but selectedDepositConfirmationHeights still runs: it allocates two maps and acquires each deposit's lock on every block for the lifetime of the swap. Gate the block-driven check on !deadlineStarted:

if !deadlineStarted && legacyMinConfsReached(...) {
    startPaymentDeadline(...)
}

---

7. riskDecisionTime closure issues two sequential DB reads in the non-persisted path
staticaddr/loopin/actions.go — riskDecisionTime closure

When the decision has not yet been persisted, the function calls GetLoopInByHash → RecordStaticAddressRiskDecision → GetLoopInByHash again only to read back ConfirmationRiskDecisionTime. That timestamp is exactly what was passed to RecordStaticAddressRiskDecision. The second read can be eliminated by having RecordStaticAddressRiskDecision return (or accept-and-echo) the stored timestamp, or by capturing time.Now() before the write and accepting the minor precision difference.

---

8. handleStaticLoopInRiskDecision does an O(N) linear scan over all risk subscribers
notifications/manager.go — handleStaticLoopInRiskDecision

for _, sub := range m.subscribers[notifType] {
    if !hasSwapHash || sub.swapHash == nil ||
        *sub.swapHash != swapHash {
        continue
    }
    notifySubscriber(sub)
}

Risk notifications are swap-scoped and there is at most one subscriber per in-flight swap. With N concurrent loop-ins, every incoming risk notification scans all N entries. The existing staticLoopInRiskAccepted / staticLoopInRiskRejected maps already key by hash; a parallel map[lntypes.Hash]subscriber for the per-swap subscription would provide O(1) dispatch.

---

Overall the approach is sound and the test coverage (reconcile, replacement, notification delivery, risk-decision persistence) is thorough. The two highest-priority fixes are the missing invoiceCanceledForNonPayment = true (finding #1) and the zero startedAt in the legacy fallback (finding #2), both of which affect swap-payment timing correctness.

[hieblmi]

On the two priority items:

1. invoiceCanceledForNonPayment: this is already covered on the current branch via the shared cancellation paths. cancelInvoice() and handleRiskRejected() both set the invoice state to canceled and set invoiceCanceledForNonPayment = true, so a later HTLC confirmation moves the deposits back to SweepHtlcTimeout instead of leaving them reusable.

2. Legacy fallback startedAt: using time.Time{} is intentional. The legacy MinConfs path is the compatibility substitute for a risk-accepted notification; before that point the client is still waiting for the deposits to become acceptable. Charging the payment timeout from swap initiation would make slow-confirming deposits consume the server payment window before the server is expected to pay, which regresses the dyn-conf behavior.

[starius]

Found one related fsm.OnError issue.

Could you streamline Git history in the PR, please? I think ae7065f "cmd/loop: update static loop-in replay fixtures" can be squashed into 5cbc3a3 "cmd/loop: warn for low-confirmation static deposits".

[hieblmi]

Follow-up on the two new risk-watcher commits:

• 276ef1e adds a small confirmationRiskWatcher helper for static loop-in confirmation-risk notifications. The helper keeps the notification plumbing in one place: it subscribes to accepted/rejected notifications for a single swap, filters out nil or wrong-hash updates, normalizes both server messages into one internal update type, and restores or records the durable risk-decision timestamp through the store. The commit also adds focused watcher tests for hash filtering and timestamp restoration/replay handling.

• 156fb86 wires that helper into MonitorInvoiceAndHtlcTxAction. The intent is cleanup rather than behavior change: the FSM action still owns the swap-state side effects, invoice cancellation, payment deadline handling, HTLC monitoring, and rejection/error path. The watcher only replaces the duplicated accepted/rejected channel handling and the inline decision-time restoration code. This commit also adds an action-level regression test that wrong-hash risk notifications do not persist a decision, cancel the invoice, or exit the monitor loop, while the correct accepted notification is still handled.

High level, this keeps the blast radius small while making the monitor action easier to read. Risk notification management is isolated, but the existing loop semantics and timestamp restoration behavior remain in the same places where they matter.