From d68dbf84c7097f3cc1d278f6fdbe447069afb8b8 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. --- lightning-tests/src/upgrade_downgrade_tests.rs | 9 ++++----- lightning/src/ln/channel.rs | 14 ++++++++------ lightning/src/ln/channelmanager.rs | 15 ++++++--------- 3 files changed, 18 insertions(+), 20 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..77ce7ab7593 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -17600,15 +17600,13 @@ 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 0.3 or 0.4 reads this manager version, it knows that the legacy maps were not written and -// acts accordingly. +// If `version == 2` 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. const RECONSTRUCT_HTLCS_FROM_CHANS_VERSION: u8 = 2; impl_writeable_tlv_based!(PhantomRouteHints, { @@ -19636,7 +19634,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); }