Add BdkActor
#750
Add BdkActor
#750
Conversation
A factory function is good enough, we were not using the struct for anything other than calling new()
An actor that manages the `bdk::Wallet` resource. It allows us to use the wallet whilst the inevitably expensive on-chain sync is happening in the background. We would like to use the async version of `EsploraBlockchain`, but bitcoindevkit/bdk#165 is still an issue. The only hacky bit stems from the fact that we still have to implement certain non-async foreign traits and to access any `bdk::Wallet` resource we now have to go via async methods, since the actor is async. To do so, at some point we have to call an async function "from a sync context". The natural way to do so (for me) would be to use `runtime.block_on`, which schedules an async task and waits for it to resolve, blocking the thread. *But*, these non-async foreign trait methods are actually called elswhere in _async_ contexts. This leads to `runtime.block_on` panicking because `tokio` is trying to prevent us from blocking a thread in an async context. This is analogous to us misusing async and doing an expensive CPU-bound computation in an async context, but here `tokio` is able to "aid" us since `block_on` is provided by `tokio`. The solution is to use the following pattern: ```rust tokio::task::block_in_place(|| { tokio::runtime::Handle::current().block_on(async { // async code }) }); ``` From the documentation of `block_in_place`, we are able to run "the provided blocking function on the current thread without blocking the executor". We therefore avoid the panic, as we no longer block the executor when calling `block_on`. This has one final side-effect, which is that all `ln-dlc-node` async tests now need to go back to using the `multi_thread` flavour. `block_in_place` works by moving all scheduled tasks to a different worker thread and without `multi_thread` there is only one worker thread, so it just panics. Co-authored-by: Mariusz Klochowicz <mariusz@klochowicz.com>
|
Thanks to @holzeis for pointing out that we will still have to wait for the sync to handle any other actor message, as we are holding a mutable reference to the whole actor every time we handle the At this stage I am questioning if this even helps at all, as even if I reintroduced the cache (it was there in a previous version of this PR) one would not be able to access the cache until each sync finishes regardless. I think this is probably a reasonable refactor of the code, but maybe we should hold off until the next version of |
|
@luckysori @klochowicz Although I think its a reasonable refactor, I wouldn't do that now and rather wait for bdk 1.0.0. I am a bit afraid of regressions. |
|
@luckysori Can we close that PR? |
Fixes #730.
An actor that manages the
bdk::Walletresource. It allows us to use the wallet whilst the inevitably expensive on-chain sync is happening in the background. But only for those things that can be cached, such as the balance, wallet history and addresses.We would like to use the async version of
EsploraBlockchain, but bitcoindevkit/bdk#165 is still an issue.The only hacky bit stems from the fact that we still have to implement certain non-async foreign traits and to access any
bdk::Walletresource we now have to go via async methods, since the actor is async.To do so, at some point we have to call an async function "from a sync context". The natural way to do so (for me) would be to use
runtime.block_on, which schedules an async task and waits for it to resolve, blocking the thread. But, these non-async foreign trait methods are actually called elswhere in async contexts. This leads toruntime.block_onpanicking becausetokiois trying to prevent us from blocking a thread in an async context. This is analogous to us misusing async and doing an expensive CPU-bound computation in an async context, but heretokiois able to "aid" us sinceblock_onis provided bytokio.The solution is to use the following pattern:
From the documentation of
block_in_place, we are able to run "the provided blocking function on the current thread without blocking the executor". We therefore avoid the panic, as we no longer block the executor when callingblock_on.This has one final side-effect, which is that all
ln-dlc-nodeasync tests now need to go back to using themulti_threadflavour.block_in_placeworks by moving all scheduled tasks to a different worker thread and withoutmulti_threadthere is only one worker thread, so it just panics.