htlcswitch: separate onchain and offchain intercpeted HTLCs cleanly#10895
htlcswitch: separate onchain and offchain intercpeted HTLCs cleanly#10895ziggie1984 wants to merge 11 commits into
Conversation
github-actions
Bot
added
the
severity-critical
PR Severity: CRITICAL
Critical (2 files):
Medium (1 file):
Low (4 files - excluded from severity bump counts):
AnalysisThis PR modifies core htlcswitch package files. Changes to held_htlc_set.go and interceptable_switch.go affect the HTLC forwarding and payment routing state machine, warranting expert review. Bump check: 4 non-test files (threshold >20), 467 lines changed (threshold >500), 1 critical package. No bump applied. To override, add a severity-override-{critical,high,medium,low} label. |
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 refactors the HTLC interception mechanism to accurately manage HTLCs that transition from off-chain forwarding to on-chain resolution, particularly in scenarios involving channel force-closures. By introducing distinct handling for off-chain and on-chain held HTLCs, the changes prevent incorrect auto-failing or misdirection of settlement attempts, thereby enhancing the robustness and reliability of HTLC management within the system. 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 refactors the heldHtlcSet to distinguish between off-chain and on-chain held HTLCs using a new heldEntry interface, ensuring proper handling of resolutions and expirations for both flows. It also updates the InterceptableSwitch and witness_beacon to support this, and adds comprehensive unit and integration tests. A critical deadlock risk was identified in witness_beacon.go where SubscribeUpdates holds a lock while making a blocking call to the interceptor, which can conflict with the switch's event loop attempting to acquire the same lock during resolution; releasing the lock early is recommended.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
ziggie1984
force-pushed
the
fix-onchain-interceptor-held-htlc
branch
2 times, most recently
from
cf2e20c to
02347b0
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the heldHtlcSet to track held HTLCs as either off-chain or on-chain entries using a new heldEntry interface. This change ensures that on-chain re-offers can replace old off-chain holds, allowing settlements to reach the witness beacon after an incoming channel force-closes. Additionally, integration tests are added to verify on-chain settlement scenarios, and minor fixes are made to the witness beacon subscription flow. The reviewer's feedback correctly points out a style guide violation where the newHeldHtlcSet function lacks a documentation comment.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
ziggie1984
force-pushed
the
fix-onchain-interceptor-held-htlc
branch
from
02347b0 to
6495243
Compare
ziggie1984
force-pushed
the
fix-onchain-interceptor-held-htlc
branch
4 times, most recently
from
5e3c3c3 to
424d1a8
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the heldHtlcSet to support both off-chain and on-chain held HTLCs via a new heldEntry interface, resolving an issue where on-chain forward interceptor settlements failed to reach the witness beacon after a channel force-close. The changes also include updates to InterceptableSwitch and witness_beacon.go, comprehensive unit and integration tests, and updated documentation. Feedback is provided to add a documentation comment to newHeldHtlcSet to align with the repository's style guide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
When an off-chain held HTLC is promoted to on-chain, the interceptor is not notified again because it already has the circuit key. A client may therefore still answer with an off-chain decision such as resume or fail. Those actions cannot be applied to the on-chain resolver, but returning an error tears down the whole interceptor stream. Log and ignore non-settle actions instead, keeping the entry available for a later settle or expiry.
On-chain held entries are replay handles for the interceptor while contractcourt waits for a preimage or on-chain expiry. Once the resolver tears down, keeping the handle until the refund timeout can replay a stale HTLC to a reconnecting interceptor. Thread a dedicated cleanup signal from the witness subscription cancel path back through the interceptable switch event loop. The held set only removes on-chain entries for that signal, leaving off-chain entries under the link flow lifecycle.
Use distinct internal deadline types for off-chain auto-fail heights and on-chain settlement deadlines instead of overloading the intercepted packet field. Project both variants back into the existing router RPC auto_fail_height field to preserve wire compatibility. Reject mismatched held HTLC deadline types in tests.
ziggie1984
force-pushed
the
fix-onchain-interceptor-held-htlc
branch
from
f6d505a to
dcc17a9
Compare
|
/gateway dismiss F2 This is documented and was chosen to not duplicate notification for current clients. Since the onchain timeout is always larger and the htlc when onchain cannot be failed anyways its ok |
|
🚫 Dismissed F2 (minor) by @ziggie1984 — no reason given Open findings on this PR: 🟠 F1 (major) · 🟡 F3 (minor) · 🟡 F4 (minor) |
|
/gateway dismiss F3 |
|
/gateway re-review |
|
🚫 Dismissed F3 (minor) by @ziggie1984 — no reason given Open findings on this PR: 🟠 F1 (major) · 🟡 F4 (minor) |
|
✅ Re-review posted: #10895 (review) Prior findings: 2 addressed, 0 still unresolved, 0 withdrawn. 🔁 Need another re-review after pushing changes? Reply with |
![lightninglabs-gateway[bot]](https://avatars.githubusercontent.com/in/3635431?s=60&v=4){kind=small}
There was a problem hiding this comment.
Both threaded prior findings are resolved: the on-chain resolve path now returns the documented ErrCannotFailOnChain/ErrCannotResumeOnChain errors instead of silently forwarding non-settle actions (F1), and the release notes now document the InterceptedPacket field change and Deadline/AutoFailHeight() migration (F4). The design is sound and the specialist sweep surfaced no new fund-loss, race, or deadlock on the settlement path — several alarming-looking candidates turned out to be intentional, documented behavior.
The one genuine blocker-to-merge is a committed Git merge-conflict artifact in the release notes. Beyond that, only minor sharp edges remain: the stream-fatal nature of the now-surfaced on-chain errors (a maintainer-discussed tradeoff whose blast radius isn't documented for clients), an implicit demotion-safety invariant in addOffChain, and a trailing-whitespace lint nit.
Findings: 🔴 0 Blocker · 🟠 1 Major · 🟡 3 Minor · 🔵 0 Nit
Status of prior findings
- F1 addressed: Resolved in
htlcswitch/held_htlc_set.go—onChainHeld.resolvenow returnsErrCannotFailOnChainforFwdActionFailandErrCannotResumeOnChainforFwdActionResume/FwdActionResumeModified, matching therouter.protocontract that on-chain HTLCs are settle-only. Covered byTestHeldHtlcSetOnChainResolve, which asserts each non-settle action returns the typed error and leaves the entry held. - F4 addressed: Resolved in
docs/release-notes/release-notes-0.21.1.md— the new bug-fix bullet now documents the breakinghtlcswitch.InterceptedPacketchange: "Go callers ... should use the newDeadlinefield to distinguish off-chain auto-fail heights from on-chain settlement deadlines, orAutoFailHeight()if they only need the legacy flattened value." This steers callers to the typedDeadlineas recommended.
| case FwdActionSettle: | ||
| return h.fwd.Settle(res.Preimage) | ||
|
|
||
| case FwdActionFail: |
There was a problem hiding this comment.
/gateway dismiss F7, we are aware of the interceptor stream error feedback capabilities right now
| // expire expires held forwards whose deadline has passed. | ||
| func (h *heldHtlcSet) expire(height uint32) []heldHtlcExpireError { | ||
| var errs []heldHtlcExpireError | ||
|
|
There was a problem hiding this comment.
/gateway dismiss F8, onchain is always the final state in case a channel goes onchain, your explicit case can not happen or only in some edge case race scenario where I think it is ok no not log anything but just ignore this call and exit early.
On-chain intercepted HTLCs can only be settled. Resume and fail actions already return concrete errors through the on-chain intercepted forward, so let those errors propagate to the interceptor client instead of converting them to success. Keep the held entry tracked on these errors so the client can reconnect and settle the HTLC later. Update the router RPC docs to describe the error behavior.
ziggie1984
force-pushed
the
fix-onchain-interceptor-held-htlc
branch
from
dcc17a9 to
b6bf686
Compare
| case FwdActionSettle: | ||
| return h.fwd.Settle(res.Preimage) | ||
|
|
||
| case FwdActionFail: |
There was a problem hiding this comment.
/gateway dismiss F7, we are aware of the interceptor stream error feedback capabilities right now
| // expire expires held forwards whose deadline has passed. | ||
| func (h *heldHtlcSet) expire(height uint32) []heldHtlcExpireError { | ||
| var errs []heldHtlcExpireError | ||
|
|
There was a problem hiding this comment.
/gateway dismiss F8, onchain is always the final state in case a channel goes onchain, your explicit case can not happen or only in some edge case race scenario where I think it is ok no not log anything but just ignore this call and exit early.
|
/gateway re-review |
|
✅ Re-review posted: #10895 (review) Prior findings: 4 addressed, 0 still unresolved, 0 withdrawn. 🔁 Need another re-review after pushing changes? Reply with |
There was a problem hiding this comment.
Re-review of the on-chain/off-chain interceptor split. All prior findings are now resolved or have been dismissed by the maintainer: the two major items (F1 on-chain non-settle rejection, F4 exported-field breaking-change note) stay addressed, and the two release-note defects (F5 Git conflict markers, F6 hard-line-break separator) are fixed in docs/release-notes/release-notes-0.21.1.md.
A fresh pass over the current diff surfaced no genuinely-new defects. The concerns the specialists raised either restate findings the maintainer already dismissed with sound reasoning (the auto_fail_height overload — F3; the stream-terminating on-chain rejection — F7; the broad addOffChain exists-guard — F8), describe the PR's intended and itest-covered design (on-chain holds surviving interceptor disconnect; type-aware expire pruning on-chain entries rather than failing them), or were already debated and settled in PR review (the deliberately unbuffered onchainInterceptDone channel).
Status of prior findings
- F1 addressed: Resolved —
onChainHeld.resolveinhtlcswitch/held_htlc_set.gonow returnsErrCannotFailOnChainforFailandErrCannotResumeOnChainforResume/ResumeModified, matching theForwardHtlcInterceptResponsecontract documented inrouter.proto. Non-settle actions no longer silently forward to a dead link. - F4 addressed: Resolved — the bug-fix bullet in
docs/release-notes/release-notes-0.21.1.mdnow documents thehtlcswitch.InterceptedPacketchange for Go callers, directing them to the newDeadlinefield to distinguish off-chain auto-fail heights from on-chain settlement deadlines, orAutoFailHeight()for the legacy flattened value. - F5 addressed: Resolved — the Deprecations section of
docs/release-notes/release-notes-0.21.1.mdno longer contains the<<<<<<</=======/>>>>>>>Git conflict markers; both### ⚠️ Warningdeprecation entries render cleanly. - F6 addressed: Resolved — the separator between the tor bullet and the new interceptor bullet is now a genuine empty line rather than a two-space hard line break.
|
/gateway approve |
|
❌ Cannot approve: 2 finding(s) still open.
Address them and run |
|
/gateway dismiss F7, we are aware of the interceptor stream error feedback capabilities right now |
|
❌ |
|
/gateway dismiss F7 we are aware of the interceptor stream error feedback capabilities right now |
|
🚫 Dismissed F7 (minor) by @saubyk — we are aware of the interceptor stream error feedback capabilities right now Open findings on this PR: 🟡 F8 (minor) |
|
/gateway dismiss F8 onchain is always the final state in case a channel goes onchain, your explicit case can not happen or only in some edge case race scenario where I think it is ok no not log anything but just ignore this call and exit early. |
|
🚫 Dismissed F8 (minor) by @saubyk — onchain is always the final state in case a channel goes onchain, your explicit case can not happen or only in some edge case race scenario where I think it is ok no not log anything but just ignore this call and exit early. All findings on this PR are addressed or dismissed. |
|
/gateway approve |
There was a problem hiding this comment.
✅ Approved — all prior findings addressed.
Findings recap
Findings:
F1(major) — addressed:onChainHeld.resolveacceptsFwdActionResume,FwdActionResumeModified, andFwdActionFailand forwards them toh.fwd.Resume()/`h.fwd…F2(minor) — unresolved:interceptOnChainonly callssendForwardwhen!wasHeld, so when an existing off-chain hold is promoted to on-chain the interceptor clie…F3(minor) — unresolved:InterceptedPacket.AutoFailHeight()projects both either-arms to a singleint32, and the RPC layer (forward_interceptor.go) maps that i…F4(minor) — addressed: The exportedAutoFailHeight int32field onhtlcswitch.InterceptedPacket(stable since 2022) is removed in favor of the `Deadline fn.Eith…F5(major) — addressed: The Deprecations section contains unresolved Git conflict markers committed into the file:<<<<<<< HEAD(line 63),=======(line 68), an…F6(minor) — addressed: The separator line between the tor bullet and the new interceptor bullet isF7(minor) — unresolved:onChainHeld.resolvereturningErrCannotFailOnChain/ErrCannotResumeOnChainis correct per the documented contract, but that error propa…F8(minor) — unresolved:addOffChaindoesif h.exists(key) { return nil }, which no-ops for any existing entry — including anonChainHeld. That correctly pre…
Dismissed:
F2by @ziggie1984 —F3by @ziggie1984 —F7by @saubyk — we are aware of the interceptor stream error feedback capabilities right nowF8by @saubyk — onchain is always the final state in case a channel goes onchain, your explicit case can not happen or only in some edge case race scenario where I think it is ok no not log anything but just ignore this call and exit early.
Approved by @saubyk via /gateway approve. Last reviewed at b6bf686. Skill v0.3.0, model claude-opus-4-8.
4 participants
Change Description
Fixes #10892
Look at the first commit which shows via itests the failure cases of the old Interceptor Implementation
The second commit introduces a new interface and distingishes between onchain and offchain HTLC for the interceptor. It does not change any public interface.