Skip to content

Generate latest local commitment transactions via monitor avoiding Channel's copy #551

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Mar 20, 2020
Merged

Generate latest local commitment transactions via monitor avoiding Channel's copy #551

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
32d62ad
Remove bogus mon_update_id += 1 fulfilling already-fulfilled HTLCs
TheBlueMatt Mar 19, 2020
82d40ee
Deduplicate HTLC preimage events from channelmonitor.
TheBlueMatt Mar 19, 2020
03b5da1
Broadcast final local txn via ChannelMonitorUpdate
TheBlueMatt Mar 18, 2020
e1c1ac7
Fetch latest local commitment txn via a macro in tests
TheBlueMatt Mar 19, 2020
feca83a
Fetch latest commitment txn via monitor, not channel in test
TheBlueMatt Mar 19, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion fuzz/src/chanmon_consistency.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ impl channelmonitor::ManyChannelMonitor<EnforcingChannelKeys> for TestChannelMon
};
let mut deserialized_monitor = <(Sha256d, channelmonitor::ChannelMonitor<EnforcingChannelKeys>)>::
read(&mut Cursor::new(&map_entry.get().1), Arc::clone(&self.logger)).unwrap().1;
deserialized_monitor.update_monitor(update.clone()).unwrap();
deserialized_monitor.update_monitor(update.clone(), &&TestBroadcaster {}).unwrap();
let mut ser = VecWriter(Vec::new());
deserialized_monitor.write_for_disk(&mut ser).unwrap();
map_entry.insert((update.update_id, ser.0));
Expand Down
4 changes: 2 additions & 2 deletions lightning/src/ln/chanmon_update_fail_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ fn test_simple_monitor_permanent_update_fail() {

*nodes[0].chan_monitor.update_ret.lock().unwrap() = Err(ChannelMonitorUpdateErr::PermanentFailure);
if let Err(APIError::ChannelUnavailable {..}) = nodes[0].node.send_payment(route, payment_hash_1) {} else { panic!(); }
check_added_monitors!(nodes[0], 1);
check_added_monitors!(nodes[0], 2);

let events_1 = nodes[0].node.get_and_clear_pending_msg_events();
assert_eq!(events_1.len(), 2);
Expand Down Expand Up @@ -120,7 +120,7 @@ fn do_test_simple_monitor_temporary_update_fail(disconnect: bool) {

// ...and make sure we can force-close a frozen channel
nodes[0].node.force_close_channel(&channel_id);
check_added_monitors!(nodes[0], 0);
check_added_monitors!(nodes[0], 1);
check_closed_broadcast!(nodes[0], false);

// TODO: Once we hit the chain with the failure transaction we should check that we get a
Expand Down
35 changes: 24 additions & 11 deletions lightning/src/ln/channel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1138,8 +1138,11 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
}

/// Per HTLC, only one get_update_fail_htlc or get_update_fulfill_htlc call may be made.
/// In such cases we debug_assert!(false) and return an IgnoreError. Thus, will always return
/// Ok(_) if debug assertions are turned on and preconditions are met.
/// In such cases we debug_assert!(false) and return a ChannelError::Ignore. Thus, will always
/// return Ok(_) if debug assertions are turned on or preconditions are met.
///
/// Note that it is still possible to hit these assertions in case we find a preimage on-chain
/// but then have a reorg which settles on an HTLC-failure on chain.
fn get_update_fulfill_htlc(&mut self, htlc_id_arg: u64, payment_preimage_arg: PaymentPreimage) -> Result<(Option<msgs::UpdateFulfillHTLC>, Option<ChannelMonitorUpdate>), ChannelError> {
// Either ChannelFunded got set (which means it won't be unset) or there is no way any
// caller thought we could have something claimed (cause we wouldn't have accepted in an
Expand Down Expand Up @@ -1167,6 +1170,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
} else {
log_warn!(self, "Have preimage and want to fulfill HTLC with payment hash {} we already failed against channel {}", log_bytes!(htlc.payment_hash.0), log_bytes!(self.channel_id()));
}
debug_assert!(false, "Tried to fulfill an HTLC that was already fail/fulfilled");
return Ok((None, None));
},
_ => {
Expand Down Expand Up @@ -1200,6 +1204,9 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
match pending_update {
&HTLCUpdateAwaitingACK::ClaimHTLC { htlc_id, .. } => {
if htlc_id_arg == htlc_id {
// Make sure we don't leave latest_monitor_update_id incremented here:
self.latest_monitor_update_id -= 1;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

c95f91b

Hitting this case would indicate a programming error right ? If so I think function description doesn't match anymore what we really do given we don't return IgnoreError anymore here. You should add a debug_assert too?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call. I fixed the underlying duplicate events, added the debug_assertions, and added a comment noting that it is possible to hit them in some reorg cases.

debug_assert!(false, "Tried to fulfill an HTLC that was already fulfilled");
return Ok((None, None));
}
},
Expand All @@ -1208,6 +1215,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
log_warn!(self, "Have preimage and want to fulfill HTLC with pending failure against channel {}", log_bytes!(self.channel_id()));
// TODO: We may actually be able to switch to a fulfill here, though its
// rare enough it may not be worth the complexity burden.
debug_assert!(false, "Tried to fulfill an HTLC that was already failed");
return Ok((None, Some(monitor_update)));
}
},
Expand Down Expand Up @@ -1259,8 +1267,11 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
}

