should callbacks accepting functions be async?

[nothingmuch]

although the payjoin code itself does no IO, in case callbacks need to do that, currently we'd need some kind of blocking thread to accomodate that.

in case the callbacks do need to do IO (which in general they would), does it make more sense to default the methods that do call them to be async? this is more general, since async code can call non async code, and since our code doesn't await or spawn tasks or anything like that this should work with any runtime, but does introduce some friction if using the code in a non async environment

the thing that makes the most sense to me is to just keep ignoring this for now, and add async variants in the future when/if they are needed, but maybe there is a clever way to make them async with no downsides to blocking code, in which case we might want to do this for 0.24 to reduce long term API churn

[DanGould]

0.24 is set for release on monday

[arminsabouri]

Forcing async can also result in some developer frustration. I've worked with some codebases where large parts of the code are meant to only do sync things and having to support an async call means updating all the downstream callers or spawn a blocking thread.

Can we solve this by having a trait bound object passed in. Similar to what we did for session persistance.

pub trait ApplicationManager {
   pub fn TestMempoolAccept(bitcoin::Transaction) -> bool
   pub fn isUtxoOwned(...) -> bool
   ...
}

[async-trait]
pub trait AsyncApplicationManager {
   pub async fn TestMempoolAccept(bitcoin::Transaction) -> bool
   pub async fn isUtxoOwned(...) -> bool
   ...
}

I think this would mean you need a async and sync version of the state transition methods. Which you could also do with the closures but its a bit less boilerplate.

[nothingmuch]

I'm specifically asking about the state transition methods, FWIW

Related: https://blog.rust-lang.org/inside-rust/2022/07/27/keyword-generics/

[arminsabouri]

Instead of create async callbacks we can create new state transition methods that accepts the output of a the existing call back. For example, for inputs not owned we have a state transition method these_inputs_are_mine(Vec<Outpoint>). And the work that is required to get those outpoints can be done in an async runtime. If vec is empty we progress to the next typestate else we record a session event of the inputs owned and fatally close.

Edit: This shouldn't break semver and thats why its in 1.1

Edit 2: TODO: we should go through each typestate and see if we can apply this approach.

[spacebear21]

In addition to callbacks, there is also the Persister trait which exposes synchronous methods for save_event, load and close. These methods are generally expected to do IO, and there are many situations where persistence can only be done async (e.g. microservice architecture, separate hardware like in lexe.app...). This concern was brought up by a dev from Bull Bitcoin Mobile:

I worry more about the persister, the db operations are async and that's a package (drift) we don't control. we could [...] let the persister write to a stream or in memory and have another function pick it up that can do the actual persisting to db

Rather than force downstream hacks and workarounds I think we'll need to make some parts of the API async. Some languages like Dart are single-threaded and don't support blocking in place like we do in Rust, so at the very least we need to expose async wrappers at the payjoin-ffi layer. I'm exploring this approach currently here.

[nothingmuch]

The app developer can choose how to implement their persister, but I do recall there being a vague agreement that we should ship both an async and a blocking persister trait and wallet devs would be free to choose to implement and call whichever (since they call it it's up to them [edit: to clarify, we would need the existing save(persister) and a new save_async(async_persister) on the transition results, but the internal unwrapping logic would be shared between these]).

I think this means we can remove all usage of blocking in place by using the appropriate trait? The fact that we don't yet ship an async persister is again because it was reasoned to not break semver compatibility, so it was fine to ship 1.0 before adding that

[arminsabouri]

The app developer can choose how to implement their persister, but I do recall there being a vague agreement that we should ship both an async and a blocking persister trait and wallet devs would be free to choose to implement and call whichever (since they call it it's up to them [edit: to clarify, we would need the existing save(persister) and a new save_async(async_persister) on the transition results, but the internal unwrapping logic would be shared between these]).

I think this means we can remove all usage of blocking in place by using the appropriate trait? The fact that we don't yet ship an async persister is again because it was reasoned to not break semver compatibility, so it was fine to ship 1.0 before adding that

Ack. Shipping async methods along side the sync methods makes sense to me

[spacebear21]

That makes sense to me too. The reason it came up now is we wanted to validate 1.0rc in a real-world implementation before shipping 1.0 and I was working on upgrading Bull Bitcoin Mobile, but got blocked due to the lack of async support. So this is softly blocking 1.0 actually getting released per our criterion. I was thinking the payjoin-ffi async wrappers could be a band-aid to move past this and release 1.0 without native async support.

[nothingmuch]

That makes sense to me too. The reason it came up now is we wanted to validate 1.0rc in a real-world implementation before shipping 1.0 and I was working on upgrading Bull Bitcoin Mobile, but got blocked due to the lack of async support. So this is softly blocking 1.0 actually getting released per our criterion. I was thinking the payjoin-ffi async wrappers could be a band-aid to move past this and release 1.0 without native async support.

i suspect the workarounds are going to be harder to write and debug than just introducing the new async method and new async persister, i think the main headache of the latter is going to be refactoring the internals trait into a bunch of private functions and two internals traits? not sure what the easiest way to do that but since it's all private it can be changed