Skip to content

Replacing reqwest crate with async_minreq crate #128

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

psg-19
Copy link

@psg-19 psg-19 commented Jun 8, 2025

Addresses #121

Overview

This PR replaces our reqwest-based async client with a lightweight async-minreq fork. All existing unit tests pass, but because async-minreq pulls in Tokio 1.44.0 and Tokio-rustls 0.26.2, the project’s MSRV must be bumped to 1.71.0 in CI.

Changes

  • Dependency swap

  • Async client rewrite

    • Updated async.rs to use the async-minreq API for GET/POST, timeouts, retries, and HTTPS.
  • Tests & MSRV

    • Verified unit tests pass on Rust 1.71.0.
    • CI workflows now declare rust-version = "1.71.0" to match Tokio’s MSRV.

Looking forward to feedback on these changes and any suggestions for improvement!

Binary Size And Dependency tree

Feature Command Used Async Minreq Reqwest
Binary Size Dependency Tree Binary Size Dependency Tree
build (debug) cargo build 6409 KB 298 6514 KB 434
build (release) cargo build -r 1627 KB 298 1616 KB 434
async cargo build --no-default-features --features async 1709 KB 290 1914 KB 396
async-https cargo build --no-default-features --features async-https 2034 KB 295 1915 KB 431
async-https-native cargo build --no-default-features --features async-https-native 1710 KB 314 1915 KB 431
async-https-rustls cargo build --no-default-features --features async-https-rustls 2034 KB 295 1876 KB 423
async-https-rustls-manual-roots cargo build --no-default-features --features async-https-rustls-manual-roots NA NA 1876 KB 420

Command used for calculating dependency tree -> cargo tree --no-default-features --features xyz | wc -l

My Machine specs -> intel i7 12th gen, rtx 3050ti

OS -> windows subsystem for linux

Checklists

All Submissions:

  • I’ve signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

New Features:

  • I’ve added tests for the new feature
  • I’ve added docs for the new feature

Copy link
Contributor

@tnull tnull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a pretty substantial change that also breaks compatibility between rust-esplora-client versions. That said, I really like the idea of eventually getting rid of the huge dependency tree of reqwest.

Given that this PR seems to be based on a draft fork-of-a-fork branch that was edited without any code review, I think it would be very important to move the async-minreq crate to the bitcoindevkit org, have it undergo proper code review, and introduce co-maintainers before we'd want to take on this new dependency.

@notmandatory notmandatory added the summer-of-bitcoin Summer of Bitcoin Project Proposal label Jun 12, 2025
Copy link
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good start and glad to see tests are passing. Per our call here are comments so far.

Cargo.toml Outdated
@@ -10,7 +10,7 @@ documentation = "https://docs.rs/esplora-client/"
description = "Bitcoin Esplora API client library. Supports plaintext, TLS and Onion servers. Blocking or async"
keywords = ["bitcoin", "esplora"]
readme = "README.md"
rust-version = "1.63.0"
rust-version = "1.70.0"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
rust-version = "1.70.0"
rust-version = "1.71.0"

