Add a per-channel record store#946
Conversation
Introduce durable, per-channel state keyed by the channel's user-facing identifier and tracking where the channel sits in its lifecycle (unfunded, funded, or closed), persisted alongside the node's other stores and loaded at startup. Wiring it in now lets upcoming per-channel features land independently of one another rather than stacking on a single change. A record currently holds only the channel's identity; the per-feature state and the transitions between lifecycle states are added by the work that needs them. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
I've assigned @tnull as a reviewer! |
| payment_store: Arc<PaymentStore>, | ||
| // Foundational per-channel record store, loaded and persisted here so that upcoming | ||
| // per-channel features can build on it. Not yet read within this crate. | ||
| #[allow(dead_code)] |
There was a problem hiding this comment.
Please drop this allow(dead_code). IMO it's fine to have the warnings on main temporarily, better than hiding and potentially never getting back to fixing them.
|
I don't think we should merge this while it is still dead code. I think the interesting part is how this store will be populated and reconciled with LDK's persisted ChannelManager/ChannelMonitor state, especially to avoid another desync between the different persistence levels? |
|
The motivation is so #882 and #930 could use a common store. Since #930 depends on #888, I pulled this out. Would y'all prefer to have #882 be based on this and review that instead?
That's a good question. #882 needs this for closed channels while #930 needs to use it for splice intents, some of which LDK doesn't persist. Though @tnull mentioned using it for other purposes: #882 (comment) |
I think the main reason is that LDK historically always had a stance of not keeping persisted state for data that is not absolutely necessary, and leaving that to the user, i.e., in this case LDK Node. And architecture-wise, with |
|
To me that historical boundary is somewhat arbitrary, and I'd try to avoid introducing new persistence layers just based on that. But let's keep the design discussion for #882 and #930 in those PRs, because they depend on a channel store in quite different ways. I do think there is enough still unresolved here that we should not merge this dead-code PR yet. |
No, these PRs are not the right place for these larger design discussions. If you feel strongly about it, please open an issue for it and we can discuss on Discord or offline. But don't derail the discussions of these PRs with overarching architectural questions, especially since it's quite late to bring this up (the PRs have been open for quite a while and we're slowly approaching agreement on approach and getting closer to land). |
|
Fair point about timing. Some of this is hard to revisit later. Up to you all how much weight to give to my concerns. |
Let's discuss live later. |
4 participants
Introduce durable, per-channel state keyed by the channel's user-facing identifier and tracking where the channel sits in its lifecycle (unfunded, funded, or closed), persisted alongside the node's other stores and loaded at startup. Wiring it in now lets upcoming per-channel features land independently of one another rather than stacking on a single change.
A record currently holds only the channel's identity; the per-feature state and the transitions between lifecycle states are added by the work that needs them.