Add a per-channel record store

[jkczyz]

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.

[ldk-reviews-bot]

I've assigned @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.

[tnull]

Looks good, already needs a rebase though.

I assume the plan is to use this via list_channels and the closed channels APIs in follow-ups / #882?

[tnull]

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.

[joostjager]

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?

[jkczyz]

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?

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?

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)

[joostjager]

I checked out #882 and #930 and left a comment on each. I think we should have a strong story for why this generic functionality needs to live in ldk-node. Crossing the LDK/ldk-node persistence boundary adds complexity, and we already have more than enough of that.

[tnull]

I checked out #882 and #930 and left a comment on each. I think we should have a strong story for why this generic functionality needs to live in ldk-node. Crossing the LDK/ldk-node persistence boundary adds complexity, and we already have more than enough of that.

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 ChannelManager persistence hopefully going away or being reduced over time, it's not even clear where we'd store (historical) channel data, as we surely don't want to keep all the ChannelMonitors around forever/after channels have been closed.

[joostjager]

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.

[tnull]

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.

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).

[joostjager]

Fair point about timing. Some of this is hard to revisit later. Up to you all how much weight to give to my concerns.

[tnull]

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.

[jkczyz]

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.

[ldk-reviews-bot]

I've assigned @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.

[tnull]

Looks good, already needs a rebase though.

I assume the plan is to use this via list_channels and the closed channels APIs in follow-ups / #882?

[tnull]

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.

[joostjager]

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?

[jkczyz]

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?

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?

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)

[joostjager]

I checked out #882 and #930 and left a comment on each. I think we should have a strong story for why this generic functionality needs to live in ldk-node. Crossing the LDK/ldk-node persistence boundary adds complexity, and we already have more than enough of that.

[tnull]

I checked out #882 and #930 and left a comment on each. I think we should have a strong story for why this generic functionality needs to live in ldk-node. Crossing the LDK/ldk-node persistence boundary adds complexity, and we already have more than enough of that.

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 ChannelManager persistence hopefully going away or being reduced over time, it's not even clear where we'd store (historical) channel data, as we surely don't want to keep all the ChannelMonitors around forever/after channels have been closed.

[joostjager]

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.

[tnull]

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.

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).

[joostjager]

Fair point about timing. Some of this is hard to revisit later. Up to you all how much weight to give to my concerns.

[tnull]

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.

[jkczyz]

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.

[ldk-reviews-bot]

I've assigned @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.

[tnull]

Looks good, already needs a rebase though.

I assume the plan is to use this via list_channels and the closed channels APIs in follow-ups / #882?

[tnull]

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.

[joostjager]

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?

[jkczyz]

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?

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?

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)

[joostjager]

I checked out #882 and #930 and left a comment on each. I think we should have a strong story for why this generic functionality needs to live in ldk-node. Crossing the LDK/ldk-node persistence boundary adds complexity, and we already have more than enough of that.

[tnull]

I checked out #882 and #930 and left a comment on each. I think we should have a strong story for why this generic functionality needs to live in ldk-node. Crossing the LDK/ldk-node persistence boundary adds complexity, and we already have more than enough of that.

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 ChannelManager persistence hopefully going away or being reduced over time, it's not even clear where we'd store (historical) channel data, as we surely don't want to keep all the ChannelMonitors around forever/after channels have been closed.

[joostjager]

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.

[tnull]

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.

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).

[joostjager]

Fair point about timing. Some of this is hard to revisit later. Up to you all how much weight to give to my concerns.

[tnull]

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.

[jkczyz]

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.

[ldk-reviews-bot]

I've assigned @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.

[tnull]

Looks good, already needs a rebase though.

I assume the plan is to use this via list_channels and the closed channels APIs in follow-ups / #882?

[tnull]

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.

[joostjager]

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?

[jkczyz]

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?

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?

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)

[joostjager]

I checked out #882 and #930 and left a comment on each. I think we should have a strong story for why this generic functionality needs to live in ldk-node. Crossing the LDK/ldk-node persistence boundary adds complexity, and we already have more than enough of that.

[tnull]