Cargo.toml Outdated
@@ -22,7 +22,8 @@ bitcoin = { version = "0.32", features = ["serde", "std"], default-features = fa
hex = { version = "0.2", package = "hex-conservative" }
log = "^0.4"
minreq = { version = "2.11.0", features = ["json-using-serde"], optional = true }
reqwest = { version = "0.12", features = ["json"], default-features = false, optional = true }
async_minreq = { git = "https://github.com/psg-19/async-minreq.git", default-features = false, optional = true }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Best to put this PR in draft mode until dependency changes like this are upstreamed and released.

Cargo.toml Outdated
@@ -22,7 +22,8 @@ bitcoin = { version = "0.32", features = ["serde", "std"], default-features = fa
hex = { version = "0.2", package = "hex-conservative" }
log = "^0.4"
minreq = { version = "2.11.0", features = ["json-using-serde"], optional = true }
reqwest = { version = "0.12", features = ["json"], default-features = false, optional = true }
async_minreq = { git = "https://github.com/psg-19/async-minreq.git", default-features = false, optional = true }
serde_json = "1.0.100"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be provided when the json-using-serde feature is enabled and not added here.

Cargo.toml Outdated
Comment on lines 48 to 49
async-https-native = ["async"]
async-https-rustls = ["async"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these features needed if they don't activate new features in async_minreq ?
Once you're able to confirm they work they should be tested in the CI matrix also.

Cargo.toml Outdated
async-https = ["async"]
async-https-native = ["async"]
async-https-rustls = ["async"]
async-https-rustls-manual-roots = ["async"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this feature isn't supported by async-minreq, two questions:

  1. can it be added as a new feature there? is it supported by minreq ?
  2. if not what projects will be impacted if it is removed?
  3. see if feature similar to blocking minreq make sense for async_minreq

src/async.rs Outdated
@@ -106,14 +77,17 @@ impl<S: Sleeper> AsyncClient<S> {
let url = format!("{}{}", self.url, path);
let response = self.get_with_retry(&url).await?;

if !response.status().is_success() {
if response.status_code > 299 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally the http status code values would be consts in the async_minreq crate, but if not you should add your own const to improve readability and as a place to document what it means.

src/async.rs Outdated
@@ -284,7 +280,7 @@ impl<S: Sleeper> AsyncClient<S> {
pub async fn get_tx_no_opt(&self, txid: &Txid) -> Result<Transaction, Error> {
match self.get_tx(txid).await {
Ok(Some(tx)) => Ok(tx),
Ok(None) => Err(Error::TransactionNotFound(*txid)),
Ok(None) => Err(Error::TransactionNotFound(*txid)), //look into
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit, can remove comment.

src/async.rs Outdated
/// Sends a GET request to the given `url`, retrying failed attempts
/// for retryable error codes until max retries hit.
async fn get_with_retry(&self, url: &str) -> Result<Response, Error> {
async fn get_with_retry(&self, url: &str) -> Result<async_minreq::Response, Error> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If possible should just be Response and have a use async_minreq::Response above.

@@ -478,8 +474,8 @@ impl<S: Sleeper> AsyncClient<S> {
}
}

fn is_status_retryable(status: reqwest::StatusCode) -> bool {
RETRYABLE_ERROR_CODES.contains(&status.as_u16())
fn is_status_retryable(status: i32) -> bool {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If async_minreq project is open to it would be nice to have a similar StatusCode enum in that project to use here and also instead of your 299 value above.

@notmandatory
Copy link
Member

notmandatory commented Jun 12, 2025

@tnull thanks for taking a look, @psg-19 is my summer of bitcoin mentee. I agree this is a big change but as you pointed out the goal is to reduce the dependency tree. I and @psg-19 will work with @BEULAHEVANJALIN on async_minreq to upstream changes we need and do any other work to get that crate ready for production use.

@psg-19 psg-19 marked this pull request as draft June 12, 2025 16:56
@coveralls
Copy link

coveralls commented Jun 12, 2025

Pull Request Test Coverage Report for Build 16756106556

Details

  • 21 of 26 (80.77%) changed or added relevant lines in 1 file are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage remained the same at 86.919%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/async.rs 21 26 80.77%
Files with Coverage Reduction New Missed Lines %
src/async.rs 1 80.0%
Totals Coverage Status
Change from base Build 16626978737: 0.0%
Covered Lines: 1010
Relevant Lines: 1162

💛 - Coveralls

@psg-19 psg-19 force-pushed the feature/async_minreq_migration branch from c1a5925 to aaf4d9c Compare July 10, 2025 15:20
@psg-19 psg-19 force-pushed the feature/async_minreq_migration branch from aaf4d9c to 008a6f0 Compare July 21, 2025 13:19
@notmandatory
Copy link
Member

notmandatory commented Jul 30, 2025

Another option we should consider for this PR is making async_minreq an optional "experimental" features so projects can help test it out while the async_minreq crate matures. The down-side is this doesn't follow cargo convention about not having conflicting features, will blow-up the number of feature flags and make the code a bit harder to read/reason about. But still worth a look, could rename features like this:

async-minreq = ["async_minreq", "async_minreq/proxy", "tokio?/time"]
async-minreq-https = ["async-minreq", "async_minreq/https"]
async-minreq-https-native = ["async-minreq", "async_minreq/https-native"]
async-minreq-https-rustls = ["async-minreq", "async_minreq/https-rustls"]
async-minreq-https-rustls-manual-roots = ["async-minreq"]

@notmandatory
Copy link
Member

notmandatory commented Aug 1, 2025

The latest commit to feature flag the async-minreq code looks good! I have a few suggestions for finishing this off:

  1. put the current async code blocks before the async-minreq blocks so that if somehow both features are enabled the current reqwest based code will return first, also the diff will be easier to review against the current code.
  2. I think I saw a couple places where the final return statements for the async feature aren't feature gated.
  3. The README should be updated to state the new MSRV is updated to 1.71 and what the new pinned dependencies are, or similar to how it's done in the bdk repo you can put all the pinning in a pin-msrv.sh script and just reference that in both the README and use it in the CI workflows.
  4. Also in the README you should describe the available feature flags and say that the async-minreq one is currently "experimental" but may become the only async client In the future once it's more mature.
  5. Cleanup commit history and follow conventional commits style commit messages (I can show you how to do this with 'git rebase').

@psg-19 psg-19 force-pushed the feature/async_minreq_migration branch 5 times, most recently from bb5f2f5 to 1cba85c Compare August 4, 2025 16:38
@psg-19 psg-19 marked this pull request as ready for review August 4, 2025 16:38
@psg-19 psg-19 force-pushed the feature/async_minreq_migration branch 3 times, most recently from 0d930d7 to 44d6d10 Compare August 5, 2025 16:49
@psg-19 psg-19 force-pushed the feature/async_minreq_migration branch from 44d6d10 to 8b79d1d Compare August 5, 2025 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
summer-of-bitcoin Summer of Bitcoin Project Proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants