Skip to content

contractcourt: fix HTLC success resolver phantom-input bug (#10840)#10869

Open
yyforyongyu wants to merge 6 commits into
lightningnetwork:masterfrom
yyforyongyu:fix-10840-success-resolver
Open

contractcourt: fix HTLC success resolver phantom-input bug (#10840)#10869
yyforyongyu wants to merge 6 commits into
lightningnetwork:masterfrom
yyforyongyu:fix-10840-success-resolver

Conversation

@yyforyongyu

Copy link
Copy Markdown
Member

Fixes #10840.

This fixes the HTLC success resolver path that could treat any spend of the original HTLC output as our own second-level HTLC success transaction. If a remote timeout spend is observed instead, the resolver now validates the spender output against the stored sweep descriptor and fails the final HTLC outcome instead of handing a phantom output to the sweeper.

The incoming contest resolver also avoids launching the success resolver after CLTV expiry and passes the current height into the immediate invoice-registry lookup.

Companion sweeper blast-radius fix: #10842.

Testing:

  • GOWORK=off go test ./contractcourt
  • git diff --check master..HEAD
  • make fmt in a clean temporary worktree
  • make lint in a clean temporary worktree
  • GOWORK=off go test ./contractcourt -run '^$' at each commit

@gemini-code-assist

Copy link
Copy Markdown

Summary of Changes

Hello, 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 addresses a bug in the HTLC success resolver where foreign spends of commitment HTLC outputs were incorrectly treated as valid success transactions. By implementing stricter validation of the spending transaction's output and ensuring that expired HTLCs do not trigger the success resolution path, the changes improve the robustness of the contract resolution process and prevent unnecessary sweeper activity.

Highlights

  • HTLC Success Resolver Validation: Added validation to the HTLC success resolver to ensure that the spending transaction of an HTLC output matches the expected second-level success output, preventing phantom inputs from being passed to the sweeper.
  • Expiry Handling: Updated the incoming contest resolver to skip launching the success path for HTLCs that have already reached their CLTV expiry.
  • Test Coverage: Introduced new unit tests to verify that the resolver correctly rejects foreign spends and handles expired HTLCs appropriately.
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.

@yyforyongyu yyforyongyu marked this pull request as draft June 1, 2026 20:46
@github-actions github-actions Bot added the severity-critical Requires expert review - security/consensus critical label Jun 1, 2026
@github-actions

github-actions Bot commented Jun 1, 2026

Copy link
Copy Markdown

🔴 PR Severity: CRITICAL

Automated classification | 3 files | 139 lines changed

🔴 Critical (2 files)
  • contractcourt/htlc_incoming_contest_resolver.go - on-chain HTLC incoming contest resolution logic
  • contractcourt/htlc_success_resolver.go - on-chain HTLC success resolver logic
🟢 Low (1 file)
  • docs/release-notes/release-notes-0.22.0.md - release notes documentation

Analysis

This PR modifies two files in the contractcourt package — htlc_incoming_contest_resolver.go and htlc_success_resolver.go — which implement on-chain dispute resolution for HTLCs. These resolvers manage critical state machine transitions for fund recovery during channel force-closes, making them among the highest-risk code paths in the daemon. Changes here require expert review to ensure correct handling of on-chain settlement and to avoid any potential loss of funds.

Severity bump check: 3 non-test/non-generated files (threshold: >20), 139 lines changed (threshold: >500), single distinct critical package — no bump triggered.

Three test/mock files (*_test.go, mock_*.go) were excluded from file and line counts per classification rules.


To override, add a severity-override-{critical,high,medium,low} label.
<!-- pr-severity-bot -->

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request addresses an issue where an incoming HTLC resolver could incorrectly treat a foreign spend of a commitment HTLC output as its own success transaction. It introduces validation of the spending transaction's expected output before sweeping, and checkpoints the resolver as failed (timeout) if a foreign spend is detected. Additionally, the resolver now avoids launching the success path for incoming HTLCs that are already expired by checking the best block height at launch. Comprehensive unit tests have been added to verify these behaviors. No review comments were provided, so there is no additional feedback.

@saubyk saubyk added this to the v0.21.1 milestone Jun 1, 2026
@saubyk saubyk added this to v0.21 Jun 1, 2026
@saubyk saubyk moved this to In progress in v0.21 Jun 1, 2026
Comment thread contractcourt/htlc_success_resolver_test.go
Comment thread contractcourt/htlc_incoming_contest_resolver_test.go Outdated
Comment thread docs/release-notes/release-notes-0.22.0.md Outdated
@yyforyongyu yyforyongyu force-pushed the fix-10840-success-resolver branch from f82a83c to 0a2f9d3 Compare June 2, 2026 14:32
Comment thread contractcourt/htlc_success_resolver.go
Comment thread contractcourt/htlc_success_resolver.go
Comment thread contractcourt/htlc_success_resolver.go Outdated
Comment thread contractcourt/htlc_success_resolver.go
@yyforyongyu yyforyongyu force-pushed the fix-10840-success-resolver branch from 0a2f9d3 to 61fb9b4 Compare June 8, 2026 18:15
Comment thread contractcourt/htlc_incoming_contest_resolver.go Outdated
Comment thread contractcourt/htlc_success_resolver.go Outdated
Comment thread contractcourt/htlc_success_resolver.go
Comment thread contractcourt/htlc_success_resolver.go Outdated
Comment thread contractcourt/htlc_success_resolver.go Outdated
Comment thread contractcourt/htlc_success_resolver.go Outdated
Comment thread contractcourt/htlc_success_resolver.go Outdated
Comment thread contractcourt/htlc_success_resolver.go Outdated
@yyforyongyu yyforyongyu force-pushed the fix-10840-success-resolver branch from 61fb9b4 to 70db302 Compare June 10, 2026 06:58
Comment thread contractcourt/htlc_success_resolver.go
@yyforyongyu

Copy link
Copy Markdown
Member Author

LGTM 🎯 (claude-opus-4-8[1m])

@yyforyongyu yyforyongyu force-pushed the fix-10840-success-resolver branch from 70db302 to 2c8fa1c Compare June 10, 2026 08:34
Comment thread contractcourt/channel_arbitrator.go Outdated
Comment thread contractcourt/htlc_success_resolver.go Outdated
@yyforyongyu

Copy link
Copy Markdown
Member Author

LGTM 🚀 (claude-opus-4-8[1m])

@yyforyongyu yyforyongyu force-pushed the fix-10840-success-resolver branch from 2c8fa1c to f3cad96 Compare June 10, 2026 09:08
Comment thread contractcourt/htlc_success_resolver.go Outdated
@yyforyongyu yyforyongyu force-pushed the fix-10840-success-resolver branch from f3cad96 to 7b2bf4b Compare June 10, 2026 10:47
Comment thread contractcourt/channel_arbitrator.go Outdated
Comment thread contractcourt/htlc_incoming_contest_resolver.go Outdated
@yyforyongyu yyforyongyu force-pushed the fix-10840-success-resolver branch from 7b2bf4b to eee8ed9 Compare June 10, 2026 11:39
Comment thread docs/release-notes/release-notes-0.21.1.md Outdated
Comment thread contractcourt/channel_arbitrator.go Outdated
@yyforyongyu yyforyongyu force-pushed the fix-10840-success-resolver branch from eee8ed9 to b6dc107 Compare June 10, 2026 12:19
@yyforyongyu yyforyongyu force-pushed the fix-10840-success-resolver branch from ddf4488 to ecfcc5e Compare June 23, 2026 17:16
yyforyongyu added a commit to yyforyongyu/lnd that referenced this pull request Jun 23, 2026
Document the user-visible fix from PR lightningnetwork#10869 in the v0.21.1 release
notes. The note covers both the foreign-spend validation and the expired
incoming HTLC launch guard.
@yyforyongyu yyforyongyu force-pushed the fix-10840-success-resolver branch from ecfcc5e to e931cf6 Compare June 23, 2026 17:26
@yyforyongyu

Copy link
Copy Markdown
Member Author

/gateway review

Comment thread contractcourt/htlc_success_resolver.go
@lightninglabs-gateway

Copy link
Copy Markdown

gateway is already active on this PR. Reply with /gateway re-review to refresh against current HEAD.

Comment thread contractcourt/htlc_success_resolver.go Outdated
Comment thread contractcourt/htlc_incoming_contest_resolver.go Outdated
Comment thread contractcourt/htlc_success_resolver.go
Comment thread docs/release-notes/release-notes-0.21.1.md Outdated
@yyforyongyu

Copy link
Copy Markdown
Member Author

No issues found by openai/gpt-5.5 ✅

yyforyongyu added a commit to yyforyongyu/lnd that referenced this pull request Jun 23, 2026
Document the user-visible fix from PR lightningnetwork#10869 in the v0.21.1 release
notes. The note covers both the foreign-spend validation and the expired
incoming HTLC launch guard.
@yyforyongyu yyforyongyu force-pushed the fix-10840-success-resolver branch from e931cf6 to 7a39941 Compare June 23, 2026 18:02
Comment thread contractcourt/htlc_incoming_contest_resolver.go Outdated
yyforyongyu added a commit to yyforyongyu/lnd that referenced this pull request Jun 23, 2026
Document the user-visible fix from PR lightningnetwork#10869 in the v0.21.1 release
notes. The note covers both the foreign-spend validation and the expired
incoming HTLC launch guard.
@yyforyongyu yyforyongyu force-pushed the fix-10840-success-resolver branch from 7a39941 to 0439a8e Compare June 23, 2026 18:12
Comment thread contractcourt/htlc_incoming_contest_resolver.go Outdated
yyforyongyu added a commit to yyforyongyu/lnd that referenced this pull request Jun 23, 2026
Document the user-visible fix from PR lightningnetwork#10869 in the v0.21.1 release
notes. The note covers both the foreign-spend validation and the expired
incoming HTLC launch guard.
@yyforyongyu yyforyongyu force-pushed the fix-10840-success-resolver branch from 0439a8e to b2546b3 Compare June 23, 2026 18:42
Comment thread contractcourt/htlc_incoming_contest_resolver.go Outdated
Comment thread contractcourt/contract_resolver.go Outdated
Comment thread contractcourt/htlc_incoming_contest_resolver.go Outdated
The success resolver previously treated any spend of the original HTLC
outpoint as confirmation of our own second-level success transaction.
That allowed a remote timeout reclaim to be misclassified as a phantom
second-level output offered to the sweeper.

Validate the spending transaction output against SweepSignDesc.Output
before proceeding. If the output does not match, checkpoint the resolver
as failed instead of registering the derived outpoint.
Add regression coverage for both success-resolver paths that can observe
a foreign spend: the initial resolve path and the restart path after
output incubation. The tests assert that no phantom second-level output
is handed to the sweeper and that the final HTLC outcome is failed.
The incoming contest resolver could launch a fresh success path based on
preimage availability after the HTLC CLTV had expired. Its immediate
invoice-registry lookup also passed currentHeight=0, bypassing
registry-side expiry checks.

Fetch the current best height in Launch and pass the real height into
the immediate registry lookup. After lookup, skip late fresh settlements
but still allow already-known or replayed preimages to continue with the
success resolver.
Add coverage for Launch after an incoming HTLC has expired. The tests
assert that already-known or replayed preimages can still launch the
success path, while fresh invoice settlements remain blocked after
expiry.

Also assert that non-expired launch-time registry lookups use the
current chain height instead of zero.
Document the user-visible fix from PR lightningnetwork#10869 in the v0.21.1 release
notes. The note covers both the foreign-spend validation and the expired
incoming HTLC launch guard.
@yyforyongyu yyforyongyu force-pushed the fix-10840-success-resolver branch from b2546b3 to e262e56 Compare June 23, 2026 20:37
@yyforyongyu

Copy link
Copy Markdown
Member Author

No issues found by claude-opus-4-8[1m] 🛰️

@yyforyongyu

Copy link
Copy Markdown
Member Author

No issues found by openai/gpt-5.5 ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gateway-active severity-critical Requires expert review - security/consensus critical

Projects

Status: In progress

Development

Successfully merging this pull request may close these issues.

[bug]: sweeper batched 4 force-close outputs into invalid tx, then removed all inputs as State=Fatal

2 participants