Fix: Remove Sync requirements on Futures returned in the Rust plugin library.

[junderw]

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.

[vincenzopalazzo]

Mh, I need to think more hard about this, but at least this should not be a breaking change because we are relaxing the requirements, and also looks like our internal plugin is not doing something that required Sync, but I need to look more into it.

In addition, the current API will have still problems due the Send requirements, see rust-lang/rust#102211

[junderw]

internal plugin is not doing something that required Sync

1. Plugin doesn't require Sync on its S (even before this PR)
2. AsyncCallback and AsyncNotificationCallback only use S for the argument (Plugin) of the callback, which has nothing to do with the anonymous closure-struct contained in the HashMaps (since they only contain items that were captured)
3. The AsyncCallback and AsyncNotificationCallback already have Send + Sync bounds on the closures themselves, and it doesn't interfere with the Future removing Sync.

So theoretically, there should be no possible way this causes a problem.

will have still problems due the Send requirements

This is more of a language problem. I have nothing but respect for the async-wg and their work on getting things to be more compatible across the ecosystem, but the problems caused by the superfluous Sync bound are much greater than the small edge case that exists for every async library.

[vincenzopalazzo]

Yeah, I see your point! To me, this change sounds good also because it is just relaxing the requirements, but I am not sure why this bound was specified, so better wait more expert people on this

[cdecker]

Thanks @junderw, to be honest when I wrote the library I was (and probably still am) a Rust novice, so I resorted to fiddling with the constraints until they worked. If we can relax some of the constraints that's perfect.

I appreciate the correction very much 👍

ACK d163230

[junderw]

Would you like me to bump the Cargo.toml version to 0.1.5? That way you can just cargo publish after merging.

[cdecker]

I have a couple more changes but will bump as soon as possible 👍

[rustyrussell]

Seems like this should go into the current release, @cdecker ? If so, please merge! (Otherwise please fix milestone!)