--- Log opened Mon Dec 21 00:00:53 2020 01:30 -!- shesek [~shesek@164.90.217.137] has joined #rust-bitcoin 01:30 -!- shesek [~shesek@164.90.217.137] has quit [Changing host] 01:30 -!- shesek [~shesek@unaffiliated/shesek] has joined #rust-bitcoin 01:34 < elichai2> tibo: nothing integrated, you could implement it the same way bitcoin core does, with a loop and checking the length. but rust-secp doesn't expose a way to pass the randomness to the ecdsa sign function 01:54 -!- jeremyrubin [~jr@c-24-4-205-116.hsd1.ca.comcast.net] has quit [Ping timeout: 260 seconds] 02:27 < elichai2> andytoshi: real_or_random I think I'll try to ask in the rust embedded+unsafe community for review on my preallocated PRs, maybe we will get some use-case insight from that(and maybe more approval for the correctness of the approaches) 03:20 -!- Aurelio71Kuphal [~Aurelio71@static.57.1.216.95.clients.your-server.de] has joined #rust-bitcoin 03:27 < stevenroose> andytoshi: the io::Result for Encodable would be really useful, mostly because it has fixes for the CommandString stuff 03:28 < stevenroose> andytoshi: https://github.com/rust-bitcoin/rust-bitcoin/pull/496 would also be great 03:31 < dr-orlovsky> stevenroos: #496 needs rebase though 03:32 < dr-orlovsky> elichai2, andytoshi: if this takes long, can we have rust-secp256k1 release w/o it (and do yet another release when it's ready) so 0.26 of rust-bitcoin will have xcoordonly pubkeys? 03:33 < dr-orlovsky> andytoshi: on my side, #498 has one ACK and needs your re-ack after rebase and is really simple 03:34 < dr-orlovsky> #471 has no reviews yet, but its a final part of PSBT refactoring and adds support for proprietary keys, which is also useful for liquid/elements, so will be really get it reviewed and merged (not sure we have time though) 03:35 -!- Aurelio71Kuphal [~Aurelio71@static.57.1.216.95.clients.your-server.de] has quit [Ping timeout: 264 seconds] 03:35 < dr-orlovsky> #524 by sanket1729 is also nice to have and already acked by andytoshi so needs just a second ack 03:36 < dr-orlovsky> so elichai2, stevenroose can you help with reviewing #524 and #471? 03:38 < stevenroose> dr-orlovsky: yeah rebasing 496 atm 03:45 < dr-orlovsky> andytoshi, stevenroose, elichai2: made a list of all mentioned PRs as a comment under version bump PR https://github.com/rust-bitcoin/rust-bitcoin/pull/533#issuecomment-748932166 03:45 < dr-orlovsky> feel free to edit/add 03:48 < dr-orlovsky> cc sgeisler 04:01 < stevenroose> andytoshi, dr-orlovsky, sgeisler: I rebased https://github.com/rust-bitcoin/rust-bitcoin/pull/496 and added to it the CommandString-related commit from 494, so it'd be cool to get that in as well. I don't think the breaking enums are too invasive, I think the number of users of our p2p messages are quite limited. 04:01 < stevenroose> Let me look at 524 and 471 next 04:14 < stevenroose> dr-orlovsky, sgeisler andytoshi: also rebased https://github.com/rust-bitcoin/rust-bitcoin/pull/508 (the fixes for PSBT JSON serialization) 04:27 -!- tibo [~tibo@2400:4050:2a83:7000:44d0:f07f:7311:7e31] has quit [Remote host closed the connection] 04:27 -!- tibo [~tibo@2400:4050:2a83:7000:44d0:f07f:7311:7e31] has joined #rust-bitcoin 04:32 -!- tibo [~tibo@2400:4050:2a83:7000:44d0:f07f:7311:7e31] has quit [Ping timeout: 260 seconds] 06:36 < sanket1729> dr-orlovsky: There is still some discussion on 524 which has not achieved consensus. It does not have to be blocker for the release. 06:38 < andytoshi> elichai2: i'd like to do a rust-secp release 06:38 < andytoshi> think we should pull the allocation stuff in or not? 06:38 < andytoshi> at the very least, do we need to mark some functions unsafe? i forget whether these already are 06:40 < andytoshi> stevenroose: i would like to move the io::Result stuff to the next (API-breaking) release 07:03 -!- DeanGuss [~dean@gateway/tor-sasl/deanguss] has joined #rust-bitcoin 07:04 -!- DeanWeen [~dean@gateway/tor-sasl/deanguss] has quit [Remote host closed the connection] 07:58 -!- belcher_ [~belcher@unaffiliated/belcher] has joined #rust-bitcoin 08:01 -!- belcher [~belcher@unaffiliated/belcher] has quit [Ping timeout: 256 seconds] 08:03 < andytoshi> #508 doesn't compile, and needs rebase 08:08 -!- yancy [~yancy@li1543-67.members.linode.com] has quit [Remote host closed the connection] 08:09 -!- yancy [~yancy@li1543-67.members.linode.com] has joined #rust-bitcoin 09:50 -!- jeremyrubin [~jr@2601:645:c200:14:d16a:4949:3622:4cc0] has joined #rust-bitcoin 10:41 -!- yancy [~yancy@li1543-67.members.linode.com] has quit [Remote host closed the connection] 10:42 -!- belcher_ is now known as belcher 10:55 -!- DeanWeen [~dean@gateway/tor-sasl/deanguss] has joined #rust-bitcoin 10:57 -!- DeanGuss [~dean@gateway/tor-sasl/deanguss] has quit [Ping timeout: 240 seconds] 11:15 < cloudhead> is it possible to calculate an address balance using bip157, without the private key? 12:18 < dr-orlovsky> stevenroose: Can you pls clarify situation with your position regarding https://github.com/rust-bitcoin/rust-bitcoin/pull/524 ? From what I am getting from your comments is that you are is neither pro or against and asks others to decide 12:19 < dr-orlovsky> however it became a blocker and even though the PR has two ACKs it can't be merged without this clarification 12:19 < dr-orlovsky> *sorry I was talking about https://github.com/rust-bitcoin/rust-bitcoin/pull/498, not #524 12:27 < stevenroose> dr-orlovsky: re:524 yeah it seems to me that the ord is quite expensive, but it also seems like the most inexpensive it can be, no? I think the cheapest Ord would be just to derive it on all levels up to the ffi where it can do memcmp 12:28 < stevenroose> *most inexpensive given the current API in rust-secp 12:28 < dr-orlovsky> Sorry, I mistakenly pointed you at #524, I was talking about #498 12:28 < stevenroose> andytoshi: rebased 508, I forgot to check 1.29. oh I might have forgotten again (I checked both times without serde) 12:43 -!- Rozella74Harvey [~Rozella74@static.57.1.216.95.clients.your-server.de] has joined #rust-bitcoin 13:14 < andytoshi> 508 doesn't build now because there are new PSBT types 13:32 -!- junderw [sid43070@gateway/web/irccloud.com/x-qnoodlvfldmuqnzb] has quit [Ping timeout: 258 seconds] 13:34 -!- junderw [sid43070@gateway/web/irccloud.com/x-ytneqqleilqeaikv] has joined #rust-bitcoin 14:08 -!- Ed0 [~edouard@137.ip-193-70-113.eu] has quit [Ping timeout: 240 seconds] 14:08 -!- Ed0 [~edouard@2001:41d0:401:3100::4897] has joined #rust-bitcoin 14:26 < andytoshi> gonna push a new version of rust-secp 14:26 < andytoshi> just testing locally 14:30 < andytoshi> https://github.com/rust-bitcoin/rust-secp256k1/pull/257 14:30 < andytoshi> elichai2: real_or_random: 14:31 < elichai2> Argh sorry. I feel pretty confident about the correctness of all those PRs 14:32 < elichai2> But I understand if people prefer to get use case feedback 14:39 < andytoshi> the alignment stuff? 14:40 < andytoshi> i think we shuold just release it and see what happens 15:08 -!- tibo [~tibo@2400:4050:2a83:7000:bc4e:d063:9de0:75af] has joined #rust-bitcoin 15:21 -!- valwal_ [sid334773@gateway/web/irccloud.com/x-xpdkkmqnrtcjkded] has quit [Ping timeout: 260 seconds] 16:35 < tibo> elichai2: thanks for confirming! I ended up implemented it in rust-secp as I couldn't really see any other way. I made a PR for it (https://github.com/rust-bitcoin/rust-secp256k1/pull/259) but if it's not desired I'll try to find a way to put it somewhere else. 16:39 -!- Rozella74Harvey [~Rozella74@static.57.1.216.95.clients.your-server.de] has quit [Ping timeout: 272 seconds] 16:40 < andytoshi> tibo: cool! at first glance this looks good 16:40 < andytoshi> i wonder if we should give it a param indicating how low to force the R :P 16:41 < andytoshi> i've considered doing this on the liquid functionaries, which have very powerful CPUs and put a lot of oshit on the bitcoin blockchain.. 16:41 < andytoshi> that maybe i should grind 3 or 4 bytes off all the signatures 16:42 < tibo> andytoshi: thanks for checking! My goal was mainly to match bitcoin core implementation, but I guess I could just pass the adequate parameter to match. 16:43 < andytoshi> yeah i think that'd be cool. pass a usize bytes_to_grind param or something 16:44 < andytoshi> and document that (a) passing 1 gives you the "low R" behavior from Core, and typically requires only a couple iterations 16:45 < andytoshi> (b) every number after that does 256 times as many sigs :P 16:45 < tibo> Ok will try that :) 16:54 < andytoshi> if we wanted to be super rustic we could use an enum because 1 feels a bit like a mogic value 16:54 < andytoshi> but i think that's overkill 16:57 < tibo> I'll try with just usize for now, it should be easy to change to enum later if desired. 17:03 < tibo> Just to confirm, what you want to do is use `bytes_to_grind` to ensure that the length of serialized DER version of the signature is < 72 - bytes_to_grind? 17:04 < tibo> (I guess <=) 17:13 < andytoshi> yep 17:15 < tibo> The reason I'm asking is because I implemented that but it results in different signatures than the one generated by bitcoin core when passing 1 (I'm not deep into DER but I guess it's possible to have the highest bit set to 1 while still having a "small R value") 17:17 < tibo> I could check for both DER length and that the compact size has first byte < 0x80 but it means converting two times so not sure if that's great. 17:17 < tibo> And in the case where you just want to grind (without caring about bitcoin core) maybe you don't care about the first byte 17:21 < andytoshi> hmm, that surprises me that you see differences 17:22 < andytoshi> and you didn't see differences with the original code you PR'd? 17:22 < andytoshi> oh i see 17:22 < andytoshi> the original code doesn't look a the DER length, it usse the compact encoding 17:23 < tibo> Yes which is what bitcoin core does AFAICT 17:23 < andytoshi> to see whether the high bit is 1 (which will cause an extra byte when DER encoding) 17:23 < andytoshi> i'm surprised these aren't equivalent 17:23 < tibo> Yes 17:23 < tibo> I might have messed up something 17:24 < tibo> I'll just commit the new version 17:25 < andytoshi> sure 17:25 < andytoshi> i pulled the old one, so it'll be easy for me to compare 17:25 < andytoshi> it's at commit ca4e66056322b29d39b16a13fea03a55b0cb1476 for reference 17:26 < andytoshi> oh 17:26 < andytoshi> the DER length can also be affected by s 17:26 < andytoshi> i wonder if this is what's causing the differene 17:26 < tibo> Ah good point 17:26 < andytoshi> althuogh - s should be short only 1/256th of the time 17:26 < andytoshi> whereas R should be short 1/2 the time 17:27 < andytoshi> weere you seeing consistent differences or infrequent differences? 17:27 < tibo> Seems like consistent. I pushed to a different branch for now: https://github.com/p2pderivatives/rust-secp256k1/commit/ca4e66056322b29d39b16a13fea03a55b0cb1476 17:28 < tibo> Wait that's wrong sorry 17:28 < tibo> https://github.com/p2pderivatives/rust-secp256k1/commit/2d84fcd4681e37839ea3f5280d57d3bf2136f868 17:28 < tibo> That's the one 17:29 < tibo> For example I had a hardcoded signature test that now fails: https://github.com/p2pderivatives/rust-secp256k1/commit/2d84fcd4681e37839ea3f5280d57d3bf2136f868#diff-b1a35a68f14e696205874893c07fd24fdb88882b47c23cc0e0c80a30c7d53759R1138 17:31 < andytoshi> can you paste the test failure 17:31 < andytoshi> or what sig you get 17:32 < tibo> This is the one I get (actual): 3045022100cf51e409730e15fbf06ebe4a8d0911642fc80462ee2395edac702ceeba1aee3d022068374fd34a9ecd45be11bf3fa57a4d4acb8836af5dc155b5ed9218298ad973e8 17:32 < tibo> This is the one I expected: 30440220047dd4d049db02b430d24c41c7925b2725bcd5a85393513bdec04b4dc363632b02201054d0180094122b380f4cfa391e6296244da773173e78fc745c1b9c79f7b713 17:32 < andytoshi> thanks 17:33 < andytoshi> this is so weird ... your two code snippets are s substantially the same 17:33 < andytoshi> and i'm pretty sure "r is 33 bytes" and "high bit of r is 1" are equivalent 17:33 < tibo> If I pass 2 to my code is passes 17:33 < andytoshi> hmmm 17:33 < andytoshi> ok, i was half expecting this was an off-by-one error 17:33 < andytoshi> but i don't see why it should be 2 :P 17:34 < tibo> lol me neither :D 17:35 < tibo> I think it's just a coincidence. If I set 2 in the `sign_and_verify` test, it sometimes succeeds and sometimes fails 17:35 < andytoshi> yeah i think it might be 17:35 < andytoshi> in the "sig you got" above there is definitly a high R 17:35 < andytoshi> and a normal s 17:36 < andytoshi> oh! 17:36 < andytoshi> it is s 17:36 < andytoshi> but you can also shave a byte off of s with 50/50 probability 17:37 < andytoshi> in both cases, if the high bit is 1 then DER makes you stick an extra 00 byte in there 17:37 < tibo> Ah I see 17:37 < andytoshi> cuz they're signed numbers 17:37 < andytoshi> hmm .. so, if core compat is important to you then you've gotta do the compact thing 17:37 < andytoshi> and my "just make it so you can grind even more bytes" idea is not gonna work 17:37 < andytoshi> i'll have to write my own code :P 17:38 < tibo> I guess I could put both if you want 17:38 < andytoshi> lol yeah, i guess you've already writte the code 17:38 < tibo> Have a common internal function that takes a check and two public one 17:38 < andytoshi> and i've already read it 17:39 < andytoshi> yeah that sounds reasonable 17:39 < tibo> Cool I'll do that then (a bit later though)! Thanks for the help debugging :) 17:40 < andytoshi> wait lol i'm confused again ... the high bit of s isn't allowed to be 1 17:40 < andytoshi> because of the low-s rule 17:40 < andytoshi> ah! so i think actually -2 is correct.. 17:41 < andytoshi> or rather, you want 71 - bytes_to_grind rather than 72 - bytes_to_grind 17:41 < andytoshi> because low_s "automatically" grinds a byte off 17:41 < andytoshi> i think 17:42 < andytoshi> are you able to get a failing example where -2 does the wrong thing? 17:43 < andytoshi> lol it never occurred to me that low-s was actually saving blockchain space ... it's probably saved multiple megs of data 17:44 < tibo> Yeah but it might be that my test does not make sense: https://github.com/p2pderivatives/rust-secp256k1/commit/2d84fcd4681e37839ea3f5280d57d3bf2136f868#diff-b1a35a68f14e696205874893c07fd24fdb88882b47c23cc0e0c80a30c7d53759R1042 17:44 < tibo> Haha yeah that's true 17:45 < tibo> Ah sorry that's the assert that fails sporadically with -2: https://github.com/p2pderivatives/rust-secp256k1/commit/2d84fcd4681e37839ea3f5280d57d3bf2136f868#diff-b1a35a68f14e696205874893c07fd24fdb88882b47c23cc0e0c80a30c7d53759R1047 17:45 < andytoshi> ah, that i expect to fail about 1/256th of the time 17:46 < tibo> That matches what I can see 17:46 < andytoshi> possibly 1/128th, i'm not certain about the probabiliteis .. but i think 1/256 17:46 < andytoshi> ok cool :) so the reason is that every so often you get a whole byte of 0s as the MSB of either R or s 17:47 < andytoshi> in particular, if the MSB of s is all 0s, you'll save a byte, even if R is high 17:47 < andytoshi> according to the compact-test 17:47 < tibo> I see that makes sense 17:47 < andytoshi> but .. as a corollary ... if you are using the "serialize and check length" method then you won't be Core-compatible 17:48 < tibo> Yeah, so that's why I was thinking to have two separate functions, one that does the compact check and one that does the DER length check, does that make sense? 17:50 < andytoshi> yep 17:51 < tibo> perfect :) 17:51 < andytoshi> you could even expose the ability to choose the function to the user ... but i think that's a bad idea, it might be possible to cryptographically wreck your sigs with the right filter (though i don't -think- so) 17:51 < andytoshi> in any case 17:51 < andytoshi> it'd be a cleaner API if you just exposed the two things 17:52 < andytoshi> core's low R (which will have an almost immeasurable perf impact for a free byte) and "grind this many bytes" (which ofc is exptime but can save you even more space) 17:53 < tibo> Yeah sounds good. Indeed there could be a boolean to chose the check, but I agree the API might be cleaner if it's separated. I'll PR with things separated anyway easy to change after. 17:55 < andytoshi> sounds great 18:19 < andytoshi> stevenroose: elichai2: i wonder if we should drop the external-symbols feature from rust-secp 18:19 < andytoshi> we original did this to avoid conflicts with libbitcoinconsensus' libsecp 18:19 < andytoshi> but we now avoid conflicts by symbol renaming 18:20 < andytoshi> and we don't/can't test this feature 18:20 < andytoshi> it breaks compilation with --all-features 18:31 -!- CubicEarth [~CubicEart@c-67-168-1-172.hsd1.wa.comcast.net] has quit [Read error: Connection reset by peer] 18:32 -!- CubicEarth [~CubicEart@c-67-168-1-172.hsd1.wa.comcast.net] has joined #rust-bitcoin 21:59 < tibo> andytoshi: updated the PR with the two functions 23:17 -!- shesek [~shesek@unaffiliated/shesek] has quit [Remote host closed the connection] --- Log closed Tue Dec 22 00:00:52 2020