--- Log opened Wed Dec 07 00:00:38 2022
01:49 -!- tibo_ [~tibo@fp5a950abc.tkyc110.ap.nuro.jp] has joined #bitcoin-rust
01:53 -!- tibo [~tibo@240d:1a:afa:4000:e598:a98b:626c:6d5] has quit [Ping timeout: 256 seconds]
04:10 -!- tibo_ [~tibo@fp5a950abc.tkyc110.ap.nuro.jp] has quit []
07:57 -!- pablomartin [~pablomart@195.234.124.17] has joined #bitcoin-rust
08:39 -!- pablomartin4btc [~pablomart@109.69.107.225] has joined #bitcoin-rust
08:42 -!- pablomartin [~pablomart@195.234.124.17] has quit [Ping timeout: 248 seconds]
09:10 < ariard> BlueMatt[m]: not sure about the #1825 fix https://github.com/lightningdevkit/rust-lightning/pull/1825#discussion_r1042453226
09:24 < BlueMatt[m]> ariard: can you open an issue with a specific description of how to hit it? Then let’s create a “blocking anchor” tag.
09:24 < BlueMatt[m]> It’s not like we’re shipping the code now, I’d rather keep it moving than be 100% sure, as long as we let track of the issues we think we may find and have good test coverage
09:25 < BlueMatt[m]> (Before we ship it)
09:48 < darosior> I'm confused by the new rust-jsonrpc release. You are now reusing the socket across requests in the simple_http transport, but the socket is never checked https://github.com/apoelstra/rust-jsonrpc/blob/fa0e459e3e8df7b5730c084084e65a1a6b9128b6/src/simple_http.rs#L143-L170. So if the other side closed the connection you would always get an EOF at the
09:48 < darosior> first read, which is when trying to read the HTTP header there https://github.com/apoelstra/rust-jsonrpc/blob/fa0e459e3e8df7b5730c084084e65a1a6b9128b6/src/simple_http.rs#L202. And in this case you would never actually discard the socket, so retrying would just fail on the EOF indefinitely.
09:49 < darosior> On top of that, you now use https://github.com/apoelstra/rust-jsonrpc/blob/fa0e459e3e8df7b5730c084084e65a1a6b9128b6/src/simple_http.rs#L266 to read the response, which actually expects the other side to close the connection otherwise it will never return!
09:50 < andytoshi> darosior: wdym the socket is never checked .. whenever there is any error we drop it and reconnect
09:50 < andytoshi> in the highlighted code there are multiple error returns
09:50 < darosior> andytoshi: 🤦 sorry i had missed it is no in "try_request"
09:51 < andytoshi> phew :)
09:51 < andytoshi> well, maybe not "phew" ... i would love if you could find some actual logic errors and fix them, because this http client doesn't work very well..
09:52 < darosior> Yeah it broke all my brittle unit tests. There is a large chance it's on my end, but i ended up actually considering it wasn't
09:57 < darosior> Alright it's because the EOF makes read_line return 0. So it returns an error but not a "socket" error, a "malformed header" error and my retry logic does not match on that since it should never happen. I guess if the socket is EOF we should return an io error there.
10:00 < darosior> I'm still wondering about from_reader though. What's the point of having the logic to reuse a socket if on every single request the other side needs to close the connection? You'll end up doing through the function twice every time: if a socket is cached it means you went through this function already, which means the other side had to close the
10:00 < darosior> socket, which means the socket you cached is unusable. So you have to call the function twice for each request
10:21 < ariard> BlueMatt[m]: did it with #1841, though note here it's "critical loss of funds if we get it wrong" style of issue
10:22 < BlueMatt[m]> right, i get that, but we're not shipping the code, so I'm fine with broken code as long as we remember to follow up on it :)
10:22 < BlueMatt[m]> its all behind cfg flags
10:22 < BlueMatt[m]> and that pr sat for two months. at this rate we're never going to get anchors
10:44 < ariard> sure, however if we have to spend half a year going after the subtle issues it's not a faster way to get anchors :)
10:48 < BlueMatt[m]> I dont think that's true. One issue with the current PR is that its not in a state where we can add test cases
10:48 < BlueMatt[m]> so, to some extent, every bug we fix now will mean less test coverage later because otherwise we'd add test coverage as we fix bugs
10:50 < andytoshi> darosior: idk what you mean by "on every request the other side needs to close the connection"
10:50 < andytoshi> if that is true then it's a glaring bug, but that is not the intended design
10:51 < andytoshi> oh, it's in serde_json::from_reader. jesus what a shitty function
10:51 < darosior> andytoshi: https://github.com/apoelstra/rust-jsonrpc/pull/80
10:51 < andytoshi> but i also don't see how that could possibly be true, serde_json::from_reader has no idea what a socket is or what it means to "close" one
10:52 < darosior> I don't know, but in my tests where i mock a bitcoind server with responses i had to actually close streams for it to make progress
10:52 < andytoshi> darosior: it may be that if you don't set the Content-Length header that we have a bug
10:52 < andytoshi> because bitcoind always sets this header, so it's easy for us to stick a Take adaptor on the socket so that it'll return EOF when expected
10:53 < darosior> Indeed i don't in my mocked bitcoind responses
10:53 < andytoshi> ok. also your PR reverts #69 by only reading a single line
10:54 < darosior> Yeah :/
10:54 < andytoshi> if you want to read the json data into a buffer we could do something shitty like reading until all the non-escaped }s have balanced the non-escaped {s (or "s or true/false or numbers or nulls)
10:54 < andytoshi> which probably would be faster, but then we're 50% of the way to writing our own json parser
10:55 < darosior> What C-lightning does is to use '\n\n' as a separator fwiw
10:55 < darosior> As far as i remember
10:56 < darosior> I don't know what's the best thing to do, i just pushed the PR to start the discussion
10:56 < darosior> I'll stick to 0.12 for now :)
10:56 < andytoshi> probably we should just get minreq into shape so that we're not dealing with this bullshit protocol alone
10:56 < andytoshi> darosior: ok, but 0.12 doesn't work on mac
10:56 < darosior> 😭
10:57 < darosior> I'll use a patched 0.12 then i guess
10:58 < andytoshi> lol .. patched how?
10:58 < darosior> Ok i didn't know the mac bug was precisely fixed by this
10:59 < darosior> lol
10:59 < andytoshi> lol yeah. http blows.
11:00 < darosior> Oh well i just won't support macOS for the first release at least i won't have to bother figuring out how to cross compile a reproducible build :p
11:01 < darosior> I managed to get a bootstrappable (and reproducible) build of my rust application using Guix btw, if that's of interest to anyone here.
11:02 < BlueMatt[m]> andytoshi what's the status of minreq?
11:02 < BlueMatt[m]> did we hear back from upstream?
11:02 < andytoshi> BlueMatt[m]: i never PRed to them, i got distracted
11:02 < BlueMatt[m]> ah, damn
11:02 < BlueMatt[m]> lmk how it does, i want to nag bdk to switch to it
11:02 < BlueMatt[m]> they keep using the stupid rust "common" http clients which pull in every dependency known to man
11:03 < andytoshi> ok lemme start working on a pr now
11:03 < BlueMatt[m]> thanks
11:09 < andytoshi> lol they have so many features, it might break my "test every combo simultaneously" script
11:14 < andytoshi> some of the optional deps are clearly not intended to be used alone, and fail compilation .. others are
11:49 < andytoshi> also there is some mysterious test failure that seems to only happen when i'm running stable and 1.48.0 at the same time :(
11:57 < andytoshi> oh it's a bug in once_cell
12:00 < andytoshi> no .. it's something else non-deterministic. holy shit this is annoying
12:12 -!- trev [~trev@user/trev] has quit [Remote host closed the connection]
13:29 -!- pablomartin4btc [~pablomart@109.69.107.225] has quit [Ping timeout: 260 seconds]
13:58 < andytoshi> ah, no, it's because they spawn a server at a fixed port # in their tests
13:58 < andytoshi> but curiously this doesn't seem to cause problems unless i am using multiple version-s of rustc at once
14:29 < BlueMatt[m]> Ugh
14:29 < BlueMatt[m]> Also once cell has a high msrv, no?
14:29 < BlueMatt[m]> They bumped it without a major version
14:41 < andytoshi> yeah, you have to do `cargo update -p`
14:41 < BlueMatt[m]> ugh
14:42 < andytoshi> but minreq supports 1.14 which works with rustc 1.48 .. the latest is 1.16 which does not
14:42 < andytoshi> yeah
14:42 < BlueMatt[m]> I'd kinda prefer to avoid any dependencies that use once-cell
14:42 < andytoshi> heh we could try to PR to go back to static_mutex .. but that would revert a change they made recently so they might not be keen on it
14:42 < BlueMatt[m]> I guess I don't get why an http lib needs a once cell
14:42 < BlueMatt[m]> but okay
14:52 < andytoshi> lemme check .. it is an optional dep fwiw
14:52 < BlueMatt[m]> ah, okay, well that's at least better
14:53 < andytoshi> oh, it's only used when you enable TLS, for the "root store"
14:53 < andytoshi> basically if you don't use TLS it has a super reasonable dep tree
14:53 < andytoshi> and if you do, it's less reasonable ... though way better than hyper
14:53 < BlueMatt[m]> ah. makes sense
14:53 < BlueMatt[m]> can you not use the native tls option and not get any of that?
14:53 < BlueMatt[m]> that should just be "link libssl"
14:54 < andytoshi> you can indeed :)
14:54 < BlueMatt[m]> I mean then you're using openssl
14:54 < BlueMatt[m]> buttttttttt
14:54 < andytoshi> it has 3 TLS backends it seems
14:54 < andytoshi> "rustls" which depneds on once_cell etc, and "native-tls", and openssl
14:55 < andytoshi> if you try to enable them all at once it fails to compile, which i'll fix (i guess to just prioritize one) but it's designed for you to pick one
14:55 < BlueMatt[m]> wait what is native-tls? I thought that was openssl
14:55 < andytoshi> ahh, it's openssl except not on windows apparently
14:55 < andytoshi> https://docs.rs/native-tls/latest/native_tls/
14:55 < BlueMatt[m]> ah lol
16:11 -!- tibo [~tibo@240d:1a:afa:4000:5925:92b1:3972:db5c] has joined #bitcoin-rust
18:46 -!- tibo_ [~tibo@fp5a950abc.tkyc110.ap.nuro.jp] has joined #bitcoin-rust
18:49 -!- tibo [~tibo@240d:1a:afa:4000:5925:92b1:3972:db5c] has quit [Ping timeout: 256 seconds]
--- Log closed Thu Dec 08 00:00:40 2022