htlcswitch: track on-chain held HTLCs#10895
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.
| // Keep the on-chain intercept available to the interceptor until | ||
| // the HTLC expires on chain. | ||
| AutoFailHeight: int32(htlc.RefundTimeout), |
There was a problem hiding this comment.
Critical Deadlock Risk in SubscribeUpdates
There is a critical deadlock risk between the preimageBeacon locking and the InterceptableSwitch event loop.
Deadlock Scenario:
-
Thread A (Contractcourt Resolver):
- Calls
preimageBeacon.SubscribeUpdates(). - Acquires
preimageBeacon.Lock()(line 66). - Reaches line 117 and calls
p.interceptor(fwd)(which isInterceptableSwitch.ForwardPacket(fwd)). ForwardPacketattempts to send tos.onchainIntercepted(line 455 ofinterceptable_switch.go). Since this channel is unbuffered, it blocks until the event loop receives from it.
- Calls
-
Thread B (InterceptableSwitch Event Loop -
run()goroutine):- Currently processing a
SETTLEresolution froms.resolutionChan(line 325 ofinterceptable_switch.go). - Calls
s.resolve(res)->heldHtlcSet.resolve(res)->onChainHeld.resolve(res)->fwd.Settle(res.Preimage). fwd.SettlecallspreimageBeacon.AddPreimages().AddPreimagesattempts to acquirepreimageBeacon.Lock()(line 172 ofwitness_beacon.go).- Deadlock: Thread B is blocked waiting for
preimageBeacon.Lock()(held by Thread A). Thread A is blocked waiting for Thread B to receive froms.onchainIntercepted.
- Currently processing a
Impact:
This deadlock will freeze the InterceptableSwitch event loop and the contractcourt resolver, preventing any further HTLC resolutions and potentially leading to a loss of funds if HTLCs expire on-chain without being swept.
Suggested Fix:
Release p.Lock() early in SubscribeUpdates as soon as the subscriber is added to the map, before calling p.interceptor(fwd).
func (p *preimageBeacon) SubscribeUpdates(
chanID lnwire.ShortChannelID, htlc *channeldb.HTLC,
payload *hop.Payload,
nextHopOnionBlob []byte) (*contractcourt.WitnessSubscription, error) {
p.Lock()
clientID := p.clientCounter
client := &preimageSubscriber{
updateChan: make(chan lntypes.Preimage, 10),
quit: make(chan struct{}),
}
p.subscribers[p.clientCounter] = client
p.clientCounter++
p.Unlock() // Release lock early to avoid deadlocks during blocking interceptor calls
srvrLog.Debugf("Creating new witness beacon subscriber, id=%v",
clientID)
sub := &contractcourt.WitnessSubscription{
WitnessUpdates: client.updateChan,
CancelSubscription: func() {
p.Lock()
defer p.Unlock()
delete(p.subscribers, clientID)
close(client.quit)
},
}
// ... rest of the function ...There was a problem hiding this comment.
fixed good catch, was even a problem before this PR change
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.
| set map[models.CircuitKey]heldEntry | ||
| } | ||
|
|
||
| func newHeldHtlcSet() *heldHtlcSet { |
There was a problem hiding this comment.
According to the repository style guide, every function must be commented with its purpose and assumptions, starting with the function name as a complete sentence. Please add a comment for newHeldHtlcSet.
// newHeldHtlcSet returns a new instance of heldHtlcSet.
func newHeldHtlcSet() *heldHtlcSet {References
- Every function must be commented with its purpose and assumptions. Function comments must begin with the function name and should be complete sentences. (link)
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.
Add the v0.21.1 release note skeleton and include the forward interceptor on-chain settlement fix from this PR.
ziggie1984
force-pushed
the
fix-onchain-interceptor-held-htlc
branch
from
02347b0 to
6495243
Compare
2 participants
Change Description
This fixes the forward interceptor handling for HTLCs that move from the
off-chain forwarding flow to the on-chain contractcourt flow after the
incoming channel force closes.
Previously, the held HTLC set stored every intercepted HTLC as a plain
InterceptedForward. That made off-chain and on-chain forwards share thesame auto-fail, resolve and release behavior, even though they have different
valid actions:
witness beacon.
That caused two problematic cases:
auto-failable forward and removed before the interceptor settled it.
contractcourt re-offered the same circuit on-chain, the held set treated it
as a duplicate and kept the old off-chain backend. A later
SETTLEthenwent to the link mailbox path instead of the witness beacon, so the incoming
contest resolver never received the preimage.
This PR makes the held set track the source of each held HTLC:
offChainHeldpreserves the existing off-chain behavior and auto-fails byfailing back through the link;
onChainHeldis settle-only and expires by pruning local interceptor state;the stored entry so future settlement reaches the witness beacon;
The on-chain interceptor packet now also carries
RefundTimeoutas itsdeadline. This keeps the existing public interceptor field populated while the
held set ensures only off-chain entries interpret that value as an auto-fail
height.
Steps to Test
New itest coverage was added for:
without restarting Bob.