fix(wallet_esplora): Gracefully handle 429s errors

[realeinherjar]

Closes bitcoindevkit/bdk#1120

Details

• BlockingClient:
  • Add an extra check inside scripthash_txs for 429 Status codes.
    If detected, hard-coded sleep for 500ms using std::thread::sleep (so it is also thread-safe).
• AsyncClient (little more convoluted than BlockingClient):

  • I had to bring tokio and async-recursion into the dependencies.

  • Inside the scripthash_txs we are checking for 429 Status codes.
    If detected, hard-coded sleep for 500ms using tokio::time::sleep.
    Since this is an "async recursion" the macro async_recursion handles that gracefully without having to deal with:

    # https://rust-lang.github.io/async-book/07_workarounds/04_recursion.html
    use futures::future::{BoxFuture, FutureExt};
    
    fn recursive() -> BoxFuture<'static, ()> {
        async move {
            recursive().await;
            recursive().await;
        }.boxed()
    }

    If we annotate the offending recursive function with #[async_recursion],
    it automatically convert an async function to one returning a boxed Future.

[evanlinjin]

Aproach NACK

Thank you for taking a shot at this.

Why 500ms? What if the server expects us to wait for 600ms? I would prefer if we checked the Retry-After header. If that does not exist, then have an exponential back-off delay as a fallback.

Do we need the async recursion? Is there a way to do it with a loop?

I also think introducing tokio as an async dependency is a big ask. Would it be trivial to implement a simple Future using Instant?

[realeinherjar]

Thanks for the feedback. It is helpful. I will try to modify the approach. 👍🏻

[LLFourn]

Aproach NACK

Thank you for taking a shot at this.

Why 500ms? What if the server expects us to wait for 600ms? I would prefer if we checked the Retry-After header. If that does not exist, then have an exponential back-off delay as a fallback.

Do we need the async recursion? Is there a way to do it with a loop?

I also think introducing tokio as an async dependency is a big ask. Would it be trivial to implement a simple Future using Instant?

Not really trivial and using std::time::Instant breaks WASM probably. We'd need to do some work to have the timer work on WASM: See: https://github.com/whizsid/wasmtimer-rs

I would avoid sleeping and just lower the number of batch requests in the next round by the number of 429s you get.

PS are we testing this crate on WASM? If not PR fixing that before this one would be good.

[benthecarman]

this will cause a panic in wasm, might need to pass in a function that sleeps. for wasm sleep needs to be async too

[realeinherjar]

I will try another approach in the bdk_esplora crate and not here.

EDIT:
More specifically, 429s could be handled inside this loop that calls scripthash_txs:

https://github.com/bitcoindevkit/bdk/blob/3569acca0b3f3540e1f1a2278794eac4642a05e4/crates/esplora/src/blocking_ext.rs#L213-L239

https://github.com/bitcoindevkit/bdk/blob/3569acca0b3f3540e1f1a2278794eac4642a05e4/crates/esplora/src/async_ext.rs#L234-L242