From b82b6a46ac07bd5d224d85ae00e00bd6e527cd09 Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Tue, 5 May 2026 17:48:24 -0400 Subject: [PATCH] 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. --- .../src/upgrade_downgrade_tests.rs | 9 ++++---- lightning/src/ln/channel.rs | 14 +++++++----- lightning/src/ln/channelmanager.rs | 22 +++++++++---------- 3 files changed, 23 insertions(+), 22 deletions(-) diff --git a/lightning-tests/src/upgrade_downgrade_tests.rs b/lightning-tests/src/upgrade_downgrade_tests.rs index 7f607bba848..7cc59227af4 100644 --- a/lightning-tests/src/upgrade_downgrade_tests.rs +++ b/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); diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index e07ee7fceab..63d96fc1682 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -368,8 +368,7 @@ enum InboundUpdateAdd { blinded_failure: Option, 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 Writeable for FundedChannel { } } let mut removed_htlc_attribution_data: Vec<&Option> = 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 Writeable for FundedChannel { 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)?; diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index a7a0942f0c8..b047c289271 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/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 = 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> = - 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.is_some_and(|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); }