invoices: contain modifier errors and bypass interception for replays

[ziggie1984]

Change Description

An error from the invoice settlement modifier currently fails the channel
link and sends a channel-level error to the peer. For example, this happens
when an Invoices.HtlcModifier client disconnects while LND is waiting for its
response. Stream errors and malformed responses take the same path. A local
application failure can therefore disable the link and may cause the peer
to force-close the channel.

This PR limits modifier errors for new HTLCs to an individual HTLC failure,
leaving the channel link active. It also resolves replayed HTLCs from their
recorded invoice state before invoking the modifier.

Behavior

• For a new HTLC, a modifier error returns
  ResultInvoiceInterceptorError without updating the invoice or canceling
  other HTLCs in its set. The underlying error is logged, and local HTLC
  event subscribers receive the new INVOICE_INTERCEPTOR_ERROR detail.
• For an accepted, settled, or canceled HTLC replay, the recorded outcome
  takes precedence and the modifier is skipped. A previously settled HTLC
  must return its preimage even if the modifier is unavailable. The replay
  check inside the atomic invoice update is retained.
• During on-chain resolution, the incoming contest resolver recognizes the
  new processing-error result and leaves the contract unresolved. A
  modifier error must not become a terminal on-chain failure decision.

The replay check is required before converting modifier errors into HTLC
failures: otherwise, an error while reprocessing an already-settled HTLC
could incorrectly replace its settlement with a failure. It also removes
the modifier as a dependency when restoring recorded HTLC outcomes.

Scope and tradeoff

This affects the invoice settlement modifier. When no client is connected,
the existing behavior already skips interception. Unexpected invoice lookup
errors still return Go errors because the HTLC's recorded state is unknown;
their recovery behavior is unchanged.

The payer receives the generic FailIncorrectDetails wire failure, while
the interceptor-specific detail remains local. An LND payer treats this as
a terminal payment failure, so a temporary modifier outage does not trigger
an automatic payment retry. This trades failure of the affected payment for
keeping the channel usable.

Validation

The complete invoice registry suite, including the new regressions, passed
locally under the race detector:

go test -race ./invoices \
  -run '^TestInvoiceRegistry$|^TestResolveReplayedHtlcSettled$|^TestHtlcModificationInterceptor$|^TestInvoiceProcessingFailResolutionResults$' \
  -count=1

The registry tests run against KV, SQLite, and PostgreSQL. Coverage includes
lookup-error recovery, accepted and settled replays, canceled replays, AMP
child-preimage recovery, and a modifier error leaving an already accepted
MPP shard unchanged. Focused on-chain exit-resolver and wire/RPC failure
mapping tests also passed under the race detector.

The strengthened processing-error tests also passed against the previous
helper-based registry implementation using a temporary Go overlay, checking
that inlining the helper preserved its behavior.

The existing wrong-payment-address integration test gains assertions that
both peers still report the channel as active after the payment fails.
That test does not exercise modifier disconnection. No end-to-end
modifier-disconnect integration test was run.

Pull Request Checklist

Testing

• Your PR passes all CI checks.
• Tests covering the positive and negative (error paths) are included.
• Bug fixes contain tests triggering the bug to prevent regressions.

Code Style and Documentation

• The change is not insubstantial.
• The change obeys the Code Documentation and Commenting guidelines, and lines wrap at 80.
• Commits follow the Ideal Git Commit Structure.
• New logging statements use an appropriate subsystem and logging level.
• No new lncli commands are introduced.
• There is a change description in the release notes.

[bhandras]

LGTM 🎉

[github-actions]

🔴 PR Severity: CRITICAL

gh pr view | 10 files | 330 lines changed

🔴 Critical (1 file)

