Release tx_signatures after async monitor update completes#4472
Release tx_signatures after async monitor update completes#4472wpaulino wants to merge 1 commit intolightningdevkit:mainfrom
Conversation
|
👋 Thanks for assigning @TheBlueMatt as a reviewer! |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4472 +/- ##
==========================================
+ Coverage 84.55% 86.18% +1.62%
==========================================
Files 135 161 +26
Lines 76569 107532 +30963
Branches 76569 107532 +30963
==========================================
+ Hits 64745 92679 +27934
- Misses 9783 12233 +2450
- Partials 2041 2620 +579
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
🔔 1st Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
|
|
|
🔔 3rd Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
|
✅ Added second reviewer: @tankyleo |
TheBlueMatt
left a comment
TheBlueMatt
left a comment
There was a problem hiding this comment.
Aside from what claude noted I didn't see any issues.
wpaulino
force-pushed
the
release-tx-signatures-after-async-monitor-update-completion
branch
from
c8c5dc0 to
86fa38c
Compare
wpaulino
force-pushed
the
release-tx-signatures-after-async-monitor-update-completion
branch
from
86fa38c to
ec60d8e
Compare
|
Needs rebase now. |
wpaulino
force-pushed
the
release-tx-signatures-after-async-monitor-update-completion
branch
from
ec60d8e to
f81bb39
Compare
|
🔔 1st Reminder Hey @jkczyz! This PR has been waiting for your review. |
jkczyz
left a comment
jkczyz
left a comment
There was a problem hiding this comment.
Looks good though consider these comments to encapsulate some logic in InteractiveTxSigningSession.
In 83b2d3e, we reworked `ChannelManager::funding_transaction_signed` such that it would also for a user to cancel a splice up until they send `commitment_signed`. Previously, we would would only emit `Event::FundingTransactionReadyForSigning` when both nodes exchanged `commitment_signed` and the corresponding monitor update completed. With the event now being generated immediately after the nodes exchange `tx_complete`, we now need to handle the monitor update not having completed by the time we are ready to send `tx_signatures`. Unfortunately, we also did not have test coverage, allowing this to go unnoticed until being caught by the fuzzer due to a debug assertion. Doing so avoids a potential funds-loss scenario if the funding transaction confirms without the counterparty's signature for our commitment being durably persisted.
wpaulino
force-pushed
the
release-tx-signatures-after-async-monitor-update-completion
branch
from
f81bb39 to
ea204f6
Compare
| if self.is_awaiting_monitor_update() { | ||
| // Although the user may have already provided our `tx_signatures`, we must not send | ||
| // them if we're waiting for the monitor to durably persist the counterparty's signature | ||
| // for our initial commitment post-splice. | ||
| debug_assert!(self.context.monitor_pending_tx_signatures); |
There was a problem hiding this comment.
Bug: debug_assert!(self.context.monitor_pending_tx_signatures) is too strict for V2 channel opens (non-splice).
monitor_pending_tx_signatures is only set to true in splice_initial_commitment_signed (line 8006), but NOT in initial_commitment_signed_v2 (line 7847). Both call monitor_updating_paused() which sets is_monitor_update_in_progress().
For V2 dual-funded channel opens where the initial watch_channel() returns InProgress, if the counterparty sends tx_signatures while the monitor persist is still pending:
- Debug builds: this debug_assert panics
- Release builds: correctly returns early (holding back tx_signatures), BUT when the monitor update completes,
monitor_updating_restoredat line 9308-9317 skips the tx_signatures release becausemonitor_pending_tx_signaturesisfalse. The tx_signatures become stuck until the next reconnection.
Consider adding self.context.monitor_pending_tx_signatures = true; in initial_commitment_signed_v2 (after the monitor_updating_paused call at line 7885), consistent with splice_initial_commitment_signed at line 8006.
There was a problem hiding this comment.
Dual funding is not really supported yet, and we may even remove the incomplete work done thus far for the time being.
| self.holder_tx_signatures | ||
| .as_ref() | ||
| .filter(|_| { | ||
| (self.has_received_commitment_signed && self.holder_sends_tx_signatures_first) |
There was a problem hiding this comment.
We don't want to release our signatures until we receive their commitment_signed , so I assume this stronger check previously was handle at the call sites / by the nature of when this was called?
| .as_ref() | ||
| .filter(|_| { | ||
| (self.has_received_commitment_signed && self.holder_sends_tx_signatures_first) | ||
| || self.has_received_tx_signatures() |
There was a problem hiding this comment.
Does this imply we've received their commitment_signed? Should we have a stronger check / assertion here?
There was a problem hiding this comment.
Not at this level but it is enforced in Channel::tx_signatures
5 participants
In 83b2d3e, we reworked
ChannelManager::funding_transaction_signedsuch that it would also for a user to cancel a splice up until they sendcommitment_signed. Previously, we would would only emitEvent::FundingTransactionReadyForSigningwhen both nodes exchangedcommitment_signedand the corresponding monitor update completed. With the event now being generated immediately after the nodes exchangetx_complete, we now need to handle the monitor update not having completed by the time we are ready to sendtx_signatures. Unfortunately, we also did not have test coverage, allowing this to go unnoticed until being caught by the fuzzer due to a debug assertion. Doing so avoids a potential funds-loss scenario if the funding transaction confirms without the counterparty's signature for our commitment being durably persisted.