Don't persist inbound committed onions in prod#4599

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
TheBlueMatt merged 1 commit into from
May 19, 2026
Merged

Don't persist inbound committed onions in prod #4599

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 4 additions & 5 deletions lightning-tests/src/upgrade_downgrade_tests.rs
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -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);
Expand Down
14 changes: 8 additions & 6 deletions lightning/src/ln/channel.rs
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -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,
}

Expand DownExpand Up@@ -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!(
Expand DownExpand Up@@ -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() {
Expand All@@ -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);
Comment on lines +15594 to +15597

@ldk-claude-review-botldk-claude-review-botMay 5, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The write side is now gated by #[cfg(test)], so in production inbound_committed_update_adds remains empty and TLV field 75 is never written. However, on the read side (line 16554), inbound_committed_update_adds_opt is initialized to Some(Vec::new()) by the optional_vec macro (when the TLV is absent, it stays as Some(empty_vec)). The deserialization code at line 16554-16564 then enters the if let Some(update_adds) block unconditionally and calls iter.next().ok_or(DecodeError::InvalidValue)? for each committed HTLC — which immediately fails because the iterator is empty.

Impact: Any production node that restarts while it has committed inbound HTLCs will fail to deserialize its channel state with DecodeError::InvalidValue. This is a critical availability bug.

Fix: The deserialization code at line 16554 needs a corresponding guard. For example:

ifletSome(update_adds) = inbound_committed_update_adds_opt {if !update_adds.is_empty(){letmut iter = update_adds.into_iter();// ... existing code ...}}

Or gate the entire read-side block with #[cfg(test)] to match the write side.

@valentinewallacevalentinewallaceMay 6, 2026

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned in claude's comment below, this was a false positive. We have test coverage of the case it was outlining as well, in upgrade_downgrade tests.

@TheBlueMattTheBlueMattMay 17, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't actually think so? cfg(test) isn't set in lightning-tests because lightning is technically being compiled as a dependency there. I can assert!(inbound_committed_update_adds.is_empty()) and lightning-tests still passes.

@joostjagerjoostjagerMay 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be visible in the coverage report too with the tests flag enabled?

@valentinewallacevalentinewallaceMay 18, 2026

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't actually think so? cfg(test) isn't set in lightning-tests because lightning is technically being compiled as a dependency there. I can assert!(inbound_committed_update_adds.is_empty()) and lightning-tests still passes.

To clarify, I meant that if the prod panic bug claude was pointing out was actually present, it would've been hit in lightning-tests because of exactly what you say (cfg(test) isn't set there). It's (to me at least) expected behavior that assert!(inbound_committed_update_adds.is_empty()) would pass in lightning-tests w/ the changes in this PR

@TheBlueMattTheBlueMattMay 18, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My assumption was that do_upgrade_mid_htlc_forward relied on the now-test-only code but I guess its really an upgrade test not a downgrade test...it does seem like its a dead test though as reconstruct_manager_from_monitors will always be false? Is it just intended to run once the code is in prod?

@valentinewallacevalentinewallaceMay 19, 2026

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's all correct. I did update the test's comment to say it was a "preemptive" test. If I rebased #4359 on top of this, it would use it.

I could remove it, but I was thinking we could just delete everything related to inbound onion persistence (or not) at the same time, rather than deleting some stuff and possibly having to remember to re-add it later.

},
&InboundHTLCState::LocalRemoved(ref removal_reason) => {
4u8.write(writer)?;
Expand Down
22 changes: 11 additions & 11 deletions lightning/src/ln/channelmanager.rs
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -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
Comment thread
joostjager marked this conversation as resolved.
// 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),
Expand DownExpand Up@@ -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) {
Comment thread
joostjager marked this conversation as resolved.
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 {
Expand DownExpand Up@@ -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(|| {
Expand DownExpand Up@@ -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);
}
Expand Down
Loading
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
Don't persist inbound committed onions in prod#4599

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

 Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and
privacy statement. We’ll occasionally send you account related emails.

Already on GitHub?
Sign in
to your account

 Merged

TheBlueMatt
merged 1 commit into

from

May 19, 2026
 Merged


Don't persist inbound committed onions in prod
#4599
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


1 commit


Select commit


File filter

Filter by extension
Filter by extension
Conversations

Failed to load comments. 
Loading
Jump to

Jump to file


Failed to load files. 
Loading
 Diff view

 Diff view


9 changes: 4 additions & 5 deletions
lightning-tests/src/upgrade_downgrade_tests.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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);
Expand Down

14 changes: 8 additions & 6 deletions
lightning/src/ln/channel.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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,
}

Expand DownExpand Up@@ -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!(
Expand DownExpand Up@@ -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() {
Expand All@@ -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);

Comment on lines

+15594
 to 
+15597

@ldk-claude-review-botldk-claude-review-botMay 5, 2026

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The write side is now gated by #[cfg(test)], so in production inbound_committed_update_adds remains empty and TLV field 75 is never written. However, on the read side (line 16554), inbound_committed_update_adds_opt is initialized to Some(Vec::new()) by the optional_vec macro (when the TLV is absent, it stays as Some(empty_vec)). The deserialization code at line 16554-16564 then enters the if let Some(update_adds) block unconditionally and calls iter.next().ok_or(DecodeError::InvalidValue)? for each committed HTLC — which immediately fails because the iterator is empty.
Impact: Any production node that restarts while it has committed inbound HTLCs will fail to deserialize its channel state with DecodeError::InvalidValue. This is a critical availability bug.
Fix: The deserialization code at line 16554 needs a corresponding guard. For example:
ifletSome(update_adds) = inbound_committed_update_adds_opt {if !update_adds.is_empty(){letmut iter = update_adds.into_iter();// ... existing code ...}}
Or gate the entire read-side block with #[cfg(test)] to match the write side.
@valentinewallacevalentinewallaceMay 6, 2026

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned in claude's comment below, this was a false positive. We have test coverage of the case it was outlining as well, in upgrade_downgrade tests.
@TheBlueMattTheBlueMattMay 17, 2026

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't actually think so? cfg(test) isn't set in lightning-tests because lightning is technically being compiled as a dependency there. I can assert!(inbound_committed_update_adds.is_empty()) and lightning-tests still passes.
@joostjagerjoostjagerMay 18, 2026

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be visible in the coverage report too with the tests flag enabled?
@valentinewallacevalentinewallaceMay 18, 2026

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't actually think so? cfg(test) isn't set in lightning-tests because lightning is technically being compiled as a dependency there. I can assert!(inbound_committed_update_adds.is_empty()) and lightning-tests still passes.
To clarify, I meant that if the prod panic bug claude was pointing out was actually present, it would've been hit in lightning-tests because of exactly what you say (cfg(test) isn't set there). It's (to me at least) expected behavior that assert!(inbound_committed_update_adds.is_empty()) would pass in lightning-tests w/ the changes in this PR
@TheBlueMattTheBlueMattMay 18, 2026

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My assumption was that do_upgrade_mid_htlc_forward relied on the now-test-only code but I guess its really an upgrade test not a downgrade test...it does seem like its a dead test though as reconstruct_manager_from_monitors will always be false? Is it just intended to run once the code is in prod?
@valentinewallacevalentinewallaceMay 19, 2026

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's all correct. I did update the test's comment to say it was a "preemptive" test. If I rebased #4359 on top of this, it would use it.
I could remove it, but I was thinking we could just delete everything related to inbound onion persistence (or not) at the same time, rather than deleting some stuff and possibly having to remember to re-add it later.
 },
 &InboundHTLCState::LocalRemoved(ref removal_reason) => {
 4u8.write(writer)?;
Expand Down

22 changes: 11 additions & 11 deletions
lightning/src/ln/channelmanager.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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
Comment thread
joostjager marked this conversation as resolved.

// 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),
Expand DownExpand Up@@ -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) {
Comment thread
joostjager marked this conversation as resolved.

 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 {
Expand DownExpand Up@@ -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(|| {
Expand DownExpand Up@@ -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);
 }
Expand Down
Loading

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Don't persist inbound committed onions in prod#4599

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

 Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and
privacy statement. We’ll occasionally send you account related emails.

Already on GitHub?
Sign in
to your account

 Merged

TheBlueMatt
merged 1 commit into

from

May 19, 2026
 Merged


Don't persist inbound committed onions in prod
#4599
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


1 commit


Select commit


File filter

Filter by extension
Filter by extension
Conversations

Failed to load comments. 
Loading
Jump to

Jump to file


Failed to load files. 
Loading
 Diff view

 Diff view


9 changes: 4 additions & 5 deletions
lightning-tests/src/upgrade_downgrade_tests.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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);
Expand Down

14 changes: 8 additions & 6 deletions
lightning/src/ln/channel.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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,
}

Expand DownExpand Up@@ -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!(
Expand DownExpand Up@@ -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() {
Expand All@@ -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);

Comment on lines

+15594
 to 
+15597

@ldk-claude-review-botldk-claude-review-botMay 5, 2026

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The write side is now gated by #[cfg(test)], so in production inbound_committed_update_adds remains empty and TLV field 75 is never written. However, on the read side (line 16554), inbound_committed_update_adds_opt is initialized to Some(Vec::new()) by the optional_vec macro (when the TLV is absent, it stays as Some(empty_vec)). The deserialization code at line 16554-16564 then enters the if let Some(update_adds) block unconditionally and calls iter.next().ok_or(DecodeError::InvalidValue)? for each committed HTLC — which immediately fails because the iterator is empty.
Impact: Any production node that restarts while it has committed inbound HTLCs will fail to deserialize its channel state with DecodeError::InvalidValue. This is a critical availability bug.
Fix: The deserialization code at line 16554 needs a corresponding guard. For example:
ifletSome(update_adds) = inbound_committed_update_adds_opt {if !update_adds.is_empty(){letmut iter = update_adds.into_iter();// ... existing code ...}}
Or gate the entire read-side block with #[cfg(test)] to match the write side.
@valentinewallacevalentinewallaceMay 6, 2026

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned in claude's comment below, this was a false positive. We have test coverage of the case it was outlining as well, in upgrade_downgrade tests.
@TheBlueMattTheBlueMattMay 17, 2026

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't actually think so? cfg(test) isn't set in lightning-tests because lightning is technically being compiled as a dependency there. I can assert!(inbound_committed_update_adds.is_empty()) and lightning-tests still passes.
@joostjagerjoostjagerMay 18, 2026

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be visible in the coverage report too with the tests flag enabled?
@valentinewallacevalentinewallaceMay 18, 2026

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't actually think so? cfg(test) isn't set in lightning-tests because lightning is technically being compiled as a dependency there. I can assert!(inbound_committed_update_adds.is_empty()) and lightning-tests still passes.
To clarify, I meant that if the prod panic bug claude was pointing out was actually present, it would've been hit in lightning-tests because of exactly what you say (cfg(test) isn't set there). It's (to me at least) expected behavior that assert!(inbound_committed_update_adds.is_empty()) would pass in lightning-tests w/ the changes in this PR
@TheBlueMattTheBlueMattMay 18, 2026

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My assumption was that do_upgrade_mid_htlc_forward relied on the now-test-only code but I guess its really an upgrade test not a downgrade test...it does seem like its a dead test though as reconstruct_manager_from_monitors will always be false? Is it just intended to run once the code is in prod?
@valentinewallacevalentinewallaceMay 19, 2026

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's all correct. I did update the test's comment to say it was a "preemptive" test. If I rebased #4359 on top of this, it would use it.
I could remove it, but I was thinking we could just delete everything related to inbound onion persistence (or not) at the same time, rather than deleting some stuff and possibly having to remember to re-add it later.
 },
 &InboundHTLCState::LocalRemoved(ref removal_reason) => {
 4u8.write(writer)?;
Expand Down

22 changes: 11 additions & 11 deletions
lightning/src/ln/channelmanager.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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
Comment thread
joostjager marked this conversation as resolved.

// 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),
Expand DownExpand Up@@ -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) {
Comment thread
joostjager marked this conversation as resolved.

 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 {
Expand DownExpand Up@@ -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(|| {
Expand DownExpand Up@@ -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);
 }
Expand Down
Loading

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Don't persist inbound committed onions in prod#4599

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

 Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and
privacy statement. We’ll occasionally send you account related emails.

Already on GitHub?
Sign in
to your account

 Merged

TheBlueMatt
merged 1 commit into

from

May 19, 2026
 Merged


Don't persist inbound committed onions in prod
#4599
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


1 commit


Select commit


File filter

Filter by extension
Filter by extension
Conversations

Failed to load comments. 
Loading
Jump to

Jump to file


Failed to load files. 
Loading
 Diff view

 Diff view


9 changes: 4 additions & 5 deletions
lightning-tests/src/upgrade_downgrade_tests.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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);
Expand Down

14 changes: 8 additions & 6 deletions
lightning/src/ln/channel.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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,
}

Expand DownExpand Up@@ -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!(
Expand DownExpand Up@@ -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() {
Expand All@@ -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);

Comment on lines

+15594
 to 
+15597

@ldk-claude-review-botldk-claude-review-botMay 5, 2026

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The write side is now gated by #[cfg(test)], so in production inbound_committed_update_adds remains empty and TLV field 75 is never written. However, on the read side (line 16554), inbound_committed_update_adds_opt is initialized to Some(Vec::new()) by the optional_vec macro (when the TLV is absent, it stays as Some(empty_vec)). The deserialization code at line 16554-16564 then enters the if let Some(update_adds) block unconditionally and calls iter.next().ok_or(DecodeError::InvalidValue)? for each committed HTLC — which immediately fails because the iterator is empty.
Impact: Any production node that restarts while it has committed inbound HTLCs will fail to deserialize its channel state with DecodeError::InvalidValue. This is a critical availability bug.
Fix: The deserialization code at line 16554 needs a corresponding guard. For example:
ifletSome(update_adds) = inbound_committed_update_adds_opt {if !update_adds.is_empty(){letmut iter = update_adds.into_iter();// ... existing code ...}}
Or gate the entire read-side block with #[cfg(test)] to match the write side.
@valentinewallacevalentinewallaceMay 6, 2026

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned in claude's comment below, this was a false positive. We have test coverage of the case it was outlining as well, in upgrade_downgrade tests.
@TheBlueMattTheBlueMattMay 17, 2026

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't actually think so? cfg(test) isn't set in lightning-tests because lightning is technically being compiled as a dependency there. I can assert!(inbound_committed_update_adds.is_empty()) and lightning-tests still passes.
@joostjagerjoostjagerMay 18, 2026

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be visible in the coverage report too with the tests flag enabled?
@valentinewallacevalentinewallaceMay 18, 2026

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't actually think so? cfg(test) isn't set in lightning-tests because lightning is technically being compiled as a dependency there. I can assert!(inbound_committed_update_adds.is_empty()) and lightning-tests still passes.
To clarify, I meant that if the prod panic bug claude was pointing out was actually present, it would've been hit in lightning-tests because of exactly what you say (cfg(test) isn't set there). It's (to me at least) expected behavior that assert!(inbound_committed_update_adds.is_empty()) would pass in lightning-tests w/ the changes in this PR
@TheBlueMattTheBlueMattMay 18, 2026

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My assumption was that do_upgrade_mid_htlc_forward relied on the now-test-only code but I guess its really an upgrade test not a downgrade test...it does seem like its a dead test though as reconstruct_manager_from_monitors will always be false? Is it just intended to run once the code is in prod?
@valentinewallacevalentinewallaceMay 19, 2026

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's all correct. I did update the test's comment to say it was a "preemptive" test. If I rebased #4359 on top of this, it would use it.
I could remove it, but I was thinking we could just delete everything related to inbound onion persistence (or not) at the same time, rather than deleting some stuff and possibly having to remember to re-add it later.
 },
 &InboundHTLCState::LocalRemoved(ref removal_reason) => {
 4u8.write(writer)?;
Expand Down

22 changes: 11 additions & 11 deletions
lightning/src/ln/channelmanager.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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
Comment thread
joostjager marked this conversation as resolved.

// 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),
Expand DownExpand Up@@ -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) {
Comment thread
joostjager marked this conversation as resolved.

 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 {
Expand DownExpand Up@@ -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(|| {
Expand DownExpand Up@@ -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);
 }
Expand Down
Loading

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
Don't persist inbound committed onions in prod#4599

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

 Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and
privacy statement. We’ll occasionally send you account related emails.

Already on GitHub?
Sign in
to your account

 Merged

TheBlueMatt
merged 1 commit into

from

May 19, 2026
 Merged


Don't persist inbound committed onions in prod
#4599
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


1 commit


Select commit


File filter

Filter by extension
Filter by extension
Conversations

Failed to load comments. 
Loading
Jump to

Jump to file


Failed to load files. 
Loading
 Diff view

 Diff view


9 changes: 4 additions & 5 deletions
lightning-tests/src/upgrade_downgrade_tests.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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);
Expand Down

14 changes: 8 additions & 6 deletions
lightning/src/ln/channel.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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,
}

Expand DownExpand Up@@ -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!(
Expand DownExpand Up@@ -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() {
Expand All@@ -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);

Comment on lines

+15594
 to 
+15597

@ldk-claude-review-botldk-claude-review-botMay 5, 2026

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The write side is now gated by #[cfg(test)], so in production inbound_committed_update_adds remains empty and TLV field 75 is never written. However, on the read side (line 16554), inbound_committed_update_adds_opt is initialized to Some(Vec::new()) by the optional_vec macro (when the TLV is absent, it stays as Some(empty_vec)). The deserialization code at line 16554-16564 then enters the if let Some(update_adds) block unconditionally and calls iter.next().ok_or(DecodeError::InvalidValue)? for each committed HTLC — which immediately fails because the iterator is empty.
Impact: Any production node that restarts while it has committed inbound HTLCs will fail to deserialize its channel state with DecodeError::InvalidValue. This is a critical availability bug.
Fix: The deserialization code at line 16554 needs a corresponding guard. For example:
ifletSome(update_adds) = inbound_committed_update_adds_opt {if !update_adds.is_empty(){letmut iter = update_adds.into_iter();// ... existing code ...}}
Or gate the entire read-side block with #[cfg(test)] to match the write side.
@valentinewallacevalentinewallaceMay 6, 2026

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned in claude's comment below, this was a false positive. We have test coverage of the case it was outlining as well, in upgrade_downgrade tests.
@TheBlueMattTheBlueMattMay 17, 2026

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't actually think so? cfg(test) isn't set in lightning-tests because lightning is technically being compiled as a dependency there. I can assert!(inbound_committed_update_adds.is_empty()) and lightning-tests still passes.
@joostjagerjoostjagerMay 18, 2026

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be visible in the coverage report too with the tests flag enabled?
@valentinewallacevalentinewallaceMay 18, 2026

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't actually think so? cfg(test) isn't set in lightning-tests because lightning is technically being compiled as a dependency there. I can assert!(inbound_committed_update_adds.is_empty()) and lightning-tests still passes.
To clarify, I meant that if the prod panic bug claude was pointing out was actually present, it would've been hit in lightning-tests because of exactly what you say (cfg(test) isn't set there). It's (to me at least) expected behavior that assert!(inbound_committed_update_adds.is_empty()) would pass in lightning-tests w/ the changes in this PR
@TheBlueMattTheBlueMattMay 18, 2026

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My assumption was that do_upgrade_mid_htlc_forward relied on the now-test-only code but I guess its really an upgrade test not a downgrade test...it does seem like its a dead test though as reconstruct_manager_from_monitors will always be false? Is it just intended to run once the code is in prod?
@valentinewallacevalentinewallaceMay 19, 2026

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's all correct. I did update the test's comment to say it was a "preemptive" test. If I rebased #4359 on top of this, it would use it.
I could remove it, but I was thinking we could just delete everything related to inbound onion persistence (or not) at the same time, rather than deleting some stuff and possibly having to remember to re-add it later.
 },
 &InboundHTLCState::LocalRemoved(ref removal_reason) => {
 4u8.write(writer)?;
Expand Down

22 changes: 11 additions & 11 deletions
lightning/src/ln/channelmanager.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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
Comment thread
joostjager marked this conversation as resolved.

// 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),
Expand DownExpand Up@@ -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) {
Comment thread
joostjager marked this conversation as resolved.

 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 {
Expand DownExpand Up@@ -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(|| {
Expand DownExpand Up@@ -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);
 }
Expand Down
Loading

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Don't persist inbound committed onions in prod#4599

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

 Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and
privacy statement. We’ll occasionally send you account related emails.

Already on GitHub?
Sign in
to your account

 Merged

TheBlueMatt
merged 1 commit into

from

May 19, 2026
 Merged


Don't persist inbound committed onions in prod
#4599
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


1 commit


Select commit


File filter

Filter by extension
Filter by extension
Conversations

Failed to load comments. 
Loading
Jump to

Jump to file


Failed to load files. 
Loading
 Diff view

 Diff view


9 changes: 4 additions & 5 deletions
lightning-tests/src/upgrade_downgrade_tests.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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);
Expand Down

14 changes: 8 additions & 6 deletions
lightning/src/ln/channel.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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,
}

Expand DownExpand Up@@ -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!(
Expand DownExpand Up@@ -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() {
Expand All@@ -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);

Comment on lines

+15594
 to 
+15597

@ldk-claude-review-botldk-claude-review-botMay 5, 2026

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The write side is now gated by #[cfg(test)], so in production inbound_committed_update_adds remains empty and TLV field 75 is never written. However, on the read side (line 16554), inbound_committed_update_adds_opt is initialized to Some(Vec::new()) by the optional_vec macro (when the TLV is absent, it stays as Some(empty_vec)). The deserialization code at line 16554-16564 then enters the if let Some(update_adds) block unconditionally and calls iter.next().ok_or(DecodeError::InvalidValue)? for each committed HTLC — which immediately fails because the iterator is empty.
Impact: Any production node that restarts while it has committed inbound HTLCs will fail to deserialize its channel state with DecodeError::InvalidValue. This is a critical availability bug.
Fix: The deserialization code at line 16554 needs a corresponding guard. For example:
ifletSome(update_adds) = inbound_committed_update_adds_opt {if !update_adds.is_empty(){letmut iter = update_adds.into_iter();// ... existing code ...}}
Or gate the entire read-side block with #[cfg(test)] to match the write side.
@valentinewallacevalentinewallaceMay 6, 2026

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned in claude's comment below, this was a false positive. We have test coverage of the case it was outlining as well, in upgrade_downgrade tests.
@TheBlueMattTheBlueMattMay 17, 2026

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't actually think so? cfg(test) isn't set in lightning-tests because lightning is technically being compiled as a dependency there. I can assert!(inbound_committed_update_adds.is_empty()) and lightning-tests still passes.
@joostjagerjoostjagerMay 18, 2026

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be visible in the coverage report too with the tests flag enabled?
@valentinewallacevalentinewallaceMay 18, 2026

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't actually think so? cfg(test) isn't set in lightning-tests because lightning is technically being compiled as a dependency there. I can assert!(inbound_committed_update_adds.is_empty()) and lightning-tests still passes.
To clarify, I meant that if the prod panic bug claude was pointing out was actually present, it would've been hit in lightning-tests because of exactly what you say (cfg(test) isn't set there). It's (to me at least) expected behavior that assert!(inbound_committed_update_adds.is_empty()) would pass in lightning-tests w/ the changes in this PR
@TheBlueMattTheBlueMattMay 18, 2026

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My assumption was that do_upgrade_mid_htlc_forward relied on the now-test-only code but I guess its really an upgrade test not a downgrade test...it does seem like its a dead test though as reconstruct_manager_from_monitors will always be false? Is it just intended to run once the code is in prod?
@valentinewallacevalentinewallaceMay 19, 2026

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's all correct. I did update the test's comment to say it was a "preemptive" test. If I rebased #4359 on top of this, it would use it.
I could remove it, but I was thinking we could just delete everything related to inbound onion persistence (or not) at the same time, rather than deleting some stuff and possibly having to remember to re-add it later.
 },
 &InboundHTLCState::LocalRemoved(ref removal_reason) => {
 4u8.write(writer)?;
Expand Down

22 changes: 11 additions & 11 deletions
lightning/src/ln/channelmanager.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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
Comment thread
joostjager marked this conversation as resolved.

// 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),
Expand DownExpand Up@@ -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) {
Comment thread
joostjager marked this conversation as resolved.

 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 {
Expand DownExpand Up@@ -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(|| {
Expand DownExpand Up@@ -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);
 }
Expand Down
Loading

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Don't persist inbound committed onions in prod#4599

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

 Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and
privacy statement. We’ll occasionally send you account related emails.

Already on GitHub?
Sign in
to your account

 Merged

TheBlueMatt
merged 1 commit into

from

May 19, 2026
 Merged


Don't persist inbound committed onions in prod
#4599
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


1 commit


Select commit


File filter

Filter by extension
Filter by extension
Conversations

Failed to load comments. 
Loading
Jump to

Jump to file


Failed to load files. 
Loading
 Diff view

 Diff view


9 changes: 4 additions & 5 deletions
lightning-tests/src/upgrade_downgrade_tests.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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);
Expand Down

14 changes: 8 additions & 6 deletions
lightning/src/ln/channel.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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,
}

Expand DownExpand Up@@ -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!(
Expand DownExpand Up@@ -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() {
Expand All@@ -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);

Comment on lines

+15594
 to 
+15597

@ldk-claude-review-botldk-claude-review-botMay 5, 2026

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The write side is now gated by #[cfg(test)], so in production inbound_committed_update_adds remains empty and TLV field 75 is never written. However, on the read side (line 16554), inbound_committed_update_adds_opt is initialized to Some(Vec::new()) by the optional_vec macro (when the TLV is absent, it stays as Some(empty_vec)). The deserialization code at line 16554-16564 then enters the if let Some(update_adds) block unconditionally and calls iter.next().ok_or(DecodeError::InvalidValue)? for each committed HTLC — which immediately fails because the iterator is empty.
Impact: Any production node that restarts while it has committed inbound HTLCs will fail to deserialize its channel state with DecodeError::InvalidValue. This is a critical availability bug.
Fix: The deserialization code at line 16554 needs a corresponding guard. For example:
ifletSome(update_adds) = inbound_committed_update_adds_opt {if !update_adds.is_empty(){letmut iter = update_adds.into_iter();// ... existing code ...}}
Or gate the entire read-side block with #[cfg(test)] to match the write side.
@valentinewallacevalentinewallaceMay 6, 2026

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned in claude's comment below, this was a false positive. We have test coverage of the case it was outlining as well, in upgrade_downgrade tests.
@TheBlueMattTheBlueMattMay 17, 2026

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't actually think so? cfg(test) isn't set in lightning-tests because lightning is technically being compiled as a dependency there. I can assert!(inbound_committed_update_adds.is_empty()) and lightning-tests still passes.
@joostjagerjoostjagerMay 18, 2026

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be visible in the coverage report too with the tests flag enabled?
@valentinewallacevalentinewallaceMay 18, 2026

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't actually think so? cfg(test) isn't set in lightning-tests because lightning is technically being compiled as a dependency there. I can assert!(inbound_committed_update_adds.is_empty()) and lightning-tests still passes.
To clarify, I meant that if the prod panic bug claude was pointing out was actually present, it would've been hit in lightning-tests because of exactly what you say (cfg(test) isn't set there). It's (to me at least) expected behavior that assert!(inbound_committed_update_adds.is_empty()) would pass in lightning-tests w/ the changes in this PR
@TheBlueMattTheBlueMattMay 18, 2026

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My assumption was that do_upgrade_mid_htlc_forward relied on the now-test-only code but I guess its really an upgrade test not a downgrade test...it does seem like its a dead test though as reconstruct_manager_from_monitors will always be false? Is it just intended to run once the code is in prod?
@valentinewallacevalentinewallaceMay 19, 2026

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's all correct. I did update the test's comment to say it was a "preemptive" test. If I rebased #4359 on top of this, it would use it.
I could remove it, but I was thinking we could just delete everything related to inbound onion persistence (or not) at the same time, rather than deleting some stuff and possibly having to remember to re-add it later.
 },
 &InboundHTLCState::LocalRemoved(ref removal_reason) => {
 4u8.write(writer)?;
Expand Down

22 changes: 11 additions & 11 deletions
lightning/src/ln/channelmanager.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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
Comment thread
joostjager marked this conversation as resolved.

// 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),
Expand DownExpand Up@@ -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) {
Comment thread
joostjager marked this conversation as resolved.

 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 {
Expand DownExpand Up@@ -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(|| {
Expand DownExpand Up@@ -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);
 }
Expand Down
Loading

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
Don't persist inbound committed onions in prod#4599

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

 Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and
privacy statement. We’ll occasionally send you account related emails.

Already on GitHub?
Sign in
to your account

 Merged

TheBlueMatt
merged 1 commit into

from

May 19, 2026
 Merged


Don't persist inbound committed onions in prod
#4599
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


1 commit


Select commit


File filter

Filter by extension
Filter by extension
Conversations

Failed to load comments. 
Loading
Jump to

Jump to file


Failed to load files. 
Loading
 Diff view

 Diff view


9 changes: 4 additions & 5 deletions
lightning-tests/src/upgrade_downgrade_tests.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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);
Expand Down

14 changes: 8 additions & 6 deletions
lightning/src/ln/channel.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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,
}

Expand DownExpand Up@@ -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!(
Expand DownExpand Up@@ -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() {
Expand All@@ -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);

Comment on lines

+15594
 to 
+15597

@ldk-claude-review-botldk-claude-review-botMay 5, 2026

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The write side is now gated by #[cfg(test)], so in production inbound_committed_update_adds remains empty and TLV field 75 is never written. However, on the read side (line 16554), inbound_committed_update_adds_opt is initialized to Some(Vec::new()) by the optional_vec macro (when the TLV is absent, it stays as Some(empty_vec)). The deserialization code at line 16554-16564 then enters the if let Some(update_adds) block unconditionally and calls iter.next().ok_or(DecodeError::InvalidValue)? for each committed HTLC — which immediately fails because the iterator is empty.
Impact: Any production node that restarts while it has committed inbound HTLCs will fail to deserialize its channel state with DecodeError::InvalidValue. This is a critical availability bug.
Fix: The deserialization code at line 16554 needs a corresponding guard. For example:
ifletSome(update_adds) = inbound_committed_update_adds_opt {if !update_adds.is_empty(){letmut iter = update_adds.into_iter();// ... existing code ...}}
Or gate the entire read-side block with #[cfg(test)] to match the write side.
@valentinewallacevalentinewallaceMay 6, 2026

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned in claude's comment below, this was a false positive. We have test coverage of the case it was outlining as well, in upgrade_downgrade tests.
@TheBlueMattTheBlueMattMay 17, 2026

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't actually think so? cfg(test) isn't set in lightning-tests because lightning is technically being compiled as a dependency there. I can assert!(inbound_committed_update_adds.is_empty()) and lightning-tests still passes.
@joostjagerjoostjagerMay 18, 2026

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be visible in the coverage report too with the tests flag enabled?
@valentinewallacevalentinewallaceMay 18, 2026

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't actually think so? cfg(test) isn't set in lightning-tests because lightning is technically being compiled as a dependency there. I can assert!(inbound_committed_update_adds.is_empty()) and lightning-tests still passes.
To clarify, I meant that if the prod panic bug claude was pointing out was actually present, it would've been hit in lightning-tests because of exactly what you say (cfg(test) isn't set there). It's (to me at least) expected behavior that assert!(inbound_committed_update_adds.is_empty()) would pass in lightning-tests w/ the changes in this PR
@TheBlueMattTheBlueMattMay 18, 2026

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My assumption was that do_upgrade_mid_htlc_forward relied on the now-test-only code but I guess its really an upgrade test not a downgrade test...it does seem like its a dead test though as reconstruct_manager_from_monitors will always be false? Is it just intended to run once the code is in prod?
@valentinewallacevalentinewallaceMay 19, 2026

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's all correct. I did update the test's comment to say it was a "preemptive" test. If I rebased #4359 on top of this, it would use it.
I could remove it, but I was thinking we could just delete everything related to inbound onion persistence (or not) at the same time, rather than deleting some stuff and possibly having to remember to re-add it later.
 },
 &InboundHTLCState::LocalRemoved(ref removal_reason) => {
 4u8.write(writer)?;
Expand Down

22 changes: 11 additions & 11 deletions
lightning/src/ln/channelmanager.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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
Comment thread
joostjager marked this conversation as resolved.

// 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),
Expand DownExpand Up@@ -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) {
Comment thread
joostjager marked this conversation as resolved.

 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 {
Expand DownExpand Up@@ -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(|| {
Expand DownExpand Up@@ -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);
 }
Expand Down
Loading