Add persistent closed channel history and `list_closed_channels()` by f3r10 · Pull Request #882 · lightningdevkit/ldk-node

f3r10 · 2026-04-20T16:31:30Z

Summary

Closes #851.

Adds persistent historical closed channel records so applications can query all channels that have ever closed on this node, surviving restarts.

New public API

Node::list_closed_channels() -> Vec<ClosedChannelDetails>
ClosedChannelDetails struct: channel_id, user_channel_id,
counterparty_node_id, funding_txo, channel_capacity_sats,
last_local_balance_msat, is_outbound, closure_reason, closed_at
Event::ChannelClosed gains three new fields: channel_capacity_sats,
channel_funding_txo, last_local_balance_msat

Implementation

Persistence (src/closed_channel.rs, src/io/): ClosedChannelDetails implements StorableObject with insert-only semantics (closed records are immutable). Records are stored under the "closed_channels" KV namespace keyed by UserChannelId.

Startup (src/builder.rs): read_closed_channels() joins the existing tokio::join! block so historical records are loaded in parallel with payments, pending payments, and node metrics — no added sequential startup cost as history gronws.

Event handlinng (src/event.rs): On ChannelClosed, the handler writes a ClosedChannelDetails record to the store and emits the enriched public event. Channel direction (is_outbound) is tracked via an in-memory outbound_channel_ids set, seeded from channel_manager.list_channels() at startup and updated on ChannelPending, since ChannelClosed does not carry that information directly.

ldk-reviews-bot · 2026-04-20T16:31:32Z

👋 Thanks for assigning @tnull as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

Camillarhi

Thanks for looking into this. I dropped some comments. Also, CI seems to be failing due to a formatting issue. Kindly run cargo fmt --all to resolve this

Camillarhi · 2026-04-20T20:28:51Z

+			closed_at: closed_at.0.ok_or(DecodeError::InvalidValue)?,
+		})
+	}
+}


Can we use impl_writeable_tlv_based! here instead? Matches the rest of the StorableObject types in the codebase

Camillarhi · 2026-04-20T20:42:26Z

