Skip to content

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

Merged
ziggie1984 merged 3 commits into
v0.21.x-branchfrom
backport-11161-to-v0.21.x-branch
Sep 23, 2026
Merged

ziggie1984 merged 3 commits into
v0.21.x-branchfrom
backport-11161-to-v0.21.x-branch

Conversation

@github-actions

Copy link
Copy Markdown

Backport of #11161


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

@github-actions github-actions Bot added this to the v0.21.4 milestone Sep 22, 2026
@github-actions

Copy link
Copy Markdown
Author

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

@ziggie1984
ziggie1984 force-pushed the backport-11161-to-v0.21.x-branch branch from 328d3aa to 8f3e7a4 Compare September 22, 2026 19:05
@ziggie1984
ziggie1984 marked this pull request as ready for review September 22, 2026 19:39
@github-actions github-actions Bot added the severity-critical Requires expert review - security/consensus critical label Sep 22, 2026
@github-actions

Copy link
Copy Markdown
Author

🔴 PR Severity: CRITICAL

file classification | 14 files | 822 lines changed

🔴 Critical (1 file)
  • contractcourt/htlc_incoming_contest_resolver.go - on-chain dispute/HTLC resolution logic (contractcourt/*)
🟠 High (4 files)
  • invoices/invoiceregistry.go - invoice management and settlement logic
  • invoices/resolution_result.go - invoice resolution result handling
  • lnrpc/routerrpc/router.proto - RPC/API definition change
  • lnrpc/routerrpc/subscribe_events.go - RPC event subscription logic
🟡 Medium (1 file)
  • lnrpc/routerrpc/router.swagger.json - generated API doc reflecting the proto change
🟢 Low (8 files)
  • contractcourt/htlc_incoming_contest_resolver_test.go - test-only change
  • 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 - test-only change
  • lnrpc/routerrpc/router.pb.go - auto-generated protobuf code
  • lnrpc/routerrpc/subscribe_events_test.go - test-only change

Analysis

This PR touches contractcourt/htlc_incoming_contest_resolver.go, which falls under the on-chain dispute resolution package — the highest-severity category in this repo, requiring expert review of breach/contract-resolution logic. It also modifies invoice settlement logic (invoices/*) and the routerrpc RPC surface (proto + generated code + subscription handling), both flagged as HIGH-severity areas.

No bump was applied: excluding tests, docs, and auto-generated files, the change touches 7 files and ~293 lines, well under the 20-file/500-line bump thresholds, and only a single distinct critical package (contractcourt) is involved.


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

Keep unexpected invoice lookup errors retryable instead of
turning them into terminal HTLC failures. Resolve recorded HTLCs
before invoking the interceptor, while retaining the atomic replay
check during the invoice update.

Only turn interceptor errors for confirmed-new HTLCs into individual
failures. Update failure reporting and multi-backend regression tests
to cover the final behavior.

(cherry picked from commit 044d260)
Assert that malformed invoice payments fail only the HTLC and leave
the channel active on both peers.

(cherry picked from commit 80b930d)
Document that lookup errors remain retryable, interceptor errors fail
new HTLCs individually, and replayed HTLCs retain their recorded
outcomes.

(cherry picked from commit 0e6f2fb)
@ziggie1984
ziggie1984 force-pushed the backport-11161-to-v0.21.x-branch branch from 8f3e7a4 to c570cb1 Compare September 23, 2026 12:17

@ziggie1984 ziggie1984 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ziggie1984
ziggie1984 merged commit 9bf096e into v0.21.x-branch Sep 23, 2026
39 of 40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-changelog severity-critical Requires expert review - security/consensus critical

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant