Uh oh!
There was an error while loading. Please reload this page.
Handle pre-startup and closed-channel monitor update completion actions#2391
Uh oh!
There was an error while loading. Please reload this page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -2346,6 +2346,7 @@ fn do_channel_holding_cell_serialize(disconnect: bool, reload_a: bool) { | ||
| RecipientOnionFields::secret_only(payment_secret_2), PaymentId(payment_hash_2.0)).unwrap(); | ||
| check_added_monitors!(nodes[0], 0); | ||
| let chan_0_monitor_serialized = get_monitor!(nodes[0], chan_id).encode(); | ||
| chanmon_cfgs[0].persister.set_update_ret(ChannelMonitorUpdateStatus::InProgress); | ||
| chanmon_cfgs[0].persister.set_update_ret(ChannelMonitorUpdateStatus::InProgress); | ||
| nodes[0].node.claim_funds(payment_preimage_0); | ||
| @@ -2365,8 +2366,9 @@ fn do_channel_holding_cell_serialize(disconnect: bool, reload_a: bool) { | ||
| // disconnect the peers. Note that the fuzzer originally found this issue because | ||
| // deserializing a ChannelManager in this state causes an assertion failure. | ||
| if reload_a { | ||
| reload_node!(nodes[0], &nodes[0].node.encode(), &[&chan_0_monitor_serialized], persister, new_chain_monitor, nodes_0_deserialized); | ||
| persister.set_update_ret(ChannelMonitorUpdateStatus::InProgress); | ||
| persister.set_update_ret(ChannelMonitorUpdateStatus::InProgress); | ||
| } else { | ||
| nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id()); | ||
| } | ||
| @@ -2406,9 +2408,14 @@ fn do_channel_holding_cell_serialize(disconnect: bool, reload_a: bool) { | ||
| assert_eq!(pending_cs.commitment_signed, cs); | ||
| } else { panic!(); } | ||
| if reload_a { | ||
| // The two pending monitor updates were replayed (but are still pending). | ||
| check_added_monitors(&nodes[0], 2); | ||
| } else { | ||
wpaulino marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| // There should be no monitor updates as we are still pending awaiting a failed one. | ||
| check_added_monitors(&nodes[0], 0); | ||
| } | ||
| check_added_monitors(&nodes[1], 0); | ||
| } | ||
| // If we finish updating the monitor, we should free the holding cell right away (this did | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -530,6 +530,13 @@ enum BackgroundEvent { | ||
| funding_txo: OutPoint, | ||
| update: ChannelMonitorUpdate | ||
| }, | ||
| /// Some [`ChannelMonitorUpdate`] (s) completed before we were serialized but we still have | ||
| /// them marked pending, thus we need to run any [`MonitorUpdateCompletionAction`] (s) pending | ||
| /// on a channel. | ||
valentinewallace marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| MonitorUpdatesComplete { | ||
| counterparty_node_id: PublicKey, | ||
| channel_id: [u8; 32], | ||
| }, | ||
| } | ||
| #[derive(Debug)] | ||
| @@ -4191,6 +4198,22 @@ where | ||
| } | ||
| let _ = handle_error!(self, res, counterparty_node_id); | ||
| }, | ||
| BackgroundEvent::MonitorUpdatesComplete { counterparty_node_id, channel_id } => { | ||
| let per_peer_state = self.per_peer_state.read().unwrap(); | ||
| if let Some(peer_state_mutex) = per_peer_state.get(&counterparty_node_id) { | ||
| let mut peer_state_lock = peer_state_mutex.lock().unwrap(); | ||
| let peer_state = &mut *peer_state_lock; | ||
| if let Some(chan) = peer_state.channel_by_id.get_mut(&channel_id) { | ||
| handle_monitor_update_completion!(self, peer_state_lock, peer_state, per_peer_state, chan); | ||
| } else { | ||
| let update_actions = peer_state.monitor_update_blocked_actions | ||
| .remove(&channel_id).unwrap_or(Vec::new()); | ||
| mem::drop(peer_state_lock); | ||
| mem::drop(per_peer_state); | ||
| self.handle_monitor_update_completion_actions(update_actions); | ||
| } | ||
| } | ||
| }, | ||
| } | ||
| } | ||
| NotifyOption::DoPersist | ||
| @@ -5001,24 +5024,29 @@ where | ||
| if peer_state_mutex_opt.is_none() { return } | ||
| peer_state_lock = peer_state_mutex_opt.unwrap().lock().unwrap(); | ||
| let peer_state = &mut *peer_state_lock; | ||
| let channel = | ||
| if let Some(chan) = peer_state.channel_by_id.get_mut(&funding_txo.to_channel_id()) { | ||
| chan | ||
| } else { | ||
| let update_actions = peer_state.monitor_update_blocked_actions | ||
| .remove(&funding_txo.to_channel_id()).unwrap_or(Vec::new()); | ||
| mem::drop(peer_state_lock); | ||
| mem::drop(per_peer_state); | ||
| self.handle_monitor_update_completion_actions(update_actions); | ||
| return; | ||
| }; | ||
| let remaining_in_flight = | ||
| if let Some(pending) = peer_state.in_flight_monitor_updates.get_mut(funding_txo) { | ||
| pending.retain(|upd| upd.update_id > highest_applied_update_id); | ||
| pending.len() | ||
| } else { 0 }; | ||
| log_trace!(self.logger, "ChannelMonitor updated to {}. Current highest is {}. {} pending in-flight updates.", | ||
| highest_applied_update_id, channel.context.get_latest_monitor_update_id(), | ||
| remaining_in_flight); | ||
| if !channel.is_awaiting_monitor_update() || channel.context.get_latest_monitor_update_id() != highest_applied_update_id { | ||
| return; | ||
| } | ||
| handle_monitor_update_completion!(self, peer_state_lock, peer_state, per_peer_state, channel); | ||
| } | ||
| /// Accepts a request to open a channel after a [`Event::OpenChannelRequest`]. | ||
| @@ -8513,6 +8541,16 @@ where | ||
| update: update.clone(), | ||
| }); | ||
| } | ||
| if $chan_in_flight_upds.is_empty() { | ||
Contributor There was a problem hiding this comment. Should we check if any updates were actually removed before generating the background event? CollaboratorAuthor There was a problem hiding this comment. We only get here if there were some updates in the | ||
| // We had some updates to apply, but it turns out they had completed before we | ||
| // were serialized, we just weren't notified of that. Thus, we may have to run | ||
| // the completion actions for any monitor updates, but otherwise are done. | ||
| pending_background_events.push( | ||
| BackgroundEvent::MonitorUpdatesComplete { | ||
| counterparty_node_id: $counterparty_node_id, | ||
| channel_id: $funding_txo.to_channel_id(), | ||
| }); | ||
| } | ||
| if $peer_state.in_flight_monitor_updates.insert($funding_txo, $chan_in_flight_upds).is_some() { | ||
| log_error!(args.logger, "Duplicate in-flight monitor update set for the same channel!"); | ||
| return Err(DecodeError::InvalidValue); | ||
| @@ -8905,6 +8943,12 @@ where | ||
| blocked_peer_state.lock().unwrap().actions_blocking_raa_monitor_updates | ||
| .entry(blocked_channel_outpoint.to_channel_id()) | ||
| .or_insert_with(Vec::new).push(blocking_action.clone()); | ||
| } else { | ||
| // If the channel we were blocking has closed, we don't need to | ||
TheBlueMatt marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| // worry about it - the blocked monitor update should never have | ||
| // been released from the `Channel` object so it can't have | ||
| // completed, and if the channel closed there's no reason to bother | ||
| // anymore. | ||
| } | ||
| } | ||
| } | ||
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't quite clear what issue the fuzzer had found before and if it would still find it after moving the initialization of
chan_0_monitor_serialized.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So when the test was written we had no concept for in-flight monitor updates - we expected the user to (magically) call monitor_update_completed on the monitor update that they'd previously marked in-flight after startup. With the change here to finally actually "complete" monitor updates that completed while we were shut down, we have to do something to mark the monitor updates as actually-not-complete, which we do by loading with the old monitor.