Skip to content

Two simple Channel cleanups #314

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 2 commits into from
Mar 22, 2019
Merged

Two simple Channel cleanups #314

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
29ed3e2
Move fail_reason into OutboundHTLCState states
TheBlueMatt Mar 3, 2019
8d9b1af
Add a useful log_trace before we release holding cell.
TheBlueMatt Mar 3, 2019
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
76 changes: 39 additions & 37 deletions src/ln/channel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,19 +106,19 @@ enum OutboundHTLCState {
Committed,
/// Remote removed this (outbound) HTLC. We're waiting on their commitment_signed to finalize
/// the change (though they'll need to revoke before we fail the payment).
RemoteRemoved,
RemoteRemoved(Option<HTLCFailReason>),
/// Remote removed this and sent a commitment_signed (implying we've revoke_and_ack'ed it), but
/// the remote side hasn't yet revoked their previous state, which we need them to do before we
/// can do any backwards failing. Implies AwaitingRemoteRevoke.
/// We also have not yet removed this HTLC in a commitment_signed message, and are waiting on a
/// remote revoke_and_ack on a previous state before we can do so.
AwaitingRemoteRevokeToRemove,
AwaitingRemoteRevokeToRemove(Option<HTLCFailReason>),
/// Remote removed this and sent a commitment_signed (implying we've revoke_and_ack'ed it), but
/// the remote side hasn't yet revoked their previous state, which we need them to do before we
/// can do any backwards failing. Implies AwaitingRemoteRevoke.
/// We have removed this HTLC in our latest commitment_signed and are now just waiting on a
/// revoke_and_ack to drop completely.
AwaitingRemovedRemoteRevoke,
AwaitingRemovedRemoteRevoke(Option<HTLCFailReason>),
Copy link

Choose a reason for hiding this comment

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

Given that a state flag misuse has been and could be a likely source of errors, maybe we should have a more verbose name to dissociate AwaitingRemoteRevokeToRemote and AwaitingRemovedRemoteRevoke, like AwaitingRemoteRevokeToFinalDrop ? Had to read again their definition to get the slightly difference between them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure! That's somewhat orthogonal, though, so I'll leave that for a later cleanup, see #323.

}

