Uh oh!
There was an error while loading. Please reload this page.
Refactor ShutdownResult type and construction - #2613
Conversation
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@## main #2613 +/- ##
==========================================
+ Coverage 88.92% 88.98% +0.05%
==========================================
Files 112 112 Lines 87632 87650 +18 Branches 87632 87650 +18 ==========================================
+ Hits 77929 77992 +63 + Misses 7473 7422 -51 - Partials 2230 2236 +6
☔ View full report in Codecov by Sentry. |
wpaulino
commented
Sep 28, 2023
Any reason this is marked as draft? |
| pub fn get_shutdown(&mut self, signer_provider: &SP, their_features: &InitFeatures, | ||
| target_feerate_sats_per_kw: Option<u32>, override_shutdown_script: Option<ShutdownScript>) | ||
| -> Result<(msgs::Shutdown, Option<ChannelMonitorUpdate>, Vec<(HTLCSource, PaymentHash)>), APIError> | ||
| -> Result<(msgs::Shutdown, Option<ChannelMonitorUpdate>, Vec<(HTLCSource, PaymentHash)>, Option<ShutdownResult>), APIError> |
There was a problem hiding this comment.
I wonder if these Option<ChannelMonitorUpdate>, Vec<(HTLCSource, PaymentHash)> can be collapsed into Option<ShutdownResult> although the use cases are slightly different.
There was a problem hiding this comment.
Maybe? I think I'd rather keep them separate though. We'd want to handle the monitor update result on the non-shutdown case.
There was a problem hiding this comment.
Right, that was my main hesitation. It seems possible to branch on the update_id of the ChannelMonitorUpdate being equal to CLOSED_CHANNEL_UPDATE_ID, but might not be something we want to unify.
Uh oh!
There was an error while loading. Please reload this page.
wvanlint
commented
Sep 28, 2023
Took another look, ready for review now. :) |
Uh oh!
There was an error while loading. Please reload this page.
9c2ee48 to
6960cb8Compare
vladimirfomene
left a comment
There was a problem hiding this comment.
Just a couple of comments here and there
| let (monitor_update_option, mut failed_htlcs, unbroadcasted_batch_funding_txid) = shutdown_res; | ||
| log_debug!(self.logger, "Finishing force-closure of channel with {} HTLCs to fail", failed_htlcs.len()); | ||
| log_debug!(self.logger, "Finishing closure of channel with {} HTLCs to fail", failed_htlcs.len()); |
There was a problem hiding this comment.
Does force here not give more context to anyone reading the logs?
There was a problem hiding this comment.
Problem is its not longer just force-closures, its all closures.
| pub reason: Option<ClosureReason>, | ||
| } | ||
| impl Default for ExpectedCloseEvent { |
There was a problem hiding this comment.
Does it not make sense to derive Default instead of manually implementing it?
There was a problem hiding this comment.
Definitely makes sense! Derived instead. :)
| let shutdown_result; | ||
| let unbroadcasted_batch_funding_txid = self.context.unbroadcasted_batch_funding_txid(); | ||
| self.context.target_closing_feerate_sats_per_kw = target_feerate_sats_per_kw; | ||
| if self.context.channel_state & !STATE_FLAGS < ChannelState::FundingSent as u32 { |
There was a problem hiding this comment.
Instead of defining shutdown_result on L5645 won't it be better to have this if/else expression return a ShutdownResult and then capture it in the shutdown_result variable.
There was a problem hiding this comment.
Done, capturing shutdown_result from if-else expressions in a couple places now.
TheBlueMatt
commented
Oct 5, 2023
Can you write more full commit messages that describe what we're doing (not just "refactor", but "refactor to do X") and why we're doing it? |
6960cb8 to
39e0b7dCompare
I've rewritten the commit subjects and added explanations to the bodies following https://cbea.ms/git-commit/, let me know if anything can be reworded there though! |
TheBlueMatt
left a comment
There was a problem hiding this comment.
LGTM, but I'd rather expand a few assertions and checks cause this is all pretty delicate code.
| channel_capacity_sats, | ||
| .. | ||
| } if ( | ||
| expected_event.channel_id.map(|expected| *channel_id == expected).unwrap_or(true) && |
There was a problem hiding this comment.
Shouldn't these all be unwrap_or(false)? We don't want to match if the caller specified a channel id to match but the event didn't have it.
There was a problem hiding this comment.
The .map function is called on the expected event field, so the unwrap_or(true) call is meant to match if the caller left expectations unspecified. Does that sound correct? A ChannelClosed event will always have a channel_id.
| pub fn check_closed_event(node: &Node, _events_count: usize, expected_reason: ClosureReason, is_check_discard_funding: bool, | ||
| expected_counterparty_node_ids: &[PublicKey], expected_channel_capacity: u64) { |
There was a problem hiding this comment.
?
| pubfn check_closed_event(node:&Node,_events_count:usize,expected_reason:ClosureReason,is_check_discard_funding:bool, | |
| expected_counterparty_node_ids:&[PublicKey], expected_channel_capacity:u64){ | |
| pubfn check_closed_event(node:&Node,events_count:usize,expected_reason:ClosureReason,is_check_discard_funding:bool, | |
| expected_counterparty_node_ids:&[PublicKey], expected_channel_capacity:u64){ | |
| assert_eq!(events_count, expected_counterparty_node_ids.len()); |
There was a problem hiding this comment.
Needed to account for the DiscardFunding events too but verified the events_count here.
| Option<Txid> | ||
| ); | ||
| /// The result of a shutdown that should be handled. | ||
| pub(crate) struct ShutdownResult { |
There was a problem hiding this comment.
| pub(crate)structShutdownResult{ | |
| #[must_use] | |
| pub(crate)structShutdownResult{ |
| let (shutdown_msg, mut monitor_update_opt, htlcs, local_shutdown_result) = | ||
| chan.get_shutdown(&self.signer_provider, their_features, target_feerate_sats_per_1000_weight, override_shutdown_script)?; | ||
| failed_htlcs = htlcs; | ||
| shutdown_result = local_shutdown_result; |
There was a problem hiding this comment.
Let's debug_assert our assumptions.
| shutdown_result = local_shutdown_result; | |
| shutdown_result = local_shutdown_result; | |
| debug_assert_eq!(shutdown_result.is_some(), chan.is_shutdown()); |
There was a problem hiding this comment.
Added this assertion to all the cooperative closing paths.
39e0b7d to
aa8a485CompareTheBlueMatt
commented
Oct 18, 2023
Ugh, sorry, this needs rebase due to a silent conflict with #2658. |
The check_closed_event function verified closure events against multiple counterparty nodes, but only a single closure reason and channel capacity. This commit introduces a check_closed_events function to verify events against descriptions of each expected event, and refactors check_closed_event in function of check_closed_events.
This refactors ShutdownResult as follows: - Makes ShutdownResult a struct instead of a tuple to represent individual results that need to be handled. This recently also includes funding batch closure propagations. - Makes Channel solely responsible for constructing ShutdownResult as it should own all channel-specific logic.
aa8a485 to
316a794Comparewvanlint
commented
Oct 19, 2023
No problem! Rebased. :) |
As a follow-up to #2486, this PR:
ShutdownResulttype and moves construction toChannelmethods.check_closed_eventto be able to share it for batch funding tests. The current implementation is not usable since there might be multipleClosureReasons. Let me know if this is not worth it though.