Skip to content
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

Use Multi threaded runtime in sync_wallets #213

Merged
merged 1 commit into from
Dec 4, 2023

Conversation

G8XSU
Copy link
Contributor

@G8XSU G8XSU commented Dec 1, 2023

This is needed since so that components(such as KVStore) can use block_in_place.
block_in_place is only allowed inside a multi-threaded runtime.
sync_wallets internally syncs channel_manager and chain_monitor which results in persistence using KVStore.
Using blocking sync_wallets should be avoided but is mostly used in functional_tests.

This is needed since so that components(such as `KVStore`) can use `block_in_place`.
`block_in_place` is only allowed inside a multi-threaded runtime.
`sync_wallets` internally syncs channel_manager and chain_monitor which results
in persistence using `KVStore`. Using blocking `sync_wallets` should be avoided
but is mostly used in functional_tests.
@G8XSU G8XSU mentioned this pull request Dec 1, 2023
Copy link
Collaborator

@tnull tnull left a comment

Choose a reason for hiding this comment

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

Ugh, yeah, I was running into this before, too when working on #152. However, luckily I generally found a cleaner way around using block_on with #205.

I think it's fine to go ahead with this for now, but generally I really dislike the all the complexity that comes with stacking different runtimes on top of each other.
E.g., IIUC in this example the block_in_place of VssStore would here let the outer runtime spawn a new worker thread (which is why it needs to be multi-threaded), but then the actual task will be spawned on VssStore's internal runtime.

@G8XSU I think I had brought this up briefly before, but could we consider also exposing a blocking variant of VssClient (switchable via feature as we do it for example for EsploraSyncClient) and use the blocking variant in VssStore until we get proper async KVStore interfaces in LDK?

@tnull tnull merged commit d46efb6 into lightningdevkit:main Dec 4, 2023
11 checks passed
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 this pull request may close these issues.

2 participants