Skip to content

fix(bitcoin-da): return MempoolSpaceRequestError on HTTP failure in get_fee_rate_from_mempool_space - #3226

Open
amathxbt wants to merge 3 commits into
chainwayxyz:nightlyfrom
amathxbt:fix/mempool-space-request-error-variant
Open

fix(bitcoin-da): return MempoolSpaceRequestError on HTTP failure in get_fee_rate_from_mempool_space#3226
amathxbt wants to merge 3 commits into
chainwayxyz:nightlyfrom
amathxbt:fix/mempool-space-request-error-variant

Conversation

@amathxbt

Copy link
Copy Markdown

Summary

get_fee_rate_from_mempool_space maps a network-level reqwest::Error (timeout, connection refused, DNS failure, etc.) to FeeServiceError::MempoolSpaceParseError instead of FeeServiceError::MempoolSpaceRequestError.

Bug

let response = get_with_timeout(url.clone(), MEMPOOL_SPACE_TIMEOUT)
    .await
    .map_err(|e| {
        trace!("Failed to fetch from {}: {:?}", url, e);
        FeeServiceError::MempoolSpaceParseError   // ← wrong variant
    })?;

MempoolSpaceParseError is intended for JSON deserialization failures. Using it here:

  1. Misleads callers and log readers who see a "parse error" for what is really a connectivity issue.
  2. Wastes the existing #[from] reqwest::Error impl on MempoolSpaceRequestError, which was added precisely for this case.

Fix

Use the correct variant so the two failure modes are distinguishable:

.map_err(|e| {
    trace!("Failed to fetch from {}: {:?}", url, e);
    FeeServiceError::MempoolSpaceRequestError(e)  // ← correct
})?;

Files changed

  • crates/bitcoin-da/src/fee.rs

…t failure

get_fee_rate_from_mempool_space was mapping reqwest::Error (a network-level
failure such as a timeout or connection refused) to MempoolSpaceParseError
instead of MempoolSpaceRequestError. This made it impossible for callers to
distinguish between a failed HTTP request and a JSON parse error, and wasted
the existing From<reqwest::Error> impl on MempoolSpaceRequestError.

Fix: use MempoolSpaceRequestError(e) in the map_err on get_with_timeout.
@amathxbt
amathxbt requested a review from a team as a code owner April 30, 2026 09:35
@auto-assign
auto-assign Bot requested a review from jfldde April 30, 2026 09:35
jfldde
jfldde previously approved these changes Apr 30, 2026
@amathxbt
amathxbt requested a review from jfldde April 30, 2026 09:59
@amathxbt

amathxbt commented May 6, 2026

Copy link
Copy Markdown
Author

@jfldde @eyusufatik — this PR is approved and ready. No lint issues found. Pinging for merge.

@codecov

codecov Bot commented Jul 20, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 84.0%. Comparing base (86ac5a3) to head (f9625a0).

Files with missing lines Patch % Lines
crates/bitcoin-da/src/fee.rs 0.0% 1 Missing ⚠️
Additional details and impacted files
Files with missing lines Coverage Δ
crates/bitcoin-da/src/fee.rs 91.5% <0.0%> (ø)

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants