Store inbound committed update_adds in Channel

We have an overarching goal of (mostly) getting rid of ChannelManager
persistence and rebuilding the ChannelManager's state from existing
ChannelMonitors, due to issues when the two structs are out-of-sync on restart.
The main issue that can arise is channel force closure.

As part of this, we plan to store at least parts of Channels in
ChannelMonitors, and that Channel data will be used in rebuilding the manager.

Once we store update_adds in Channels, we can use them on restart when
reconstructing ChannelManager maps such as forward_htlcs and
pending_intercepted_htlcs. Upcoming commits will start doing this
reconstruction.

Author: valentinewallace

lightning/src/ln/channel.rs

@@ -211,7 +211,14 @@ enum InboundHTLCState {
 	/// channel (before it can then get forwarded and/or removed).
 	/// Implies AwaitingRemoteRevoke.
 	AwaitingAnnouncedRemoteRevoke(InboundHTLCResolution),
-	Committed,
+	/// An HTLC irrevocably committed in the latest commitment transaction, ready to be forwarded or
+	/// removed.
+	Committed {
+		/// Used to rebuild `ChannelManager` HTLC state on restart. Previously the manager would track
+		/// and persist all HTLC forwards and receives itself, but newer LDK versions avoid relying on
+		/// its persistence and instead reconstruct state based on `Channel` and `ChannelMonitor` data.
+		update_add_htlc_opt: Option<msgs::UpdateAddHTLC>,
+	},
 	/// Removed by us and a new commitment_signed was sent (if we were AwaitingRemoteRevoke when we
 	/// created it we would have put it in the holding cell instead). When they next revoke_and_ack
 	/// we'll drop it.
@@ -235,7 +242,7 @@ impl From<&InboundHTLCState> for Option<InboundHTLCStateDetails> {
 			InboundHTLCState::AwaitingAnnouncedRemoteRevoke(_) => {
 				Some(InboundHTLCStateDetails::AwaitingRemoteRevokeToAdd)
 			},
-			InboundHTLCState::Committed => Some(InboundHTLCStateDetails::Committed),
+			InboundHTLCState::Committed { .. } => Some(InboundHTLCStateDetails::Committed),
 			InboundHTLCState::LocalRemoved(InboundHTLCRemovalReason::FailRelay(_)) => {
 				Some(InboundHTLCStateDetails::AwaitingRemoteRevokeToRemoveFail)
 			},
@@ -256,7 +263,7 @@ impl fmt::Display for InboundHTLCState {
 			InboundHTLCState::RemoteAnnounced(_) => write!(f, "RemoteAnnounced"),
 			InboundHTLCState::AwaitingRemoteRevokeToAnnounce(_) => write!(f, "AwaitingRemoteRevokeToAnnounce"),
 			InboundHTLCState::AwaitingAnnouncedRemoteRevoke(_) => write!(f, "AwaitingAnnouncedRemoteRevoke"),
-			InboundHTLCState::Committed => write!(f, "Committed"),
+			InboundHTLCState::Committed { .. } => write!(f, "Committed"),
 			InboundHTLCState::LocalRemoved(_) => write!(f, "LocalRemoved"),
 		}
 	}
@@ -268,7 +275,7 @@ impl InboundHTLCState {
 			InboundHTLCState::RemoteAnnounced(_) => !generated_by_local,
 			InboundHTLCState::AwaitingRemoteRevokeToAnnounce(_) => !generated_by_local,
 			InboundHTLCState::AwaitingAnnouncedRemoteRevoke(_) => true,
-			InboundHTLCState::Committed => true,
+			InboundHTLCState::Committed { .. } => true,
 			InboundHTLCState::LocalRemoved(_) => !generated_by_local,
 		}
 	}
@@ -296,7 +303,7 @@ impl InboundHTLCState {
 				},
 				InboundHTLCResolution::Resolved { .. } => false,
 			},
-			InboundHTLCState::Committed | InboundHTLCState::LocalRemoved(_) => false,
+			InboundHTLCState::Committed { .. } | InboundHTLCState::LocalRemoved(_) => false,
 		}
 	}
 }
