Skip to content

Async persister #3905

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

Async persister #3905

wants to merge 5 commits into from
+450 −221

Conversation

joostjager
Copy link
Contributor

Stripped down version of #3778

@ldk-reviews-bot
Copy link

👋 Hi! I see this is a draft PR.
I'll wait to assign reviewers until you mark it as ready for review.
Just convert it out of draft status when you're ready for review!

joostjager
joostjager commented Jul 2, 2025
View reviewed changes
lightning-background-processor/src/lib.rs
/// # struct Store {}
/// # impl lightning::util::persist::KVStore for Store {
/// # struct StoreSync {}
/// # impl lightning::util::persist::KVStoreSync for StoreSync {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what we'd get with a partial conversion. Chain monitor still requires a sync implementation of the kv store for now.

Do we really not want to provide a wrapper here that uses block_on to syncify the async kvstore?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really don't buy that we do, given the rate of issues that we've seen with hang-risk from block_on. In practice I imagine most implementations should be able to do it themselves pretty easily.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So go with the dual trait approach for now, requiring async users to implement both?

@joostjager joostjager force-pushed the async-persister branch 4 times, most recently from 651ea9f to 1006cee Compare July 3, 2025 06:52
@joostjager joostjager force-pushed the async-persister branch 3 times, most recently from 3fb7d6b to 1847e8d Compare July 3, 2025 09:57
@joostjager joostjager self-assigned this Jul 3, 2025
@joostjager joostjager mentioned this pull request May 12, 2025
Open
22 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TheBlueMatt TheBlueMatt TheBlueMatt left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@joostjager joostjager

Labels
None yet
Projects
Weekly Goals
Status: No status
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@joostjager @ldk-reviews-bot @TheBlueMatt