Futures returned in library are not send

[LLFourn]

Send is explicitly opted out of here:

bdk/macros/src/lib.rs

Line 49 in 43cb033

#[async_trait(?Send)]

.

This is a bit of a problem. For example, after potentially receiving some funds you might like to do:

tokio::spawn(wallet.sync(...).then(|res| info!(the result of the sync was "{:?}", res))

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?

[afilini]

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 sync() in that thread, without necessarily having to spawn a task using tokio and going through the async interface.

Actually, I think having a thread-safe wallet is probably the first requirement to then make the futures Send, otherwise it doesn't make sense to have a background task run sync() if you can't do anything else on the wallet because there's a Mutex that locks everything.

I guess the biggest problem with thread-safe wallets in general is that the database is in an inconsistent state during sync. We would probably need to have transactions of some sorts, so that you only commit the full changes at the end of sync and always keep the database in a consistent state.

[afilini]

So unless I have misunderstood something, I guess there are a few steps before we can get to working on Send futures:

1. Make Database thread-safe
2. Add transactional operations to Database
3. Update all the Blockchain types to use the transactions with Database
4. Make Blockchain thread-safe, or wrap it in a Mutex. Here we need some kind of locking mechanism to avoid having multiple sync running at the same time
5. Make Wallet thread-safe, remove RefCells. At this point you can already achieve the "sync in background" by spawning a thread and calling sync inside
6. ??? to make futures Send

[notmandatory]

I agree the first priorities should be to make the Database, Blockchain and Wallet thread safe. For our bdk-jni android example the app code is spawning it's own threads from the JVM to sync the wallet or get balance updates, etc. So if bdk blocks on those calls it's not a problem.

[LLFourn]

The difficulty with the approach is that now you have to have two impls -- one using tokio::sync::Mutex and one using RefCell to account for no_std/WASM. I don't see how to get around this but I haven't got much experience finding work arounds. I feel it will be further complicated by maybe_await stuff because you now have to maybe_await the lock on the components as well (or just borrow_mut if using RefCell).

I think a better solution is to just remove interior mutability from Wallet (i.e. remove RefCell) and make &self mutable in methods like sync.
This would circumvent the need for all the rather involved steps @afilini mentions above.

Then if you want to do a sync in a background thread:

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 sync is running is going to have to wait.
There could be ways of easing this in the future -- for example you expose a sync_next_batch method which only does a little bit of work (and indicates whether the sync is done). This would let another thread do something between successive calls to sync_next_batch.

[thomaseizinger]

I just ran into this while using bdk::Wallet within an actor framework.

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 Send futures makes it impossible to use the EsploraBlockchain because without async-interface it panics that it would block a worker thread:

thread 'main' panicked at 'Cannot start a runtime from within a runtime. This happens because a function (like block_on) attempted to block the current thread while the thread is being used to drive asynchronous tasks.', /home/thomas/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.15.0/src/runtime/enter.rs:39:9

and with async-interface, the code doesn't compile because the async-trait implementation of Blockchain on EsploraBlockchain returns Futures that are not Send.

[thomaseizinger]

I think a better solution is to just remove interior mutability from Wallet (i.e. remove RefCell) and make &self mutable in methods like sync.

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).

[LLFourn]

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 &mut or not. For example, a database API may have &mut self for every call because it has only one connection and the underlying connection is mutable. Others may only have &mut self for writing operations but not for reading (this is what BDK database assumes). Furthermore, others may only have &mut self when starting and ending a transaction/batch update. Finally, some have no mutability at all and handle everything internally or using thread local variables like sled.

Therefore I don't think we can manage when the database should and shouldn't bee mutable and we should force the Database itself to handle thread safety i.e. make the database trait Sync. I don't think there is much of a cost to doing this and it sure would simplify a lot . Perhaps we'd need to make an exception for wasm32 so as to avoid using Mutex in MemoryDatabase. But the better solution here is just to make a proper wasm databse that uses web apis!

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 script_sync.rs). This makes things easier to reason about and means even if doing an async sync the database will never be locked across an .await point.
As far as I can tell it is sound to do multiple syncs at the same time and if not it is a problem with the sync logic. After #461 the db state should be consistent after a sync with esplora and electrum (I think!). It should probably be avoided anyway because it's redundant but it is not the responsibility of the database to ensure this.

[thomaseizinger]

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?

[LLFourn]

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.

[thomaseizinger]

An alternative design could be to decouple the wallet and the database from each other. A call to sync could:

• Require a SyncStatus struct as a parameter that can be obtained from the database
• Return a SyncResult struct that can be applied to a database

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 Database trait entirely.

It is an ergonomics hit although I don't think it would be particularly bad. In my experience with BDK, I always wrapped the bdk::Wallet in a Wallet struct myself to provide slightly different APIs and/or compose more logic on top of it. Having to compose a few more pieces from bdk in my own Wallet struct wouldn't be a big deal here.

Going down this route would result in removal of all type parameters from Wallet and users would have to compose the wallet with a database and a client by passing data back and forth between those components. WDYT?

[LLFourn]

@thomaseizinger thanks for thinking more deeply about this problem. Here's the issue with your suggestion:

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 Database trait entirely.

The wallet would need the database trait still:

1. The wallet needs to use it to create transactions.
2. The wallet actually mutates it when generating new addresses.
3. In general you need to be able to query the database for stuff -- it's handy if it has the same API regardless of the underlying concrete DB.

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 Database trait and making it Sync makes sense and would fix the problem at hand. I think this is true even if we adopted a sync API more like what you were suggesting unless we can stop all database queries during the sync. This might be possible but requires some careful design work!

[thomaseizinger]

In any case it seems to me that we do need a Database trait and making it Sync makes sense and would fix the problem at hand. I think this is true even if we adopted a sync API more like what you were suggesting unless we can stop all database queries during the sync. This might be possible but requires some careful design work!

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?
Not being able to use an async-native backend with bdk is mildly annoying. I'd really like to move away from the blocking interface. Our work-around for #521 is to scan for 1000 addresses and together with a blocking client, this means we are blocking one of tokio's executor threads for ~4 seconds every time a sync is triggered.

Happy to contribute a PR if we can get some mentoring notes on how to change things :)