From: Matt Corallo Date: Sat, 28 Jul 2018 22:32:43 +0000 (-0400) Subject: Clean up forward_/claimable_htlcs handling and document consistency X-Git-Tag: v0.0.12~359^2~1 X-Git-Url: http://git.bitcoin.ninja/?a=commitdiff_plain;h=5c32e3cb91e98c84d8ac8093229ded2d912d5b10;p=rust-lightning Clean up forward_/claimable_htlcs handling and document consistency --- diff --git a/src/ln/channelmanager.rs b/src/ln/channelmanager.rs index 67837af71..45abf6960 100644 --- a/src/ln/channelmanager.rs +++ b/src/ln/channelmanager.rs @@ -115,14 +115,19 @@ struct ChannelHolder { short_to_id: HashMap, next_forward: Instant, /// short channel id -> forward infos. Key of 0 means payments received + /// Note that while this is held in the same mutex as the channels themselves, no consistency + /// guarantees are made about there existing a channel with the short id here, nor the short + /// ids in the PendingForwardHTLCInfo! forward_htlcs: HashMap>, + /// Note that while this is held in the same mutex as the channels themselves, no consistency + /// guarantees are made about the channels given here actually existing anymore by the time you + /// go to read them! claimable_htlcs: HashMap<[u8; 32], PendingOutboundHTLC>, } struct MutChannelHolder<'a> { by_id: &'a mut HashMap<[u8; 32], Channel>, short_to_id: &'a mut HashMap, next_forward: &'a mut Instant, - /// short channel id -> forward infos. Key of 0 means payments received forward_htlcs: &'a mut HashMap>, claimable_htlcs: &'a mut HashMap<[u8; 32], PendingOutboundHTLC>, } @@ -884,6 +889,12 @@ impl ChannelManager { self.fail_htlc_backwards_internal(self.channel_state.lock().unwrap(), payment_hash, HTLCFailReason::Reason { failure_code: 0x4000 | 15, data: Vec::new() }) } + /// Fails an HTLC backwards to the sender of it to us. + /// Note that while we take a channel_state lock as input, we do *not* assume consistency here. + /// There are several callsites that do stupid things like loop over a list of payment_hashes + /// to fail and take the channel_state lock for each iteration (as we take ownership and may + /// drop it). In other words, no assumptions are made that entries in claimable_htlcs point to + /// still-available channels. fn fail_htlc_backwards_internal(&self, mut channel_state: MutexGuard, payment_hash: &[u8; 32], onion_error: HTLCFailReason) -> bool { let mut pending_htlc = { match channel_state.claimable_htlcs.remove(payment_hash) { @@ -904,7 +915,7 @@ impl ChannelManager { } match pending_htlc { - PendingOutboundHTLC::CycledRoute { .. } => { panic!("WAT"); }, + PendingOutboundHTLC::CycledRoute { .. } => unreachable!(), PendingOutboundHTLC::OutboundRoute { .. } => { mem::drop(channel_state); @@ -1000,7 +1011,7 @@ impl ChannelManager { } match pending_htlc { - PendingOutboundHTLC::CycledRoute { .. } => { panic!("WAT"); }, + PendingOutboundHTLC::CycledRoute { .. } => unreachable!(), PendingOutboundHTLC::OutboundRoute { .. } => { if from_user { panic!("Called claim_funds with a preimage for an outgoing payment. There is nothing we can do with this, and something is seriously wrong if you knew this..."); @@ -1016,13 +1027,20 @@ impl ChannelManager { let (node_id, fulfill_msgs) = { let chan_id = match channel_state.short_to_id.get(&source_short_channel_id) { Some(chan_id) => chan_id.clone(), - None => return false + None => { + // TODO: There is probably a channel manager somewhere that needs to + // learn the preimage as the channel already hit the chain and that's + // why its missing. + return false + } }; let chan = channel_state.by_id.get_mut(&chan_id).unwrap(); match chan.get_update_fulfill_htlc_and_commit(payment_preimage) { Ok(msg) => (chan.get_their_node_id(), msg), Err(_e) => { + // TODO: There is probably a channel manager somewhere that needs to + // learn the preimage as the channel may be about to hit the chain. //TODO: Do something with e? return false; }, @@ -1571,7 +1589,7 @@ impl ChannelMessageHandler for ChannelManager { pending_forward_info.prev_short_channel_id = short_channel_id; (short_channel_id, chan.update_add_htlc(&msg, pending_forward_info)?) }, - None => return Err(HandleError{err: "Failed to find corresponding channel", action: None}), //TODO: panic? + None => return Err(HandleError{err: "Failed to find corresponding channel", action: None}), }; match claimable_htlcs_entry { @@ -1581,7 +1599,7 @@ impl ChannelMessageHandler for ChannelManager { &mut PendingOutboundHTLC::OutboundRoute { ref route, ref session_priv } => { (route.clone(), session_priv.clone()) }, - _ => { panic!("WAT") }, + _ => unreachable!(), }; *outbound_route = PendingOutboundHTLC::CycledRoute { source_short_channel_id,