terminal, config: fix tapd startup deadlock, require an HTLC interceptor - #1371
terminal, config: fix tapd startup deadlock, require an HTLC interceptor#1371GeorgeTsagk wants to merge 6 commits into
Conversation
There was a problem hiding this comment.
Here an (llm) analysis. I tried to see if this can be reproduced with an itest (see here https://github.com/bitromortac/lightning-terminal/tree/pr-1371-force-close-stall-itest - needs a cleanup) and was able to analyze it a bit. The tldr is that it can be reproduced with an itest and that only removing the sync wait isn't enough, we need to start tapd before the other subservers. It could be that running the lnd/tapd bundle on the tap repo has a similar problem (but haven't looked into that further).
It reproduces in an itest, and any channel will do
itest/litd_startup_stall_test.go reproduces it deterministically: force close a channel, mine past the CSV while litd is down, restart, and measure how long tapd takes to report running.
The channel does not have to be an asset channel. lnd asks every configured aux sweeper for an extra sweep output whenever one is present (fn.MapOptionZ(t.cfg.AuxSweeper, ...), sweep/fee_bumper.go:1659), and tapd's DeriveSweepAddr does no asset-input filtering before it blocks. A vanilla, non-asset force close is therefore enough to reach into tapd and park the caller, which makes the exposure much wider than "nodes with asset channels".
It is a stall, not a deadlock
The wait is bounded. lnd's blockbeat dispatcher gives up on a stuck consumer after DefaultProcessBlockTimeout (60s in v0.21), logs the failure and carries on, so the startup resumes on its own:
CHIO Height[472]: Failed to process block: consumer TxPublisher:
process block timeout
CHIO Height[472]: Notified all consumers on new block in 1m0.007482933s
The blocked consumer is TxPublisher, not UtxoSweeper — prepareSweepTx and its DeriveSweepAddr call live in sweep/fee_bumper.go. It becomes unbounded only while the chain backend is still feeding lnd blocks faster than one per timeout (initial block download, neutrino catching up), which is presumably where the "retry forever" observation came from. Worth restating in the commit message and release note, because someone debugging a slow startup will not recognise the symptom from the word "deadlock".
Skipping the chain sync wait alone does not fix it
Measured on this branch as submitted, three runs: 1m0.24s / 1m0.01s / 1m0.22s for tapd to come up. litd itself no longer waits, but StartIntegratedServers iterates a Go map, so loop, pool or faraday can be started before tapd — and their startup calls GetInfo. getChainSyncInfo calls BlockbeatDispatcher.CurrentHeight(), which is served by the same goroutine that is blocked notifying consumers, so that sub-server inherits the exact wait this PR removed from litd. Whether a given startup pays the 60s depends on map iteration order.
LITD Not waiting for lnd to be synced ... <- the change works
LITD Full lnd client connected <- 1s
LOOPD Protocol version: MuSig2 <- loop starts first
RPCS [/lnrpc.Lightning/GetInfo] requested <- and blocks
... 60s ...
CHIO Height[472]: consumer TxPublisher: process block timeout
LOOP Connected to lnd node 'Stalled' <- GetInfo returns
TSVR Version: 0.8.0-alpha <- tapd finally starts
So tapd's start has to be separated out
Starting tapd before the sub-servers that query lnd's sync state closes it, and is a complete fix rather than a mitigation: waitForReady blocks until tapd is ready, not for a fixed 60s, and nothing in tapd's own startup depends on the blocked dispatcher. Verified against stock tapd v0.8.0:
LITD Full lnd client connected
TSVR Version: 0.8.0-alpha <- tapd first, +3ms
TCHN Starting AuxSweeper
SWPR Publishing sweep tx ..., num_inputs=1
CHIO Height[472]: Notified all consumers in 167.872926ms
LOOPD Protocol version: MuSig2 <- loop after tapd
167.87ms instead of 1m0.008s, the parked sweep broadcasts immediately, and the force close resolves one block later. It also removes the map-order non-determinism and gives loop/pool/faraday a synced lnd again.
One warning for anyone tempted by the obvious upstream fix: making tapd's aux methods fail instead of block does remove the stall (402ms), but lnd classifies the aux error as fatal, drops the monitor record and the input, and
the commitment output is then never swept — the sweeper reports attempt sweeping 0 inputs forever. That is why the itest drives the force close all the way to resolution; a test that only asserts prompt startup passes against that regression.
Before starting the sub-servers, we wait for lnd to be fully synced to its chain backend. lnd only reports itself as synced once its blockbeat dispatcher caught up, and block processing can be blocked on tapd's aux components: if a channel is being resolved on chain, the sweeper calls into tapd's aux sweeper, which blocks until tapd is started. Since tapd is only started after the full lnd client is created, litd stalls here for as long as lnd takes to give up on the blocked consumer, which is 60 seconds per block. For as long as blocks arrive faster than that, we never get past this point at all and just keep logging "Retrying to create LND Services client" while tapd never comes up. We therefore skip the chain sync wait when both lnd and tapd run in-process, which is the only combination where the aux components are wired into lnd. We still wait for the chain notifier to be ready, which is what the sub-servers need to subscribe to blocks. Note that this alone is not sufficient, the sub-servers also need to be started in the right order. That is done in the next commit.
Not waiting for lnd's chain sync ourselves isn't enough to get tapd up while lnd's block processing is blocked on tapd's aux sweeper. The sub-servers are started by iterating a map, so loop, pool or faraday can be started before tapd, and their startup calls lnd's GetInfo. That call asks the blockbeat dispatcher for its current height, and the dispatcher is the very goroutine that is waiting on the aux sweeper, so the sub-server inherits the exact wait we just removed. Whether a startup pays for it depends on Go's map iteration order. We now start tapd first, which releases the aux sweeper immediately, and return the remaining sub-servers in a stable order so startup no longer depends on map iteration order at all.
tapd's RFQ subsystem enforces the agreed upon quote for asset HTLCs through lnd's HTLC interceptor. Whenever no interceptor is attached, lnd forwards HTLCs without any of those checks. That window is narrow, as tapd's aux traffic shaper blocks forwards until tapd is ready, but it is real: it exists whenever tapd has to re-establish its interception stream, and for the brief moment between tapd signalling readiness and its interceptor actually registering. By setting lnd's requireinterceptor option whenever tapd runs in-process we make lnd fail forwards back with a temporary channel failure instead of forwarding them unchecked. HTLCs we receive ourselves are unaffected, as those are resolved through the invoice registry and never reach the forwarding interceptor.
Force closes a channel, mines past the CSV while the node is down, then restarts and measures how long tapd takes to report itself as running. Because lnd asks every configured aux sweeper for an extra sweep output regardless of whether any input carries assets, a vanilla channel force close is enough to reach into tapd, so no asset channel is needed. The test also drives the force close all the way to resolution, so that a change which removes the stall by failing the aux call that arrived too early doesn't pass: that would trade the stall for a sweep that is never retried.
Fails the resolution assertion with a clear message if the node's lnd client couldn't be restored after the restart, rather than panicking on a nil client while the test is already unwinding. Also re-wraps a few comment lines that exceeded the line length limit.
cf601f3 to
b9b3ebb
Compare
|
Thanks for the thorough review @bitromortac. Added your test. Ready for another round. |
|
@ViktorT-11: review reminder |
3 participants
Description
Two changes for integrated tapd mode:
We no longer wait for lnd to be synced to chain before creating the full lnd client: lnd only reports synced once its blockbeat has caught up, and block processing can block on tapd's aux sweeper, which blocks until tapd starts, which only happens after this client exists.
We set lnd's
requireinterceptorwhen tapd runs in-process, so lnd fails forwards back rather than forwarding them with no RFQ policy check while tapd's interceptor isn't attached. Both are gated on lnd and tapd being integrated.Reviewer notes
Skipping the sync wait also lets faraday/loop/pool start against a catching-up lnd, seconds on a restart but longer during IBD, and I haven't audited their behaviour there. The narrower fix would start tapd first and wait before the remaining sub-servers, which needs
StartIntegratedServerssplit up. Neither commit has targeted tests: the itests never forward an HTLC, and none reproduce the deadlock.