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