struct OutboundHTLCOutput {
Expand All @@ -128,8 +128,6 @@ struct OutboundHTLCOutput {
payment_hash: PaymentHash,
state: OutboundHTLCState,
source: HTLCSource,
/// If we're in a removed state, set if they failed, otherwise None
fail_reason: Option<HTLCFailReason>,
}

/// See AwaitingRemoteRevoke ChannelState for more info
Expand Down Expand Up @@ -858,9 +856,9 @@ impl Channel {
let (include, state_name) = match htlc.state {
OutboundHTLCState::LocalAnnounced(_) => (generated_by_local, "LocalAnnounced"),
OutboundHTLCState::Committed => (true, "Committed"),
OutboundHTLCState::RemoteRemoved => (generated_by_local, "RemoteRemoved"),
OutboundHTLCState::AwaitingRemoteRevokeToRemove => (generated_by_local, "AwaitingRemoteRevokeToRemove"),
OutboundHTLCState::AwaitingRemovedRemoteRevoke => (false, "AwaitingRemovedRemoteRevoke"),
OutboundHTLCState::RemoteRemoved(_) => (generated_by_local, "RemoteRemoved"),
OutboundHTLCState::AwaitingRemoteRevokeToRemove(_) => (generated_by_local, "AwaitingRemoteRevokeToRemove"),
OutboundHTLCState::AwaitingRemovedRemoteRevoke(_) => (false, "AwaitingRemovedRemoteRevoke"),
};

if include {
Expand All @@ -869,13 +867,11 @@ impl Channel {
} else {
log_trace!(self, " ...not including outbound HTLC {} (hash {}) with value {} due to state ({})", htlc.htlc_id, log_bytes!(htlc.payment_hash.0), htlc.amount_msat, state_name);
match htlc.state {
OutboundHTLCState::AwaitingRemoteRevokeToRemove|OutboundHTLCState::AwaitingRemovedRemoteRevoke => {
if htlc.fail_reason.is_none() {
value_to_self_msat_offset -= htlc.amount_msat as i64;
}
OutboundHTLCState::AwaitingRemoteRevokeToRemove(None)|OutboundHTLCState::AwaitingRemovedRemoteRevoke(None) => {
value_to_self_msat_offset -= htlc.amount_msat as i64;
},
OutboundHTLCState::RemoteRemoved => {
if !generated_by_local && htlc.fail_reason.is_none() {
OutboundHTLCState::RemoteRemoved(None) => {
if !generated_by_local {
value_to_self_msat_offset -= htlc.amount_msat as i64;
}
},
Expand Down Expand Up @@ -1644,10 +1640,9 @@ impl Channel {
OutboundHTLCState::LocalAnnounced(_) =>
return Err(ChannelError::Close("Remote tried to fulfill/fail HTLC before it had been committed")),
OutboundHTLCState::Committed => {
htlc.state = OutboundHTLCState::RemoteRemoved;
htlc.fail_reason = fail_reason;
htlc.state = OutboundHTLCState::RemoteRemoved(fail_reason);
},
OutboundHTLCState::AwaitingRemoteRevokeToRemove | OutboundHTLCState::AwaitingRemovedRemoteRevoke | OutboundHTLCState::RemoteRemoved =>
OutboundHTLCState::AwaitingRemoteRevokeToRemove(_) | OutboundHTLCState::AwaitingRemovedRemoteRevoke(_) | OutboundHTLCState::RemoteRemoved(_) =>
return Err(ChannelError::Close("Remote tried to fulfill/fail HTLC that they'd already fulfilled/failed")),
}
return Ok(&htlc.source);
Expand Down Expand Up @@ -1800,8 +1795,10 @@ impl Channel {
}
}
for htlc in self.pending_outbound_htlcs.iter_mut() {
if let OutboundHTLCState::RemoteRemoved = htlc.state {
htlc.state = OutboundHTLCState::AwaitingRemoteRevokeToRemove;
if let Some(fail_reason) = if let &mut OutboundHTLCState::RemoteRemoved(ref mut fail_reason) = &mut htlc.state {
Some(fail_reason.take())
} else { None } {
htlc.state = OutboundHTLCState::AwaitingRemoteRevokeToRemove(fail_reason);
need_our_commitment = true;
}
}
Expand Down Expand Up @@ -1854,6 +1851,8 @@ impl Channel {
fn free_holding_cell_htlcs(&mut self) -> Result<Option<(msgs::CommitmentUpdate, ChannelMonitor)>, ChannelError> {
assert_eq!(self.channel_state & ChannelState::MonitorUpdateFailed as u32, 0);
if self.holding_cell_htlc_updates.len() != 0 || self.holding_cell_update_fee.is_some() {
log_trace!(self, "Freeing holding cell with {} HTLC updates{}", self.holding_cell_htlc_updates.len(), if self.holding_cell_update_fee.is_some() { " and a fee update" } else { "" });

let mut htlc_updates = Vec::new();
mem::swap(&mut htlc_updates, &mut self.holding_cell_htlc_updates);
let mut update_add_htlcs = Vec::with_capacity(htlc_updates.len());
Expand Down Expand Up @@ -2019,9 +2018,9 @@ impl Channel {
} else { true }
});
pending_outbound_htlcs.retain(|htlc| {
if let OutboundHTLCState::AwaitingRemovedRemoteRevoke = htlc.state {
if let &OutboundHTLCState::AwaitingRemovedRemoteRevoke(ref fail_reason) = &htlc.state {
log_trace!(logger, " ...removing outbound AwaitingRemovedRemoteRevoke {}", log_bytes!(htlc.payment_hash.0));
if let Some(reason) = htlc.fail_reason.clone() { // We really want take() here, but, again, non-mut ref :(
if let Some(reason) = fail_reason.clone() { // We really want take() here, but, again, non-mut ref :(
revoked_htlcs.push((htlc.source.clone(), htlc.payment_hash, reason));
} else {
// They fulfilled, so we sent them money
Expand Down Expand Up @@ -2072,9 +2071,12 @@ impl Channel {
if let OutboundHTLCState::LocalAnnounced(_) = htlc.state {
log_trace!(logger, " ...promoting outbound LocalAnnounced {} to Committed", log_bytes!(htlc.payment_hash.0));
htlc.state = OutboundHTLCState::Committed;
} else if let OutboundHTLCState::AwaitingRemoteRevokeToRemove = htlc.state {
}
if let Some(fail_reason) = if let &mut OutboundHTLCState::AwaitingRemoteRevokeToRemove(ref mut fail_reason) = &mut htlc.state {
Some(fail_reason.take())
} else { None } {
log_trace!(logger, " ...promoting outbound AwaitingRemoteRevokeToRemove {} to AwaitingRemovedRemoteRevoke", log_bytes!(htlc.payment_hash.0));
htlc.state = OutboundHTLCState::AwaitingRemovedRemoteRevoke;
htlc.state = OutboundHTLCState::AwaitingRemovedRemoteRevoke(fail_reason);
require_commitment = true;
}
}
Expand Down Expand Up @@ -2230,7 +2232,7 @@ impl Channel {
self.next_remote_htlc_id -= inbound_drop_count;

for htlc in self.pending_outbound_htlcs.iter_mut() {
if let OutboundHTLCState::RemoteRemoved = htlc.state {
if let OutboundHTLCState::RemoteRemoved(_) = htlc.state {
// They sent us an update to remove this but haven't yet sent the corresponding
// commitment_signed, we need to move it back to Committed and they can re-send
// the update upon reconnection.
Expand Down Expand Up @@ -3248,7 +3250,6 @@ impl Channel {
cltv_expiry: cltv_expiry,
state: OutboundHTLCState::LocalAnnounced(Box::new(onion_routing_packet.clone())),
source,
fail_reason: None,
});

let res = msgs::UpdateAddHTLC {
Expand Down Expand Up @@ -3313,8 +3314,10 @@ impl Channel {
}
}
for htlc in self.pending_outbound_htlcs.iter_mut() {
if let OutboundHTLCState::AwaitingRemoteRevokeToRemove = htlc.state {
htlc.state = OutboundHTLCState::AwaitingRemovedRemoteRevoke;
if let Some(fail_reason) = if let &mut OutboundHTLCState::AwaitingRemoteRevokeToRemove(ref mut fail_reason) = &mut htlc.state {
Some(fail_reason.take())
} else { None } {
htlc.state = OutboundHTLCState::AwaitingRemovedRemoteRevoke(fail_reason);
}
}

Expand Down Expand Up @@ -3580,7 +3583,6 @@ impl Writeable for Channel {
htlc.cltv_expiry.write(writer)?;
htlc.payment_hash.write(writer)?;
htlc.source.write(writer)?;
write_option!(htlc.fail_reason);
match &htlc.state {
&OutboundHTLCState::LocalAnnounced(ref onion_packet) => {
0u8.write(writer)?;
Expand All @@ -3589,14 +3591,17 @@ impl Writeable for Channel {
&OutboundHTLCState::Committed => {
1u8.write(writer)?;
},
&OutboundHTLCState::RemoteRemoved => {
&OutboundHTLCState::RemoteRemoved(ref fail_reason) => {
2u8.write(writer)?;
write_option!(*fail_reason);
},
&OutboundHTLCState::AwaitingRemoteRevokeToRemove => {
&OutboundHTLCState::AwaitingRemoteRevokeToRemove(ref fail_reason) => {
3u8.write(writer)?;
write_option!(*fail_reason);
},
&OutboundHTLCState::AwaitingRemovedRemoteRevoke => {
&OutboundHTLCState::AwaitingRemovedRemoteRevoke(ref fail_reason) => {
4u8.write(writer)?;
write_option!(*fail_reason);
},
}
}
Expand Down Expand Up @@ -3759,13 +3764,12 @@ impl<R : ::std::io::Read> ReadableArgs<R, Arc<Logger>> for Channel {
cltv_expiry: Readable::read(reader)?,
payment_hash: Readable::read(reader)?,
source: Readable::read(reader)?,
fail_reason: Readable::read(reader)?,
state: match <u8 as Readable<R>>::read(reader)? {
0 => OutboundHTLCState::LocalAnnounced(Box::new(Readable::read(reader)?)),
1 => OutboundHTLCState::Committed,
2 => OutboundHTLCState::RemoteRemoved,
3 => OutboundHTLCState::AwaitingRemoteRevokeToRemove,
4 => OutboundHTLCState::AwaitingRemovedRemoteRevoke,
2 => OutboundHTLCState::RemoteRemoved(Readable::read(reader)?),
3 => OutboundHTLCState::AwaitingRemoteRevokeToRemove(Readable::read(reader)?),
4 => OutboundHTLCState::AwaitingRemovedRemoteRevoke(Readable::read(reader)?),
_ => return Err(DecodeError::InvalidValue),
},
});
Expand Down Expand Up @@ -4158,7 +4162,6 @@ mod tests {
payment_hash: PaymentHash([0; 32]),
state: OutboundHTLCState::Committed,
source: HTLCSource::dummy(),
fail_reason: None,
};
out.payment_hash.0 = Sha256::hash(&hex::decode("0202020202020202020202020202020202020202020202020202020202020202").unwrap()).into_inner();
out
Expand All @@ -4171,7 +4174,6 @@ mod tests {
payment_hash: PaymentHash([0; 32]),
state: OutboundHTLCState::Committed,
source: HTLCSource::dummy(),
fail_reason: None,
};
out.payment_hash.0 = Sha256::hash(&hex::decode("0303030303030303030303030303030303030303030303030303030303030303").unwrap()).into_inner();
out
Expand Down