• contractcourt/htlc_incoming_contest_resolver.go - on-chain dispute/breach resolution logic (contractcourt/*)

🟠 High (2 files)

• invoices/invoiceregistry.go - invoice settlement/resolution logic (invoices/*)
• invoices/resolution_result.go - invoice resolution result handling (invoices/*)

🟢 Low (7 files)

• contractcourt/htlc_incoming_contest_resolver_test.go - test-only change
• docs/release-notes/release-notes-0.20.5.md - release notes
• docs/release-notes/release-notes-0.21.4.md - release notes
• htlcswitch/link_test.go - test-only change
• invoices/invoiceregistry_test.go - test-only change
• invoices/resolution_result_test.go - test-only change
• itest/lnd_payment_test.go - integration test-only change

Analysis

The highest-severity file touched is contractcourt/htlc_incoming_contest_resolver.go, which is part of the on-chain contract resolution/dispute-handling code (contractcourt/*), a package that requires expert review per policy. The accompanying invoices/* changes (also touched) are HIGH severity on their own. Non-test, non-doc code changes are small (≈58 lines across 3 files), so no additional severity bump applies from file/line-count thresholds. Given the change touches incoming HTLC contest resolution combined with invoice resolution-result handling, and is already flagged for backport to both v0.20.x and v0.21.x branches, expert review of the contractcourt logic is warranted.

---

_{To override, add a severity-override-{critical,high,medium,low} label.}

Superseded by inline review

[GeorgeTsagk]

Thanks for the cleanup, the core change is sound and well covered by unit tests. Verified locally: the new unit tests pass, full invoices and contractcourt suites pass, and htlcswitch passes with -tags dev. Requesting changes for the rpcFailureResolution gap, the rest is informational. (Supersedes my earlier dismissed review, same content as inline threads.)

[GeorgeTsagk]

Blocking: these two new outcomes also need handling in rpcFailureResolution (lnrpc/routerrpc/subscribe_events.go). That switch's default returns unknown fail resolution, and the fail path emits a LinkFailEvent carrying the outcome as FailureDetail (link.go sendHTLCError -> NotifyLinkFailEvent), so any open SubscribeHtlcEvents stream terminates with that error once such an event fires (router_server.go returns the error from rpcHtlcEvent). Confirmed with a quick test calling rpcFailureResolution(ResultInvoiceLookupError): unknown fail resolution: invoice lookup failed.

[ziggie1984]

Fixed in the rewritten first commit (e7111c4f77). I added INVOICE_LOOKUP_ERROR and INVOICE_INTERCEPTOR_ERROR to the router RPC FailureDetail enum, mapped both outcomes in rpcFailureResolution, regenerated the protobuf/Swagger outputs, and added focused mapping tests. make rpc, go test ./lnrpc/routerrpc, and make lint pass.

[GeorgeTsagk]

Note this doesn't exercise the new code path: wrong payment address fails via ResultAddressMismatch inside UpdateInvoice, which was already an HTLC-level fail before this PR. Covering the new paths in an itest would require injecting DB or interceptor faults, which the itest framework can't easily do, so unit-test coverage is the practical option. The added assertions still have value as hardening of a related path.

[ziggie1984]

Agreed. This assertion hardens the related ResultAddressMismatch path; it does not exercise either new pre-update processing failure. Direct itest coverage would require DB/interceptor fault injection that the harness does not currently expose, so the invoice registry, link, and contractcourt unit tests are the intended coverage for the new paths. I will keep this assertion as related channel-stability hardening in its separate itest commit.

[GeorgeTsagk]

Discussion / non-blocking: this changes semantics on transient errors, not just link stability. A transient lookup error now definitively fails the payment (FailIncorrectDetails), whereas before the HTLC replayed after link re-establishment and could still settle once the DB recovered. Arguably the lesser evil given the old path also sent a BOLT-1 Error to the peer, but senders now see a terminal-looking failure for a possibly transient local problem.

[ziggie1984]

Agreed. This is an intentional tradeoff. Previously the error tore down the link and left the HTLC eligible for replay after reconnection; now the link stays active, but FailIncorrectDetails makes the sender stop the current payment attempt even when the local fault was transient. We prefer containing an unexpected processing fault to the affected HTLC rather than disconnecting the peer. The PR description calls out this before/after behavior explicitly, so I do not plan a code change for this thread.

[GeorgeTsagk]

Minor: the returned error drops the underlying DB/interceptor error and only carries the outcome. It's logged at Error level in the registry with invoice ref and circuit key, so it's recoverable, but wrapping it here would make contractcourt logs self-contained.

[ziggie1984]

Agreed that the contractcourt error is less self-contained. The registry logs the original cause together with the invoice reference and circuit key before returning the resolution. Carrying that cause further would require extending HtlcFailResolution (which currently contains only semantic resolution metadata) or introducing another resolution type. Since the cause is already retained in the daemon logs and this does not affect resolver correctness, I would avoid that extra plumbing in this PR and treat self-contained contractcourt errors as possible follow-up hardening.

[GeorgeTsagk]

LGTM

[yyforyongyu]

i think this needs to distinguish a new HTLC from a replay before returning a fail resolution.

• the first pass may already have recorded this circuit as accepted or settled while the channel response is not locked in yet.
• these lookup/interceptor branches return before resolveReplayedHtlc, so the replay can get a different outcome.
• in the settled case, the invoice can remain SETTLED while the payer receives FailIncorrectDetails and gets the HTLC back.

can we resolve a known replay first, and keep or retry the HTLC when lookup fails and we cannot read its recorded state?

[yyforyongyu]

can we run this through the existing invoice DB matrix instead of creating a KV-only factory here?

• TestInvoiceRegistry already owns the KV, sqlite, and postgres setup.
• these cases should also cover accepted and settled replays, since that is where returning an immediate failure becomes unsafe.

that would keep the regression tied to the actual affected backends.

[litbot-9000]

@ziggie1984, remember to re-request review from reviewers when ready

[ziggie1984]

I am proposing closing this cleanup, it just is too less of a code improvement which justifies this code change.

[ziggie1984]

To help decide whether this PR is worth keeping, these are the concrete behavior changes relative to the existing implementation. This concerns the invoice settlement modifier (Invoices.HtlcModifier).

Situation	Existing behavior	With this PR
Modifier errors while processing a new HTLC	The error propagates to the link, which fails and sends a channel-level “internal error” to the peer.	Fail the affected HTLC and leave the channel link active.
An accepted, settled, or canceled HTLC is replayed	Invoke the modifier again before checking the recorded outcome. A modifier error or stalled response can prevent recovery.	Check the recorded outcome first and skip the modifier.
Local failure reporting	The modifier error follows the internal link-error path.	Log the underlying error and expose a distinct INVOICE_INTERCEPTOR_ERROR detail to local HTLC-event subscribers.

For example, restarting the modifier application while it is handling a payment currently risks disabling the link. With this PR, that new HTLC fails while the channel stays usable. Avoiding a channel-level error also avoids giving the peer that particular reason to force-close; a peer force-close is a possible consequence of the old behavior, not a guaranteed one.

The replay distinction is important: the existing software already preserves recorded outcomes once it reaches the replay check. This PR moves that check ahead of the external modifier. A previously settled HTLC can therefore return its recorded preimage without depending on another modifier response.

Some behavior is unchanged:

• If no modifier is connected when interception is checked, processing already bypasses it.
• Unexpected invoice lookup errors still return Go errors because the recorded HTLC state is unknown.
• Modifier processing errors during on-chain resolution still leave the contract unresolved. The added resolver branch preserves this behavior after introducing the new failure result.

The early replay check and on-chain exception are necessary safeguards for converting modifier errors into HTLC failures. An already-settled replay must not be failed, and an on-chain processing error must not be treated as a terminal invoice decision.

Tradeoff: the payer receives generic FailIncorrectDetails, which an LND sender treats as a terminal payment failure. A temporary modifier outage therefore fails the payment rather than requesting an automatic retry. The error does not update the invoice or cancel already accepted sibling HTLCs.

Assessment: this is worthwhile, narrowly scoped reliability hardening for deployments using the modifier. Its justification is limiting the consequences of a local application failure. We have not established its production frequency, so the case for keeping it should rest on whether that failure isolation and reduced replay dependency justify the added branches and tests.

Regression checks found no introduced production regression. The registry race suite passed on KV, SQLite, and PostgreSQL, including canceled/AMP replays and preservation of an accepted MPP shard when another shard encounters a modifier error. The strengthened processing-error tests also passed against the pre-cleanup helper-based implementation. The remaining validation gap is an end-to-end modifier-disconnect test; the wrong-payment-address integration test does not exercise that scenario.

[gijswijs]

The old code returned a Go error, which propagated up and tore down the channel link. The new code instead returns a fail resolution with a new outcome, ResultInvoiceInterceptorError, meaning "fail just this one HTLC, channel stays up."

But "whenever the modifier errors, fail the HTLC" is not safe as a blanket rule. There are two situations where producing a fail resolution would be actively wrong. Those are the two safeguards:

1. The early replay check: once an HTLC has a recorded outcome, the registry may never decide differently the second time
2. the on-chain exception: ResultInvoiceInterceptorError isn't a decision about the invoice at all, it's a local processing failure. The modifier might come back; the HTLC might be perfectly settleable. Treating "our own modifier crashed" as "this contract is definitively failed" could cause the resolver to give up on claiming funds it's entitled to. So the exception is needed for proper on-chain handling.

Both carve-outs are correct. The PR is sound.

But there is a tradeoff:

1. Under the old behavior, the channel-level internal error was at least a retryable-class failure from the sender's perspective: its router would penalize that channel and try another route. So in a setup where the payer has alternative paths to the payee, the old behavior could yield an eventual payment success (at the cost of channel disruption), while the new behavior yields a definitive failure with the channel intact. The PR consciously shifts the cost of a modifier outage from the receiver's channel health onto the payer's payment success rate.
2. MPP partial-failure lockup: "The error does not update the invoice or cancel already accepted sibling HTLCs" means: for a multi-part payment, if one shard hits the modifier error, only that shard fails. The already-accepted siblings stay locked until the MPP timeout fires. The payer's funds in those shards are stuck for that window, and the invoice sits in a half-accepted state. (Ironically, the old channel-teardown behavior released in-flight HTLCs faster.) It's a bounded, self-healing annoyance, not a correctness issue.
3. Beyond being generic, the failure reason is wrong: wallets and automated systems see "incorrect payment details," which they may key logic off: no-retry policies, refund flows, alerting, user messaging. Nothing in the failure says "transient, try again." The sender's software has no way to distinguish this from an actually-broken invoice.

Bottom line: The tradeoff is "a rare, transient receiver-side outage terminally fails in-flight payments instead of being retryable, and MPP siblings linger until timeout". Weighed against "the same outage currently takes down the whole channel and may trigger a peer force-close."

My call on this, is to merge. (After rebase)

[gijswijs]

LGTM 🚢

[ziggie1984]

@yyforyongyu CI passed PTAL

[yyforyongyu]

Pending a rebase, otherwise can be merged!

[github-actions]

Created backport PR for v0.20.x-branch:

• [v0.20.x-branch] Backport #11161: invoices: contain modifier errors and bypass interception for replays #11224 with remaining conflicts!

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin backport-11161-to-v0.20.x-branch
git worktree add --checkout .worktree/backport-11161-to-v0.20.x-branch backport-11161-to-v0.20.x-branch
cd .worktree/backport-11161-to-v0.20.x-branch
git reset --hard HEAD^
git cherry-pick -x 044d260bb6b4fb93d52b966376b0a7e0645dac6b 80b930ded9cea479befeb2b2e136e45c4a9a92f3 0e6f2fbd947b0da5ab95aa009a09fa62c1c40193
git push --force-with-lease

[github-actions]

Created backport PR for v0.21.x-branch:

• [v0.21.x-branch] Backport #11161: invoices: contain modifier errors and bypass interception for replays #11225 with remaining conflicts!

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin backport-11161-to-v0.21.x-branch
git worktree add --checkout .worktree/backport-11161-to-v0.21.x-branch backport-11161-to-v0.21.x-branch
cd .worktree/backport-11161-to-v0.21.x-branch
git reset --hard HEAD^
git cherry-pick -x 0e6f2fbd947b0da5ab95aa009a09fa62c1c40193
git push --force-with-lease