-
Notifications
You must be signed in to change notification settings - Fork 368
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
Monitor updating persister #2359
Monitor updating persister #2359
Conversation
Mh, if we want to go this way, may I suggest that we upstream LDK Node's This might make a slight migration necessary as namespaces are not handled as implicit path prefixes anymore, but are dedicated variables. However, it also has generally been designed to be persistence-backend agnostic, i.e., it doesn't make assumptions about |
Should keep the ability to only persist the monitors still. IIRC persisting all the updates takes more storage. |
Sounds like upstreaming the The Thanks for this feedback, I'll report back with some updates when I get some more time to work on it. |
I don't feel like this needs a separate trait - we can have an option to turn the update storage off, but its nice for the general trait to have reads so that we can provide utilities to load things at startup rather than everyone having to write it by hand.
Sadly |
Can you suggest a good way to do that? This occurred to me, but I wasn't seeing a way to get that configuration into the scope of the
That's a good point. It's cumbersome to get strings out of it, too. The FWIW, the implementation seen here does store updates in a directory:
|
Hmm, lol, yea, that's actually not trivial...We'd have to replace the generic impl with a wrapper struct that takes a |
Yeah, so then it becomes opt-in by way of using that wrapper, instead of the trait directly. Which seems easy enough, but also very similar to what I was imagining by using the I think what I will do is wait to see a PR for upstreaming |
Sorry for the delay here. Not 100% sure how we want to go about adding a get/list/delelte-capable KV trait on LDK, but a goal for 0.0.117 on my end is to add an async-capable KV trait on LDK so I may be able to think on it when I get there. Tentatively assigning myself for that, but please ping me in a month if I forget and don't have any followup here. |
Yeah, unfortunately this is still blocked on me upstreaming the @domZippilli excuse the delay here! |
Excuse the delay once again. Now finally came around to upstream the Feel free to rebase this PR on top to make use of the updated interface! (Note I now also included LDK Node's variant of |
dac1604
to
a1f6c67
Compare
Hi @tnull and @TheBlueMatt, I went ahead and revised this to use the trait under review in #2472. I have not tested it yet. I'm sure there are bugs. In particular, the addition of namespace to the KV API means I need to handle key paths differently, and that's good for tripping me up. But, that will all shake out when I can set up a test environment. Here's where I think this stands:
So, PTAL, advise on things you'd like to see changed, |
a1f6c67
to
8658c7d
Compare
Hi @tnull and @TheBlueMatt, This isn't done yet, but it's significantly updated and probably at a good stopping point to get more feedback, and let final decisions on #2472 get made. My changes are in Some key points to review:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did a first high-level pass, didn't look to closely at the logic itself and the tests yet. You might need to rebase as the KVStore
PR now maintains the code in the lightning-persister
crate and I now also included the TestStore
upstreamed from LDK Node so you don't have to roll your own.
a7962e3
to
8de5176
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically LGTM, I think.
c5d8a36
to
de73d99
Compare
Fixes #1426 |
d7ebca1
to
8f409c4
Compare
Commit needs to be merged into single commit, where it's description contains motivation. removing everything else about other commit msgs. |
To update the group generally:
|
ab0fc9d
to
b3b92a6
Compare
I made the significant tweaks that I knew I had to, and then also formatted one last time with Should be good to go for more detailed formatting review (and of course other stuff). |
I pushed an update to the changelog that tells more of the upgrade story (attn: @G8XSU who suggested this). It does occur to me -- is there a dangerous situation we could get into if one immediately wants to downgrade? Imagine this:
At that point you would load stale
Thoughts? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty much good to me, only a few minor things.
I think we should land this soon to make sure it lands for the release. And, if any further minor feedback (docs/formatting) comes up, address it as a part of a follow-up.
The documented start-up procedure is to write all rust-lightning/lightning/src/ln/channelmanager.rs Lines 8910 to 8913 in 827b17c
We probably can't rely on (2) in case shutdown doesn't happen gracefully. And I'd shy away from (3) as it seems like we shouldn't have a side-effect when reading, IMO. |
Sounds good. I think anybody who is building from one of the LDK exemplars will be doing this even without reading the manual. |
Yes, I think we should be covered since we mention it in backward compat section last point. |
Another issue I wanted to bring up: Should we be giving guidance for I don't have such data, so I could characterize the trade off. As the number (N) increases:
Given all that, 10 or 100 is probably right for most people. You'll attenuate your I/O churn by ~1/N and have smoother throughput on deletes, and lower typical worst case bounds for cleanup listing. |
I think it will very much depend on scale of number of payments. We can leave this upto client given they have to understand these tradeoffs and make a design choice regarding their storage. Out of the things you mentioned, main thing would be "waves" of deletes of updates. And for that maybe smaller number probably makes sense. But if a node is dealing with huge scale of # payments, then they might want to just have a higher N and offload deletes for later. |
That's fine. I veered into recommending a number (for most people), and that's not necessarily what I want to do at this stage, it's more explaining the tradeoffs that I think would benefit the customer. Maybe adding a short paragraph that summarizes two or three of those considerations? I think you're probably right that at very high volumes, deletes may be something you don't do at all, at least not in-band with monitor writes. Though at that scale you're probably looking at this struct as a reference and not using it unmodified anyway. |
Agreed. It's fine to lay out the tradeoffs and let them experiment with what works for them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM modulo remaining comments and documenting tradeoffs for maximum_pending_updates
.
Please wrap commit messages at 72 characters as per: https://cbea.ms/git-commit/
cddc8e7
to
faa0f56
Compare
Based on instruction from LDK Discord, I'm going to do one final squash of the 11 fixup commits I have at this point. |
4ee435e
to
ce1e88b
Compare
lightning/src/util/persist.rs
Outdated
if let Ok((_, old_monitor)) = maybe_old_monitor { | ||
// Updates must be > the old monitor's update_id. If we can't add, the channel | ||
// is already stored closed so we can skip this. | ||
if let Some(start) = old_monitor.get_latest_update_id().checked_add(1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to start cleanup from old_monitor.get_latest_update_id(); (not add +1)
there is a case where a monitor_update is persisted.
and immediately afterwards a block is connected.
in case of block_connected, we will end up persisting full monitor with same latest_update_id.
hence we might not be deleting the monitor_update persisted earlier if we use start+1;
(after this we might issue some extra deletes in other cases but that should be fine)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for helping me get to the bottom of this (looked scary for a minute!). I've FP'd a change that removes the checked add, bypasses the assertion we're not deleting non-existent things on start
of this loop, and removes the hacky conditions on the test that exercised this. Should all be working now.
ce1e88b
to
9298de0
Compare
One more FP, I turned off some local feedback on my build and missed a mistake when refactoring the trait bounds. |
MonitorUpdatingPersister is an implementation of Persister that stores ChannelMonitorUpdates separately from ChannelMonitors. Its RFC is in lightningdevkit#2545, at https://github.com/orgs/lightningdevkit/discussions/2545. Co-Authored-By: Elias Rohrer <dev@tnull.de>
9298de0
to
0430d33
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets Go !
More or less waiting for CI now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Followups can come later.
signer_provider: SP, | ||
) -> Self | ||
where | ||
ES::Target: EntropySource + Sized, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These bounds are redundant, I think we can just drop the full where clause here.
ES::Target: EntropySource + Sized, | ||
SP::Target: SignerProvider + Sized, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two bounds look redundant and can likely be removed (same in following function).
Monitor-updating KV store persister
Adds a new MonitorUpdatingPersister struct that customers may choose to use, so that new events on a channel do not serialize the entire monitor to storage.
How it works
monitors/
monitor_updates/
>
the monitor's, or is the closed channel update constant.Migration Path
No migration should be necessary! It should "just work" since the full monitors are stored the same way. When this persister runs the first time, it'll deserialize the existing full monitors, and the start writing updates.