Switch from `reqwest` to `bitreq` HTTP client #56

tnull · 2026-01-20T09:08:41Z

Replace the reqwest dependency with bitreq, co-Authored-By: HAL 9000.

Replace the `reqwest` dependency with `bitreq`, a lighter HTTP(s) client that reduces code bloat and dependency overhead. Key changes: - Use `bitreq::Client` for connection pooling and reuse - Update all HTTP request handling to use bitreq's API - Remove `reqwest::header::HeaderMap` in favor of `HashMap<String, String>` - Simplify `LnurlAuthToJwtProvider::new()` to no longer return a `Result` - Use `serde_json::from_slice()` directly for JSON parsing - Build script uses bitreq's blocking `send()` method Co-Authored-By: HAL 9000 Signed-off-by: Elias Rohrer <dev@tnull.de>

ldk-reviews-bot · 2026-01-20T09:08:44Z

👋 Thanks for assigning @tankyleo as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

tnull · 2026-01-20T09:10:15Z

src/headers/lnurl_auth_jwt.rs

+		let auth_response =
+			auth_request.send_async().await.map_err(VssHeaderProviderError::from)?;
+		let lnurl_auth_response: LnurlAuthResponse =
+			serde_json::from_slice(&auth_response.into_bytes()).map_err(|e| {


We could use bitreq's json-using-serde which goes through UTF-8 parsing first, but this seemed simpler and followed what the reqwest::Response::json method did (cf https://docs.rs/reqwest/latest/src/reqwest/async_impl/response.rs.html#269-273). Not sure if anyone has an opinion here?

either is fine to me 🤷‍♂️

@tnull sounds to me like this is something we can upstream to bitreq no ? ie pass the Vec<u8> body straight into serde_json::from_slice, and skip the utf8 parsing, which seems a net plus.

@tnull sounds to me like this is something we can upstream to bitreq no ? ie pass the Vec<u8> body straight into serde_json::from_slice, and skip the utf8 parsing, which seems a net plus.

Yeah, I considered that too. Just haven't fully made my mind up whether not validating UTF8 is okay. Maybe it's here, but not necessarily in the general case?

IIUC i would expect serde_json::from_slice to validate UTF-8 as necessary during deserialization...

tnull

Honestly not sure why the idna crate suddenly started failing to build under 1.75.0, and we aim to soon replace the whole url dependency with our own Url type, but for now we can also just fix the issue by bumping MSRV to 1.85, since LDK Node already requires that anyways and we aim too soon also update LDK to that.

TheBlueMatt · 2026-01-20T12:10:33Z

and we aim too soon also update LDK to that.

Wait, what? This is news to me. Can we just remove the cursed url crate?

src/client.rs

TheBlueMatt · 2026-01-20T12:15:35Z

src/headers/lnurl_auth_jwt.rs

+		let auth_response =
+			auth_request.send_async().await.map_err(VssHeaderProviderError::from)?;
+		let lnurl_auth_response: LnurlAuthResponse =
+			serde_json::from_slice(&auth_response.into_bytes()).map_err(|e| {


either is fine to me 🤷‍♂️

tnull · 2026-01-20T12:21:46Z

Wait, what? This is news to me.

No, Matt, we discussed this plan for hours at the Rust Summit. We landed on that most ecosystem crates (BDK, etc) would update ~right away given their release cycles, but that LDK would defer bumping until after Ubuntu 2604 is out, which will of course happen this April.

Can we just remove the cursed url crate?

Yeah, working on it as we speak. Just did a minurl release (cf. https://docs.rs/minurl/latest/minurl/), but should also have a PR for upstreaming it to bitreq up by the end of today.

TheBlueMatt · 2026-01-20T12:27:36Z

that LDK would defer bumping until after Ubuntu 2604 is out, which will of course happen this April.

Right, April is a ways off though and we likely don't want to bump the day that a new release comes out (I'm kinda tired of us rushing to bump the day something comes out for the sake of it without a specific motivating rust feature, at least now that we've stripped out the stupid HTTP deps that keep causing dep-MSRV bumps), and I thought we wanted to upstream long before then lightningdevkit/rust-lightning#4323

tnull · 2026-01-20T12:31:16Z

src/client.rs

 const APPLICATION_OCTET_STREAM: &str = "application/octet-stream";
-const DEFAULT_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(10);
+const DEFAULT_TIMEOUT_SECS: u64 = 10;
+const MAX_RESPONSE_BODY_SIZE: usize = 500 * 1024 * 1024; // 500 MiB


Now bumped this considerably, as of course 10MB would be much too small. Let me know if you have an opinion on a better value here @TheBlueMatt @tankyleo

Mmm, yea, good question. On the server side we defaulted to 1G because its postgres' limit. Not that "just because its postgres' limit" is a good reason to do anything, though. 500 seems fine enough to me, honestly, though. As long as its documented I don't feel super strongly.

As 10 MB is def to small

Signed-off-by: Elias Rohrer <dev@tnull.de>

tankyleo · 2026-01-21T02:04:15Z

src/client.rs

+			.with_body(request_body)
+			.with_timeout(DEFAULT_TIMEOUT_SECS)
+			.with_max_body_size(Some(MAX_RESPONSE_BODY_SIZE))
+			.with_pipelining();


Hmm bitreq says "Note that because pipelined requests may be replayed in case of failure, you should only set this on idempotent requests", and our PutObjectRequest is not idempotent.

and our PutObjectRequest is not idempotent

Why is it not idempotent? If the request fails, we also assume the HTTP client to retry? This is essentially the same, no?

hmm right i was thinking if we replay the same versioned put object request, the server will return an error instead of ok, and hence that call is not idempotent.

the concern here is request A is actually successful, version increments server side, but for some reason bitreq considers it a failure, replays the request, and becomes request B.

request B fails because of a version mismatch, and kicks off the retry policy client side, and eventually results in a failure getting bubbled back up to LDK-Node even though request A succeeded.

tankyleo · 2026-01-21T02:06:54Z

src/headers/lnurl_auth_jwt.rs

 	parent_key: Xpriv,
 	url: String,
 	default_headers: HashMap<String, String>,
-	client: reqwest::Client,


Are we sure we want to delete the cached client here ? Thought we could cache it since the first get request in the lnurl auth flow is always to the same URL.

AFAIU, we we'll only retrieve a new JWT token if the old one expired, which should be in the order of days or at least ours. It seems to me that keeping a client around (and potentially even keeping a connection open) is a bunch of unnecessary overhead if we just make single requests that rarely.

sounds good to me thank you

tankyleo · 2026-01-21T02:12:15Z

src/headers/lnurl_auth_jwt.rs

-			.map_err(VssHeaderProviderError::from)?;
+		let lnurl_request = bitreq::get(&self.url)
+			.with_headers(self.default_headers.clone())
+			.with_timeout(DEFAULT_TIMEOUT_SECS);


I know this is a trusted server, but do we want to set max body size here to prevent any possible OOM crashes ? same for the get request below.

True, I overlooked that. Do you have a suggestion for a good limit here?

How is 16KiB ? just from intuition after some brief googling, we are fetching a URL here.

tankyleo · 2026-01-21T02:14:02Z

src/client.rs

-		if status.is_success() {
+
+		let http_request = bitreq::post(url)
+			.with_header("content-type", APPLICATION_OCTET_STREAM)


nit: like you've done elsewhere, let's put this hard coded string in a constant ? Here and also in the lnurl auth module.

tankyleo · 2026-01-21T02:19:19Z

src/error.rs

 impl VssError {
 	/// Create new instance of `VssError`
-	pub fn new(status: StatusCode, payload: Bytes) -> VssError {
+	pub fn new(status_code: i32, payload: Vec<u8>) -> VssError {


nit: we don't need ownership and can pass a &[u8] here.

Hmm, but if we don't end up cloneing at the callsite, isn't move semantics even preferable to passing a reference?

Hmm I have always had the habit of not asking for ownership if I don't need it. I wouldn't want someone to clone later down the line to accommodate the Vec<u8> here, forgetting to look inside here and realize cloning was not necessary.

isn't move semantics even preferable to passing a reference?

Do you mean that once we receive an error status code, the payload should be owned by VssError, and any subsequent interactions with that payload should happen through the VssError API ?

tankyleo · 2026-01-21T02:27:13Z

src/headers/lnurl_auth_jwt.rs

+		let auth_response =
+			auth_request.send_async().await.map_err(VssHeaderProviderError::from)?;
+		let lnurl_auth_response: LnurlAuthResponse =
+			serde_json::from_slice(&auth_response.into_bytes()).map_err(|e| {


@tnull sounds to me like this is something we can upstream to bitreq no ? ie pass the Vec<u8> body straight into serde_json::from_slice, and skip the utf8 parsing, which seems a net plus.

tankyleo · 2026-01-22T07:11:13Z

src/headers/lnurl_auth_jwt.rs

-			.map_err(VssHeaderProviderError::from)?;
+		let auth_request = bitreq::get(&signed_lnurl)
+			.with_headers(self.default_headers.clone())
+			.with_timeout(DEFAULT_TIMEOUT_SECS);


I'd put a 16KiB limit here too, this should be a short response.

tnull requested review from TheBlueMatt and tankyleo January 20, 2026 09:08

tnull commented Jan 20, 2026

View reviewed changes

tnull mentioned this pull request Jan 20, 2026

Upstream VssStoreBuilder and VssStore to lightning-persister lightningdevkit/rust-lightning#4323

Draft

TheBlueMatt reviewed Jan 20, 2026

View reviewed changes

tnull commented Jan 20, 2026

View reviewed changes

tnull added 4 commits January 20, 2026 13:31

f Bump MAX_RESPONSE_BODY_SIZE

eed3592

As 10 MB is def to small

f Enable pipelining

a366503

Update proto file link to the current location

c9d7764

Bump MSRV to 1.85.0.

ef17d83

Signed-off-by: Elias Rohrer <dev@tnull.de>

tnull force-pushed the 2026-01-switch-to-bitreq branch from 15114d4 to ef17d83 Compare January 20, 2026 12:31

tankyleo reviewed Jan 21, 2026

View reviewed changes

Drop unused force_refresh flag in JWT auth

ef39744

tnull added this to Weekly Goals Jan 21, 2026

tnull moved this to Goal: Merge in Weekly Goals Jan 21, 2026

tankyleo self-requested a review January 21, 2026 19:52

tankyleo reviewed Jan 22, 2026

View reviewed changes

Switch from reqwest to bitreq HTTP client #56

Are you sure you want to change the base?

Switch from reqwest to bitreq HTTP client #56

Conversation

tnull commented Jan 20, 2026

Uh oh!

ldk-reviews-bot commented Jan 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull Jan 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Jan 20, 2026

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull commented Jan 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheBlueMatt commented Jan 20, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tankyleo Jan 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull Jan 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tankyleo Jan 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tankyleo Jan 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Switch from `reqwest` to `bitreq` HTTP client #56

Switch from `reqwest` to `bitreq` HTTP client #56

ldk-reviews-bot commented Jan 20, 2026 •

edited

Loading

tnull Jan 21, 2026 •

edited

Loading

tnull commented Jan 20, 2026 •

edited

Loading

tankyleo Jan 22, 2026 •

edited

Loading

tnull Jan 21, 2026 •

edited

Loading

tankyleo Jan 22, 2026 •

edited

Loading

tankyleo Jan 22, 2026 •

edited

Loading