staticaddr: dyn-conf-tracker#1141
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! 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
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 AssistThe 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
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 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
|
![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 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
force-pushed
the
dyn-conf-tracker-staging
branch
4 times, most recently
from
7bee626 to
ec42d48
Compare
hieblmi
force-pushed
the
dyn-conf-tracker-staging
branch
2 times, most recently
from
f8f474e to
d424169
Compare
hieblmi
force-pushed
the
dyn-conf-tracker-staging
branch
from
d424169 to
d630c2e
Compare
hieblmi
force-pushed
the
dyn-conf-tracker-staging
branch
4 times, most recently
from
49241e1 to
f6caba3
Compare
hieblmi
force-pushed
the
dyn-conf-tracker-staging
branch
4 times, most recently
from
8304783 to
da5253e
Compare
|
@claude please review |
Code Review — PR #1141: staticaddr: dyn-conf-trackerThis 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 fallbackFile: The new // 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 Bug (Medium) —
|
hieblmi
force-pushed
the
dyn-conf-tracker-staging
branch
from
da5253e to
80a5b44
Compare
|
@claude please re-review |
Code Review — PR #1141: staticaddr: dyn-conf-trackerThis 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. Findings1. When the payment deadline fires, 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 Fix: add 2. Legacy MinConfs fallback grants the server the full payment timeout instead of the remaining time startPaymentDeadline(
"legacy confirmation fallback",
time.Time{}, // ← zero value
)Inside 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 3. context.AfterFunc(ctx, func() {
m.removeSubscriber(NotificationTypeStaticLoopInRiskAccepted, sub)
m.Lock()
delete(m.staticLoopInRiskAccepted, swapHash) // ← unconditional
m.Unlock()
close(notifChan)
})Problematic sequence:
Consider not deleting the cache entry in AfterFunc, or only deleting it after verifying the notification was actually delivered to the subscriber. 4. Old code added any confirmed UTXO that had no DB record to 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 5. The function manually reproduces the sort/filter logic from 6. After if !deadlineStarted && legacyMinConfsReached(...) {
startPaymentDeadline(...)
}7. When the decision has not yet been persisted, the function calls 8. 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 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 |
|
On the two priority items:
|
hieblmi
force-pushed
the
dyn-conf-tracker-staging
branch
3 times, most recently
from
a56abf3 to
f447cae
Compare
hieblmi
force-pushed
the
dyn-conf-tracker-staging
branch
2 times, most recently
from
d1ae51d to
dd27c88
Compare
|
Follow-up on the two new risk-watcher commits:
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. |
hieblmi
force-pushed
the
dyn-conf-tracker-staging
branch
2 times, most recently
from
2ab869d to
5b29bb1
Compare
starius
left a comment
starius
left a comment
There was a problem hiding this comment.
I propose to change the loopIn struct so DepositOutpoints and Deposits are the same set (always the originally selected deposits). See my full comment below.
| deposit.Lock() | ||
| previousConfirmationHeight := deposit.ConfirmationHeight | ||
|
|
||
| confirmationHeight, err := confirmationHeightForUtxo( | ||
| currentHeight, utxo, | ||
| ) | ||
| if err != nil { | ||
| deposit.Unlock() | ||
| return err | ||
| } | ||
|
|
||
| if deposit.ConfirmationHeight == confirmationHeight { | ||
| deposit.Unlock() | ||
| continue | ||
| } | ||
|
|
||
| deposit.ConfirmationHeight = confirmationHeight | ||
|
|
||
| err = m.cfg.Store.UpdateDeposit(ctx, deposit) | ||
| if err != nil { | ||
| deposit.ConfirmationHeight = previousConfirmationHeight | ||
| deposit.Unlock() | ||
| return err | ||
| } | ||
|
|
||
| deposit.Unlock() |
There was a problem hiding this comment.
Manual Unlock is a footgun. If we add more continue, break or return statements in the future, this will result in a deadlock.
Let's rewrite in using a closure:
err := func() error {
deposit.Lock()
defer deposit.Unlock()
...
}()
if err != nil {
return err
}There was a problem hiding this comment.
Addressed: the deposit lock scope now uses an immediately invoked closure with defer deposit.Unlock(), while preserving the confirmation-height rollback and error behavior.
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func TestConfirmationHeightForUtxo(t *testing.T) { |
There was a problem hiding this comment.
Addressed: added a Godoc comment for TestConfirmationHeightForUtxo.
| error) | ||
| } | ||
|
|
||
| func (l *listUnspentOverride) ListUnspent(ctx context.Context, |
There was a problem hiding this comment.
Addressed: added a Godoc comment for listUnspentOverride.ListUnspent.
| m.mu.Lock() | ||
| toActivate := make([]*Deposit, 0, len(utxos)) | ||
| for _, utxo := range utxos { | ||
| deposit, ok := m.deposits[utxo.OutPoint] | ||
| if !ok { | ||
| continue | ||
| } | ||
|
|
||
| if _, active := m.activeDeposits[utxo.OutPoint]; active { | ||
| continue | ||
| } | ||
|
|
||
| if !deposit.IsInState(Deposited) { | ||
| continue | ||
| } | ||
|
|
||
| toActivate = append(toActivate, deposit) | ||
| } | ||
|
|
||
| toDeactivate := make( | ||
| []deactivatedDeposit, 0, len(m.activeDeposits), | ||
| ) | ||
| for outpoint, fsm := range m.activeDeposits { | ||
| if _, ok := currentUtxos[outpoint]; ok { | ||
| continue | ||
| } | ||
|
|
||
| if fsm == nil || fsm.deposit == nil { | ||
| continue | ||
| } | ||
|
|
||
| if !fsm.deposit.IsInState(Deposited) { | ||
| continue | ||
| } | ||
|
|
||
| delete(m.activeDeposits, outpoint) | ||
| toDeactivate = append(toDeactivate, deactivatedDeposit{ | ||
| outpoint: outpoint, | ||
| fsm: fsm, | ||
| }) | ||
| } | ||
| m.mu.Unlock() |
There was a problem hiding this comment.
I propose to move to a function/closure to make Unlock call in defer.
There was a problem hiding this comment.
Addressed: the manager critical section now uses a closure with defer m.mu.Unlock().
| // missing wallet outpoint is removed from the live active set without mutating | ||
| // its historical DB state. | ||
| func TestReconcileDepositsDeactivatesVanishedUnconfirmedDeposit(t *testing.T) { | ||
| ctx := context.Background() |
There was a problem hiding this comment.
Addressed: this test now uses t.Context().
| retryTimer := time.NewTimer(monitorRetryDelay) | ||
| defer retryTimer.Stop() | ||
|
|
||
| select { | ||
| case <-retryTimer.C: | ||
| return OnRecover | ||
|
|
||
| case <-ctx.Done(): | ||
| return fsm.NoOp | ||
| } |
There was a problem hiding this comment.
Simplify:
select {
case <-time.After(monitorRetryDelay):
return OnRecover
case <-ctx.Done():
return fsm.NoOp
}There was a problem hiding this comment.
Addressed: the retry select now uses time.After(monitorRetryDelay) directly.
| _ fsm.EventContext) fsm.EventType { | ||
|
|
||
| f.cancelSwapInvoice() | ||
| _ = f.cancelSwapInvoice() |
There was a problem hiding this comment.
Should we process the error here?
| // TestMonitorInvoiceStreamErrorRecoversSettlement verifies that a dead invoice | ||
| // stream checks the latest invoice state before entering recovery. | ||
| func TestMonitorInvoiceStreamErrorRecoversSettlement(t *testing.T) { | ||
| ctx, cancel := context.WithTimeout(t.Context(), 5*time.Second) |
There was a problem hiding this comment.
Make a file level const for 5 seconds, so we can bump it when CI becomes slow.
There was a problem hiding this comment.
Addressed: added a file-level testTimeout constant and replaced the five-second test deadlines and wait with it.
| return f, depositMgr | ||
| } | ||
|
|
||
| type failingCancelInvoices struct { |
There was a problem hiding this comment.
add godocs here and below
There was a problem hiding this comment.
Addressed: added Godoc comments for failingCancelInvoices, flakySubscribeInvoices, and firstLookupBarrier.
|
|
||
| decisionTime, ok := w.durableDecisionTime(ctx, decision) | ||
| if !ok { | ||
| return time.Now() |
There was a problem hiding this comment.
For a future cleanup, I think we should either use lnd/clock or (better) cover all such time.Now() calls with testing/synctest.
Retain static-address deposits as soon as lnd reports the UTXO, even when the output is still unconfirmed. Store the first confirmation height once the output confirms. Derive confirmation heights from the current wallet view because lnd reports confirmation counts instead of first-confirmation heights.
The first block epoch is consumed before recovered deposit FSMs exist. Replay that startup height after recovery so already-expired deposits can run expiry handling immediately after restart.
Treat lnd's wallet view as the source of spendable static-address outpoints while keeping historical deposit records in the DB. Reconcile active FSMs against the current wallet view, reactivate known deposits that reappear, and hide stale Deposited records from the visible deposit set.
Build list and summary responses from tracked deposit records instead of raw wallet UTXOs so RPC clients see the manager availability state. Split unconfirmed value from confirmed deposited value in summaries, and reject manual loop-in quotes for selected deposits that are not currently Deposited.
Deposited can now include mempool outputs for static loop-ins, but withdrawals and static channel opens still require confirmed funding inputs. Filter automatic channel-open selection to confirmed deposits and reject explicit unconfirmed selections, including withdraw-all requests that would otherwise silently include mempool deposits.
Static address deposits with no confirmation height have not started their CSV timeout yet, so keep them eligible for loop-in selection instead of treating them as already near expiry. Prefer confirmed deposits before unconfirmed ones during automatic selection, and share the remaining-lifetime calculation used by the selector.
Use the shared deposit-expiry helper when building autoloop DP candidates so unconfirmed deposits do not look like the earliest-expiring options. This keeps the no-change selector's expiry tie-break aligned with the generic loop-in deposit selection rules.
Refresh the active static-address deposit set against lnd's wallet view before quote, loop-in, withdrawal, channel-open, and autoloop selection paths. This prevents stale persisted Deposited records from being selected after replacement, reorg, or an external spend.
Use the deposit manager's visible-deposit view for normal list and summary RPCs so historical Deposited rows whose outpoints vanished from lnd's wallet view are not exposed as available funds.
Check the originally selected deposit outpoints before signing a static loop-in HTLC transaction. If any selected outpoint is no longer available, cancel the swap invoice and fail the signing action instead of producing signatures for stale inputs.
Add cached per-swap notification fanout for static loop-in confirmation-risk acceptance and rejection notifications so loop-in FSMs can subscribe by swap hash and receive decisions that arrived before subscription.
Subscribe to static loop-in confirmation-risk notifications before starting the payment deadline. Start that deadline only after server acceptance or the legacy confirmation fallback, and cancel the swap invoice when the server rejects the risk wait. Refresh selected deposits before the legacy fallback so recovered monitors use current confirmation heights.
Add schema, sqlc queries, store fields, and SqlStore support for recording server confirmation-risk decisions with static loop-in swaps. Store the decision timestamp so payment-deadline recovery can reconstruct elapsed time after restart.
Persist static loop-in confirmation-risk decisions before fanout when a persistence callback is configured. Keep unpersisted decisions cached for replay so notification delivery is not lost if the swap row is not available yet.
Create the static loop-in SQL store before the notification manager and pass a persistence callback so server confirmation-risk decisions are durably recorded before fanout.
Track whether the invoice was canceled for non-payment while monitoring the HTLC. If the HTLC never confirms before timeout, unlock the deposits; if it did confirm, transition them to the HTLC-timeout sweep state without issuing duplicate transitions.
Record replayed server risk decisions through the loop-in store, recover accepted payment-deadline timers using the persisted decision time, and handle persisted rejections on restart. This lets recovered static loop-ins keep pending confirmation-risk state instead of restarting payment timing from scratch.
Warn before dispatching a static loop-in that selects deposits below the conservative six-confirmation threshold. Mirror automatic coin selection before prompting so the warning reflects both manual and auto-selected deposits. Cover manual and auto-selected warning paths in CLI tests.
hieblmi
force-pushed
the
dyn-conf-tracker-staging
branch
from
5b29bb1 to
f338c9e
Compare
|
Also addressed the earlier history-cleanup request: the static loop-in replay fixture update is now squashed into the cmd/loop low-confirmation warning commit. |
4 participants
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: