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 @jkczyz as a reviewer! |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4472 +/- ##
==========================================
- Coverage 86.08% 86.00% -0.08%
==========================================
Files 159 159
Lines 105186 105489 +303
Branches 105186 105489 +303
==========================================
+ Hits 90546 90727 +181
- Misses 12131 12249 +118
- Partials 2509 2513 +4
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. |
Automated Review (Updated)After extensive re-analysis of all code paths (monitor_updating_restored, tx_signatures_received, signer_maybe_unblocked, channel_reestablish, funding_transaction_signed, and handle_channel_resumption), my three prior concerns have been re-evaluated and found to be incorrect:
No new issues found. The logic correctly handles all examined scenarios including async signer + async monitor update combinations, disconnection during pending monitor updates, and reconnection with tx_signatures retransmission. 🤖 Generated with Claude Code |
|
🔔 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
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
86fa38c to
ec60d8e
Compare
|
Needs rebase now. |
4 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.