Make backend objects Send + Sync for compatibility with cln-plugin

[chrisguida]

Describe the enhancement
Not sure if this is a feature request or a bug. It sounds like BDK v1.0 wants to have good async support, so it seems this would be good to get working before then? But I'll just put it in as a feature request in case not :)

I'm trying to get this CLN plugin repo to build: https://github.com/chrisguida/watchdescriptor/tree/wip/esplora_client

Currently it seems the support for async in BDK is not quite where cln-plugin (the official Rust CLN plugin framework) is expecting it to be, as it requires everything to be Send + Sync.

I discussed this with @notmandatory and he says:

my steps were going to be to 

1. look at all bdk return types from async functions called in your code; 
2. try to determine which ones are not send+sync; 
3. try to make the necessary type(s) send+sync; 
4. iterate with fields of those types until needed return types are send+sync.
once all this research/testing is done will need to put it into a PR.. 
if any needed types come from other crates then we'll need to 
figure out if/where to wrap them in a mutex or figure out if 
a PR is needed for the dependency crate (like rust-bitcoin or rust-miniscript, 
or some other dependency)

Use case
To be able to use BDK inside CLN plugins

I will take this on myself if someone wants to coach me.

[notmandatory]

@chrisguida for reference also add a comment with the error message you got, the one that mentions the missing send+sync.

[chrisguida]

Here is the error:

error: future cannot be shared between threads safely
   --> src/main.rs:55:10
    |