/// Per HTLC, only one get_update_fail_htlc or get_update_fulfill_htlc call may be made.
/// In such cases we debug_assert!(false) and return an IgnoreError. Thus, will always return
/// Ok(_) if debug assertions are turned on and preconditions are met.
/// In such cases we debug_assert!(false) and return a ChannelError::Ignore. Thus, will always
/// return Ok(_) if debug assertions are turned on or preconditions are met.
///
/// Note that it is still possible to hit these assertions in case we find a preimage on-chain
/// but then have a reorg which settles on an HTLC-failure on chain.
pub fn get_update_fail_htlc(&mut self, htlc_id_arg: u64, err_packet: msgs::OnionErrorPacket) -> Result<Option<msgs::UpdateFailHTLC>, ChannelError> {
if (self.channel_state & (ChannelState::ChannelFunded as u32)) != (ChannelState::ChannelFunded as u32) {
panic!("Was asked to fail an HTLC when channel was not in an operational state");
Expand All @@ -1277,6 +1288,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
match htlc.state {
InboundHTLCState::Committed => {},
InboundHTLCState::LocalRemoved(_) => {
debug_assert!(false, "Tried to fail an HTLC that was already fail/fulfilled");
return Ok(None);
},
_ => {
Expand All @@ -1297,11 +1309,13 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
match pending_update {
&HTLCUpdateAwaitingACK::ClaimHTLC { htlc_id, .. } => {
if htlc_id_arg == htlc_id {
debug_assert!(false, "Tried to fail an HTLC that was already fulfilled");
return Err(ChannelError::Ignore("Unable to find a pending HTLC which matched the given HTLC ID"));
}
},
&HTLCUpdateAwaitingACK::FailHTLC { htlc_id, .. } => {
if htlc_id_arg == htlc_id {
debug_assert!(false, "Tried to fail an HTLC that was already failed");
return Err(ChannelError::Ignore("Unable to find a pending HTLC which matched the given HTLC ID"));
}
},
Expand Down Expand Up @@ -3760,7 +3774,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
/// those explicitly stated to be allowed after shutdown completes, eg some simple getters).
/// Also returns the list of payment_hashes for channels which we can safely fail backwards
/// immediately (others we will have to allow to time out).
pub fn force_shutdown(&mut self) -> (Vec<Transaction>, Vec<(HTLCSource, PaymentHash)>) {
pub fn force_shutdown(&mut self, should_broadcast: bool) -> (Option<OutPoint>, ChannelMonitorUpdate, Vec<(HTLCSource, PaymentHash)>) {
assert!(self.channel_state != ChannelState::ShutdownComplete as u32);

// We go ahead and "free" any holding cell HTLCs or HTLCs we haven't yet committed to and
Expand All @@ -3783,12 +3797,11 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {

self.channel_state = ChannelState::ShutdownComplete as u32;
self.update_time_counter += 1;
if self.channel_monitor.is_some() {
(self.channel_monitor.as_mut().unwrap().get_latest_local_commitment_txn(), dropped_outbound_htlcs)
} else {
// We aren't even signed funding yet, so can't broadcast anything
(Vec::new(), dropped_outbound_htlcs)
}
self.latest_monitor_update_id += 1;
(self.funding_txo.clone(), ChannelMonitorUpdate {
update_id: self.latest_monitor_update_id,
updates: vec![ChannelMonitorUpdateStep::ChannelForceClosed { should_broadcast }],
}, dropped_outbound_htlcs)
}
}

Expand Down
Loading