I checked out #882 and #930 and left a comment on each. I think we should have a strong story for why this generic functionality needs to live in ldk-node. Crossing the LDK/ldk-node persistence boundary adds complexity, and we already have more than enough of that.

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 ChannelManager persistence hopefully going away or being reduced over time, it's not even clear where we'd store (historical) channel data, as we surely don't want to keep all the ChannelMonitors around forever/after channels have been closed.

[joostjager]

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.

[tnull]

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.

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).

[joostjager]

Fair point about timing. Some of this is hard to revisit later. Up to you all how much weight to give to my concerns.

[tnull]

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.

[jkczyz]

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.

[ldk-reviews-bot]

I've assigned @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.

[tnull]

Looks good, already needs a rebase though.

I assume the plan is to use this via list_channels and the closed channels APIs in follow-ups / #882?

[tnull]

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.

[joostjager]

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?

[jkczyz]

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?

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?

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)

[joostjager]

I checked out #882 and #930 and left a comment on each. I think we should have a strong story for why this generic functionality needs to live in ldk-node. Crossing the LDK/ldk-node persistence boundary adds complexity, and we already have more than enough of that.

[tnull]

I checked out #882 and #930 and left a comment on each. I think we should have a strong story for why this generic functionality needs to live in ldk-node. Crossing the LDK/ldk-node persistence boundary adds complexity, and we already have more than enough of that.

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 ChannelManager persistence hopefully going away or being reduced over time, it's not even clear where we'd store (historical) channel data, as we surely don't want to keep all the ChannelMonitors around forever/after channels have been closed.

[joostjager]

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.

[tnull]

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.

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).

[joostjager]

Fair point about timing. Some of this is hard to revisit later. Up to you all how much weight to give to my concerns.

[tnull]

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.

[jkczyz]

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.

[ldk-reviews-bot]

I've assigned @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.

[tnull]

Looks good, already needs a rebase though.

I assume the plan is to use this via list_channels and the closed channels APIs in follow-ups / #882?

[tnull]

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.

[joostjager]

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?

[jkczyz]

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?

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?

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)

[joostjager]

I checked out #882 and #930 and left a comment on each. I think we should have a strong story for why this generic functionality needs to live in ldk-node. Crossing the LDK/ldk-node persistence boundary adds complexity, and we already have more than enough of that.

[tnull]

I checked out #882 and #930 and left a comment on each. I think we should have a strong story for why this generic functionality needs to live in ldk-node. Crossing the LDK/ldk-node persistence boundary adds complexity, and we already have more than enough of that.

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 ChannelManager persistence hopefully going away or being reduced over time, it's not even clear where we'd store (historical) channel data, as we surely don't want to keep all the ChannelMonitors around forever/after channels have been closed.

[joostjager]

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.

[tnull]

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.

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).

[joostjager]

Fair point about timing. Some of this is hard to revisit later. Up to you all how much weight to give to my concerns.

[tnull]

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.

[jkczyz]

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.

[ldk-reviews-bot]

I've assigned @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.

[tnull]

Looks good, already needs a rebase though.

I assume the plan is to use this via list_channels and the closed channels APIs in follow-ups / #882?

[tnull]

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.

[joostjager]

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?

[jkczyz]

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?

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?

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)

[joostjager]

I checked out #882 and #930 and left a comment on each. I think we should have a strong story for why this generic functionality needs to live in ldk-node. Crossing the LDK/ldk-node persistence boundary adds complexity, and we already have more than enough of that.

[tnull]

I checked out #882 and #930 and left a comment on each. I think we should have a strong story for why this generic functionality needs to live in ldk-node. Crossing the LDK/ldk-node persistence boundary adds complexity, and we already have more than enough of that.

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 ChannelManager persistence hopefully going away or being reduced over time, it's not even clear where we'd store (historical) channel data, as we surely don't want to keep all the ChannelMonitors around forever/after channels have been closed.

[joostjager]

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.

[tnull]

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.

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).

[joostjager]

Fair point about timing. Some of this is hard to revisit later. Up to you all how much weight to give to my concerns.

[tnull]

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.