55  |         .rpcmethod(
    |          ^^^^^^^^^ future returned by `watchdescriptor` is not `Sync`
    |
    = help: the trait `Sync` is not implemented for `dyn std::future::Future<Output = Result<LocalUpdate<KeychainKind, ConfirmationTimeAnchor>, bdk_esplora::esplora_client::Error>> + Send`
note: future is not `Sync` as it awaits another future which is not `Sync`
   --> src/main.rs:370:18
    |
370 |       let update = client
    |  __________________^
371 | |         .scan(
372 | |             local_chain,
373 | |             keychain_spks,
...   |
377 | |             PARALLEL_REQUESTS,
378 | |         )
    | |_________^ await occurs here on type `Pin<Box<dyn std::future::Future<Output = Result<LocalUpdate<KeychainKind, ConfirmationTimeAnchor>, bdk_esplora::esplora_client::Error>> + Send>>`, which is not `Sync`
note: required by a bound in `cln_plugin::Builder::<S, I, O>::rpcmethod`
   --> /Users/steve/.cargo/git/checkouts/lightning-a9dc92ee913af919/9675463/plugins/src/lib.rs:193:47
    |
189 |     pub fn rpcmethod<C, F>(mut self, name: &str, description: &str, callback: C) -> Builder<S, I, O>
    |            --------- required by a bound in this associated function
...
193 |         F: Future<Output = Response> + Send + Sync + 'static,
    |                                               ^^^^ required by this bound in `Builder::<S, I, O>::rpcmethod`

[LLFourn]

It looks like this is a problem with rpcmethod in the lightning plugins library?

But it's weird that something would need a Future to be Sync. See discussion: https://users.rust-lang.org/t/requiring-a-sync-future-in-a-tokio-app/59784/3 So maybe the solution is to remove that bound from the library.

[chrisguida]

cc @cdecker Please see the above comment. Are you sure the cln-plugin framework needs to require Futures to be Sync?

I'm trying to determine whether this is a problem with cln-plugin or bdk (or both).

[chrisguida]

Also FYI I got this error while building this time:

error[E0599]: the function or associated item `new_from_path` exists for struct `Store<'_, LocalChangeSet<KeychainKind, ConfirmationTimeAnchor>>`, but its trait bounds were not satisfied
   --> src/main.rs:333:47
    |
333 |     let db = Store::<bdk::wallet::ChangeSet>::new_from_path(DB_MAGIC.as_bytes(), db_path)?;
    |                                               ^^^^^^^^^^^^^ function or associated item cannot be called due to unsatisfied trait bounds
    |
   ::: /home/cguida/.cargo/git/checkouts/bdk-bbcd84aeefa047ed/8f38e96/crates/chain/src/keychain.rs:123:1
    |
123 | pub struct LocalChangeSet<K, A> {
    | ------------------------------- doesn't satisfy `_: Append`
    |
    = note: the following trait bounds were not satisfied:
            `LocalChangeSet<KeychainKind, ConfirmationTimeAnchor>: bdk_chain::tx_data_traits::Append`

Seems unrelated to the Send + Sync issue I was hitting on previous versions of this code (and which @notmandatory hit on this version of the code. His is the Sync + Send error in the previous message above). Does this warrant a new issue, or am I just doing something dumb?

[junderw]

require Futures to be Sync?

Considering that a Future only uses Self in the context of Pin<&mut Self> in the poll method, and &mut references can not be obtained by something shared between threads (ie. an Arc of a Future has no way to get &mut Self)...

I don't think Sync should ever be needed to bind a Future.

https://users.rust-lang.org/t/future-is-not-sync-when-calling-async-fn-of-dyn-trait-obj-in-another-async-fn/83285/2

Generally, futures should not be required to be Sync. Send should be enough. Any time you get an error saying "this future is not Sync", the solution is to remove that requirement — you should not try to make the future Sync.

[junderw]

Also FYI I got this error while building this time:

Can you try Store::<'_, bdk::wallet::ChangeSet>::new_from_path(DB_MAGIC.as_bytes(), db_path)?;

[chrisguida]

@junderw same error as before:

error[E0599]: the function or associated item `new_from_path` exists for struct `Store<'_, LocalChangeSet<KeychainKind, ConfirmationTimeAnchor>>`, but its trait bounds were not satisfied
   --> src/main.rs:333:51
    |
333 |     let db = Store::<'_, bdk::wallet::ChangeSet>::new_from_path(DB_MAGIC.as_bytes(), db_path)?;
    |                                                   ^^^^^^^^^^^^^ function or associated item cannot be called due to unsatisfied trait bounds
    |
   ::: /home/cguida/.cargo/git/checkouts/bdk-bbcd84aeefa047ed/8f38e96/crates/chain/src/keychain.rs:123:1
    |
123 | pub struct LocalChangeSet<K, A> {
    | ------------------------------- doesn't satisfy `_: Append`
    |
    = note: the following trait bounds were not satisfied:
            `LocalChangeSet<KeychainKind, ConfirmationTimeAnchor>: bdk_chain::tx_data_traits::Append`

[LLFourn]

pinging @evanlinjin in case he an figure out how this LocalChangeSet doesn't do Append anymore.

KeychainKind has to be Ord but I would expect the error message to say if that was the problem. Where does KeychainKind come from? bdk?

[chrisguida]

Ok, I managed to fix the above error by replacing

bdk_file_store = { version = "0.2.0" }

with

bdk_file_store = { path = "../bdk/crates/file_store" }

(i have the bdk master branch stored locally there)

Now I'm back to the original Send + Sync error that this issue is about:

error: future cannot be sent between threads safely
   --> src/main.rs:56:10
    |
56  |         .rpcmethod(
    |          ^^^^^^^^^ future returned by `watchdescriptor` is not `Send`
    |
    = help: within `impl std::future::Future<Output = Result<serde_json::Value, anyhow::Error>>`, the trait `Send` is not implemented for `Rc<RefCell<&mut Wallet<Store<'_, LocalChangeSet<KeychainKind, ConfirmationTimeAnchor>>>>>`
note: future is not `Send` as this value is used across an await
   --> src/main.rs:407:27
    |
397 |     let mut tx_builder = wallet.build_tx();
    |         -------------- has type `TxBuilder<'_, Store<'_, LocalChangeSet<KeychainKind, ConfirmationTimeAnchor>>, BranchAndBoundCoinSelection, CreateTx>` which is not `Send`
...
407 |     client.broadcast(&tx).await?;
    |                           ^^^^^ await occurs here, with `mut tx_builder` maybe used later
...
411 | }
    | - `mut tx_builder` is later dropped here
note: required by a bound in `cln_plugin::Builder::<S, I, O>::rpcmethod`
   --> /home/cguida/.cargo/git/checkouts/lightning-a9dc92ee913af919/9675463/plugins/src/lib.rs:193:40
    |
189 |     pub fn rpcmethod<C, F>(mut self, name: &str, description: &str, callback: C) -> Builder<S, I, O>
    |            --------- required by a bound in this associated function
...
193 |         F: Future<Output = Response> + Send + Sync + 'static,
    |                                        ^^^^ required by this bound in `Builder::<S, I, O>::rpcmethod`

error: future cannot be shared between threads safely
   --> src/main.rs:56:10
    |
56  |         .rpcmethod(
    |          ^^^^^^^^^ future returned by `watchdescriptor` is not `Sync`
    |
    = help: within `impl std::future::Future<Output = Result<serde_json::Value, anyhow::Error>>`, the trait `Sync` is not implemented for `Rc<RefCell<&mut Wallet<Store<'_, LocalChangeSet<KeychainKind, ConfirmationTimeAnchor>>>>>`
note: future is not `Sync` as this value is used across an await
   --> src/main.rs:407:27
    |
397 |     let mut tx_builder = wallet.build_tx();
    |         -------------- has type `TxBuilder<'_, Store<'_, LocalChangeSet<KeychainKind, ConfirmationTimeAnchor>>, BranchAndBoundCoinSelection, CreateTx>` which is not `Sync`
...
407 |     client.broadcast(&tx).await?;
    |                           ^^^^^ await occurs here, with `mut tx_builder` maybe used later
...
411 | }
    | - `mut tx_builder` is later dropped here
note: required by a bound in `cln_plugin::Builder::<S, I, O>::rpcmethod`
   --> /home/cguida/.cargo/git/checkouts/lightning-a9dc92ee913af919/9675463/plugins/src/lib.rs:193:47
    |
189 |     pub fn rpcmethod<C, F>(mut self, name: &str, description: &str, callback: C) -> Builder<S, I, O>
    |            --------- required by a bound in this associated function
...
193 |         F: Future<Output = Response> + Send + Sync + 'static,
    |                                               ^^^^ required by this bound in `Builder::<S, I, O>::rpcmethod`

error: future cannot be shared between threads safely
   --> src/main.rs:56:10
    |
56  |         .rpcmethod(
    |          ^^^^^^^^^ future returned by `watchdescriptor` is not `Sync`
    |
    = help: the trait `Sync` is not implemented for `dyn std::future::Future<Output = Result<LocalUpdate<KeychainKind, ConfirmationTimeAnchor>, bdk_esplora::esplora_client::Error>> + Send`
note: future is not `Sync` as it awaits another future which is not `Sync`
   --> src/main.rs:370:18
    |
370 |       let update = client
    |  __________________^
371 | |         .scan(
372 | |             local_chain,
373 | |             keychain_spks,
...   |
377 | |             PARALLEL_REQUESTS,
378 | |         )
    | |_________^ await occurs here on type `Pin<Box<dyn std::future::Future<Output = Result<LocalUpdate<KeychainKind, ConfirmationTimeAnchor>, bdk_esplora::esplora_client::Error>> + Send>>`, which is not `Sync`
note: required by a bound in `cln_plugin::Builder::<S, I, O>::rpcmethod`
   --> /home/cguida/.cargo/git/checkouts/lightning-a9dc92ee913af919/9675463/plugins/src/lib.rs:193:47
    |
189 |     pub fn rpcmethod<C, F>(mut self, name: &str, description: &str, callback: C) -> Builder<S, I, O>
    |            --------- required by a bound in this associated function
...
193 |         F: Future<Output = Response> + Send + Sync + 'static,
    |                                               ^^^^ required by this bound in `Builder::<S, I, O>::rpcmethod`

warning: `watchdescriptor` (bin "watchdescriptor") generated 16 warnings
error: could not compile `watchdescriptor` (bin "watchdescriptor") due to 3 previous errors; 16 warnings emitted

[chrisguida]

Btw the above is just a straight copy/paste of the wallet_esplora_async example-crate's main function into a cln-plugin callback function, so I would expect it to "just work".

[junderw]

Try adding drop(tx_builder); after you call finish()

[junderw]

Then try tokio::runtime::Handle::try_current()?.block_on(client.scan(...))? instead of just as-is...

[junderw]

These are work arounds...... the main cause is the Sync requirement on cln-plugin library which should be removed and is arguably a bug since there is no need to restrict to Sync types.

[junderw]

To explain:

1. tx_builder isn't Send or Sync... yet it lives across an await point, which means the Future's anonymous struct must hold on to its ownership, and therefore can not be Sync nor Send.

2. The future returned by scan contains tons of generic things with Send bounds, but none of them have Sync bounds so the returned Future can not be Sync... The JoinHandle returned from tokio::spawn implements Send and Sync if the Future's output is Send... which scan's output is Send, so the JoinHandle will implement Send and Sync for us...

[junderw]

These workarounds are kind of hacky... but tbh the dropping tx_builder thing would have been needed regardless of cln-plugin's Sync requirement.

[LLFourn]

Right tx_builder should never held across an await boundry. Writing it like this will prevent this.

let tx = {
    let mut tx_builder = wallet.build_tx();
    // build the tx
    tx_builder.finish()
};

[junderw]

(In case the question comes up)

Why does the example code in this repository that I copy pasted allow holding a tx_builder over an await boundary?

The reason why is the function signature of block_on (which is used in #[tokio::main])... since block_on will wait for the Future to run til completion on the same thread, so it doesn't require Send, Sync, nor 'static... therefore holding tx_builder over an await point is OK.

[junderw]

I have made a PR upstream to fix this issue.

However, please keep in mind about the tx_builder and not holding it over an await point.

I think this can be closed as irrelevant to bdk.

Thanks for helping me find and diagnose the issue @chrisguida!

[LLFourn]

Actually I unconvinced myself that I understand what's going on here. TxBuilder should not be held across the await boundry since you call finish which consumes self to get the tx you are going to broadcast on the lines further down anyway.

finish must be called somewhere in 397-407 right? Could be a rust bug.

[junderw]

aaaaaahhhh! lol

TxBuilder is invariant in the first generic type (Store<..>) which means that the tx_builder is seen as lasting as long as the wallet and db variables by the borrow checker.....

And since it uses Rc internally, it does not impl Send... oof.

TxBuilder is pretty tricky to use in async then...

Using tokio::task::spawn_local and awaiting it immediately, creatively should help.

[junderw]

This change was on top of 0c00bc077fe8fd3da937bc1c5b79d77fe9f8bae4. @chrisguida

This should fix the issue for now until the Sync fix is merged in cln-plugin.

diff --git a/src/main.rs b/src/main.rs
index 912af84..b360ed9 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -367,7 +367,7 @@ async fn watchdescriptor(
             (k, k_spks)
         })
         .collect();
-    let update = client
+    let update = tokio::runtime::Handle::try_current()?.block_on(client
         .scan(
             local_chain,
             keychain_spks,
@@ -376,7 +376,7 @@ async fn watchdescriptor(
             STOP_GAP,
             PARALLEL_REQUESTS,
         )
-        .await?;
+    )?;
     println!();
     wallet.apply_update(update)?;
     wallet.commit()?;

[junderw]

This seems to compile RE: tx_builder.

    let mut psbt;
    {
        let mut tx_builder = wallet.build_tx();
        tx_builder
            .add_recipient(faucet_address.script_pubkey(), SEND_AMOUNT)
            .enable_rbf();

        (psbt, _) = tx_builder.finish()?;
    }
    let finalized = wallet.sign(&mut psbt, SignOptions::default())?;
    assert!(finalized);

I was able to get everything to compile locally after using the handle try_current block_on method.

[LLFourn]

I was just able to get https://github.com/chrisguida/watchdescriptor/tree/wip/esplora_client to compile after fixing the verisons etc and removing sync requirement in the cln-plugin library. Thanks for your help @junderw.

[chrisguida]

Thanks @junderw !! Excellent work, gentlemen!!!

By the way, there seems to be a new error when building with latest master. When I apply @junderw's patch from above and checkout bdk, bdk_file_store and bdk_esplora at 8f38e96 on my local machine, the issue is fixed, but when i checkout latest master (d73669e) I'm getting this:

error[E0599]: `AsyncClient` is not an iterator
   --> src/main.rs:370:73
    |
370 |     let update = tokio::runtime::Handle::try_current()?.block_on(client.scan(
    |                                                                  -------^^^^ `AsyncClient` is not an iterator
    |
   ::: /home/cguida/.cargo/registry/src/index.crates.io-6f17d22bba15001f/esplora-client-0.5.0/src/async.rs:30:1
    |
30  | pub struct AsyncClient {
    | ---------------------- doesn't satisfy `AsyncClient: Iterator`
    |
    = note: the following trait bounds were not satisfied:
            `AsyncClient: Iterator`
            which is required by `&mut AsyncClient: Iterator`

Shall I report this as a new issue? Perhaps just the wallet_esplora_async example needs to be updated?

Also FWIW I did not have to update the cln-plugin dependency after pinning all three crates to 8f38e96 and applying the patch. The build completes successfully and there are no warnings about futures needing to be Sync... am I missing something?

Looks like this is the relevant upstream PR, thanks again for all your help @junderw @LLFourn @notmandatory !

[chrisguida]

Also one more thing, I tried to run the code above and I got this error:

$ l1-cli watchdescriptor "wpkh(tpubEBr4i6yk5nf5DAaJpsi9N2pPYBeJ7fZ5Z9rmN4977iYLCGco1VyjB9tvvuvYtfZzjD5A8igzgw3HeWeeKFmanHYqksqZXYXGsw5zjnj7KM9/*)"
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.', src/main.rs:370:57
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Does this warrant a new issue?

Thanks!

[junderw]

Nope that's just my workaround not working. :P

Just wait on the Sync removal from cln-plugin.

I'll try to make another workaround later when I have time.

[junderw]

Does this warrant a new issue?

Replace that whole block_on thing with this instead: (This ugly workaround can be removed once the Sync bound is removed)

    let handle = tokio::runtime::Handle::try_current()?;
    let update = std::thread::scope(|scope| {
        scope
            .spawn(|| {
                handle.block_on(client.scan(
                    local_chain,
                    keychain_spks,
                    [],
                    [],
                    STOP_GAP,
                    PARALLEL_REQUESTS,
                ))
            })
            .join()
        // join() only returns an Err if scan panicked.
    })
    .expect("Propagating scan() panic")?;

[junderw]

(Explanation: client contains references, and is thus non-static, so even spawn_local requires 'static, so we can't spawn a new task on the runtime... so we need to use block_on... but we can't use block_on on the main runtime thread... so we need to use a separate thread but thread::spawn also requires 'static... so we use scoped threads which don't require 'static, and block_on in the new thread (which we know will not be managed by the runtime, so block_on shouldn't error.)

[chrisguida]

Hmm, it builds, but I'm having trouble running it still. Now it's just hanging indefinitely.

When you say "Just wait on the Sync removal from cln-plugin", does that mean that the code in the wallet_esplora_async example works now with the branch of cln-plugin in the PR that removes the Sync requirement? Or are there other changes I need to make?

I'd like to get started working with the version of cln-plugin I know is going to work if possible.

[junderw]

Can you verify exactly where it's hanging?

My PR removing Sync should allow it to work just with client.scan(...).await

[chrisguida]

Hmm no, I cannot determine where it's hanging. For some reason when I include the above workaround in the watchdescriptor rpc method callback, i cannot get any debug messages to show up when the callback is run, even when I place the debug messages at the very beginning of the callback. I suspect that this has to do with how cln-plugin is running the callbacks, if they don't perform exactly perfectly the rpc method will hang forever, I've seen this behavior before but I haven't been able to determine when/why it happens. I suspect it has to do with CLN waiting for all threads to be complete before returning? But I don't have the expertise to determine.

However, I did finally try @LLFourn's fix of tx_builder with the newly merged upstream PR, and that seems to work great! :tada:

[chrisguida]

@junderw are you in the BDK Discord btw? I'm available anytime to take a closer look if you're up for it, I'd super appreciate it :)