-
Notifications
You must be signed in to change notification settings - Fork 442
Reconstruct ChannelManager forwarded HTLCs maps from Channels
#4227
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
4561bc5
4f055ac
c27093d
26992e1
005da38
7c4d021
64de989
cb398f6
a24dcff
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7778,6 +7778,20 @@ where | |
| Ok(()) | ||
| } | ||
|
|
||
| /// Useful for reconstructing the set of pending HTLCs when deserializing the `ChannelManager`. | ||
| pub(super) fn get_inbound_committed_update_adds(&self) -> Vec<msgs::UpdateAddHTLC> { | ||
| self.context | ||
| .pending_inbound_htlcs | ||
| .iter() | ||
| .filter_map(|htlc| match htlc.state { | ||
| InboundHTLCState::Committed { ref update_add_htlc_opt } => { | ||
| update_add_htlc_opt.clone() | ||
| }, | ||
| _ => None, | ||
| }) | ||
| .collect() | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bleh, the common case as of this PR is that we don't actually use the newly-cloned onion, they're generally always forwarded already. Maybe we should just fix that part and not worry about it here, but it would be nice to avoid copying everything every time just to not need it.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is addressed in #4303 because (as of that PR) we prune inbound onions once they're irrevocably forwarded, so if they're pruned they can't be cloned here. But let me know if that's off. |
||
| } | ||
|
|
||
| /// Marks an outbound HTLC which we have received update_fail/fulfill/malformed | ||
| #[inline] | ||
| fn mark_outbound_htlc_removed( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17358,6 +17358,7 @@ where | |
| decode_update_add_htlcs_legacy.unwrap_or_else(|| new_hash_map()); | ||
| let mut pending_intercepted_htlcs_legacy = | ||
| pending_intercepted_htlcs_legacy.unwrap_or_else(|| new_hash_map()); | ||
| let mut decode_update_add_htlcs = new_hash_map(); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we have logic to decode into both simultaneously. If we have
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Part of the reason was to get test coverage for the new paths and gain confidence in them with prod usage. Seems nice to have the same behavior in test and prod. But we could instead always prefer the old maps in prod + avoid writing the old maps in tests only, would you prefer something along those lines?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That might be simpler. I'm fine with wanting to use the new code, but we have a lot of extra logic to handle both objects which seems like we're trying too hard when we could just do what you describe here. Also nothing wrong with keeping the old logic in prod for now...
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in #4289 |
||
| let peer_storage_dir: Vec<(PublicKey, Vec<u8>)> = peer_storage_dir.unwrap_or_else(Vec::new); | ||
| if fake_scid_rand_bytes.is_none() { | ||
| fake_scid_rand_bytes = Some(args.entropy_source.get_secure_random_bytes()); | ||
|
|
@@ -17669,6 +17670,21 @@ where | |
| let mut peer_state_lock = peer_state_mtx.lock().unwrap(); | ||
| let peer_state = &mut *peer_state_lock; | ||
| is_channel_closed = !peer_state.channel_by_id.contains_key(channel_id); | ||
| if let Some(chan) = peer_state.channel_by_id.get(channel_id) { | ||
| if let Some(funded_chan) = chan.as_funded() { | ||
| let inbound_committed_update_adds = | ||
| funded_chan.get_inbound_committed_update_adds(); | ||
| if !inbound_committed_update_adds.is_empty() { | ||
| // Reconstruct `ChannelManager::decode_update_add_htlcs` from the serialized | ||
| // `Channel`, as part of removing the requirement to regularly persist the | ||
| // `ChannelManager`. | ||
| decode_update_add_htlcs.insert( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can't this be done in the next loop over the monitors where the update_add_htlcs are deduped? This loop seems to be about payments.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, IMO the first loop is about repopulating HTLC maps from monitors, and the second loop is about pruning maps from monitors (hence having two loops so we don't prune before the maps are completely filled out), so this approach seems to fit.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought the first loop is about repopulating (outbound) payments. We probably mean the same thing.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But just for populating decode_update_add_htlcs, I don't think there is a two step processes need. I've been playing around a bit along this line. Not completely the same, but you get the idea. diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 299ff60ba..5fedf6fb9 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -17670,21 +17670,6 @@ where
let mut peer_state_lock = peer_state_mtx.lock().unwrap();
let peer_state = &mut *peer_state_lock;
is_channel_closed = !peer_state.channel_by_id.contains_key(channel_id);
- if let Some(chan) = peer_state.channel_by_id.get(channel_id) {
- if let Some(funded_chan) = chan.as_funded() {
- let inbound_committed_update_adds =
- funded_chan.get_inbound_committed_update_adds();
- if !inbound_committed_update_adds.is_empty() {
- // Reconstruct `ChannelManager::decode_update_add_htlcs` from the serialized
- // `Channel`, as part of removing the requirement to regularly persist the
- // `ChannelManager`.
- decode_update_add_htlcs.insert(
- funded_chan.context.outbound_scid_alias(),
- inbound_committed_update_adds,
- );
- }
- }
- }
}
if is_channel_closed {
@@ -17719,9 +17704,17 @@ where
for (channel_id, monitor) in args.channel_monitors.iter() {
let mut is_channel_closed = true;
let counterparty_node_id = monitor.get_counterparty_node_id();
+ let mut inbound_committed_update_adds = Vec::new();
if let Some(peer_state_mtx) = per_peer_state.get(&counterparty_node_id) {
let mut peer_state_lock = peer_state_mtx.lock().unwrap();
let peer_state = &mut *peer_state_lock;
+ if let Some(chan) = peer_state.channel_by_id.get(channel_id) {
+ if let Some(funded_chan) = chan.as_funded() {
+ inbound_committed_update_adds =
+ funded_chan.get_inbound_committed_update_adds();
+ }
+ }
+
is_channel_closed = !peer_state.channel_by_id.contains_key(channel_id);
}
@@ -17746,12 +17739,9 @@ where
// still have an entry for this HTLC in `forward_htlcs`,
// `pending_intercepted_htlcs`, or `decode_update_add_htlcs`, we were apparently not
// persisted after the monitor was when forwarding the payment.
- dedup_decode_update_add_htlcs(
- &mut decode_update_add_htlcs,
- &prev_hop_data,
- "HTLC was forwarded to the closed channel",
- &args.logger,
- );
+ inbound_committed_update_adds.retain(|update_add| {
+ update_add.htlc_id != prev_hop_data.htlc_id
+ });
dedup_decode_update_add_htlcs(
&mut decode_update_add_htlcs_legacy,
&prev_hop_data,
@@ -17877,6 +17867,8 @@ where
}
}
+ decode_update_add_htlcs.insert(*channel_id, inbound_committed_update_adds);
+
// Whether the downstream channel was closed or not, try to re-apply any payment
// preimages from it which may be needed in upstream channels for forwarded
// payments.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are tests passing for you with that diff? I tried moving the repopulating down to the lower loop and got test failures
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't try, but they also fail for me. Did a few more restructurings below that make the tests pass. But the restructure did lead to the realization that when the channel is closed, there are no diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 299ff60ba..8a101e791 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -17670,21 +17670,6 @@ where
let mut peer_state_lock = peer_state_mtx.lock().unwrap();
let peer_state = &mut *peer_state_lock;
is_channel_closed = !peer_state.channel_by_id.contains_key(channel_id);
- if let Some(chan) = peer_state.channel_by_id.get(channel_id) {
- if let Some(funded_chan) = chan.as_funded() {
- let inbound_committed_update_adds =
- funded_chan.get_inbound_committed_update_adds();
- if !inbound_committed_update_adds.is_empty() {
- // Reconstruct `ChannelManager::decode_update_add_htlcs` from the serialized
- // `Channel`, as part of removing the requirement to regularly persist the
- // `ChannelManager`.
- decode_update_add_htlcs.insert(
- funded_chan.context.outbound_scid_alias(),
- inbound_committed_update_adds,
- );
- }
- }
- }
}
if is_channel_closed {
@@ -17717,12 +17702,21 @@ where
}
}
for (channel_id, monitor) in args.channel_monitors.iter() {
+ let mut chan_state = None;
let mut is_channel_closed = true;
let counterparty_node_id = monitor.get_counterparty_node_id();
if let Some(peer_state_mtx) = per_peer_state.get(&counterparty_node_id) {
let mut peer_state_lock = peer_state_mtx.lock().unwrap();
let peer_state = &mut *peer_state_lock;
- is_channel_closed = !peer_state.channel_by_id.contains_key(channel_id);
+ if let Some(chan) = peer_state.channel_by_id.get(channel_id) {
+ if let Some(funded_chan) = chan.as_funded() {
+ chan_state = Some((
+ funded_chan.context.outbound_scid_alias(),
+ funded_chan.get_inbound_committed_update_adds(),
+ ));
+ }
+ is_channel_closed = false;
+ }
}
if is_channel_closed {
@@ -17746,12 +17740,12 @@ where
// still have an entry for this HTLC in `forward_htlcs`,
// `pending_intercepted_htlcs`, or `decode_update_add_htlcs`, we were apparently not
// persisted after the monitor was when forwarding the payment.
- dedup_decode_update_add_htlcs(
- &mut decode_update_add_htlcs,
- &prev_hop_data,
- "HTLC was forwarded to the closed channel",
- &args.logger,
- );
+
+ // No need to do this because chan_state is None?
+ //
+ // chan_state.1.retain(|update_add| {
+ // update_add.htlc_id != prev_hop_data.htlc_id
+ // });
dedup_decode_update_add_htlcs(
&mut decode_update_add_htlcs_legacy,
&prev_hop_data,
@@ -17875,6 +17869,12 @@ where
completion_action,
));
}
+
+ // Nothing to add because the channel is closed??
+ //
+ // if !chan_state.1.is_empty() {
+ // decode_update_add_htlcs.insert(chan_state.0, chan_state.1);
+ // }
}
// Whether the downstream channel was closed or not, try to re-apply any payment
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After some staring and print statements, what's missing from your thoughts above/our offline discussion is the case where we add an HTLC forward to
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh interesting, didn't think of that. Do you want to add that explanation as a code comment, so that the knowledge is recorded?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in #4289 |
||
| funded_chan.context.outbound_scid_alias(), | ||
| inbound_committed_update_adds, | ||
| ); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if is_channel_closed { | ||
|
|
@@ -17727,9 +17743,15 @@ where | |
| }; | ||
| // The ChannelMonitor is now responsible for this HTLC's | ||
| // failure/success and will let us know what its outcome is. If we | ||
| // still have an entry for this HTLC in `forward_htlcs` or | ||
| // `pending_intercepted_htlcs`, we were apparently not persisted after | ||
| // the monitor was when forwarding the payment. | ||
| // still have an entry for this HTLC in `forward_htlcs`, | ||
| // `pending_intercepted_htlcs`, or `decode_update_add_htlcs`, we were apparently not | ||
| // persisted after the monitor was when forwarding the payment. | ||
| dedup_decode_update_add_htlcs( | ||
| &mut decode_update_add_htlcs, | ||
| &prev_hop_data, | ||
| "HTLC was forwarded to the closed channel", | ||
| &args.logger, | ||
| ); | ||
| dedup_decode_update_add_htlcs( | ||
| &mut decode_update_add_htlcs_legacy, | ||
| &prev_hop_data, | ||
|
|
@@ -18220,6 +18242,31 @@ where | |
| } | ||
| } | ||
|
|
||
| // De-duplicate HTLCs that are present in both `failed_htlcs` and `decode_update_add_htlcs`. | ||
| // Omitting this de-duplication could lead to redundant HTLC processing and/or bugs. | ||
valentinewallace marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| for (src, _, _, _, _, _) in failed_htlcs.iter() { | ||
| if let HTLCSource::PreviousHopData(prev_hop_data) = src { | ||
| dedup_decode_update_add_htlcs( | ||
valentinewallace marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| &mut decode_update_add_htlcs, | ||
| prev_hop_data, | ||
| "HTLC was failed backwards during manager read", | ||
| &args.logger, | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| // See above comment on `failed_htlcs`. | ||
| for htlcs in claimable_payments.values().map(|pmt| &pmt.htlcs) { | ||
valentinewallace marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| for prev_hop_data in htlcs.iter().map(|h| &h.prev_hop) { | ||
| dedup_decode_update_add_htlcs( | ||
| &mut decode_update_add_htlcs, | ||
| prev_hop_data, | ||
| "HTLC was already decoded and marked as a claimable payment", | ||
| &args.logger, | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| let best_block = BestBlock::new(best_block_hash, best_block_height); | ||
| let flow = OffersMessageFlow::new( | ||
| chain_hash, | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.