+	/// Retrieve a list of closed channels.
+	///
+	/// Channels are added to this list when they are closed and will be persisted across restarts.
+	pub fn list_closed_channels(&self) -> Vec<ClosedChannelDetails> {


The new list_closed_channels() API isn't exposed in bindings.

ldk-reviews-bot · 2026-04-22T16:42:32Z

🔔 1st Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-04-25T00:00:41Z

🔔 2nd Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-04-27T00:01:50Z

🔔 3rd Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

tnull

Thanks, already looks pretty good. Some comments.

tnull · 2026-04-27T11:18:05Z

+	let mut set = tokio::task::JoinSet::new();
+
+	// Fill JoinSet with tasks if possible
+	while set.len() < BATCH_SIZE && !stored_keys.is_empty() {


Makes sense to DRY this up for now, but depending on when #876 lands we might need to adapt this to use the new BatchingStore.

rebased to use the latest read_all_objects

tnull · 2026-04-27T11:25:34Z

+		assert_eq!(record.funding_txo.unwrap().txid, funding_txo.txid);
+		assert!(record.closure_reason.is_some());
+
+		closed_channel_before = record.clone();


nit: Why do we clone here?

tnull · 2026-04-27T11:26:12Z

+		node_b.sync_wallets().unwrap();
+
+		// Verify the record is present before restart.
+		let closed_a = node_a.list_closed_channels();


Should we also check that b also persisted the closed channel?

tnull · 2026-04-27T11:36:46Z

I think we should be good breaking backwards compat with LDK node v0.1, so let's make this a required field. Then we can also avoid having the Option propagate. Please add a not regarding serialization compatibility as part of a # Pending section in CHANGELOG.md

Introduce `ClosedChannelDetails`, a new record type persisted to the KV store under the `"closed_channels"` namespace whenever a channel closes. Records are written in the `ChannelClosed` event handler and loaded back at startup in parallel with other stores via `tokio::join!`. Add `Node::list_closed_channels()` to expose the full history of closed channels across restarts. Track outbound channel direction via an in-memory `outbound_channel_ids` set seeded from `channel_manager.list_channels()` at startup and updated on `ChannelPending` events, since `ChannelClosed` does not carry that information directly.

…sed event We no longer need to maintain backwards compatibility with LDK Node v0.1.0, so we can make this a required field and avoid propagating the Option through the API.

Camillarhi

Thanks! I left a few more comments.

ldk-reviews-bot · 2026-05-16T00:00:58Z

🔔 1st Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-05-18T00:02:00Z

🔔 2nd Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-05-20T00:02:28Z

🔔 3rd Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

tnull

Feel free to squash. Two minor comments, and then we should move forward with this.

tnull · 2026-05-21T15:34:40Z


-				let event = Event::ChannelClosed {
+				let user_channel_id = UserChannelId(user_channel_id);
+				let is_outbound = self


Codex:

[P2] Persist channel flags for replayed closures — /home/tnull/workspace/ldk-node/src/event.rs:1646-1655
When a ChannelClosed LDK event is replayed after a restart or after handle_event returns ReplayEvent, the channel can already be gone from list_channels() and these process-local sets are not reconstructed. In that case .remove() returns false, so an outbound/public channel is persisted in
ClosedChannelDetails as inbound/unannounced; a failed first attempt also consumes the entries before replay. Store these flags durably before closure or make this path idempotent.

Fall back to the already-persisted record for is_outbound/is_announced when in-memory sets are empty on replay, use insert_or_update to avoid overwriting correct values, and propagate persist failures as ReplayEvent.

ldk-reviews-bot · 2026-05-25T00:00:41Z

🔔 1st Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-05-27T00:01:01Z

🔔 2nd Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-05-30T00:01:32Z

🔔 3rd Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-06-01T00:02:11Z

🔔 4th Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

tnull

Excuse the delay here! Generally looks good mod some minor comments.

The only more fundamental question is https://github.com/lightningdevkit/ldk-node/pull/882/changes#r3340832531 which we probably should clarify before moving on.

tnull · 2026-06-02T11:08:23Z

+	/// Will be `None` if the channel was closed before the counterparty's node ID could be
+	/// determined (e.g., very early in the channel negotiation process).


Hmm, is this correct? Wouldn't we always get a ChannelClosed event that by now has the required field?

tnull · 2026-06-02T11:12:09Z

+	(10, last_local_balance_msat, option),
+	(12, is_outbound, required),
+	(14, closure_reason, upgradable_option),
+	(16, closed_at, (default_value, SystemTime::now().duration_since(UNIX_EPOCH).unwrap_or(Duration::from_secs(0)).as_secs())),


Hmm, given we don't have any backwards compat. issues, let's just make this a required field and set the value upon initialization rather than via default_value?

tnull · 2026-06-02T11:18:47Z

+					.as_secs();
+
+				let funding_txo =
+					channel_funding_txo.map(|op| OutPoint { txid: op.txid, vout: op.index as u32 });


nit: Should be able to use into_bitcoin_outpoint here.

tnull · 2026-06-02T11:32:02Z

+				// the channel is no longer in list_channels() and the in-memory sets are
+				// not repopulated for it, so .remove() returns false. Fall back to any
+				// already-persisted record so we don't overwrite correct values with false.
+				let (is_outbound, is_announced) = self


IIUC, this will fall back to (false, false) in case of a replayed event, leading to inaccurate information. This makes we wonder if we don't need to start storing channel metadata generally (i.e., prior to close), not only once it's already gone and we struggle to maintain access to all necessary information?

ldk-reviews-bot requested a review from tnull April 20, 2026 16:42

Camillarhi suggested changes Apr 20, 2026

View reviewed changes

tnull reviewed Apr 27, 2026

View reviewed changes

f3r10 force-pushed the feat/persist_historical_channels branch 3 times, most recently from ac11095 to 20b75bb Compare May 13, 2026 18:55

f3r10 requested review from Camillarhi and tnull May 13, 2026 18:55

f3r10 added 2 commits May 13, 2026 14:43

Make counterparty_node_id non-optional on ChannelReady and ChannelClo…

1960f50

…sed event We no longer need to maintain backwards compatibility with LDK Node v0.1.0, so we can make this a required field and avoid propagating the Option through the API.

f3r10 force-pushed the feat/persist_historical_channels branch from 20b75bb to 1960f50 Compare May 13, 2026 19:43

Camillarhi suggested changes May 15, 2026

View reviewed changes

Comment thread src/closed_channel.rs

Comment thread src/event.rs

jkczyz mentioned this pull request May 21, 2026

Add splice RBF support #888

Open

tnull reviewed May 21, 2026

View reviewed changes

Comment thread src/event.rs

Comment thread src/closed_channel.rs

tnull reviewed May 21, 2026

View reviewed changes

f3r10 added 2 commits May 22, 2026 11:16

Make ChannelClosed persistence idempotent and retry on failure

583b2b0

Fall back to the already-persisted record for is_outbound/is_announced when in-memory sets are empty on replay, use insert_or_update to avoid overwriting correct values, and propagate persist failures as ReplayEvent.

Document when optional fields of ClosedChannelDetails can be None

afa2c7b

f3r10 requested a review from tnull May 22, 2026 16:19

tnull reviewed Jun 2, 2026

View reviewed changes

		/// Will be `None` if the channel was closed before the counterparty's node ID could be
		/// determined (e.g., very early in the channel negotiation process).

Conversation

f3r10 commented Apr 20, 2026

Summary

New public API

Implementation

Uh oh!

ldk-reviews-bot commented Apr 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Camillarhi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ldk-reviews-bot commented Apr 22, 2026

Uh oh!

ldk-reviews-bot commented Apr 25, 2026

Uh oh!

ldk-reviews-bot commented Apr 27, 2026

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Camillarhi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ldk-reviews-bot commented May 16, 2026

Uh oh!

ldk-reviews-bot commented May 18, 2026

Uh oh!

ldk-reviews-bot commented May 20, 2026

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ldk-reviews-bot commented May 25, 2026

Uh oh!

ldk-reviews-bot commented May 27, 2026

Uh oh!

ldk-reviews-bot commented May 30, 2026

Uh oh!

ldk-reviews-bot commented Jun 1, 2026

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

ldk-reviews-bot commented Apr 20, 2026 •

edited

Loading