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!

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.

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.

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.

Can't we make store_triggered_appointment add to the responder trackers and let the responder handle that tracker on the next block feed?
This has a downside though: we are skipping the opportunity to publish this tx (if it's valid) in the very next block.

Can't we make store_triggered_appointment add to the responder trackers and let the responder handle that tracker on the next block feed?
This has a downside though: we are skipping the opportunity to publish this tx (if it's valid) in the very next block.

Or if we can make handle_breach an aysnc method, we can make it a timeout based logic:
If we don't get a response from the backend before timeout T (say 100ms) we add a tracker to the responder trackers and hand the user the receipt early and let the handle breach method retry on the next connected block.

I don't really think there is a way of doing this, is there? Adding something to the responder means that we need some sort of ConfirmationStatus. The only two options that apply for an ongoing tracker on the Responder are ConfirmedIn(x) and InMempoolSince(x). If the transaction was already sent by someone else, x<=current_block (< if it was confirmed, = if it's on the mempool). However, if we are the ones sending it we'll need a InMempoolSince(now) return.

So in order to make this work we'd either need an additional Pending state that would mean reworking the logic of the Responder to check these pending trackers, or instead of storing this in the Responder, we'd need to temporarily store it on the Watcher's end, and add this temp collection to valid_breaches whenever a new block is seen by the Watcher, so they can be handed to the Responder.