Uh oh!
There was an error while loading. Please reload this page.
lightning-block-sync: switch to bitreq, drop chunked_transfer - #4350
Conversation
👋 Thanks for assigning @TheBlueMatt as a reviewer! |
Apostlex0
commented
Jan 27, 2026
@TheBlueMatt the ci fails due to the problem in payment/bolt11.rs in ldk-node, which is unrelated to this task, should i open a pr to fix this in ldk-node as i have made the changes locally while testing. |
tnull
left a comment
There was a problem hiding this comment.
Thanks for taking a look, had a quick first look, here are some initial remarks.
Uh oh!
There was an error while loading. Please reload this page.
| rpc-client = [ "serde_json", "chunked_transfer" ] | ||
| rest-client = [ "serde_json", "bitreq" ] | ||
| rpc-client = [ "serde_json", "bitreq" ] | ||
| tokio = [ "dep:tokio", "bitreq/async" ] |
There was a problem hiding this comment.
I'm confused: why does tokio depend on bitreq?
There was a problem hiding this comment.
changing this to bitreq?/async this is because the rest and rpc client enables the sync mode of bitreq by default so this would enable the async mode of bitreq which uses send_async_with_client() if bitreq exists.
| /// Server for handling HTTP client requests with a stock response. | ||
| pub struct HttpServer { | ||
| address: std::net::SocketAddr, | ||
| #[allow(dead_code)] |
There was a problem hiding this comment.
Why do we need to allow dead code here?
There was a problem hiding this comment.
it was causing a warning so i kept that before, though now I've Implemented Drop for HttpServer that will use all the fields.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@## main #4350 +/- ##
==========================================
- Coverage 85.90% 85.86% -0.05%
==========================================
Files 156 156 Lines 103965 103731 -234 Branches 103965 103731 -234 ==========================================
- Hits 89316 89071 -245 - Misses 12128 12160 +32 + Partials 2521 2500 -21
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @@ -572,27 +334,54 @@ mod endpoint_tests { | |||
| #[cfg(test)] | |||
| pub(crate) mod client_tests { | |||
There was a problem hiding this comment.
I'm not convinced that any of the remaining tests in this file test anything useful now.
There was a problem hiding this comment.
This wasn't addressed. Do you agree? Are there some tests you think are useful here?
There was a problem hiding this comment.
I'm unsure on what else can be tested in this, there isn't any heavy stuff that we can test here, so if you can suggest any then i'll add those.
There was a problem hiding this comment.
If there's tests that aren't testing anything useful, we should remove them.
ldk-reviews-bot
commented
Jan 29, 2026
🔔 1st Reminder Hey @tnull! This PR has been waiting for your review. |
| .with_header("Host", host) | ||
| .with_header("Connection", "keep-alive") |
There was a problem hiding this comment.
please let bitreq handle this
| .with_max_headers_size(Some(MAX_HTTP_MESSAGE_HEADER_SIZE)) | ||
| .with_max_status_line_length(Some(MAX_HTTP_MESSAGE_HEADER_SIZE)) |
There was a problem hiding this comment.
Please let bitreq handle this
| Some(address) => address, | ||
| }; | ||
| // Verify reachability by attempting a connection. |
There was a problem hiding this comment.
If you're gonna verify, please make a request
| } | ||
| /// Converts a bitreq error to an std::io::Error. | ||
| fn bitreq_to_io_error(err: bitreq::Error) -> std::io::Error { |
There was a problem hiding this comment.
Please just change the API?
There was a problem hiding this comment.
doing that would mean that we will have to make changes in the ldk-node as well, so should i do that?
There was a problem hiding this comment.
Yes, that would be much appreciated.
| @@ -572,27 +334,54 @@ mod endpoint_tests { | |||
| #[cfg(test)] | |||
| pub(crate) mod client_tests { | |||
There was a problem hiding this comment.
This wasn't addressed. Do you agree? Are there some tests you think are useful here?
83f1e7b to
d274f6fCompareApostlex0
commented
Jan 31, 2026
@TheBlueMatt I've removed some of the tests, if this seems ok then i can move to the ldk-node side changes. |
TheBlueMatt
left a comment
There was a problem hiding this comment.
This still has some random "make it compile"-kinda stuff. Mind going through it more carefully and making sure the API that's being exposed is consistent and makes sense?
| match &e { | ||
| // Transport errors (connection, timeout, etc.) are transient | ||
| HttpClientError::Transport(_) => BlockSourceError::transient(e), | ||
| // HTTP non-2xx errors are transient - e.g. "not found" must not stop polling |
There was a problem hiding this comment.
Presumably a 404 or 403, for example, would, in fact, indicate a persistent issue (though a 503 might not)? Maybe 5XX should be a transient issue and anything else persistent?
There was a problem hiding this comment.
yeah that makes sense, will do that👍
| HttpClientError::Transport(_) => BlockSourceError::transient(e), | ||
| // HTTP non-2xx errors are transient - e.g. "not found" must not stop polling | ||
| HttpClientError::Http(_) => BlockSourceError::transient(e), | ||
| // I/O errors follow the same logic as std::io::Error |
| /// HTTP error response | ||
| Http(HttpError), | ||
| /// I/O error (DNS resolution, etc.) | ||
| Io(std::io::Error), |
There was a problem hiding this comment.
It seems this is only used for conversion errors, never io errors...
There was a problem hiding this comment.
This is basically for parsing/conversion errors, so now I've changed the comments describing it.
There was a problem hiding this comment.
Then let's avoid wrapping the errors in std::io::Error and instead just put the error here directly? Even a &'static str probably makes more sense.
d274f6f to
133dc12CompareTheBlueMatt
left a comment
There was a problem hiding this comment.
There's still more here that we really shouldn't do - let bitreq handle the HTTP logic. All of it :)
| impl HttpClient { | ||
| /// Opens a connection to an HTTP endpoint. | ||
| pub fn connect<E: ToSocketAddrs>(endpoint: E) -> std::io::Result<Self> { | ||
| pub fn connect<E: ToSocketAddrs>(endpoint: E) -> Result<Self, HttpClientError> { |
There was a problem hiding this comment.
Now that we no longer need to do socket resolution immediately, let's defer it and let bitreq handle it? IOW let's just accept a domain name here and use it.
| Err(HttpClientError::Io(e)) | ||
| }, | ||
| Err(HttpClientError::Transport(_)) => { | ||
| // Reconnect and retry on transport failures. This can happen if the connection |
2de6b7b to
ffa084eCompareffa084e to
6f427eaCompareApostlex0
commented
Feb 12, 2026
should be clean now |
TheBlueMatt
left a comment
There was a problem hiding this comment.
Basically LGTM, two small comments. Please go ahead and squash down all the fixup commits, breaking the total work up logically (if possible, may not be, one commit is fine if you can't) and write a description of what changed and what decisions were made (don't use an LLM for this, they really suck at decent commit messages!), wrapping lines at ~70 chars.
| /// | ||
| /// The base URL should include the scheme, host, and port (e.g., "http://127.0.0.1:8332"). | ||
| /// DNS resolution is deferred until the first request is made. | ||
| pub fn new(base_url: &str) -> Self { |
There was a problem hiding this comment.
do not take an &str only to convert it to a string, take the string.
| let url = format!("{}{}", self.base_url, uri); | ||
| let request = bitreq::get(url) | ||
| .with_timeout(TCP_STREAM_RESPONSE_TIMEOUT) | ||
| .with_max_body_size(Some(MAX_HTTP_MESSAGE_BODY_SIZE)); |
There was a problem hiding this comment.
for both this and post requests below please enable pipelining.
6f427ea to
0bfa602Comparesquashed everything into one commit since separating them by files/upgrades wasn't possible since the changes were mixed up in the 10 or so commits, but the commit message should explain every change made pretty clearly. |
TheBlueMatt
left a comment
There was a problem hiding this comment.
oops one more, this wasn't really resolved.
| /// HTTP error response | ||
| Http(HttpError), | ||
| /// I/O error (DNS resolution, etc.) | ||
| Io(std::io::Error), |
There was a problem hiding this comment.
Then let's avoid wrapping the errors in std::io::Error and instead just put the error here directly? Even a &'static str probably makes more sense.
I changed the comment regarding the io error field but kept the field itself as it is since the internal tryfroms in the convert.rs for binary and json response used io:errors everywhere, so should i just changes the error api field from io to simple `static str and keep the internal as it is, or should i change all the tryfroms as well( there are around 15 tryfroms)? |
In general we want to and up with as little code as possible, so it might be nice to change the |
Apostlex0
commented
Feb 16, 2026
cool i'll change it to &`static str and string |
we changed the http layer from the manual tcpstream client implementation to bitreq, this change lets us rely on bitreq for http request formatting, sockets, HTTP parsing, chunked bodies, pooling, and async support instead of having to manually implement them. cargo.toml: we added bitreq 0.3 and updates the rest-client and rpc-client feature wiring to depend on bitreq. A tokio feature is also enabled to allow bitreq async support and pipelining. http.rs: The old HttpEndpoint builder and all manual TCP/socket timeout logic are dropped along with the manual GET/POST construction and response parsing. The client API now uses base_url and get/post return Result with a typed HttpClientError instead of std::io::Result. HttpClientError splits transport failures (bitreq::Error), non-2xx HTTP responses (HttpError), and response decoding issues (std::io::Error). rest.rs and rpc.rs: HttpEndpoint and the Mutex<Option<HttpClient>> caching pattern are removed and both clients now own an HttpClient directly using base_url. rpc.rs also adds RpcClientError so we can represent HTTP failures, JSON-RPC errors from the server, and malformed responses instead of just giving out std::io::Error. convert.rs: it maps HttpClientError and RpcClientError into BlockSourceError with this retry classification: transport errors and HTTP 5xx are transient, HTTP 4xx and invalid data are persistent, and RPC errors are treated as transient.
0bfa602 to
909041fComparefb244d2 to
16ea4c7CompareThere was a problem hiding this comment.
There's quite a maze of error types and still quite a few tests in http.rs that are testing bitreq, rather than our own code. Still, there's no reason to keep going back and forth on this, so just gonna land it.
As a followup, if you're interested, can you clean up the error types that we've ended up with? Maybe just drop all of them in favor of always using BlockSourceError everywhere and dropping the internal Box'd std::error::Error in favor of a simple string?
| } | ||
| /// Conversion from `HttpClientError` into `BlockSourceError`. | ||
| impl From<HttpClientError> for BlockSourceError { |
There was a problem hiding this comment.
This should be behind a cfg(feature = rest-client)
| }, | ||
| None => Err(e)?, | ||
| Err(HttpClientError::Http(http_error)) => { | ||
| // Try to parse the error body as JSON-RPC response |
There was a problem hiding this comment.
Let's drop this fallback? I don't think it adds anything.
| /// Sends a `GET` request for a resource identified by `uri`. | ||
| /// | ||
| /// Returns the response body in `F` format. | ||
| #[allow(dead_code)] |
| assert_eq!(e.kind(), std::io::ErrorKind::InvalidInput); | ||
| assert_eq!(e.get_ref().unwrap().to_string(), "unsupported transfer coding"); | ||
| }, | ||
| async fn connect_with_invalid_host() { |
There was a problem hiding this comment.
Why is this a useful test?
Uh oh!
There was an error while loading. Please reload this page.
TheBlueMatt
commented
Feb 19, 2026
Also, please do take a look at updating ldk node to use the new API! |
jkczyz
commented
Feb 20, 2026
My two merged PRs sandwich this one, so I'll handle all three PRs in lightningdevkit/ldk-node#794. |
fixes#4325
the ldk-node tests doesn't seem to need any changes as the changes here preserve the public api, hence what happens internally doesn't really matter as all the existing tests pass without any problems.
Though changes to payment/bolt11.rs were needed to compile and run the tests.