Require all HTLC success states to hold their corresponding preimage

This allows us to DRY the code that calculates the
`value_to_self_msat_offset` in `ChannelContext::build_commitment_stats`.

HTLC success states have held their corresponding preimage since
0.0.105, and the release notes of 0.1 already require users running
0.0.123 and earlier to resolve their HTLCs before upgrading to 0.1. So
this change is fully compatible with existing upgrade paths to
the yet-to-be-shipped 0.2 release.

Author: tankyleo

lightning/src/ln/channel.rs

@@ -364,9 +364,11 @@ impl OutboundHTLCState {
 
 	fn preimage(&self) -> Option<PaymentPreimage> {
 		match self {
-			OutboundHTLCState::RemoteRemoved(OutboundHTLCOutcome::Success(p)) |
-			OutboundHTLCState::AwaitingRemoteRevokeToRemove(OutboundHTLCOutcome::Success(p)) |
-			OutboundHTLCState::AwaitingRemovedRemoteRevoke(OutboundHTLCOutcome::Success(p)) => p.as_ref().copied(),
+			OutboundHTLCState::RemoteRemoved(OutboundHTLCOutcome::Success(preimage)) |
+			OutboundHTLCState::AwaitingRemoteRevokeToRemove(OutboundHTLCOutcome::Success(preimage)) |
+			OutboundHTLCState::AwaitingRemovedRemoteRevoke(OutboundHTLCOutcome::Success(preimage)) => {
+			Some(*preimage)
+		},
 			_ => None,
 		}
 	}
@@ -375,20 +377,12 @@ impl OutboundHTLCState {
 #[derive(Clone)]
 #[cfg_attr(test, derive(Debug, PartialEq))]
 enum OutboundHTLCOutcome {
-	/// LDK version 0.0.105+ will always fill in the preimage here.
-	Success(Option<PaymentPreimage>),
+	/// We started always filling in the preimages here in 0.0.105, and the requirement
+	/// that the preimages always be filled in was added in 0.2.
+	Success(PaymentPreimage),
 	Failure(HTLCFailReason),
 }
 
-impl From<Option<HTLCFailReason>> for OutboundHTLCOutcome {
-	fn from(o: Option<HTLCFailReason>) -> Self {
-		match o {
-			None => OutboundHTLCOutcome::Success(None),
-			Some(r) => OutboundHTLCOutcome::Failure(r)
-		}
-	}
-}
-
 impl<'a> Into<Option<&'a HTLCFailReason>> for &'a OutboundHTLCOutcome {
 	fn into(self) -> Option<&'a HTLCFailReason> {
 		match self {
@@ -3876,7 +3870,7 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
 				}
 				remote_htlc_total_msat += htlc.amount_msat;
 			} else {
-				if let InboundHTLCState::LocalRemoved(InboundHTLCRemovalReason::Fulfill(_preimage)) = htlc.state {
+				if htlc.state.preimage().is_some() {
 					value_to_self_msat_offset += htlc.amount_msat as i64;
 				}
 			}
@@ -3889,10 +3883,7 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
 				}
 				local_htlc_total_msat += htlc.amount_msat;
 			} else {
-				if let OutboundHTLCState::AwaitingRemoteRevokeToRemove(OutboundHTLCOutcome::Success(_)) |
-					OutboundHTLCState::AwaitingRemovedRemoteRevoke(OutboundHTLCOutcome::Success(_)) |
-					OutboundHTLCState::RemoteRemoved(OutboundHTLCOutcome::Success(_))
-				= htlc.state {
+				if htlc.state.preimage().is_some() {
 					value_to_self_msat_offset -= htlc.amount_msat as i64;
 				}
 			}
@@ -5801,16 +5792,18 @@ impl<SP: Deref> FundedChannel<SP> where
 	#[inline]
 	fn mark_outbound_htlc_removed(&mut self, htlc_id: u64, check_preimage: Option<PaymentPreimage>, fail_reason: Option<HTLCFailReason>) -> Result<&OutboundHTLCOutput, ChannelError> {
 		assert!(!(check_preimage.is_some() && fail_reason.is_some()), "cannot fail while we have a preimage");
+		assert!(!(check_preimage.is_none() && fail_reason.is_none()), "either provide a preimage, or a failure reason");
 		for htlc in self.context.pending_outbound_htlcs.iter_mut() {
 			if htlc.htlc_id == htlc_id {
 				let outcome = match check_preimage {
-					None => fail_reason.into(),
+					// We checked that either check_preimage or fail_reason is set above, so we can safely unwrap here
+					None => OutboundHTLCOutcome::Failure(fail_reason.unwrap()),
 					Some(payment_preimage) => {
 						let payment_hash = PaymentHash(Sha256::hash(&payment_preimage.0[..]).to_byte_array());
 						if payment_hash != htlc.payment_hash {
 							return Err(ChannelError::close(format!("Remote tried to fulfill HTLC ({}) with an incorrect preimage", htlc_id)));
 						}
-						OutboundHTLCOutcome::Success(Some(payment_preimage))
+						OutboundHTLCOutcome::Success(payment_preimage)
 					}
 				};
 				match htlc.state {
@@ -6014,10 +6007,10 @@ impl<SP: Deref> FundedChannel<SP> where
 			if let &mut OutboundHTLCState::RemoteRemoved(ref mut outcome) = &mut htlc.state {
 				log_trace!(logger, "Updating HTLC {} to AwaitingRemoteRevokeToRemove due to commitment_signed in channel {}.",
 					&htlc.payment_hash, &self.context.channel_id);
-				// Grab the preimage, if it exists, instead of cloning
-				let mut reason = OutboundHTLCOutcome::Success(None);
+				// Swap against a dummy variant to avoid a potentially expensive clone of `OutboundHTLCOutcome::Failure(HTLCFailReason)`
+				let mut reason = OutboundHTLCOutcome::Success(PaymentPreimage([0u8; 32]));
 				mem::swap(outcome, &mut reason);
-				if let OutboundHTLCOutcome::Success(Some(preimage)) = reason {
+				if let OutboundHTLCOutcome::Success(preimage) = reason {
 					// If a user (a) receives an HTLC claim using LDK 0.0.104 or before, then (b)
 					// upgrades to LDK 0.0.114 or later before the HTLC is fully resolved, we could
 					// have a `Success(None)` reason. In this case we could forget some HTLC
@@ -6435,8 +6428,8 @@ impl<SP: Deref> FundedChannel<SP> where
 				}
 				if let &mut OutboundHTLCState::AwaitingRemoteRevokeToRemove(ref mut outcome) = &mut htlc.state {
 					log_trace!(logger, " ...promoting outbound AwaitingRemoteRevokeToRemove {} to AwaitingRemovedRemoteRevoke", &htlc.payment_hash);
-					// Grab the preimage, if it exists, instead of cloning
-					let mut reason = OutboundHTLCOutcome::Success(None);
+					// Swap against a dummy variant to avoid a potentially expensive clone of `OutboundHTLCOutcome::Failure(HTLCFailReason)`
+					let mut reason = OutboundHTLCOutcome::Success(PaymentPreimage([0u8; 32]));
 					mem::swap(outcome, &mut reason);
 					htlc.state = OutboundHTLCState::AwaitingRemovedRemoteRevoke(reason);
 					require_commitment = true;
@@ -9011,8 +9004,8 @@ impl<SP: Deref> FundedChannel<SP> where
 		for htlc in self.context.pending_outbound_htlcs.iter_mut() {
 			if let &mut OutboundHTLCState::AwaitingRemoteRevokeToRemove(ref mut outcome) = &mut htlc.state {
 				log_trace!(logger, " ...promoting outbound AwaitingRemoteRevokeToRemove {} to AwaitingRemovedRemoteRevoke", &htlc.payment_hash);
-				// Grab the preimage, if it exists, instead of cloning
-				let mut reason = OutboundHTLCOutcome::Success(None);
+				// Swap against a dummy variant to avoid a potentially expensive clone of `OutboundHTLCOutcome::Failure(HTLCFailReason)`
+				let mut reason = OutboundHTLCOutcome::Success(PaymentPreimage([0u8; 32]));
 				mem::swap(outcome, &mut reason);
 				htlc.state = OutboundHTLCState::AwaitingRemovedRemoteRevoke(reason);
 			}
@@ -10637,7 +10630,9 @@ impl<SP: Deref> Writeable for FundedChannel<SP> where SP::Target: SignerProvider
 			}
 		}
 
-		let mut preimages: Vec<&Option<PaymentPreimage>> = vec![];
+		// The elements of this vector will always be `Some` starting in 0.2,
+		// but we still serialize the option to maintain backwards compatibility
+		let mut preimages: Vec<Option<&PaymentPreimage>> = vec![];
 		let mut pending_outbound_skimmed_fees: Vec<Option<u64>> = Vec::new();
 		let mut pending_outbound_blinding_points: Vec<Option<PublicKey>> = Vec::new();
 
@@ -10664,15 +10659,15 @@ impl<SP: Deref> Writeable for FundedChannel<SP> where SP::Target: SignerProvider
 				&OutboundHTLCState::AwaitingRemoteRevokeToRemove(ref outcome) => {
 					3u8.write(writer)?;
 					if let OutboundHTLCOutcome::Success(preimage) = outcome {
-						preimages.push(preimage);
+						preimages.push(Some(preimage));
 					}
 					let reason: Option<&HTLCFailReason> = outcome.into();
 					reason.write(writer)?;
 				}
 				&OutboundHTLCState::AwaitingRemovedRemoteRevoke(ref outcome) => {
 					4u8.write(writer)?;
 					if let OutboundHTLCOutcome::Success(preimage) = outcome {
-						preimages.push(preimage);
+						preimages.push(Some(preimage));
 					}
 					let reason: Option<&HTLCFailReason> = outcome.into();
 					reason.write(writer)?;
@@ -11011,15 +11006,30 @@ impl<'a, 'b, 'c, ES: Deref, SP: Deref> ReadableArgs<(&'a ES, &'b SP, &'c Channel
 					1 => OutboundHTLCState::Committed,
 					2 => {
 						let option: Option<HTLCFailReason> = Readable::read(reader)?;
-						OutboundHTLCState::RemoteRemoved(option.into())
+						let outcome = match option {
+							Some(r) => OutboundHTLCOutcome::Failure(r),
+							// Initialize this variant with a dummy preimage, the actual preimage will be filled in further down
+							None => OutboundHTLCOutcome::Success(PaymentPreimage([0u8; 32])),
+						};
+						OutboundHTLCState::RemoteRemoved(outcome)
 					},
 					3 => {
 						let option: Option<HTLCFailReason> = Readable::read(reader)?;
-						OutboundHTLCState::AwaitingRemoteRevokeToRemove(option.into())
+						let outcome = match option {
+							Some(r) => OutboundHTLCOutcome::Failure(r),
+							// Initialize this variant with a dummy preimage, the actual preimage will be filled in further down
+							None => OutboundHTLCOutcome::Success(PaymentPreimage([0u8; 32])),
+						};
+						OutboundHTLCState::AwaitingRemoteRevokeToRemove(outcome)
 					},
 					4 => {
 						let option: Option<HTLCFailReason> = Readable::read(reader)?;
-						OutboundHTLCState::AwaitingRemovedRemoteRevoke(option.into())
+						let outcome = match option {
+							Some(r) => OutboundHTLCOutcome::Failure(r),
+							// Initialize this variant with a dummy preimage, the actual preimage will be filled in further down
+							None => OutboundHTLCOutcome::Success(PaymentPreimage([0u8; 32])),
+						};
+						OutboundHTLCState::AwaitingRemovedRemoteRevoke(outcome)
 					},
 					_ => return Err(DecodeError::InvalidValue),
 				},
@@ -11166,7 +11176,9 @@ impl<'a, 'b, 'c, ES: Deref, SP: Deref> ReadableArgs<(&'a ES, &'b SP, &'c Channel
 		// only, so we default to that if none was written.
 		let mut channel_type = Some(ChannelTypeFeatures::only_static_remote_key());
 		let mut channel_creation_height = 0u32;
-		let mut preimages_opt: Option<Vec<Option<PaymentPreimage>>> = None;
+		// Starting in 0.2, all the elements in this vector will be `Some`, but they are still
+		// serialized as options to maintain backwards compatibility
+		let mut preimages: Vec<Option<PaymentPreimage>> = Vec::new();
 
 		// If we read an old Channel, for simplicity we just treat it as "we never sent an
 		// AnnouncementSignatures" which implies we'll re-send it on reconnect, but that's fine.
@@ -11220,7 +11232,7 @@ impl<'a, 'b, 'c, ES: Deref, SP: Deref> ReadableArgs<(&'a ES, &'b SP, &'c Channel
 			(10, monitor_pending_update_adds, option), // Added in 0.0.122
 			(11, monitor_pending_finalized_fulfills, optional_vec),
 			(13, channel_creation_height, required),
-			(15, preimages_opt, optional_vec),
+			(15, preimages, required_vec), // The preimages transitioned from optional to required in 0.2
 			(17, announcement_sigs_state, required),
 			(19, latest_inbound_scid_alias, option),
 			(21, outbound_scid_alias, required),
@@ -11248,23 +11260,27 @@ impl<'a, 'b, 'c, ES: Deref, SP: Deref> ReadableArgs<(&'a ES, &'b SP, &'c Channel
 
 		let holder_signer = signer_provider.derive_channel_signer(channel_keys_id);
 
-		if let Some(preimages) = preimages_opt {
-			let mut iter = preimages.into_iter();
-			for htlc in pending_outbound_htlcs.iter_mut() {
-				match &htlc.state {
-					OutboundHTLCState::AwaitingRemoteRevokeToRemove(OutboundHTLCOutcome::Success(None)) => {
-						htlc.state = OutboundHTLCState::AwaitingRemoteRevokeToRemove(OutboundHTLCOutcome::Success(iter.next().ok_or(DecodeError::InvalidValue)?));
-					}
-					OutboundHTLCState::AwaitingRemovedRemoteRevoke(OutboundHTLCOutcome::Success(None)) => {
-						htlc.state = OutboundHTLCState::AwaitingRemovedRemoteRevoke(OutboundHTLCOutcome::Success(iter.next().ok_or(DecodeError::InvalidValue)?));
-					}
-					_ => {}
+		let mut iter = preimages.into_iter();
+		for htlc in pending_outbound_htlcs.iter_mut() {
+			match &mut htlc.state {
+				OutboundHTLCState::AwaitingRemoteRevokeToRemove(OutboundHTLCOutcome::Success(ref mut preimage)) => {
+					// This variant was initialized like this further above
+					debug_assert_eq!(preimage, &PaymentPreimage([0u8; 32]));
+					// Flatten and unwrap the preimage; they are always set starting in 0.2.
+					*preimage = iter.next().flatten().ok_or(DecodeError::InvalidValue)?;
 				}
+				OutboundHTLCState::AwaitingRemovedRemoteRevoke(OutboundHTLCOutcome::Success(ref mut preimage)) => {
+					// This variant was initialized like this further above
+					debug_assert_eq!(preimage, &PaymentPreimage([0u8; 32]));
+					// Flatten and unwrap the preimage; they are always set starting in 0.2.
+					*preimage = iter.next().flatten().ok_or(DecodeError::InvalidValue)?;
+				}
+				_ => {}
 			}
-			// We expect all preimages to be consumed above
-			if iter.next().is_some() {
-				return Err(DecodeError::InvalidValue);
-			}
+		}
+		// We expect all preimages to be consumed above
+		if iter.next().is_some() {
+			return Err(DecodeError::InvalidValue);
 		}
 
 		let chan_features = channel_type.unwrap();