Ignore stale splice initial commitment_signed#4751
Conversation
After we complete a splice negotiation and see a `FundingTransactionReadyForSigning` event, the counterparty may already have sent its initial `commitment_signed` for the splice funding transaction. If we then cancel the funding contribution, our local channel state no longer tracks the pending splice attempt and queues `tx_abort`, but the in-flight `commitment_signed` can still arrive first. Handling that message against the post-abort channel state attempts to validate a signature for the now-stale splice funding transaction and can force-close the still-live channel. We fix this by checking the optional `funding_txid` (which we expect all implementations to always include by default) included in `commitment_signed` before validating the commitment signature. If it does not match the channel's locked funding txid, we can safely ignore the stale message.
|
👋 Thanks for assigning @jkczyz as a reviewer! |
|
No issues found. The fix is correct and backward-compatible:
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4751 +/- ##
==========================================
+ Coverage 84.55% 86.94% +2.38%
==========================================
Files 137 161 +24
Lines 77617 111643 +34026
Branches 77617 111643 +34026
==========================================
+ Hits 65627 97065 +31438
- Misses 9948 12071 +2123
- Partials 2042 2507 +465
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| if let Some(funding_txid) = msg.funding_txid { | ||
| let locked_funding_txid = | ||
| self.funding.get_funding_txid().expect("funded channel must have known txid"); | ||
| if funding_txid != locked_funding_txid { | ||
| return Err(ChannelError::Ignore(format!( | ||
| "Ignoring commitment_signed for stale funding txid {funding_txid}" | ||
| ))); | ||
| } | ||
| } |
There was a problem hiding this comment.
Do we need to do this in commitment_signed_batch, too? If so, should we move the check to validate_commitment_signed?
There was a problem hiding this comment.
The initial commitment_signed should never go through that path, and commitment_signed_batch should already ignore stale funding attempts.
|
👋 The first review has been submitted! Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer. |
4 participants
After we complete a splice negotiation and see a
FundingTransactionReadyForSigningevent, the counterparty may already have sent its initialcommitment_signedfor the splice funding transaction. If we then cancel the funding contribution, our local channel state no longer tracks the pending splice attempt and queuestx_abort, but the in-flightcommitment_signedcan still arrive first. Handling that message against the post-abort channel state attempts to validate a signature for the now-stale splice funding transaction and can force-close the still-live channel.We fix this by checking the optional
funding_txid(which we expect all implementations to always include by default) included incommitment_signedbefore validating the commitment signature. If it does not match the channel's locked funding txid, we can safely ignore the stale message.Fixes #4730.