lightningd: update graceful notifications when HTLC states change#9305
Open
ksedgwic wants to merge 1 commit into
Open
lightningd: update graceful notifications when HTLC states change#9305ksedgwic wants to merge 1 commit into
ksedgwic wants to merge 1 commit into
Conversation
The graceful command notifies watchers about the closest-expiry HTLC, including its state, whenever that message changes. But nothing re-evaluated the message when an HTLC changed state: only HTLC removal, peer disconnect and another graceful invocation re-ran the check. If graceful was invoked while a commitment dance was in flight, the initial notification named a transient state (e.g. RCVD_ADD_REVOCATION) and no follow-up ever announced the settled state. test_graceful_htlc waits for exactly that follow-up (since 6994681 "flake: Fix test_graceful_htlc to be flexible for notifs"), so it times out whenever graceful catches the dance mid-flight, which valgrind CI runs make likely. In one CI failure graceful caught the outgoing HTLC in RCVD_ADD_REVOCATION, the dance completed 600ms later, and no notification followed for the remaining 180 seconds. Re-check graceful progress in the handlers that advance HTLC states (peer_sending_commitsig, peer_got_commitsig, peer_got_revoke). The check is a no-op unless a graceful command is outstanding, and identical messages are already deduplicated. The dance now generates transient-state notifications, so rewrite the test to match expected notifications as an ordered subsequence instead of by exact index. That also removes two accidents the old indexing depended on: the RCVD_ADD_REVOCATION special case (the settled state now always notifies), and the wait for a notification after l1's disconnect, which was really satisfied by the graceful(1) call's own initial notification arriving on the shared rpc socket -- the disconnect itself never notifies, since the message text describes the still-connected peer l3 and does not change. Fixes: ElementsProject#9219 Changelog-Fixed: JSON-RPC: `graceful` notifications now update when a pending HTLC changes state.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the test_graceful_htlc CI flake (#9219), observed again on a run
for #9286 (Valgrind Test CLN 11/12):
https://github.com/ElementsProject/lightning/actions/runs/29122024616/job/86461830316
graceful only re-evaluated its "Next HTLC ..." notification on HTLC
removal, peer disconnect, or another graceful invocation -- never on
HTLC state transitions. When graceful catches a commitment dance in
flight (likely under valgrind), the initial notification names a
transient state (e.g. RCVD_ADD_REVOCATION) and no follow-up ever
announces the settled state. The flake handler added in 6994681
waits for exactly that follow-up, so the test times out. In the CI
failure above, graceful caught the outgoing HTLC in
RCVD_ADD_REVOCATION, the dance completed 600ms later, and no
notification followed for the remaining 180 seconds.
The fix re-checks graceful progress in the three handlers that advance
HTLC states (peer_sending_commitsig, peer_got_commitsig,
peer_got_revoke). The check is a no-op unless a graceful command is
outstanding, and identical messages are already deduplicated.
Since the dance now generates transient-state notifications, the test
is rewritten to match expected notifications as an ordered subsequence
instead of by exact index. That also removes two accidents the old
indexing depended on; see the commit message for details.
Fixes: #9219