contractcourt: fix HTLC success resolver phantom-input bug (#10840)#10869
contractcourt: fix HTLC success resolver phantom-input bug (#10840)#10869yyforyongyu wants to merge 6 commits into
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 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
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
|
github-actions
Bot
added
the
severity-critical
🔴 PR Severity: CRITICAL
🔴 Critical (2 files)
🟢 Low (1 file)
AnalysisThis PR modifies two files in the 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 ( To override, add a |
![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 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.
| testHtlcSuccess(t, twoStageResolution, checkpoints) | ||
| } | ||
|
|
||
| func TestHtlcSuccessResolverRejectsForeignSpend(t *testing.T) { |
There was a problem hiding this comment.
all the methods in this commit are missing godoc, plus the superficial AAA comments - we should explain arrange what, act how, assert what
| ctx.waitForResult(false) | ||
| } | ||
|
|
||
| func TestHtlcIncomingResolverLaunchSkipsPreimageAfterExpiry(t *testing.T) { |
There was a problem hiding this comment.
same here, missing godocs
| the close transaction is actually broadcast, and | ||
| `WaitingCloseChannel.ClosingTx` is never empty. | ||
|
|
||
| * [Fixed an issue](https://github.com/lightningnetwork/lnd/issues/10840) |
There was a problem hiding this comment.
this should be in release 0.21.1
Make the existing two-stage success resolver test use an output that matches the stored sweep descriptor. This keeps the fixture consistent with the resolver invariant enforced by the following commit.
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 launch-time invoice-registry lookup passed currentHeight=0, bypassing registry-side expiry checks for immediate resolutions. Fetch the current best height in Launch and pass it into the immediate registry lookup. Known preimages can still launch the success resolver after expiry so direct preimage sweeps are not suppressed on restart.
Add coverage for Launch still using an already-known preimage after HTLC expiry, matching restart behavior where the preimage was learned before shutdown. Also assert launch-time registry lookups use the current chain height instead of zero.
Document the user-visible fix for issue lightningnetwork#10840 in the v0.21.1 release notes. The note covers both the foreign-spend validation and the launch-time registry height fix.
yyforyongyu
force-pushed
the
fix-10840-success-resolver
branch
from
f82a83c to
0a2f9d3
Compare
2 participants
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: