Watcher::store_triggered_appointment should be called as an async task

[sr-gi]

The current codebase has an edge case where, if bitcoind is unreachable while an already triggered appointment is added, and store_triggered_appointment is called as result, the request will hang and end up timing out but be accepted. However, the user will not be handed the receipt.

This could be fixed making store_triggered_appointment be called as an async task, so the receipt can be handed straightaway. This is surprisingly not as straightforward as it sounds, and implies dealing with lifetimes in a way I don't fully understand (at least with my current knowledge of Rust 😅).

[carterian8]

Hey @sr-gi, I have a fix which turns store_triggered_appointment into an async function like you suggested but I am running into a design issue that I'd like your input on. As you know, store_triggered_appointment ends up calling the carrier's hang_until_bitcoind_reachable which blocks the current thread until bitcoind_reachable is true. This is problematic if store_triggered_appointment is an async function because the std::sync::Condvar used to notify the carrier can only be used between threads, not asynchronous calls. This problem is solvable but would require changing the carrier's bitcoind_reachable type to something thats async compatible and including either async_std or using tokio::sync::Notify to signal the carrier when bitcoind is reachable.

So from here I see two immediate options:

1. Continue down the async path
2. Spawn a thread from within add_appointment which calls store_triggered_appointment. This option might raise some questions pertaining to thread resource management to limit thread creation overhead (think thread pools).

I have some other thoughts rolling around but wanted to start a discussion about how to move forward. Certainly open to criticism and other alternatives!

[sr-gi]

Hey @carterian8, happy to see someone is up to help with this.

My approach when I tried to solve this was going the async route, but I couldn't find a proper way to do so why my knowledge of Rust async. I don't think it'll be a big deal having to replace std::sync::Condvar for something that works with async as long as the functionality is preserved (I'd lean towards using something from tokio to prevent having to add an additional dependency).

Have in mind that bitcoind_reachable is used by multiple components of the tower, so it may require a bigger change that just update the Carrier. I'm curious what's the issue with async and Condvar. Mainly asking since currently it is being used in some async methods (like here or here), so that may be wrong.

[carterian8]

Hey @sr-gi, happy to help!

First regarding std::sync::Condvar, I'm basing my conclusions off of tests I've written within the watcher.rs test module and this open tokio issue. What I've found is that the std::sync::Condvar for bitcoind_reachable does not get notified after calling notify_all() or notify_one() from the tests. Just want to open up my testing/thought process to others to ensure I've not made any errors myself.

Assuming std::sync::Condvar is not compatible with tokio, and since tokio does not implement a Condvar of its own, I think tokio::sync::Notify could be a suitable replacement. The idea being any component finding bitcoind_reachable = false will wait to be notified of a change in state. I'm in the process of implementing this but have found async bubbling up to things like the responder's chain::Listen trait. This is problematic because async functions are not allowed in traits. My point is, there could be some non-trivial restructuring to turn the Condvar.wait()s into something async. I'm certainly up for taking it on but wanted to make you aware and continue the discussion.

In the meantime I'll charge forward and continue to brainstorm cleaner solutions.

[sr-gi]

Replacing std::sync::Condvar for something else that works with async is certainly an option. As long as we can achieve the same functionality (and the design does not become too intricate), I'm happy with it.