Add persistent closed channel history and list_closed_channels()#882
Add persistent closed channel history and list_closed_channels()#882f3r10 wants to merge 6 commits into
list_closed_channels()#882Conversation
|
👋 Thanks for assigning @tnull as a reviewer! |
Camillarhi
left a comment
Camillarhi
left a comment
There was a problem hiding this comment.
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
| closed_at: closed_at.0.ok_or(DecodeError::InvalidValue)?, | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Can we use impl_writeable_tlv_based! here instead? Matches the rest of the StorableObject types in the codebase
| /// 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> { |
There was a problem hiding this comment.
The new list_closed_channels() API isn't exposed in bindings.
|
🔔 1st Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 3rd Reminder Hey @tnull! This PR has been waiting for your review. |
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
Thanks, already looks pretty good. Some comments.
| let mut set = tokio::task::JoinSet::new(); | ||
|
|
||
| // Fill JoinSet with tasks if possible | ||
| while set.len() < BATCH_SIZE && !stored_keys.is_empty() { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
rebased to use the latest read_all_objects
| assert_eq!(record.funding_txo.unwrap().txid, funding_txo.txid); | ||
| assert!(record.closure_reason.is_some()); | ||
|
|
||
| closed_channel_before = record.clone(); |
| node_b.sync_wallets().unwrap(); | ||
|
|
||
| // Verify the record is present before restart. | ||
| let closed_a = node_a.list_closed_channels(); |
There was a problem hiding this comment.
Should we also check that b also persisted the closed channel?
| @@ -252,6 +255,18 @@ pub enum Event { | |||
There was a problem hiding this comment.
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
f3r10
force-pushed
the
feat/persist_historical_channels
branch
3 times, most recently
from
ac11095 to
20b75bb
Compare
f3r10
force-pushed
the
feat/persist_historical_channels
branch
from
20b75bb to
1960f50
Compare
Camillarhi
left a comment
Camillarhi
left a comment
There was a problem hiding this comment.
Thanks! I left a few more comments.
|
🔔 1st Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 3rd Reminder Hey @tnull! This PR has been waiting for your review. |
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
Feel free to squash. Two minor comments, and then we should move forward with this.
|
|
||
| let event = Event::ChannelClosed { | ||
| let user_channel_id = UserChannelId(user_channel_id); | ||
| let is_outbound = self |
There was a problem hiding this comment.
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.
|
🔔 1st Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 3rd Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 4th Reminder Hey @tnull! This PR has been waiting for your review. |
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
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.
| /// 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). |
There was a problem hiding this comment.
Hmm, is this correct? Wouldn't we always get a ChannelClosed event that by now has the required field?
| (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())), |
There was a problem hiding this comment.
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?
| .as_secs(); | ||
|
|
||
| let funding_txo = | ||
| channel_funding_txo.map(|op| OutPoint { txid: op.txid, vout: op.index as u32 }); |
There was a problem hiding this comment.
nit: Should be able to use into_bitcoin_outpoint here.
| // 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
🤔 I have been thinking about this problem, and you're right, and the correct fix is the durable approach: persisting the flags at ChannelPending time.
- 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.
From the previous review of codex, I chose the idempotent path because it was quicker to implement. The insert_or_update + fallback change covers the restart replay case where a prior insert did succeed, but it doesn't fully solve the problem.
I can try the durable approach but it looks more difficult and would take more time. Should I make this change on this PR or open a follow-up issue? wdyt @tnull ?
There was a problem hiding this comment.
We're not super pressed for time here, so it would be good to do it in one PR. Given it's basically a pretty different approach you could even consider whether to open it as an alternative PR.
There was a problem hiding this comment.
ok cool, I am going to start with the durable approach and let you know when is ready
There was a problem hiding this comment.
During Monday's dev sync, @tnull suggested that this storage could be generalized further to hold channel splice intents.
Essentially, when a user initiates a splice, LDK needs to reach quiescence with the counterparty, negotiated and sign the new funding transaction, and wait for the splice to lock. If we restart before signatures are exchanged, we need to retry the splice. Similar for v2 opens, but we don't support those yet.
I had Claude plan this out to work with this PR. We'd store a ChannelRecord that has three variants (Pending, Funded and Closed) where the Funded variant can hold a SpliceIntent. Something like this:
enum ChannelRecord { // src/...; StorableObject, Id = UserChannelId
/// Channel being established — holds the open/funding intent. [opens — later]
Unfunded { counterparty_node_id: PublicKey, intent: OpenIntent },
/// Channel live — holds an in-flight splice intent, if any.
Funded {
counterparty_node_id: PublicKey,
channel_id: ChannelId,
pending_splice: Option<SpliceIntent>, // [this PR]
// is_outbound / funding_txo / capacity_sats / is_announced fold in with #882
},
/// Channel closed — #882's immutable snapshot. [#882 — later]
Closed { /* channel_id, funding_txo, capacity_sats, is_outbound, is_announced, reason, closed_at */ },
}
struct SpliceIntent {
pre_splice_funding_txo: OutPoint, // completion discriminator — changes only when the splice locks
contribution: FundingContribution, // re-fed → funding_contributed(.., None)
kind: SpliceKind, // user params — rebuilds a fresh contribution when the stored one is stale
attempts: u8,
}
enum SpliceKind { In { amount_sats: u64 }, Out { outputs: Vec<TxOut> }, Rbf }
// `kind` serves two purposes: Rbf changes the probe interpretation (prior `Some` is *expected*),
// and In/Out carry the params needed to rebuild a contribution via a fresh splice_channel template.
// enum OpenIntent { V1 { amount_sats, push_msat, config, announce, zero_reserve }, // → create_channel [later]
// V2 { contribution: FundingContribution } } // → funding_contributed [later]@f3r10 Would this be sufficient to cover your use cases?
There was a problem hiding this comment.
oh ok great thanks @jkczyz I am going to check the draft and push a commit
f3r10
force-pushed
the
feat/persist_historical_channels
branch
from
2c7e16f to
f532dca
Compare
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.
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.
…s()` Replace the in-memory set + `insert_or_update` fallback approach with a fully durable solution for tracking `is_outbound`/`is_announced` flags. The previous approach stored these flags in ephemeral `HashSet`s that were seeded from `list_channels()` at startup and consumed on `ChannelClosed`. A fallback to any existing `ClosedChannelDetails` record was added to handle `ReplayEvent`, but a gap remained: if `insert_or_update` failed (returning `ReplayEvent`) and the node restarted before the retry, the in-memory sets would be empty (closed channels don't appear in `list_channels()`) and no persisted record would exist yet, causing both flags to silently default to `falsee`. Fix this by persisting a `PendingChannelInfo` record (containing `is_outbound` and `is_announced`) to the KV store at `ChannelPending` time under a new `pending_channels/` namespace. The `ChannelClosed` handler now resolves the flags with the following priority: 1. `pending_channel_store` — durable, survives restarts and replays 2. In-memory sets — covers channels opened before this version 3. Existing `ClosedChannelDetails` record — idempotency guard The `PendingChannelInfo` record is deleted after `event_queue.add_event` succeeds. It is intentionally kept alive until that point so that any replay of `ChannelClosed` (e.g. due to a failed `insert_or_update` or `add_event`) still finds the correct flags in the store.
…s()` Generalize PendingChannelInfo into ChannelRecord::Funded. Rather than shipping two separate per-channel stores, introduce a ChannelRecord enum with a single Funded variant that already carries the fields the splice PR will need (counterparty_node_id, channel_id) in addition to is_outbound/is_announced flags. This lets the splice PR add pending_splice: Option<SpliceIntent> as a new optional TLV field without touching the store infrastructure.
f3r10
force-pushed
the
feat/persist_historical_channels
branch
from
f532dca to
1ff7639
Compare
5 participants
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
Implementation
Closed channel persistence (src/closed_channel.rs, src/io/): ClosedChannelDetails implements StorableObject with insert_or_update semantics (closed records are immutable — update() always returns false). Records are stored under the closed_channels KV namespace keyed by UserChannelId. insert_or_update makes the ChannelClosed handler idempotent: if the event is replayed after a crash between persistence and add_event, the second write is a no-op and the event goes throuh cleanly. Failure to persist returns ReplayEvent so LDK retries.
Per-channel state store (src/channel/store.rs, new): Introduces a ChannelRecord enum with a single Funded variant that durably captures counterparty_node_id, channel_id, is_outbound, and is_announced at ChannelPending time (before funding confirms). Records are stored under the channel_records KV namespace and removed once ChannelClosed is fully processed. This is more robust than a purely in-memory approach: if the node crashes between ChannelPending and ChannelClosed, the flags survive the restart. The enum design is intentionally extensible — a future pending_splice field can be added as a new optional TLV without breking existing records (motivated by the planned splice-retry support).
ChannelClosed lookup chain: is_outbound and is_announced are resolved with a priority fallback: (1) channel_record_store.get() — the durable record written at ChannelPending; (2) closed_channel_store.get() — handles the replay case where insert_or_update already succeeded but add_event failed and the channel record was alreaddy removed; (3) in-memory outbound_channel_ids / announced_channel_ids sets — fallback for channels opened before this upgrade.
Startup (src/builder.rs): Loading closed channel records and channel records joins the existing tokio::join! block alongside payments, pending payments, and node metrics — no added sequential startup cost as history grows.
Serialization compatibility: counterparty_node_id on ChannelReady and ChannelClosed events is now a required field. Events persisted by LDK Node v0.1.0 and prior that are missing this field will fail to deserialize. closed_at in ClosedChannelDetails is also required (no default_value fallback) since there are no pre-existing records to migrate.