Merge pull request #4599 from valentinewallace/2026-05-no-persist-onions-yet

Don't persist inbound committed onions in prod

Author: TheBlueMatt

lightning-tests/src/upgrade_downgrade_tests.rs

@@ -540,11 +540,10 @@ fn upgrade_mid_htlc_intercept_forward() {
 }
 
 fn do_upgrade_mid_htlc_forward(test: MidHtlcForwardCase) {
-	// In 0.3, we started reconstructing the `ChannelManager`'s HTLC forwards maps from the HTLCs
-	// contained in `Channel`s, as part of removing the requirement to regularly persist the
-	// `ChannelManager`. However, HTLC forwards can only be reconstructed this way if they were
-	// received on 0.3 or higher. Test that HTLC forwards that were serialized on <=0.2 will still
-	// succeed when read on 0.3+.
+	// In an upcoming version, we plan to start reconstructing the `ChannelManager`'s HTLC forwards
+	// maps from the HTLCs contained in `Channel`s, as part of removing the requirement to regularly
+	// persist the `ChannelManager`. Preemptively test that HTLC forwards that were serialized on
+	// <=0.2 will still succeed when read on this upcoming version.
 	let (node_a_ser, node_b_ser, node_c_ser, mon_a_1_ser, mon_b_1_ser, mon_b_2_ser, mon_c_1_ser);
 	let (node_a_id, node_b_id, node_c_id);
 	let (payment_secret_bytes, payment_hash_bytes, payment_preimage_bytes);

lightning/src/ln/channel.rs

@@ -369,8 +369,7 @@ enum InboundUpdateAdd {
 		blinded_failure: Option<BlindedFailure>,
 		outbound_hop: OutboundHop,
 	},
-	/// This HTLC was received pre-LDK 0.3, before we started persisting the onion for inbound
-	/// committed HTLCs.
+	/// This HTLC was received before we started persisting the onion for inbound committed HTLCs.
 	Legacy,
 }
 
@@ -8051,8 +8050,9 @@ where
 		Ok(())
 	}
 
-	/// Returns true if any committed inbound HTLCs were received pre-LDK 0.3 and cannot be used
-	/// during `ChannelManager` deserialization to reconstruct the set of pending HTLCs.
+	/// Returns true if any committed inbound HTLCs were received before we started serializing
+	/// inbound committed payment onions in `Channel` and cannot be used during `ChannelManager`
+	/// deserialization to reconstruct the set of pending HTLCs.
 	pub(super) fn has_legacy_inbound_htlcs(&self) -> bool {
 		self.context.pending_inbound_htlcs.iter().any(|htlc| {
 			matches!(
@@ -15873,6 +15873,7 @@ impl<SP: SignerProvider> Writeable for FundedChannel<SP> {
 			}
 		}
 		let mut removed_htlc_attribution_data: Vec<&Option<AttributionData>> = Vec::new();
+		#[cfg_attr(not(test), allow(unused_mut))]
 		let mut inbound_committed_update_adds: Vec<&InboundUpdateAdd> = Vec::new();
 		(self.context.pending_inbound_htlcs.len() as u64 - dropped_inbound_htlcs).write(writer)?;
 		for htlc in self.context.pending_inbound_htlcs.iter() {
@@ -15893,9 +15894,10 @@ impl<SP: SignerProvider> Writeable for FundedChannel<SP> {
 					2u8.write(writer)?;
 					htlc_resolution.write(writer)?;
 				},
-				&InboundHTLCState::Committed { ref update_add_htlc } => {
+				&InboundHTLCState::Committed { update_add_htlc: ref _update_add } => {
 					3u8.write(writer)?;
-					inbound_committed_update_adds.push(update_add_htlc);
+					#[cfg(test)]
+					inbound_committed_update_adds.push(_update_add);
 				},
 				&InboundHTLCState::LocalRemoved(ref removal_reason) => {
 					4u8.write(writer)?;

lightning/src/ln/channelmanager.rs

@@ -17713,16 +17713,16 @@ pub fn provided_init_features(config: &UserConfig) -> InitFeatures {
 const SERIALIZATION_VERSION: u8 = 1;
 const MIN_SERIALIZATION_VERSION: u8 = 1;
 
-// We plan to start writing this version in 0.5.
+// We plan to start writing this version a few versions after we start writing inbound committed
+// payment onions in `Channel`, which is already done in tests but not yet switched on in prod.
 //
-// LDK 0.5+ will reconstruct the set of pending HTLCs from `Channel{Monitor}` data that started
-// being written in 0.3, ignoring legacy `ChannelManager` HTLC maps on read and not writing them.
-// LDK 0.5+ will automatically fail to read if the pending HTLC set cannot be reconstructed, i.e.
-// if we were last written with pending HTLCs on 0.2- or if the new 0.3+ fields are missing.
+// If we see this version on read, we will use said onions when reconstructing the set of pending
+// HTLCs, ignoring legacy `ChannelManager` HTLC maps on read and not writing them. We'll also
+// automatically fail to read if the pending HTLC set cannot be reconstructed, i.e. if the new
+// payment onion field is missing.
 //
-// If 0.3 or 0.4 reads this manager version, it knows that the legacy maps were not written and
-// acts accordingly.
-const RECONSTRUCT_HTLCS_FROM_CHANS_VERSION: u8 = 2;
+// Left as `None` for now until we are committed to writing inbound committed onions in `Channel`s.
+const RECONSTRUCT_HTLCS_FROM_CHANS_VERSION: Option<u8> = None;
 
 impl_writeable_tlv_based!(PhantomRouteHints, {
 	(2, channels, required_vec),
@@ -18549,7 +18549,7 @@ impl<'a, ES: EntropySource, SP: SignerProvider, L: Logger>
 		}
 
 		let forward_htlcs_legacy: HashMap<u64, Vec<HTLCForwardInfo>> =
-			if version < RECONSTRUCT_HTLCS_FROM_CHANS_VERSION {
+			if RECONSTRUCT_HTLCS_FROM_CHANS_VERSION.map_or(true, |v| version < v) {
 				let forward_htlcs_count: u64 = Readable::read(reader)?;
 				let mut fwds = hash_map_with_capacity(cmp::min(forward_htlcs_count as usize, 128));
 				for _ in 0..forward_htlcs_count {
@@ -19695,7 +19695,8 @@ impl<
 		// `reconstruct_manager_from_monitors` is set below. Currently we set in tests randomly to
 		// ensure the legacy codepaths also have test coverage.
 		#[cfg(not(test))]
-		let reconstruct_manager_from_monitors = _version >= RECONSTRUCT_HTLCS_FROM_CHANS_VERSION;
+		let reconstruct_manager_from_monitors =
+			RECONSTRUCT_HTLCS_FROM_CHANS_VERSION.is_some_and(|v| _version >= v);
 		#[cfg(test)]
 		let reconstruct_manager_from_monitors =
 			args.reconstruct_manager_from_monitors.unwrap_or_else(|| {
@@ -19758,7 +19759,6 @@ impl<
 					if reconstruct_manager_from_monitors {
 						if let Some(chan) = peer_state.channel_by_id.get(channel_id) {
 							if let Some(funded_chan) = chan.as_funded() {
-								// Legacy HTLCs are from pre-LDK 0.3 and cannot be reconstructed.
 								if funded_chan.has_legacy_inbound_htlcs() {
 									return Err(DecodeError::InvalidValue);
 								}