Repository navigation
Conversation
While a channel replays a CommitmentSigned and a RevokeAndAck that the peer is still owed (the dual-owed case of nervosnetwork#1111), it defers the TLC updates the peer sends and flushes them right before it verifies the peer's next CommitmentSigned. The deferral flag and the queue are not tied to the connection: if the connection drops before that CommitmentSigned arrives, both stay as they are. After the reconnect the peer replays every update it has not had acknowledged, including the ones already queued, so the queue holds the same update twice. The flush applies the first copy and fails on the second ("Received tlc id .. is not the expected next id .." for an AddTlc, "TLC is already removed" for a RemoveTlc), and the error is returned out of handle_commitment_signed_peer_message, which drops the CommitmentSigned the flush was done for. The peer then never gets a RevokeAndAck for that CommitmentSigned, and the RevokeAndAck that follows it is processed on its own. The two sides end up with their commitment numbers out of step, which later shows up as Musig2VerifyError(BadSignature) and a force close, or as a side that waits for an ack forever. Clear the flag and the queue when the peer disconnects. The peer replays what was queued, so nothing is lost. Add a test that queues an update, loses the connection together with the CommitmentSigned behind it, and reconnects: on develop the payment never completes. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
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 #1689.
Problem
During a dual-owed replay (#1111), a channel defers the peer's TLC updates until the peer's next
CommitmentSigned. The queue isn't cleared on disconnect, so after a reconnect the peer replays the same updates and the queue holds them twice. The flush fails on the duplicate and drops theCommitmentSigned, which leaves the commitment numbers out of step: aBadSignatureforce close, or an ack that never comes.Change
Clear
defer_peer_tlc_updatesanddeferred_peer_tlc_updatesinon_peer_disconnected. The peer replays what was queued, and both fields are alreadyskip_store, so a reconnect now behaves like a restart.Tests
test_deferred_peer_tlc_updates_do_not_outlive_the_connection: fails on develop (the payment never completes) and passes with this change (5/5).cargo fmt, both nativeclippyruns with-D warnings,typos,make check-migrate, thefnnlib suite (rocksdb and sqlite) andtest_node_restartpass.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.I'm assuming the peer always replays the updates queued here. Is that right?