Futures returned in library are not send #165
Comments
|
Isn't this somewhat connected to the discussion we had about making the wallet thread safe? If the wallet was thread safe then you could spawn a background thread and call Actually, I think having a thread-safe wallet is probably the first requirement to then make the futures I guess the biggest problem with thread-safe wallets in general is that the database is in an inconsistent state during |
|
So unless I have misunderstood something, I guess there are a few steps before we can get to working on Send futures:
|
|
I agree the first priorities should be to make the |
|
The difficulty with the approach is that now you have to have two impls -- one using I think a better solution is to just remove interior mutability from Wallet (i.e. remove RefCell) and make &self mutable in methods like Then if you want to do a let wallet: Arc<Mutex<Wallet<...>>> = Arc::clone(&wallet);
tokio::spawn(async move {
let mut wallet = wallet.lock().await;
wallet.sync(...).await;
})I think this will work. Although rust has a way of foiling my plans in this area. The disadvantage of this is that now anyone wanting to use the wallet while |
|
I just ran into this while using I have one actor had controls access to the wallet and thus, the wallet itself does not need to be thread-safe. The message handlers are all async though and will be run on the tokio runtime. At the moment, the lack of
and with |
I would also advocate for this. Accessing the wallet from multiple threads / tasks should be an application decision and can be solved in various ways (actors, mutex, etc). |
|
I've changed my thinking on this topic. After working with various database APIs they are not at all consistent about why they need to be Therefore I don't think we can manage when the database should and shouldn't bee mutable and we should force the Side note: #461 made database updates atomic (for esplora and electrum) -- the sync logic no longer updates the database while syncing but puts everything into one big batch update at the end (see |
|
Is making the DB thread-safe going to make the entire wallet thread-safe? Because if not, then why bother with internal mutability when users need to wrap it in a Mutex anyway? |
Yes I think so. Well this would be necessary condition for going in this direction. |
|
An alternative design could be to decouple the wallet and the database from each other. A call to
This makes doing a sync a bit more verbose as users would have to do something like: let status = db.get_sync_status();
let result = wallet.sync(status);
db.apply_sync_result(result);However, the benefits would be that the wallet does not care whether or not the DB is mutable / immutable or sync / async. Using such a design may remove the need for It is an ergonomics hit although I don't think it would be particularly bad. In my experience with BDK, I always wrapped the Going down this route would result in removal of all type parameters from |
|
@thomaseizinger thanks for thinking more deeply about this problem. Here's the issue with your suggestion:
The wallet would need the database trait still:
Your line of questioning about what exactly should be the relationship between components is really important. What is a wallet? Is it even a good abstraction to base this library around? I think a lot about this but I don't think it's directly relevant here. In any case it seems to me that we do need a |
I agree with you. Is someone currently working on this? If not, can we have some design notes on what is expected so it can be picked up? Happy to contribute a PR if we can get some mentoring notes on how to change things :) |
|
Ok so I will paint my grand vision of how this should go:
let sync_session = wallet.start_sync(options)?;
let result = blockchain.sync(&sync_session)?; // this is the one that might be async
wallet.apply_sync(&result)?;So this would fix things because we wouldn't care about whether the database is What is a
So consider the following structures // given to the blockchain
struct SyncSession {
// txs that were previously in the mempool.
// So we don't need to download them again and can tell if we need to delete them.
prev_mempool: Vec<Txid>,
// The blockid of the last sync
last_sync_block: (BlockHash, u32)
// the list of addresses to look at for each keychain
script_pubkeys_interested: HashMap<KeychainKind, Box<dyn Iterator<Item=Script> + Send>>
}
// returned from the blockchain
struct SyncResult {
transactions: SyncTxs,
// this will be set to last_sync_block next time
sync_block: (BlockHahsh, u32)
// where we are up to for each keychain
keychain_indexes: HashMap<KeychainKind, u32>
}
enum SyncTxs {
Partial {
// txs we haven't seen before that are in mempool or chain now
new: Vec<TransactionDetails>,
// txs that were in the mempool but are now gone
mempool_confirmed: Vec<Txid>,
// these txs have disappeared from the mempool
mempool_gone: Vec<Txid>
},
Full {
// all transactions ever associated with this wallet.
// wallet should hose the current database and replace it with the data here.
txs: Vec<TransactionDetails>
}
}This stops us needing a reference to the Database during sync and allows us to just exchange data back and forth with the blockchain. The slight downside is we do a full download in the case where we previously synced on a stale block. But this will happen very rarely in practice like once a month if you are syncing all the time. If this is actually a problem we could store the last two blockhashes which would decrease this. On a side not I think #521 could be fixed at the same time by architecting the script pubkey iterators properly. Thoughts? |
If we can make this work then I believe this would be a really clean way of resolving the async/sync duality of blockchain operations. Pre-condition for that is that we can gather all required information in one go, do the sync and come back with a result. With #535, perhaps the API should actually be the other way round: blockchain.sync(&wallet).await?;Maybe we can do the same thing for the database: database.sync(&wallet).await?;A wallet could by default have an in-memory storage and "sync" with a persistent backend like sqlite or seld. |
|
I was discussing with @rajarshimaitra and he brought up a good point to consider, which is if always sync the I like the |
|
Using something like an let blockchain: AsyncBlockchain = ...; // impl an async version of a blockchain
let database: AsyncDatabase = ...; // impl an async version of a database
wallet.sync(&blockchain, &database).await?; // async version of the functions
wallet.get_new_address(&database).await?; And if let blockchain: Blockchain = ...; // impl a blocking version of a blockchain
let database: Database = ...; // impl a blocking version of a database
wallet.sync(&blockchain, &database); // blocking versions of the functions
wallet.get_new_address(&database);Though in this approach the EDIT: I should have refreshed my memory on how the |
|
Feature flags really should never change APIs because they are evaluated additively by cargo. Assume the following situation:
You cannot use crate A and B in the same dependency graph now because A will fail to compile because its code is written against the non-async API. Feature flags work best if they add code, like additional functions. (Be careful with enum variants, those are also nasty). |
|
Just wanted to second what @thomaseizinger said. One of the main goals of this refactoring is to remove all this |
|
Thanks for the explanation, making features additive makes sense and sounds like the right way to design things. It's even in the Cargo Book which apparently I need to read more closely! 😵💫 |
|
Ideally this would be implemented in such a way that the same code that works on web can also be easily targeted to work on desktop. Imagine a use-case where the isomorphic UI library Iced can compile to both using the bulk of the same application logic and UI code, with platform-specific edge cases handled through conditional compilation macros. |
|
FWIW I hacked out a workaround using a number of refactors. Definitely too ugly to be upstreamed but it gets the job done if you really really need BDK's futures to be Here's the branch: https://github.com/lexe-tech/bdk/tree/max/thread-safe And the two commits that implement it: which at this point can be cherry-picked onto the You can confirm that the futures are indeed cargo clippy --all-targets --features=async-interface,use-esplora-async --no-default-features -- --allow=warningsImportant to read the commit messages to understand the caveats for using this patch correctly (i.e. |
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 <[email protected]>
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 <[email protected]>
Send is explicitly opted out of here:
bdk/macros/src/lib.rs
Line 49 in 43cb033
This is a bit of a problem. For example, after potentially receiving some funds you might like to do:
To sync the wallet in the background. This is not possible since the future must be Send to send to
tokio::spawn.It will take some re-engineering of the internals to make this work.
@afilini do you have opinions on how to address this?
The text was updated successfully, but these errors were encountered: