lightning-block-sync: switch to bitreq, drop chunked_transfer #4350
Conversation
|
👋 Thanks for assigning @tnull as a reviewer! |
|
@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
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.
| /// Client for making HTTP requests. | ||
| pub(crate) struct HttpClient { | ||
| address: SocketAddr, | ||
| stream: TcpStream, |
There was a problem hiding this comment.
Note that you'll want to hold a bitreq::Client here to reuse the same connection pool for subsequent requests.
I do however wonder if we still need the HttpClient wrapper at all?
lightning-block-sync/Cargo.toml
Outdated
| 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.
lightning-block-sync/src/http.rs
Outdated
| /// 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 86.08% 86.08% -0.01%
==========================================
Files 156 156
Lines 102428 102326 -102
Branches 102428 102326 -102
==========================================
- Hits 88179 88084 -95
- Misses 11754 11769 +15
+ Partials 2495 2473 -22
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:
|
| } | ||
|
|
||
| #[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.
4 participants
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.