Uh oh!
There was an error while loading. Please reload this page.
Locked splice bug fixes during channel reestablishment - #4624
Conversation
👋 Thanks for assigning @jkczyz as a reviewer! |
I've completed a thorough re-review of the entire PR diff, examining all changes across
No issues found. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@## main #4624 +/- ##
==========================================
+ Coverage 86.59% 86.62% +0.03%
==========================================
Files 159 159 Lines 110420 110568 +148 Branches 110420 110568 +148 ==========================================
+ Hits 95619 95784 +165 + Misses 12267 12250 -17
Partials 2534 2534
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:
|
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
16fb802 to
24bac54Compare| let funding_locked_txid_sent_in_reestablish = | ||
| self.context.funding_locked_txid_sent_in_reestablish.take(); |
There was a problem hiding this comment.
Is there any situation where we'd get here before calling get_channel_reestablish?
There was a problem hiding this comment.
No because get_channel_reestablish gets called as soon as the peer is tracked in the manager as connected
| if let Some(channel_ready_msg) = need_lnd_workaround { | ||
| self.internal_channel_ready_with_peer_state(counterparty_node_id, &channel_ready_msg, peer_state)?; | ||
| } | ||
| }; | ||
| self.handle_holding_cell_free_result(holding_cell_res); | ||
| // A reestablish may infer a missed `splice_locked`; apply it before freeing holding | ||
| // cells so we don't generate commitment updates against stale splice state. | ||
| let post_splice_locked_update = if let Some(splice_locked) = inferred_splice_locked { | ||
| self.internal_splice_locked_with_peer_state(counterparty_node_id, &splice_locked, peer_state)? | ||
| } else { | ||
| None | ||
| }; | ||
| if let Some(channel_ready_msg) = need_lnd_workaround { | ||
| self.internal_channel_ready(counterparty_node_id, &channel_ready_msg)?; | ||
| } | ||
| let holding_cell_res = self.check_free_peer_holding_cells(peer_state); | ||
| (post_splice_locked_update, holding_cell_res) | ||
| }; | ||
| if let Some(splice_locked) = inferred_splice_locked { | ||
| self.internal_splice_locked(counterparty_node_id, &splice_locked)?; | ||
| if let Some(data) = post_splice_locked_update { | ||
| self.handle_post_monitor_update_chan_resume(data); | ||
| } | ||
| self.handle_holding_cell_free_result(holding_cell_res); |
There was a problem hiding this comment.
Should we do most of this inside of the hash_map::Entry::Occupied arm above? Was thinking we'd avoid the duplicate lookups, too.
There was a problem hiding this comment.
Even just doing this change wasn't really worth it since this isn't a hot path
| mem::drop(peer_state_lock); | ||
| mem::drop(per_peer_state); |
There was a problem hiding this comment.
Why don't we need to do the same in internal_channel_reestablish?
There was a problem hiding this comment.
Because there the peer state is declared within a nested scope
In most cases, we end up sending our `splice_locked` either implicitly during reestablishment via `ChannelReestablish::my_current_funding_locked`, or explicitly after reestablishment. However, we did not consider that it's possible for the node to be notified of the splice confirmation after connecting to their peer but prior to reestablishing their channel. In such cases, we need to explicitly send the `splice_locked` since it wasn't included in `my_current_funding_locked`, but only after the channel has been reestablished. Found by the chanmon_consistency fuzz target.
Upon channel reestablishment, we free our holding cells to send any pending updates to our peer. If we happened to implicitly lock a pending splice during reestablishment, we want to make sure any updates we send after the fact are considering the new channel state (post-splice), even if the update was queued while the splice was still pending. Therefore, we must always handle the inferred `splice_locked` first. Found by the `chanmon_consistency` fuzz target.
24bac54 to
5e14a3fCompare
This PR includes fixes for two bugs/edge cases when handling a locked splice during reestablishment. These were found by the
chanmon_consistencyfuzz target.