htlcswitch: separate onchain and offchain intercpeted HTLCs cleanly#10895
htlcswitch: separate onchain and offchain intercpeted HTLCs cleanly#10895ziggie1984 wants to merge 9 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.
ziggie1984
changed the title
ziggie1984
force-pushed
the
fix-onchain-interceptor-held-htlc
branch
from
424d1a8 to
ebe03ed
Compare
github-actions
Bot
added
severity-critical
ziggie1984
force-pushed
the
fix-onchain-interceptor-held-htlc
branch
3 times, most recently
from
d16ea33 to
c16a193
Compare
|
For clarity, here is the bug/failure-mode breakdown this PR fixes:
So the short version is: the two itests cover the two main incident paths, and the follow-up commits harden the surrounding on-chain held HTLC lifecycle so disconnects, invalid actions, action errors, and witness-beacon error handling do not regress the same settlement path. |
ziggie1984
force-pushed
the
fix-onchain-interceptor-held-htlc
branch
from
c16a193 to
8b7ee4a
Compare
ziggie1984
added
the
backport-v0.21.x-branch
ziggie1984
force-pushed
the
fix-onchain-interceptor-held-htlc
branch
from
8b7ee4a to
7546618
Compare
Add coverage for held forwards that move on chain after the incoming channel force closes. The restart case exercises the path where Bob loses the in-memory held set and contractcourt re-offers the HTLC through the witness beacon. The no-restart case keeps the original off-chain hold and proves that settlement must still reach the on-chain resolver.
Store held forwards as off-chain or on-chain entries instead of a raw InterceptedForward map. Off-chain entries keep the existing resume, fail, settle and auto-fail behavior. On-chain entries are settle-only and expire by pruning local interceptor state. When contractcourt re-offers a circuit that is already held off-chain, replace the stored entry with the on-chain forward so a later SETTLE reaches the witness beacon instead of the old link mailbox path. Also set the on-chain interceptor deadline to the HTLC refund timeout. This keeps the public interceptor deadline populated while ensuring only off-chain held entries use that value to fail back.
Release the preimage beacon lock before invoking the on-chain interceptor. The interceptor path can block on the htlcswitch event loop, while resolution of another held on-chain HTLC can call back into the beacon to add a preimage. If interceptor delivery fails after the subscriber was registered, cancel the subscription before returning the error.
Only off-chain held HTLCs can be released when an optional interceptor disconnects, because they can resume into the link forwarding flow. On-chain held HTLCs have no link flow to resume. Keep them in the held set so a reconnecting interceptor can replay and settle them while contractcourt waits for the preimage or on-chain expiry.
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.
ziggie1984
force-pushed
the
fix-onchain-interceptor-held-htlc
branch
from
7546618 to
6e385f6
Compare
3 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.