Clean up persistence constants #4105
Conversation
Though users maybe shouldn't use `MonitorUpdatingPersister` if they don't actually want to persist `ChannelMonitorUpdate`s, we also shouldn't panic if `maximum_pending_updates` is set to zero.
In the coming commits `MonitorUpdatingPersister`'s internal state will be reworked. To avoid spurious test diff, we instead use the public API of `MonitorUpdatingPersister` rather than internal bits in tests.
In the coming commits, we'll use the `MonitorUpdatingPersister` as *the* way to do async monitor updating in the `ChainMonitor`. However, to support folks who don't actually want a `MonitorUpdatingPersister` in that case, we explicitly support them setting `maximum_pending_updates` to 0, disabling all of the update-writing behavior.
As we've done with several other structs, this adds an async variant of `MonitorUpdatingPersister` and adds an async-sync wrapper for those using `KVStoreSync`. Unlike with other structs, we leave `MonitorUpdatingPersister` as the sync variant and make the new async logic a `MonitorUpdatingPersisterAsync` as the async monitor updating flow is still considered beta. This does not yet expose the async monitor updating logic anywhere, as doing a standard `Persist` async variant would not work for ensuring the `ChannelManager` and `ChainMonitor` don't block on async writes or suddenly require a runtime.
Pre-0.1, after a channel was closed we generated `ChannelMonitorUpdate`s with a static `update_id` of `u64::MAX`. In this case, when using `MonitorUpdatingPersister`, we had to read the persisted `ChannelMonitor` to figure out what range of monitor updates to remove from disk. However, now that we have a `list` method there's no reason to do this anymore, we can just use that. Simplifying code that we anticipate never hitting anymore is always a win.
In the next commit we'll use this to spawn async persistence operations in the background, but for now we just move the `lightning-block-sync` `FutureSpawner` into `lightning`.
In the next commit we'll add the ability to use an async `KVStore` as the backing for a `ChainMonitor`. Here we tee this up by adding an async API to `MonitorUpdatingPersisterAsync`. Its not intended for public use and is thus only `pub(crate)` but allows spawning all operations via a generic `FutureSpawner` trait, initiating the write via the `KVStore` before any `await`s (or async functions). Because we aren't going to make the `ChannelManager` (or `ChainMonitor`) fully async, we need a way to alert the `ChainMonitor` when a persistence completes, but we leave that for the next commit.
This finally adds support for full native Rust `async` persistence to `ChainMonitor`. Way back when, before we had any other persistence, we added the `Persist` trait to persist `ChannelMonitor`s. It eventualy grew homegrown async persistence support via a simple immediate return and callback upon completion. We later added a persistence trait in `lightning-background-processor` to persist the few fields that it needed to drive writes for. Over time, we found more places where persistence was useful, and we eventually added a generic `KVStore` trait. In dc75436 we removed the `lightning-background-processor` `Persister` in favor of simply using the native `KVStore` directly. Here we continue that trend, building native `async` `ChannelMonitor` persistence on top of our native `KVStore` rather than hacking support for it into the `chain::Persist` trait. Because `MonitorUpdatingPersister` already exists as a common way to wrap a `KVStore` into a `ChannelMonitor` persister, we build exclusively on that (though note that the "monitor updating" part is now optional), utilizing its new async option as our native async driver. Thus, we end up with a `ChainMonitor::new_async_beta` which takes a `MonitorUpdatingPersisterAsync` rather than a classic `chain::Persist` and then operates the same as a normal `ChainMonitor`. While the requirement that users now use a `MonitorUpdatingPersister` to wrap their `KVStore` before providing it to `ChainMonitor` is somewhat awkward, as we move towards a `KVStore`-only world it seems like `MonitorUpdatingPersister` should eventually merge into `ChainMonitor`.
`TestStore` recently got the ability to make async writes, but wasn't a very useful test as all writes actually completed immediately. Instead, here, we make the writes actually-async, forcing the test to mark writes complete as required.
We only require `Send + Sync` on the `Future`s which are spawned in std builds, so its weird to require them on the trait itself in all builds. Instead, make them consistent.
We already have pretty good coverage of the `MonitorUpdatingPersister` itself as well as `ChainMonitor`'s update-completion handling, so here we just focus on an end-to-end test to make sure `ChainMonitor` behaves at all correctly in the new async mode.
The constants in `lightning::util::persist` are sufficiently long that its often difficult eyeball their correctness which nearly led to several bugs when adding async support. Here we take the first step towards condensing them by making them somewhat more concise, dropping words which are obvious from context.
The constants in `lightning::util::persist` are sufficiently long that its often difficult eyeball their correctness which nearly led to several bugs when adding async support. Here we take a further step towards condensing them by making them somewhat more concise, dropping words which are obvious from context. Importantly, this changes the prefix for monitor *update* persistence namespaces from monitor persistence namespaces so that they are visually distinct. This runs the following replacements: * s/_PERSISTENCE_/_/g * s/CHANNEL_MONITOR_UPDATE/MONITOR_UPDATE/g * s/ARCHIVED_CHANNEL_MONITOR/ARCHIVED_MONITOR/g
The constants in `lightning::util::persist` are sufficiently long that its often difficult eyeball their correctness which nearly led to several bugs when adding async support. As it turns out, all of the `*_SECONDARY_NAMESPACE` constants were simply "", so having a huge pile of them everywhere is quite verbose and makes scanning the `*_NAMESPACE` constants more difficult. Here, we simply drop the `*_SECONDARY_NAMESPACE` constants entirely.
|
👋 Hi! This PR is now in draft status. |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| CHANNEL_MONITOR_PERSISTENCE_SECONDARY_NAMESPACE, | ||
| )?; | ||
| async fn cleanup_stale_updates(&self, lazy: bool) -> Result<(), io::Error> { | ||
| let monitor_keys = self.kv_store.list(pCHANNEL_MONITOR_NAMESPACE, "").await?; |
There was a problem hiding this comment.
There's a typo in the constant name on line 994: pCHANNEL_MONITOR_NAMESPACE should be CHANNEL_MONITOR_NAMESPACE. This will cause a compilation error when building.
| let monitor_keys = self.kv_store.list(pCHANNEL_MONITOR_NAMESPACE, "").await?; | |
| let monitor_keys = self.kv_store.list(CHANNEL_MONITOR_NAMESPACE, "").await?; |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
|
Okay, but "they were intentionally done this way" isn't really a strong argument for keeping them that way. My point here is that, in writing some persistence logic, I almost screwed it up at least three times, with one missed by reviewers and only caught (luckily) by graphite. That tells me the way we have it is totally the wrong tradeoff. If you prefer some other way of rejiggering them to make it more clear I'm all ears, of course. |
I really don't see how a more distinct expressive constant is harder to misuse? Why did you screw up exactly? It looked like a copy/paste mistake?
Glad Graphite caught it, but I doubt there was a review round between the mistake and Graphite discovering it?
The way I see it: they are very expressive, clearly spelling out what they are for and can never be misunderstood across LDK and LDK Node codebases (where we have quite a few more of these, following exactly the same pattern). We also made sure to explicitly always including the corresponding secondary namespace constants for unformity, exactly so that you can't mess up writing "" in the wrong place etc. FWIW, the only way how to make these more foolproof would be to introduce distinct types for primary namespace / secondary namespace / key instead of having them be generic |
Here we clean up the constants, removing words from them that are clear from context and dropping constants that aren't adding anything.
Based on #4063