@@ -4102,7 +4109,7 @@ where
 
 		if self.pending_inbound_htlcs.iter()
 			.any(|htlc| match htlc.state {
-				InboundHTLCState::Committed => false,
+				InboundHTLCState::Committed { .. } => false,
 				// An HTLC removal from the local node is pending on the remote commitment.
 				InboundHTLCState::LocalRemoved(_) => true,
 				// An HTLC add from the remote node is pending on the local commitment.
@@ -4531,7 +4538,7 @@ where
 				(InboundHTLCState::RemoteAnnounced(..), _) => true,
 				(InboundHTLCState::AwaitingRemoteRevokeToAnnounce(..), _) => true,
 				(InboundHTLCState::AwaitingAnnouncedRemoteRevoke(..), _) => true,
-				(InboundHTLCState::Committed, _) => true,
+				(InboundHTLCState::Committed { .. }, _) => true,
 				(InboundHTLCState::LocalRemoved(..), true) => true,
 				(InboundHTLCState::LocalRemoved(..), false) => false,
 			})
@@ -7320,7 +7327,7 @@ where
 					payment_preimage_arg
 				);
 				match htlc.state {
-					InboundHTLCState::Committed => {},
+					InboundHTLCState::Committed { .. } => {},
 					InboundHTLCState::LocalRemoved(ref reason) => {
 						if let &InboundHTLCRemovalReason::Fulfill { .. } = reason {
 						} else {
@@ -7413,7 +7420,7 @@ where
 
 		{
 			let htlc = &mut self.context.pending_inbound_htlcs[pending_idx];
-			if let InboundHTLCState::Committed = htlc.state {
+			if let InboundHTLCState::Committed { .. } = htlc.state {
 			} else {
 				debug_assert!(
 					false,
@@ -7548,7 +7555,7 @@ where
 		for (idx, htlc) in self.context.pending_inbound_htlcs.iter().enumerate() {
 			if htlc.htlc_id == htlc_id_arg {
 				match htlc.state {
-					InboundHTLCState::Committed => {},
+					InboundHTLCState::Committed { .. } => {},
 					InboundHTLCState::LocalRemoved(_) => {
 						return Err(ChannelError::Ignore(format!("HTLC {} was already resolved", htlc.htlc_id)));
 					},
@@ -8716,7 +8723,7 @@ where
 					false
 				};
 				if swap {
-					let mut state = InboundHTLCState::Committed;
+					let mut state = InboundHTLCState::Committed { update_add_htlc_opt: None };
 					mem::swap(&mut state, &mut htlc.state);
 
 					if let InboundHTLCState::AwaitingRemoteRevokeToAnnounce(resolution) = state {
@@ -8755,14 +8762,23 @@ where
 									PendingHTLCStatus::Forward(forward_info) => {
 										log_trace!(logger, " ...promoting inbound AwaitingAnnouncedRemoteRevoke {} to Committed, attempting to forward", &htlc.payment_hash);
 										to_forward_infos.push((forward_info, htlc.htlc_id));
-										htlc.state = InboundHTLCState::Committed;
+										htlc.state = InboundHTLCState::Committed {
+											// HTLCs will only be in state `InboundHTLCResolution::Resolved` if they were
+											// received on an old pre-0.1 version of LDK. In this case, we can't set
+											// `update_add_htlc_opt` here, but the HTLC will be present in the legacy
+											// `ChannelManager` maps so we'll process it from there if it's still around
+											// on the next restart.
+											update_add_htlc_opt: None,
+										};
 									},
 								}
 							},
 							InboundHTLCResolution::Pending { update_add_htlc } => {
 								log_trace!(logger, " ...promoting inbound AwaitingAnnouncedRemoteRevoke {} to Committed", &htlc.payment_hash);
-								pending_update_adds.push(update_add_htlc);
-								htlc.state = InboundHTLCState::Committed;
+								pending_update_adds.push(update_add_htlc.clone());
+								htlc.state = InboundHTLCState::Committed {
+									update_add_htlc_opt: Some(update_add_htlc),
+								};
 							},
 						}
 					}
@@ -9297,7 +9313,7 @@ where
 					// in response to it yet, so don't touch it.
 					true
 				},
-				InboundHTLCState::Committed => true,
+				InboundHTLCState::Committed { .. } => true,
 				InboundHTLCState::LocalRemoved(_) => {
 					// We (hopefully) sent a commitment_signed updating this HTLC (which we can
 					// re-transmit if needed) and they may have even sent a revoke_and_ack back
@@ -14518,6 +14534,7 @@ where
 			}
 		}
 		let mut removed_htlc_attribution_data: Vec<&Option<AttributionData>> = Vec::new();
+		let mut inbound_committed_update_adds: Vec<Option<msgs::UpdateAddHTLC>> = Vec::new();
 		(self.context.pending_inbound_htlcs.len() as u64 - dropped_inbound_htlcs).write(writer)?;
 		for htlc in self.context.pending_inbound_htlcs.iter() {
 			if let &InboundHTLCState::RemoteAnnounced(_) = &htlc.state {
@@ -14537,8 +14554,9 @@ where
 					2u8.write(writer)?;
 					htlc_resolution.write(writer)?;
 				},
-				&InboundHTLCState::Committed => {
+				&InboundHTLCState::Committed { ref update_add_htlc_opt } => {
 					3u8.write(writer)?;
+					inbound_committed_update_adds.push(update_add_htlc_opt.clone());
 				},
 				&InboundHTLCState::LocalRemoved(ref removal_reason) => {
 					4u8.write(writer)?;
@@ -14914,6 +14932,7 @@ where
 			(69, holding_cell_held_htlc_flags, optional_vec), // Added in 0.2
 			(71, holder_commitment_point_previous_revoked, option), // Added in 0.3
 			(73, holder_commitment_point_last_revoked, option), // Added in 0.3
+			(75, inbound_committed_update_adds, optional_vec),
 		});
 
 		Ok(())
@@ -14997,7 +15016,7 @@ where
 						};
 						InboundHTLCState::AwaitingAnnouncedRemoteRevoke(resolution)
 					},
-					3 => InboundHTLCState::Committed,
+					3 => InboundHTLCState::Committed { update_add_htlc_opt: None },
 					4 => {
 						let reason = match <u8 as Readable>::read(reader)? {
 							0 => InboundHTLCRemovalReason::FailRelay(msgs::OnionErrorPacket {
@@ -15301,6 +15320,7 @@ where
 
 		let mut pending_outbound_held_htlc_flags_opt: Option<Vec<Option<()>>> = None;
 		let mut holding_cell_held_htlc_flags_opt: Option<Vec<Option<()>>> = None;
+		let mut inbound_committed_update_adds_opt: Option<Vec<Option<msgs::UpdateAddHTLC>>> = None;
 
 		read_tlv_fields!(reader, {
 			(0, announcement_sigs, option),
@@ -15350,6 +15370,7 @@ where
 			(69, holding_cell_held_htlc_flags_opt, optional_vec), // Added in 0.2
 			(71, holder_commitment_point_previous_revoked_opt, option), // Added in 0.3
 			(73, holder_commitment_point_last_revoked_opt, option), // Added in 0.3
+			(75, inbound_committed_update_adds_opt, optional_vec),
 		});
 
 		let holder_signer = signer_provider.derive_channel_signer(channel_keys_id);
@@ -15473,6 +15494,17 @@ where
 				return Err(DecodeError::InvalidValue);
 			}
 		}
+		if let Some(update_adds) = inbound_committed_update_adds_opt {
+			let mut iter = update_adds.into_iter();
+			for htlc in pending_inbound_htlcs.iter_mut() {
+				if let InboundHTLCState::Committed { ref mut update_add_htlc_opt } = htlc.state {
+					*update_add_htlc_opt = iter.next().ok_or(DecodeError::InvalidValue)?;
+				}
+			}
+			if iter.next().is_some() {
+				return Err(DecodeError::InvalidValue);
+			}
+		}
 
 		if let Some(attribution_data_list) = removed_htlc_attribution_data {
 			let mut removed_htlcs = pending_inbound_htlcs.iter_mut().filter_map(|status| {
@@ -16057,7 +16089,7 @@ mod tests {
 			amount_msat: htlc_amount_msat,
 			payment_hash: PaymentHash(Sha256::hash(&[42; 32]).to_byte_array()),
 			cltv_expiry: 300000000,
-			state: InboundHTLCState::Committed,
+			state: InboundHTLCState::Committed { update_add_htlc_opt: None },
 		});
 
 		node_a_chan.context.pending_outbound_htlcs.push(OutboundHTLCOutput {
@@ -16903,7 +16935,7 @@ mod tests {
 			amount_msat: 1000000,
 			cltv_expiry: 500,
 			payment_hash: PaymentHash::from(payment_preimage_0),
-			state: InboundHTLCState::Committed,
+			state: InboundHTLCState::Committed { update_add_htlc_opt: None },
 		});
 
 		let payment_preimage_1 =
@@ -16913,7 +16945,7 @@ mod tests {
 			amount_msat: 2000000,
 			cltv_expiry: 501,
 			payment_hash: PaymentHash::from(payment_preimage_1),
-			state: InboundHTLCState::Committed,
+			state: InboundHTLCState::Committed { update_add_htlc_opt: None },
 		});
 
 		let payment_preimage_2 =
@@ -16953,7 +16985,7 @@ mod tests {
 			amount_msat: 4000000,
 			cltv_expiry: 504,
 			payment_hash: PaymentHash::from(payment_preimage_4),
-			state: InboundHTLCState::Committed,
+			state: InboundHTLCState::Committed { update_add_htlc_opt: None },
 		});
 
 		// commitment tx with all five HTLCs untrimmed (minimum feerate)
@@ -17342,7 +17374,7 @@ mod tests {
 			amount_msat: 2000000,
 			cltv_expiry: 501,
 			payment_hash: PaymentHash::from(payment_preimage_1),
-			state: InboundHTLCState::Committed,
+			state: InboundHTLCState::Committed { update_add_htlc_opt: None },
 		});
 
 		chan.context.pending_outbound_htlcs.clear();
@@ -17593,7 +17625,7 @@ mod tests {
 			amount_msat: 5000000,
 			cltv_expiry: 920150,
 			payment_hash: PaymentHash::from(htlc_in_preimage),
-			state: InboundHTLCState::Committed,
+			state: InboundHTLCState::Committed { update_add_htlc_opt: None },
 		}));
 
 		chan.context.pending_outbound_htlcs.extend(
@@ -17656,7 +17688,7 @@ mod tests {
 				amount_msat,
 				cltv_expiry: 920150,
 				payment_hash: PaymentHash::from(htlc_in_preimage),
-				state: InboundHTLCState::Committed,
+				state: InboundHTLCState::Committed { update_add_htlc_opt: None },
 			},
 		));
 
@@ -17722,7 +17754,7 @@ mod tests {
 			amount_msat: 100000,
 			cltv_expiry: 920125,
 			payment_hash: htlc_0_in_hash,
-			state: InboundHTLCState::Committed,
+			state: InboundHTLCState::Committed { update_add_htlc_opt: None },
 		});
 
 		let htlc_1_in_preimage =
@@ -17740,7 +17772,7 @@ mod tests {
 			amount_msat: 49900000,
 			cltv_expiry: 920125,
 			payment_hash: htlc_1_in_hash,
-			state: InboundHTLCState::Committed,
+			state: InboundHTLCState::Committed { update_add_htlc_opt: None },
 		});
 
 		chan.context.pending_outbound_htlcs.extend(
@@ -17792,7 +17824,7 @@ mod tests {
 				amount_msat: 30000,
 				payment_hash,
 				cltv_expiry: 920125,
-				state: InboundHTLCState::Committed,
+				state: InboundHTLCState::Committed { update_add_htlc_opt: None },
 			},
 		));
 
@@ -17833,7 +17865,7 @@ mod tests {
 				amount_msat: 29525,
 				payment_hash,
 				cltv_expiry: 920125,
-				state: InboundHTLCState::Committed,
+				state: InboundHTLCState::Committed { update_add_htlc_opt: None },
 			},
 		));
 
@@ -17870,7 +17902,7 @@ mod tests {
 				amount_msat: 29525,
 				payment_hash,
 				cltv_expiry: 920125,
-				state: InboundHTLCState::Committed,
+				state: InboundHTLCState::Committed { update_add_htlc_opt: None },
 			},
 		));
 
@@ -17907,7 +17939,7 @@ mod tests {
 				amount_msat: 29753,
 				payment_hash,
 				cltv_expiry: 920125,
-				state: InboundHTLCState::Committed,
+				state: InboundHTLCState::Committed { update_add_htlc_opt: None },
 			},
 		));
 
@@ -17959,7 +17991,7 @@ mod tests {
 				amount_msat,
 				cltv_expiry,
 				payment_hash,
-				state: InboundHTLCState::Committed,
+				state: InboundHTLCState::Committed { update_add_htlc_opt: None },
 			}),
 		);