Sqlite db race condition

[kirillzh]

Original context from 03/16/2023:
If we get a new address with New n times, and then we get a new address with LastUnused, we get back the n+1th address (expected behavior).

However, if we send funds to the n-th address, the next time we get a new address with LastUnused, we get back the 2n-th address
And shedding a bit more light on the internals that folks on the team was able to uncover:

Long version: The BDK wallet maintains a database (sqlite in our case). When the wallet is sync’d or when a new address is requested, the wallet caches a large (100) batch of script pubkeys (addresses). These scriptpubkeys are stored in a table with the schema keychain [internal or external], child number, scriptpubkey. When the wallet pulls down transaction data from the electrum server, gets a list of the scriptpubkeys from that database table, and then joins that list to the list of transaction IDs that it gets from the electrum server. It uses this combined list to tell what the last-used address index is. For example, it might see that there are transactions for scriptpubkey 1, 3, and 5. So it knows that the next address it should generate (to prevent re-use) is 6. The problem is that in the version of BDK that we rely on, there is no uniqueness constraints on the table containing the list of scriptpubkeys. That means that under some circumstances, the list of scriptpubkeys can be duplicated. So instead of having 0,1,2,3,4,..., we’ve observed the table contain 0,0,0,1,1,1,2,2,2,3,3,3,... When the wallet sync happens, the fetch of the scriptpubkey list would get these duplicates and join them to the list of transactions. Meaning that if the “latest” transaction was on child 3, it might show up as the 9th entry in the list, causing the wallet to think that the next address it needs to generate is the 10th address.

BDK: 0.25.0

[kirillzh]

03/18/2023:

solution:

• sqlite db connection for bdk is not thread safe
• we were making various concurrent calls to bdk (sync, bdk wallet creation, address generation) on Dispatchers.IO coroutine dispatcher - uses concurrent thread pool
• making concurrent calls to bdk caused the race condition described in the original post
• the fix is to ensure that calls to bdk are concurrent to avoid the db race condition. We settled on single instance Dispatchers.IO. limitedParallelism(1) for all bdk calls to limit thread count to 1 while still offloading work to a background thread, off the main thread.

/**
 * Dispatcher for running BDK write operations on. Uses [Dispatchers.Default] but guarantees
 * that no more than 1 coroutine is executed at the same time.
 *
 * We use this dispatcher to prevent [potential deadlocks](https://github.com/bitcoindevkit/bdk-ffi/issues/205)
 * when executing multiple concurrent BDK write calls (e.g. wallet sync). That's because our current
 * BDK UniFFI implementation is not inherently thread safe.
 */
@Suppress("UnusedReceiverParameter")
internal val Dispatchers.BdkIO: CoroutineDispatcher get() = dispatcherBdkIO

/**
 * This is a singleton to ensure we reuse the same dispatcher instance.
 */
private val dispatcherBdkIO: CoroutineDispatcher = Dispatchers.IO.limitedParallelism(1)

[notmandatory]

Thanks for the detailed explanation, I've added a BDK 1.0.0 label on this so we make sure to verify with 1.0.0 that DB concurrency issues like this get fixed. I think part of the solution will also be to support async-await in our bdk-ffi wrappers.

See also: mozilla/uniffi-rs#1409

[cgarrett-square]

In case it's helpful, when we saw these duplicate values, the reason is because the idx_script_pks_unique constraint was not there, even though the creation of it was defined in the migrations. What we found out is, those migrations ran concurrently because of calls to the BDK from different threads, and would result in the unique constraint being dropped immediately after being created. Then that opened up the door for the bug where we had duplciate script_pubkeys entries.

[notmandatory]

Ok that makes sense and could be something we can do a patch fix for, I think all we'd need to do is a mutex lock when running the migration. Would it help if we can get a fix that can be back-ported to a 0.25.x patch release?

[cgarrett-square]

What do you think @kirillzh? Would we be better off undoing our dispatch fix and having the BDK do the mutex lock?

[notmandatory]

After thinking about it a little more I think you're opening the same DB file from different instances of the wallet.. and in that case adding a mutex in the wallet won't help this issue.

[kirillzh]

After thinking about it a little more I think you're opening the same DB file from different instances of the wallet

That seems to be the case - we are mindlessly have been initing Wallet instance wherever we need it, without caching it. We will look into how we can cache it.

[notmandatory]

Caching and using the same wallet instance should fix the problem since each new wallet only creates one new DB connection, and the migrations execute in a new DB transaction when the connection is created.

[notmandatory]

In bdk_wallet 1.2 you will be able to put your sqlite Connection in a mutex without having to lock the whole wallet (see #1874).