Skip to content

ChannelManager persistence #743

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.

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

Closed
valentinewallace opened this issue Oct 20, 2020 · 3 comments · Fixed by #752
Closed

ChannelManager persistence #743

valentinewallace opened this issue Oct 20, 2020 · 3 comments · Fixed by #752

Comments

@valentinewallace
Copy link
Contributor

The two data structures that needed to be persisted within RL are ChannelMonitors and the ChannelManager. ChannelMonitor persistence was mostly done in #681, but automated ChannelManager persistence hasn't been touched yet.

Afaik the design of ChannelManager persistence isn't spelled out anywhere. I'm not sure what it would ideally look like, but wanted to put out 2 design ideas for critique:

  1. We could have a similar Persister API, i.e. channelmanager::Persist as the sister trait to channelmonitor::Persist (downside: for every e.g. update_channel callsite, there needs to be an update_manager call added too)

  2. We could just kick off ChannelManager persistence in the background in the FilesystemPersister methods (i.e. asynchronously persist the CM on persist_channel and update_channel) and document well that all implementers of channelmonitor::Persist are required to do so

This is all assuming that it's best if the ChannelManager is re-persisted on every update to a ChannelMonitor, which I think is the case but I haven't verified this myself yet.

relevant discussion: #681 (comment)

@TheBlueMatt
Copy link
Collaborator

Blocking writing when we don't strictly need to seems not-ideal, especially if we can kick it off to the background, which should be roughly as fast in terms of latency between ChannelMonitor/Channel storage (during which time restarting implies channel closure). Still, not doing so means committing to a runtime, or at least spawning a single thread of our own. Given its a separate module I think I'd Lean towards a single thread, at least if we put a few other timer functions in it.

@ariard
Copy link

ariard commented Oct 20, 2020

What the severity of failing to backup a ChannelManager ? Is a direct funds loss or only a channel force-closure case ?

Post #611, I think it's only the second case, if so we can allow to only schedule persistence on a given timer, not at every channel update ? At least it could be an option as a latency boost, only drop the ChannelMonitor.

I think I'd Lean towards a single thread, at least if we put a few other timer functions in it.

You lean towards 2. IIUC ?

As of today, InMemoryChannelKeys is serialized as part of ChannelMonitor. I wonder if we shouldn't clarify this, as an external signer will have its own serialization logic and thus may not to drop to disk each time, even if you call .write(). Implementation of Writeable for ChannelKeys must be a disk persistence. For secure elements a disk persistence might trigger a whole encryption+hashing of payload for storing beyond the trusted enclase, and thus might "batch" by default.

@TheBlueMatt
Copy link
Collaborator

What the severity of failing to backup a ChannelManager ? Is a direct funds loss or only a channel force-closure case ?

Right, its only the second - ChannelMonitors alone should always be sufficient to close everything out on-chain correctly, with or without an up-to-date ChannelManager (though I'm not sure that we can do it without a ChannelManager at all, though a stale one should be fine).

You lean towards 2. IIUC ?

Right, I meant that we'd spawn our own thread in a ChannelManager-persistence crate, ie a single thread that we own but two threads in a common use.

I wonder if we shouldn't clarify this, as an external signer will have its own serialization logic and thus may not to drop to disk each time

I believe its discussed in the docs (and its somewhat implied by the fact that ChannelMonitor isn't writeable unless the ChannelKeys is writeable) - if you have an external signer, you probably just want to write out an ID and let the external signer store state as it sees fit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants
@TheBlueMatt @valentinewallace @ariard and others