Introduce RenegotiatedFundingLocked monitor update variant #3894
Conversation
|
👋 Thanks for assigning @TheBlueMatt as a reviewer! |
|
🔔 1st Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 3rd Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 4th Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 5th Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
👋 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. |
wpaulino
force-pushed
the
renegotiated-funding-locked-monitor-update
branch
2 times, most recently
from
00e8bb3 to
c1ead6a
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3894 +/- ##
==========================================
- Coverage 89.65% 88.96% -0.70%
==========================================
Files 164 166 +2
Lines 134658 121410 -13248
Branches 134658 121410 -13248
==========================================
- Hits 120734 108012 -12722
+ Misses 11246 10988 -258
+ Partials 2678 2410 -268
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:
|
wpaulino
force-pushed
the
renegotiated-funding-locked-monitor-update
branch
from
c1ead6a to
a6d4b1e
Compare
| // if we know the RBF doesn't belong to a splice. | ||
| if !is_pending_splice && self.first_confirmed_funding_txo == self.funding.funding_outpoint() | ||
| { | ||
| self.first_confirmed_funding_txo = new_funding.funding_outpoint(); |
There was a problem hiding this comment.
Given this is only used to check if this channel is v1 or v2, and its "set at init", calling it first_confirmed_funding_txo seems like a misnomer. Its often set to a funding txo pre-first-confirmation, and is used pre-first-confirmation as well. It should probably be renamed and maybe we should decide what we actually want to use it for (should it be first-funding-txo or first-build-funding-txo?)
There was a problem hiding this comment.
Added a commit renaming it to first_negotiated_funding_txo
lightning/src/ln/channelmanager.rs
Outdated
| .and_then(Channel::as_funded_mut) | ||
| { | ||
| let funding_txo = channel.funding.get_funding_txo().unwrap(); | ||
| handle_new_monitor_update!( |
There was a problem hiding this comment.
monitor updates have to be handled in the same mutex where the channel generated them, otherwise they can end up getting applied out-of-order which isn't allowed. You can handle the post-update actions (which may need an unlock) later, though, see REMAIN_LOCKED_UPDATE_ACTIONS_PROCESSED_LATER and the logic in finish_close_channel.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
wpaulino
force-pushed
the
renegotiated-funding-locked-monitor-update
branch
from
a6d4b1e to
3bdf268
Compare
This is a new `ChannelMonitorUpdateStep` variant intended to be used whenever a new funding transaction that was negotiated and applied via the `RenegotiatedFunding` update reaches its intended confirmation depth and both sides of the channel exchange `channel_ready`/`splice_locked`. This commit primarily focuses on its use for splices, but future work will expand where needed to support RBFs for a dual funded channel. This monitor update ensures that the monitor can safely drop all prior commitment data since it is now considered invalid/unnecessary. Once the update is applied, only state for the new funding transaction is tracked going forward, until the monitor receives another `RenegotiatedFunding` update.
It's only intended to be set during initialization and used to check if the channel is v1 or v2. We rename it to `first_negotiated_funding_txo` to better reflect its purpose.
wpaulino
force-pushed
the
renegotiated-funding-locked-monitor-update
branch
from
3bdf268 to
6c43f5d
Compare
|
🔔 1st Reminder Hey @TheBlueMatt @jkczyz! This PR has been waiting for your review. |
1 similar comment
|
🔔 1st Reminder Hey @TheBlueMatt @jkczyz! This PR has been waiting for your review. |
| ); | ||
|
|
||
| // The swap above places the previous `FundingScope` into `pending_funding`. | ||
| for funding in self.pending_funding.drain(..) { |
There was a problem hiding this comment.
I'm a bit hesitant to accept that we should totally discard other funding scopes and never claim from them. If a user accepts 0conf splices, that's great, but if a commitment tx on the previous funding ends up getting confirmed, completely ignoring it and not trying to claim any of the value on it does seem a bit weird. I get that under our security model we shouldn't need to (eg if the user accepts 0conf funding we'd ignore it too) but it does seem worth fixing....
We don't need to fix it now, but opening an issue and addressing it eventually would be nice, IMO.
| .unwrap_or_else(|| Vec::new()); | ||
| mem::drop(peer_state); | ||
| mem::drop(per_peer_state); | ||
| self.handle_monitor_update_completion_actions(update_actions); |
There was a problem hiding this comment.
This needs to call the handle_monitor_update_completion macro (e.g. to call Channel::monitor_updating_restored), not this directly. Calling handle_monitor_update_completion_actions directly is only for closed channels.
This is a new
ChannelMonitorUpdateStepvariant intended to be used whenever a new funding transaction that was negotiated and applied via theRenegotiatedFundingupdate reaches its intended confirmation depth and both sides of the channel exchangechannel_ready/splice_locked. This commit primarily focuses on its use for splices, but future work will expand where needed to support RBFs for a dual funded channel.This monitor update ensures that the monitor can safely drop all prior commitment data since it is now considered invalid/unnecessary. Once the update is applied, only state for the new funding transaction is tracked going forward, until the monitor receives another
RenegotiatedFundingupdate.