search

BP-WG / bp-wallet

Modern, lightweight & standard-compliant bitcoin wallet runtime & cli without rust-bitcoin dependencies
Apache License 2.0
17 stars 12 forks source link

error updating wallet caused by esplora rate limit #70

Open zoedberg opened 2 months ago

zoedberg commented 2 months ago

As shown in https://github.com/RGB-WG/rgb-integration-tests/pull/7 there's an issue when using the official mainnet esplora URL (https://blockstream.info/api) when calling Wallet::update.

This https://github.com/Blockstream/esplora/issues/449 is a related issue, where it's explained that to dodge the 429 error we shoule leave a gap of around 250ms between each API call. Please also check https://github.com/bitcoindevkit/bdk/issues/1120#issuecomment-2331794726

Not sure we want to fix this here or in https://github.com/BP-WG/bp-esplora-client

dr-orlovsky commented 2 months ago

Hm, I am not sure how we can fix them. What specific fix you have in mind?

zoedberg commented 2 months ago

Hm, I am not sure how we can fix them. What specific fix you have in mind?

IMO the best fix would be to handle this on bp-esplora-client, where every API call (blocking and async) should handle the 429 error by waiting some milliseconds and then retrying to do the call, something similar to what it has been done here https://github.com/Blockstream/lwk/blob/0533774f8d9b3fef2d579ad5b9bdd497ffa74165/lwk_wollet/src/clients/esplora_wasm_client.rs#L511. But this would require refactoring the bp-esplora-client code, since currently every API call has its own error handling code. In this regard, some days ago I've cherry-picked a commit from the original rust-esplora-client (https://github.com/bitcoindevkit/rust-esplora-client/commit/df9c6940114f3986e6b3036a365fe999a723eeb5), thinking it would have helped fixing the issues we were having with rustls. It didn't, so I didn't open a PR, but the commit refactored the blocking API calls making most calls share the same error handling code (check https://github.com/zoedberg/bp-esplora-client/commit/a6d323ccc9d9fdf289ea6e8614996bcc9edaabd9). If you think this is the right direction I could complete this work and introduce also the code needed to handle the 429 error.

An alternative and much faster fix would be to handle the error on bp-wallet during the Wallet::update call. More specifically in the get_scripthash_txs_all method, where we should check if the call to client.inner.scripthash_txs returns a HttpResponse(429) and in that case retry the call after some short time. This would fix only the Wallet::update call though, if in other places we'll be calling the esplora APIs too frequently then we would encounter this Too Many Requests issue again.

dr-orlovsky commented 2 months ago

Refactoring of bp-esplora-client is anyway required, since it is a fork of BDK and is written in a complete shitcode. But I'd like to postpone this refactoring on later since it is not a high priority at all.

So I propose to use fastest route - and when we will be refactoring (rewriting) the whole code later we will address the issue at its root.

dr-orlovsky commented 2 months ago

I am also opened to the idea of re-basing on top of the updated BDK esplora client code - I can work on that if you'd like

zoedberg commented 2 months ago

Refactoring of bp-esplora-client is anyway required, since it is a fork of BDK and is written in a complete shitcode. But I'd like to postpone this refactoring on later since it is not a high priority at all.

So I propose to use fastest route - and when we will be refactoring (rewriting) the whole code later we will address the issue at its root.

I agree refactoring is not a priority, but having RGB working on mainnet will soon be and this 429 error is preventing it. We can try the fastest route but I'm not sure that we'll not encounter other issues later on.

I am also opened to the idea of re-basing on top of the updated BDK esplora client code - I can work on that if you'd like

I'm not sure this would help, they are not handling the 429 error and the main changes between the upstream repo and this one are contained in the commit I've cherry-picked. Anyway we can consider this issue not as urgent as others, so I propose to do nothing for now and then the first of us who finishes their more urgent tasks will start the refactor.