Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
Tracked by #70
chrisguida opened this issue Jul 25, 2023 · 33 comments
Closed
Tracked by #70

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

chrisguida opened this issue Jul 25, 2023 · 33 comments
Labels
new feature New feature or request

Comments

@chrisguida
Copy link

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.

@chrisguida chrisguida added the new feature New feature or request label Jul 25, 2023
@notmandatory
Copy link
Member

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

@chrisguida
Copy link
Author

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
Copy link
Contributor

LLFourn commented Jul 27, 2023

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
Copy link
Author

chrisguida commented Jul 27, 2023

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
Copy link
Author

chrisguida commented Jul 27, 2023

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
Copy link

junderw commented Jul 31, 2023

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
Copy link

junderw commented Jul 31, 2023

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
Copy link
Author

@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
Copy link
Contributor

LLFourn commented Aug 1, 2023

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
Copy link
Author

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
Copy link
Author

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
Copy link

junderw commented Aug 3, 2023

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

@junderw
Copy link

junderw commented Aug 3, 2023

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

@junderw
Copy link

junderw commented Aug 3, 2023

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
Copy link

junderw commented Aug 3, 2023

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
Copy link

junderw commented Aug 3, 2023

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
Copy link
Contributor

LLFourn commented Aug 3, 2023

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
Copy link

junderw commented Aug 3, 2023

(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 added a commit to junderw/lightning that referenced this issue Aug 3, 2023
…library.

See: bitcoindevkit/bdk#1047 (comment)

In general, futures produced by most libraries in the ecosystem of Rust, and bounds placed
on users of famous runtimes like tokio and its spawn method all lack Sync requirements.

Because of this, anyone who creates a callback using any sort of library that returns a
non-Sync future (which most libraries fit this description) inside of it will get some
cryptic error messages (async error messages still leave a lot to be desired).

Removing these Sync requirements will make the library more useful.
@junderw
Copy link

junderw commented Aug 3, 2023

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
Copy link
Contributor

LLFourn commented Aug 3, 2023

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.

image

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

@junderw
Copy link

junderw commented Aug 3, 2023

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
Copy link

junderw commented Aug 3, 2023

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
Copy link

junderw commented Aug 3, 2023

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
Copy link
Contributor

LLFourn commented Aug 3, 2023

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.

@LLFourn LLFourn closed this as completed Aug 3, 2023
@chrisguida
Copy link
Author

chrisguida commented Aug 3, 2023

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
Copy link
Author

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
Copy link

junderw commented Aug 3, 2023

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
Copy link

junderw commented Aug 3, 2023

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
Copy link

junderw commented Aug 3, 2023

(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
Copy link
Author

chrisguida commented Aug 7, 2023

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
Copy link

junderw commented Aug 7, 2023

Can you verify exactly where it's hanging?

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

rustyrussell pushed a commit to ElementsProject/lightning that referenced this issue Aug 8, 2023
…library.

See: bitcoindevkit/bdk#1047 (comment)

In general, futures produced by most libraries in the ecosystem of Rust, and bounds placed
on users of famous runtimes like tokio and its spawn method all lack Sync requirements.

Because of this, anyone who creates a callback using any sort of library that returns a
non-Sync future (which most libraries fit this description) inside of it will get some
cryptic error messages (async error messages still leave a lot to be desired).

Removing these Sync requirements will make the library more useful.
@chrisguida
Copy link
Author

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! 🎉

@chrisguida
Copy link
Author

@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 :)

litch pushed a commit to litch/lightning that referenced this issue Aug 11, 2023
…library.

See: bitcoindevkit/bdk#1047 (comment)

In general, futures produced by most libraries in the ecosystem of Rust, and bounds placed
on users of famous runtimes like tokio and its spawn method all lack Sync requirements.

Because of this, anyone who creates a callback using any sort of library that returns a
non-Sync future (which most libraries fit this description) inside of it will get some
cryptic error messages (async error messages still leave a lot to be desired).

Removing these Sync requirements will make the library more useful.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants