Don't persist inbound committed onions in prod

A few PRs ago, we started persisting inbound committed HTLC onions in Channels.
These onions were persisted to lay groundwork for reconstructing the
ChannelManager's pending HTLC maps from them in a future version. However,
we've since discovered a different direction where we can instead reconstruct
these same maps using persistent monitor events, which may mean that we don't
need to persist these onions.

Since persisting a bunch of onions on every manager write is a big commitment
that we're not fully confident in yet, switch it off for now until we either
confirm the monitor events direction and can delete all this onion persisting
code OR realize that we definitely do need it.

Author: valentinewallace

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

@@ -368,8 +368,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,
 }
 
@@ -7982,8 +7981,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!(
@@ -15570,6 +15570,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() {
@@ -15590,9 +15591,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

@@ -17600,16 +17600,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),
@@ -18435,7 +18435,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 {
@@ -19573,7 +19573,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.map_or(false, |v| _version >= v);
 		#[cfg(test)]
 		let reconstruct_manager_from_monitors =
 			args.reconstruct_manager_from_monitors.unwrap_or_else(|| {
@@ -19636,7 +19637,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);
 								}