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...

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

I'm not quite sure? The Deserializer::deserialize_bytes docs state:

The behavior of serde_json is specified to fail on non-UTF-8 strings when deserializing into Rust UTF-8 string types such as String, and succeed with the bytes representing the WTF-8 encoding of code points when deserializing using this method.

and then go ahead and use from_slice in the example further below. Might need to run a test to validate either way.

Right so it seems to me that flow is used when the destination is some &[u8] ?

When the destination is some &str, we call fn deserialize_str, which calls Read::parse_str, which for SliceRead (the pub fn from_slice deserializer), calls SliceRead::parse_str_bytes with the as_str function as the result parameter, and this as_str calls str::from_utf8 on the full byte slice.

Right so it seems to me that flow is used when the destination is some &[u8] ?

When the destination is some &str, we call fn deserialize_str, which calls Read::parse_str, which for SliceRead (the pub fn from_slice deserializer), calls SliceRead::parse_str_bytes with the as_str function as the result parameter, and this as_str calls str::from_utf8 on the full byte slice.

Ah, that sounds about right. Mind making the change and opening a PR towards bitreq (ofc. also providing the same rationale)?

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.

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.

Well, a) this shouldn't happen, it should only replay if it actually failed to deliver it and b) why would it result in a version mismatch? The version field wouldn't magically change when replayed, no?

a) this shouldn't happen, it should only replay if it actually failed to deliver it

Then why the warning that pipelined requests should be idempotent ? Sounds to me it is warning that the server could receive two copies of the same request, hence each request should be idempotent. Do we need a documentation update on bitreq's side ?

why would it result in a version mismatch? The version field wouldn't magically change when replayed, no?

The version field in the request does not change, but the server increments the version after request A succeeds. Request B did not increment the version, so that request will fail on "version mismatch"

Then why the warning that pipelined requests should be idempotent ? Sounds to me it is warning that the server could receive two copies of the same request, hence each request should be idempotent. Do we need a documentation update on bitreq's side ?

Indeed, there's some risk that the server sends a Connection: close after it has processed as many requests as it wants and suddenly we have to resend a request. The server shouldn't have processed the first request, but we can't guarantee that so IMO we should avoid setting pipelining on non-idempotent requests (sorry, I know I requested it!).

Is it easy to set it on only GET/list request?

First impression, I would add a with_pipelining: bool in the async fn post_request parameters that sets whether this request should be pipelined.

src/headers/lnurl_auth_jwt.rs

src/client.rs

src/error.rs

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.

src/headers/lnurl_auth_jwt.rs

tankyleo · 2026-01-27T00:27:24Z

@tnull let's finish this so we can ship 0.4.2 ? This release will unblock the sig-auth authorizer PR in ldk-node as far as I see

tnull · 2026-01-27T07:23:08Z

@tnull let's finish this so we can ship 0.4.2 ? This release will unblock the sig-auth authorizer PR in ldk-node as far as I see

Well, I think we want to wait for the next bitreq release anyways which should happen any day now, hopefully. I intended to bump the dependency in this PR still, but if you insist I can do that in a follow up. In any case we shouldn't ship 0.4.2 depending on the old version if we're about to ship the new one, IMO.

tnull · 2026-01-27T08:48:32Z

Addressed comments so far, let me know if I can squash.

TheBlueMatt · 2026-01-27T12:00:03Z

Well, I think we want to wait for the next bitreq release anyways which should happen any day now, hopefully. I intended to bump the dependency in this PR still, but if you insist I can do that in a follow up. In any case we shouldn't ship 0.4.2 depending on the old version if we're about to ship the new one, IMO.

I don't think we need to bump the bitreq minor version - the URL changes I believe are all internal/new APIs so we should be able to do it in an 0.3.2. Thus ISTM we don't need to hold this (unless it wants to use the URL stuff directly, I haven't looked)

tnull · 2026-01-27T12:15:03Z

Well, I think we want to wait for the next bitreq release anyways which should happen any day now, hopefully. I intended to bump the dependency in this PR still, but if you insist I can do that in a follow up. In any case we shouldn't ship 0.4.2 depending on the old version if we're about to ship the new one, IMO.

I don't think we need to bump the bitreq minor version - the URL changes I believe are all internal/new APIs so we should be able to do it in an 0.3.2. Thus ISTM we don't need to hold this (unless it wants to use the URL stuff directly, I haven't looked)

As mentioned above, yes, we don't need to hold this specific PR but we'll likely want to wait with the release to include the version bump, given that we might not immediately want to do another release in 2 days and it would be preferable to not immediately starting out with 0.3 and 0.4 in the dependency tree if possible.

TheBlueMatt · 2026-01-27T12:16:28Z

I'm confused, I don't see a reason why bitreq would release an 0.4 in the near future. Why would we need a release again in a few days?

tnull · 2026-01-27T12:24:28Z

I'm confused, I don't see a reason why bitreq would release an 0.4 in the near future. Why would we need a release again in a few days?

Ah, sorry, rightfully so. I misread, and on top thought that we made other changes since 0.3.1. Indeed it seems we could get away with adding Url in patch release. So we should be good merging this and cutting a release, assuming we don't want to start making use of Url for the VSS URL here.

tankyleo · 2026-01-27T17:51:26Z

Addressed comments so far, let me know if I can squash.

@tnull You might have addressed them but not pushed them ? In any case feel free to push the squashed result :)

tankyleo · 2026-01-27T17:53:13Z

There is the pipelining thread above we still need to resolve

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

src/headers/lnurl_auth_jwt.rs Show resolved Hide resolved

tnull self-assigned this Jan 22, 2026

tnull requested a review from tankyleo January 27, 2026 08:48

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!

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 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!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tankyleo commented Jan 27, 2026

Uh oh!

tnull commented Jan 27, 2026

Uh oh!

tnull commented Jan 27, 2026

Uh oh!

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

Uh oh!

Uh oh!

tnull commented Jan 27, 2026

Uh oh!

TheBlueMatt commented Jan 27, 2026

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

TheBlueMatt commented Jan 27, 2026 •

edited

Loading