Repository navigation
fix: force-close channels whose revocation nonce ledger stays unbalanced - #1701
Conversation
When a channel's revocation nonce ledger is left unbalanced at rest (one side missing its send nonce, the other its verify nonce), is_waiting_tlc_ack blocks AddTlc/RemoveTlc on both sides while waiting_ack is false. The existing peer-response watchdog therefore never fires, and the channel stays ChannelReady but can never move a TLC again. After every handled message, track whether the ledger is unbalanced while the channel is ready, online and not waiting for its own ack. When it becomes unbalanced, record the time and schedule a CheckRevocationNonceLedger event that carries it, PEER_CHANNEL_RESPONSE_TIMEOUT later. If the channel still holds the same timestamp when the event arrives, force-close it through the existing path. A balanced ledger, or going offline, clears the timestamp. The new field is runtime-only. The event carries the timestamp instead of comparing elapsed wall-clock time. The timer runs on the monotonic clock and can fire a moment before the wall clock shows the full timeout, so an elapsed-time check can miss, and nothing would retry it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Security ReviewResult: No reportable findings in the PR #1701 watchdog change and its directly supporting channel, persistence, and force-close paths. Validation:
Notes:
|
|
Could we reuse the existing We could keep tracking when the nonce ledger becomes unbalanced after each handled message, then check the elapsed time during maintenance and force close if needed. This would avoid the extra event and stale timer handling. The trade off is timing: maintenance runs every 60 seconds in release builds, so a 30 second timeout could take up to roughly 90 seconds to trigger. If the goal is to prevent an indefinite stall rather than enforce an exact deadline, this seems simpler. |
…lcs tick Drop the CheckRevocationNonceLedger event and its one-shot timer, as suggested in review. The time the ledger became unbalanced is still recorded after every handled message, and the periodic MaintainChannelTlcs tick now force-closes the channel once that is older than PEER_CHANNEL_RESPONSE_TIMEOUT. A tick that lands just before the deadline is followed by another one, so the early-firing timer that made the event carry its own timestamp no longer matters. The channel is force-closed within one CHECK_CHANNELS_INTERVAL after the timeout (up to about 90 s in release builds), which still bounds a stall that would otherwise never clear. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
@quake Thanks, that's simpler. I've pushed it as a second commit (7021cee). The time the ledger became unbalanced is still recorded after each handled message, and the The event only carried a timestamp because a one-shot timer can fire just before the deadline and nothing would retry it. With a periodic check the next tick covers that, so up to about 90 s in release seems fine for a stall that otherwise never clears. Both watchdog tests still pass (5/5), and I've updated the description. cc @chenyukang, since the change moved after the review request. |
…nels `is_revocation_nonce_ledger_unbalanced` guards the V1 per-round ledger (`remote_revocation_nonce_for_send` / `_verify`). A V2 channel keeps its nonces in `session_v2` and never advances those fields, so an idle V2 channel looked permanently unbalanced and #1701's watchdog force-closed it one `PEER_CHANNEL_RESPONSE_TIMEOUT` after the last round: cannot add or remove TLCs: its revocation nonce ledger has been unbalanced for over 30000 ms (send_present=false, verify_present=false) ... shutting down it forcefully Channel V2 is enabled by default (`gen_node_features`), so every e2e channel is V2 and `e2e test for router-pay` failed on both CI runs: the channel force-closed mid-test and the following payment found no route. Skip the V1 ledger for V2 channels. V2 staleness stays bounded by `peer_response_wait_v2` / `ack_peer_wait_v2`, which already drive the force close for a V2 round that is never answered. Regression test asserts that a V2 channel with both ledger fields cleared neither reports the stall nor keeps the unbalanced timestamp (it fails without the fix).
…cy channel `test_revocation_nonce_stall_is_force_closed_after_peer_response_timeout` provokes the V1 per-round revocation nonce ledger to stay unbalanced and expects the #1701 watchdog to force close. With channel V2 enabled by default the channel is V2, which does not use that ledger, so the watchdog no longer fires and the test timed out. Use `create_legacy_nodes_with_established_channel`, matching the two other ledger tests and the behaviour this case was written for. The V2 counterpart is covered by `test_v2_channel_ignores_v1_revocation_nonce_watchdog`.
Refs #1561.
A stopgap for channels that stay on V1. It bounds the revocation-nonce stall with a force close and doesn't sign anything or change nonce handling, as chainTe suggested in #1561 and #1628.
Problem
When the revocation nonce ledger is left unbalanced at rest (one side missing its send nonce, the other its verify nonce),
is_waiting_tlc_ack()blocksAddTlc/RemoveTlcon both sides.waiting_ackis false, so the existing peer-response check never fires, and the channel staysChannelReadybut can never move a TLC again. The ignored #1584 test reproduces it, and in Jepsen it follows ordinary process crashes.Change
revocation_nonce_unbalanced_since. A balanced ledger or going offline clears it.MaintainChannelTlcstick, if the ledger has been unbalanced for longer thanPEER_CHANNEL_RESPONSE_TIMEOUT, force-close throughnotify_network_actor_shutdown_me, the pathCheckActiveChanneluses. The close comes within oneCHECK_CHANNELS_INTERVALafter the timeout (up to about 90 s in release builds).skip_store). No message, nonce or persisted-state change.Tests
test_revocation_nonce_stall_is_force_closed_after_peer_response_timeout(Payments may remain Inflight after rapid channel reconnects due to a stuck retryable TLC operation queue #1584 schedule): on develop the channel staysChannelReady(send/verify present: A false/true, B true/false); with this change it is force-closed on the first maintenance tick after the timeout (5/5).test_revocation_nonce_watchdog_keeps_healthy_idle_channel_open: a healthy channel, including one round with a delayedCommitmentSigned, stays open past the timeout and can still pay (5/5).MaintainChannelTlcs:cargo fmt,clippy -D warnings(sqlite) and thefiber::tests::channelmodule (207 passed) pass. The checks below were run on the first version.cargo fmt, both nativeclippyruns with-D warnings,typos,make check-migrate, thefnnlib suite (rocksdb and sqlite) andtest_node_restartpass. One lib test,mpp::test_mpp_waiting_ack_settlement_hands_off_to_durable_retry_queue, also fails on develop since fix: scope invoice settlement to local payments #1672; it is unrelated.Questions
PEER_CHANNEL_RESPONSE_TIMEOUT(30 s) the right timeout, or should it be longer? The stall is permanent, and in one partition run 30 s closed a channel whose message was only delayed.ShuttingDownuntilCheckChannelsShutdownsees the peer's commitment on chain. Is that acceptable?