--- Day changed Wed Sep 30 2015 02:47 -!- CodeShark_ [CodeShark@cpe-76-167-237-202.san.res.rr.com] has quit [Ping timeout: 252 seconds] 08:58 -!- andytoshi [~andytoshi@wpsoftware.net] has quit [Changing host] 08:58 -!- andytoshi [~andytoshi@unaffiliated/andytoshi] has joined #secp256k1 09:00 < cfields> sipa: ping 09:00 < cfields> sipa: what's the reason for not enabling all modules by default for travis tests? 09:03 < cfields> seems to me we should enable them everywhere, and only disable in a test or two to ensure that build doesn't break without them 09:06 <@sipa> cfields: time, i guess 09:08 < cfields> mm, ok 09:09 < cfields> i'm about to PR a change that adds native osx testing, so that'll bump up the test time pretty significantly 09:09 < cfields> i guess we should do that before discussing other things that would slow it down 09:45 <@sipa> let's enable as much as possiblr 09:45 <@sipa> we can always find smaller subsets later if it hurts merging 09:45 < andytoshi> does travis have rules about how large/long its tests can be? 09:45 < andytoshi> or is this just for our own convenience? 09:46 <@sipa> andytoshi: well we have limited cpu time, so if you submit many PRs that need long processing, we'll get long wait times 09:46 < andytoshi> ok 09:46 < andytoshi> at the current PR volume i could probably be running every config on every PR on my system, i might setup a bot that does that (in addition to travis) 09:46 < andytoshi> so we have travis in 5-10 minutes then maybe a day later get an ACK from my bot 09:47 < andytoshi> did the "parameterize # of test runs" pr get merged? 09:50 <@sipa> it has always been parametrized? 09:51 <@sipa> ./tests 5 09:53 <@gmaxwell> if you run too long without output travis kills your process I thought? 09:53 < andytoshi> sipa: oh, right, i meant benchmarks 09:55 < andytoshi> gmaxwell: for an individual test yes 09:55 < andytoshi> gmaxwell: but i'm not sure what it'd do if we just gave it a 1000-entry matrix 09:55 < andytoshi> also you can configure the timeout i think 10:30 -!- fkhan [weechat@gateway/vpn/mullvad/x-xpcqfjvlalgcqvdn] has quit [Read error: Connection reset by peer] 10:51 -!- fkhan [~weechat@unaffiliated/loteriety] has joined #secp256k1 11:31 -!- fkhan [~weechat@unaffiliated/loteriety] has quit [Ping timeout: 240 seconds] 11:45 -!- fkhan [weechat@gateway/vpn/mullvad/x-jfozayxakgwlhkes] has joined #secp256k1 12:34 -!- GAit [~GAit@2.230.161.158] has joined #secp256k1 13:02 -!- GAit [~GAit@2.230.161.158] has quit [Quit: Leaving.] 13:35 -!- rusty [~rusty@pdpc/supporter/bronze/rusty] has joined #secp256k1 13:35 < rusty> sipa: should the failure path for weird BER encodings be in libsecp though? 13:35 <@sipa> the alternative is saying that every application that accepts BER signatures (for legacy reasons, or whatever standard compliance) needs to implement their own BER parser + DER reencoder 13:36 <@sipa> i fear that may encourage more crappy behaviour 13:36 < rusty> sipa: hmm, libweirddersig? 13:36 <@gmaxwell> Anything that can use the strict partser + lowS check really should. But things that can't, probably want a full BER parser. 13:37 <@sipa> rusty: it's 130 lines of pure C code 13:37 <@gmaxwell> I am kind of sad that we can't do more to encourage lowS checking for new applications. 13:37 < rusty> sipa: mmm... it's still a bad idea and should be contained in its own sad little world. 13:38 <@gmaxwell> We joked about giving it a dismissive function name. 13:38 <@gmaxwell> _sloppy_parser 13:38 <@sipa> gmaxwell: how about making the signature checking only accept low S, but have a function that converts non-low-S to low-S before checking? 13:38 <@gmaxwell> sipa: I like that. 13:38 <@sipa> that gets you a signature normalizer (for externally produced ones) for free 13:39 <@gmaxwell> may result in some annoying test updates, but if so I'll do the work. 13:39 < rusty> gmaxwell: _parse_not_quite_der ? _parse_OMG_what_is_that_thing? 13:39 <@gmaxwell> _parse_smelly_encoding() 13:40 < rusty> _parse_reluctantly() 13:40 <@sipa> parse_yes_i_agree_to_the_terms_and_conditions_just_do_it_please 13:41 <@sipa> _parse_der_permissive would be acceptable to me 13:41 <@gmaxwell> people_got_fired_for_calling_this_function_parse() 13:41 < rusty> gmaxwell: stop it, others in the house are trying to sleep while I'm chuckling :) 13:42 <@gmaxwell> more seriously, say the important thing about what it does? parse_malleable_encoding() 13:44 < rusty> gmaxwell: makes sense. 13:44 <@gmaxwell> sipa: so for testing can there be some easy to to instrument this thing to find out what parts of BER it's using? The issue is that you will accept a superset of openssl, which means that I can't do a differential test against openssl without having additional checks to tell it to ignore the cases we expect openssl to reject. 13:48 < rusty> BTW, would like proper sig type for schnorr_verify. https://twitter.com/rusty_twit/status/649172874238922753 13:51 <@gmaxwell> that applies even more strongly to the msg32 actually, as we recently had a user that thought msg32 (on ecdsa) was the message, instead of a message hash, god knows how they could have possibly thought that! ( :) ) 13:52 <@gmaxwell> Though on the schnorr interface, if we ever expect anyone else to use this we ought to support non-prehashed schnorr. 13:52 < rusty> gmaxwell: that's a bit harder, because it could be the message, so you are forcing a copy if you type that. 13:54 <@gmaxwell> rusty: So in the ECDSA case, you really really have to be passing in a hash there or doom will befall you.. In the schnorr case, it's fine to not prehash, though right now we have requirement that the input must be exactly 32 bytes. 13:55 < rusty> gmaxwell: the interface has become something of a mix now. It used to be all unsigned char, and I was going to wrap it with my own types for an API. 13:55 <@gmaxwell> (for ecdsa anyone can trivially forge a signature for the zero message) 13:55 < rusty> gmaxwell: why does ECDSA have to be a hash? 13:55 < rusty> gmaxwell: snap, OK. 13:56 <@gmaxwell> I've kinda wanted to actually make verify reject those cases.. but the result wouldn't be ECDSA anymore. 13:56 <@gmaxwell> and it would technically be a consensus risk in bitcoin, if someone ever figured out a way to get the signature hasher to return 0. 13:56 <@gmaxwell> then again if they could do that, they could likely steal anyones coins. 13:56 < rusty> gmaxwell: least of worries then, isn't hasher double-sha in bitcoin? :) 13:57 <@gmaxwell> yea, though a type safty bug means there is a way in bitcoin to pass a "1" out of the hasher! 13:57 <@sipa> rusty: so we need new types for schnorr signatures, hashed messages, two types of partial signatures, ...? 13:57 <@gmaxwell> just not zero (bullet dodged) 13:58 <@sipa> rusty: just by the amount of work it takes to adapt the test code to wrapped types for public keys and ecdsa signatures, i feel it's already annoying overhead 13:58 <@gmaxwell> rusty: for schnorr signatures there is no parsing it's just 32 bytes, so it does seem a little odd to have the copy, but that said-- the type safty is probably worth the overhead, if you've actually swapped the arguments. 13:58 <@sipa> i understand type safety yes... 13:59 <@sipa> it seems pretty silly to have a "parse_message" function... which is internally just a memcpy 13:59 < rusty> sipa: skip the parser, that's silly. Just struct wrap it. 13:59 <@sipa> hmm 14:00 <@sipa> that may be confusing... some types being "open" (with a well-defined and accessible representation) and others opaque 14:00 < rusty> sipa: I'm not convinced that making ecdsa sigs opaque was a win, but I'm probably missing something. 14:01 < rusty> eg. this is waht ccan's sha256 uses: 14:01 < rusty> struct sha256 { union { 14:01 < rusty> unsigned char u8[32]; 14:01 < rusty> uint32_t u32[8]; 14:01 < rusty> } u; }; 14:01 <@sipa> rusty: that's not endian neutral 14:02 < rusty> sipa: yeah, you don't want to be doing endian conversions internally? 14:02 <@sipa> i don't understand 14:03 <@sipa> those uint32_t are only actually semantically integers on big endian platforms 14:04 < rusty> sipa: yes. 14:04 <@sipa> i think that makes it a bad idea to expose them as integers 14:04 < rusty> sipa: (the field should be called be32). 14:05 <@sipa> anyway, reason for making them opaque is potential future optimization that changes the representation 14:05 <@gmaxwell> Its sometimes debated if that structure is in fact specification compliant C. 14:05 <@sipa> for example, store signatures in host-endian form 14:06 <@gmaxwell> (language laywers wonk over _any_ coersion of data of different types via a union is holy and pure... of course it's a partical necessity in much code, that it's a pointless argument.) 14:07 < rusty> sipa: that seems unlikely to help noticeably. I dislike exposed structures which are "don't look at me". You end up inevitably with a whole heap of trivial "conversion" functions for API purity. 14:08 < rusty> sipa: I would really prefer you expose the r and s values in the sig, rather than try to guess what I'm going to do with it. 14:08 <@sipa> rusty: another argument is when not every representation is valid, and you want to prevent people from just copying something in, and bypassing checks 14:09 < rusty> sipa: sure, have verify functions. I want them too! 14:09 < rusty> Whether opaque or not, I can still corrupt stuff. 14:09 <@sipa> only by doing actively stupid things, or using them uninitialized 14:10 <@gmaxwell> Sure but you can think that the correct way to use it is to copy data in (pubkey would be a great example) and then really bad things happen. 14:10 <@gmaxwell> We do want to set things up so that no combination of locally sensible ways of using the library can result in a cryptographic vulnerabilty in the caller... at least to the greatest extent possible. 14:11 < rusty> gmaxwell: sure, but I'd argue your pubkey should be in neutral form, not machine-specific. 14:11 -!- TD-Linux [~Thomas@about/essy/indecisive/TD-Linux] has left #secp256k1 ["Leaving"] 14:11 -!- TD-Linux [~Thomas@about/essy/indecisive/TD-Linux] has joined #secp256k1 14:11 < rusty> Anyway, APIs are hard, and you've kind of chosen a middle ground. I don't think there's Right vs Wrong from here. 14:11 <@gmaxwell> scribbling over the memory of an opaque type is something you _might_ do, but at least it's not locally sensible. 14:11 <@gmaxwell> Sure. 14:12 <@gmaxwell> rusty: tecnically they are not currently 'machine specific' but they've gone through a computationally expensive verification, and if you copy pubkey data right into the type you'll bypass that. 14:12 <@sipa> gmaxwell: pubkey definitely is architecture specific 14:13 <@sipa> the internal data will be different on LE vs BE 14:13 < rusty> gmaxwell: ah, but you've said it's ok to memcmp them. And presumably assign them. It's a bit of a weird line for an API user. 14:14 <@sipa> * The exact representation of data inside is implementation defined and not 14:14 <@sipa> * guaranteed to be portable between different platforms or versions. It is 14:14 <@sipa> * however guaranteed to be 64 bytes in size, and can be safely copied/moved. 14:14 <@sipa> * If you need to convert to a format suitable for storage or transmission, use 14:14 <@sipa> * secp256k1_ec_pubkey_serialize and secp256k1_ec_pubkey_parse. 14:14 < TD-Linux> rusty, that restriction is pretty common among C libraries 14:14 <@gmaxwell> yea, usually opaque types have undefined size and they're just pointers--. But we didn't want to force heap allocation of them. 14:15 -!- evoskuil [~evoskuil@c-73-225-134-208.hsd1.wa.comcast.net] has joined #secp256k1 14:15 < rusty> Anyway, I was planning on writing a secp_for_dummies wrapper lib which added type, back when it was all unsigned char. Now I'm not so sure. Hence I'm not sure if you should wrap schnorr or not. 14:16 <@gmaxwell> rusty: you asked for stronger typing. we listened! :) 14:16 < rusty> gmaxwell: but you convinced me :) 14:16 <@sipa> the distinction is now "is there a canonical representation where every X byte sequence is valid" 14:16 <@sipa> this is true for schnorr sigs and messages 14:16 <@sipa> it is not true for public keys and ecdsa sigs 14:17 < rusty> sipa: gotchya. Hmm, might be nice to add a _verify for those, so people can assert? And a _fixup for sigs, I guess. 14:17 <@sipa> rusty: what is there to verify? 14:17 < rusty> gtg, child awake. 14:18 <@sipa> rusty: ah, you mean a _verify for pubkey and ecdsa sig? 14:18 < rusty> sipa: yeah 14:18 < rusty> or _check or some name good in (assert(). 14:18 <@sipa> perhaps, if it's clear that it's not necessarily for normal usage 14:19 <@gmaxwell> isnt_corrupted() 15:42 -!- rusty [~rusty@pdpc/supporter/bronze/rusty] has quit [Ping timeout: 264 seconds] 16:59 -!- rusty [~rusty@pdpc/supporter/bronze/rusty] has joined #secp256k1 17:00 < rusty> gmaxwell: hate inverse tests. _corrupted() please. 17:00 <@gmaxwell> ha, isnt() was because you said assert(), you primed my inversion. 17:03 <@sipa> _intact() 17:09 < rusty> sipa: hmm... _ok() ? Doesn't mean "good", just "reasonable". 17:10 < rusty> sipa: _we_bikeshedded_the_name_and_got_stuck_with_this_ok(). 17:10 <@sipa> _fuck_off() 17:13 <@gmaxwell> #define OFFENSIVE_SECP256K1_NAMES. 17:14 < rusty> OK, in the interest of progress I give my vote, fwiw, to sipa. Don't really care... 17:16 <@sipa> _blame_rusty_russell_for_this_fucked_up_name 17:17 < rusty> SHIP IT! 17:23 <@sipa> i realize that we're having a conversion across 3 people, no two of which are less than 7.5 hour apart in terms of timezones 17:23 <@sipa> that's very near to the worst possible 17:23 <@gmaxwell> I am the sanely timed one! 17:23 < rusty> sipa: That implies one of us is sleep-typing, and won't remember in the morning... 17:24 <@gmaxwell> best way to write APIs. 17:24 <@gmaxwell> then you get the users' expirence in the morning. 17:24 < rusty> The purple grasshopper by my keyboard is certain it's not me. 17:25 <@sipa> clearly the purple grasshopper is the one that's out of place 17:25 <@sipa> and by general relativity, out of time 17:25 <@sipa> i think i may require sleep 17:25 <@gmaxwell> yea, your jokes get less funny when you're tired.:P 17:26 <@gmaxwell> sipa: does the BER spec have test vectors that are useful to us? 17:49 < rusty> BTW, with your justifcation given, I no longer propose a type for shnorr sigs. It was a once-off (conversion issue), and it's a very detectible error. 18:29 * rusty files a gcc bug, since I can't get gcc to compile-time detect bad object sizes for libsecp https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67793 18:30 <@sipa> i don't think that C has arrays as a type that can be passed 18:30 < rusty> sipa: naah, it works, equiv to ptr. 18:30 <@sipa> it collapses to a pointer 18:30 < rusty> sipa: yes, but if gcc were to intuit the size from that, it would be "great" as I said. 18:31 < rusty> sipa: I use that for documentation purposes sometimes. 18:31 <@sipa> sure 18:31 < rusty> Unf. it confuses newcomers sometimes, so it's not clear that it's better than "unsigned char *msg32". 18:31 <@sipa> but p is coverted to char* before passing to f 18:32 < rusty> But if gcc actually did something cool with it at compile time to detect errors, it would shift the balance. 18:32 <@sipa> right 18:33 < rusty> sipa: we already have __builtin_object_size(), so gcc knows this stuff. Unf, as I said, it doesn't give a compile time constant. 18:33 < rusty> sipa: I was thinking about proposing it for libsecp, but it's far less attractive if it's runtime only. 18:34 <@sipa> well for documentation purposes... perhaps we should declare all known-size array pointers to array declarationa 18:34 < rusty> sipa: naah, as I said, it confuses readers sometimes. They often naively think it makes a copy. 18:34 <@sipa> that seems likely 18:35 <@sipa> ok, i get what you were referring to now 18:36 < rusty> sipa: so, we could add #ifdef DEBUG (or even #ifndef NDEBUG) __builtin_object_size() wrappers without breaking the ABI later (since macros can have same name as functions, with some trickery), so I'll leave it alone for now. 18:38 < rusty> Hm,, actually, just a debug version of the library could do it. 18:42 <@sipa> such macros make it unsafe to have effectful expressions as arguments, though 18:42 < rusty> sipa: __builtin_object_size is magic no-side-effects, for exactly that reason (though it returns (size_t)-1 then) 18:44 < rusty> sipa: hey, got that low_s check for me? MEanwhile I have this: 18:45 < rusty> /* FIXME */ 18:45 < rusty> u8 der[72]; 18:45 < rusty> size_t len = 72; 18:45 < rusty> secp256k1_context* ctx = secp256k1_context_create(0); 18:45 < rusty> /* To have the high bit set in S, it would have to be encoded with 18:45 < rusty> * 0 in front. */ 18:45 < rusty> if (!secp256k1_ecdsa_signature_serialize_der(ctx, der, &len, &sig->sig) 18:45 < rusty> || !IsValidSignatureEncoding(der, len) 18:45 < rusty> || sig[sig[3] + 6] & 0x80) { 18:45 < rusty> secp256k1_context_destroy(ctx); 18:45 < rusty> return false; 18:45 < rusty> } 18:45 <@sipa> you need a slightly stronger check 18:46 <@sipa> the low-s requirement is that s is below n/2 18:46 <@sipa> and n is around 2^256 - 2^128 18:46 <@sipa> i'll PR some signature parsing changes tomorroe 18:48 < rusty> sipa: sure, but it's equiv to what I had before. The sig check might fail, but it can't be malleated AFAICT. 18:49 <@sipa> if s is below (p-order), it can 18:49 <@sipa> which is constructible using public key recovery 18:50 < rusty> sipa: oh :( Thanks. I will put a PR and wait for proper fix. 18:50 <@sipa> no randomly created signature will be malleable, though 18:50 <@sipa> where "no" is defined as "less likely than being struck by lightning 3 times in the next minute" 18:53 < rusty> sipa: Have to worry about malice. If you malleate such a sig, will the result be BIP62 compatible? 18:54 < rusty> sipa: (as lightning assumes BIP62) 18:54 <@sipa> no 18:54 < rusty> Actually, doesn't matter. You could screw the current code up by making me think you had given me a valid sig, but actually hadn't, and I'd lose. 18:55 <@sipa> bip62 has the correct low s specification 18:55 <@sipa> your implementation is just slightly too lax 18:56 < rusty> https://github.com/ElementsProject/lightning/issues/7 thanks 22:10 -!- rusty [~rusty@pdpc/supporter/bronze/rusty] has left #secp256k1 [] 23:22 -!- CodeShark_ [~CodeShark@cpe-76-167-237-202.san.res.rr.com] has joined #secp256k1