--- Day changed Sat Aug 11 2018 01:31 < dongcarl> Question about the thought process behind ChildNumber... Was the intention to encapsulate the ChildNumber _index_ as a u32 in the struct, which would be valid for 0..2^31-1 for _both_ Normal and Hardened, or was it to encapsulate the actual number? (0..2^31-1 for Normal, 2^31..2^32-1 for Hardened) I see that the Serialize implementation for ChildNumber seem to assume the former, and the docs before 01:32 < dongcarl> my changes seem to assume the former, but everything else seem to assume the latter? 02:10 < dongcarl> fmt::Display also seem to assume former... 13:22 < andytoshi> the intent was to encapsulate the index, but the internal representation isn't too important to that 13:22 < andytoshi> but it's clear that i got confused about what the internal representation was 13:23 < andytoshi> in particular the `Display` impl is just wrong .. (a) it's showing the wrong number for hardened keys, (b) it's using `h` instead of ' as a differentiator, i have no idea where i got that from 13:24 < andytoshi> BIP32 uses subscript-H some places, maybe that's what i was going for .. but BIP44 does the more common ' thing, which is what we should do 17:12 < dongcarl> andytoshi: So in that case, I think we should make both variants structs with one private field: index. That way we can enforce index being in 0..2^31-1, and we can have a method that converts to the full 0..2^32-1 (u32) by adding 2^31 to the hardened index? 17:12 < dongcarl> Private field so that people can't create an invalid ChildNumber 17:13 < dongcarl> e.g. both ChildNumber::Normal(2^31) and ChildNumber::Hardened(2^31) should be invalid 17:21 < andytoshi> yes please 17:21 < andytoshi> and i guess the `From` impl should assert! that its input is in range 17:23 < dongcarl> andytoshi: Hmmm... we can `impl From for Result` I believe 17:23 < dongcarl> Will PR 17:23 < andytoshi> that's really unergonomic 17:23 < andytoshi> generally rust just panics on range checks on integers 17:24 < andytoshi> in practice i expect people to communicate these using serde, which does have proper error handling ... the `From` impl are for programmers to use, and they can check preconditions in their own code 17:25 < dongcarl> andytoshi: Okay will do... Should I provide a ChildNumber::from_u32 that returns a Result? Or always panic? 17:26 < andytoshi> up to you 17:26 < andytoshi> i wouldn't, but i wouldn't complain if you did it 17:26 < dongcarl> Cool 17:37 < dongcarl> Just leaving this here: Actually, we don't need to panic at all. ChildNumber is an enum for Normal and Hardened, so if the u32 input is 2^31..2^32-1, we do hardened of input - 2^31, otherwise, do normal of input 17:38 < andytoshi> oh right, yeah 17:39 < andytoshi> perfect 18:00 < andytoshi> dongcarl: do you have your curren tversion of the `Descriptor` struct somewhere? 18:02 < dongcarl> andytoshi: Yeah gimme a sec to tidy it up 18:07 < dongcarl> andytoshi: Take a look: https://gist.github.com/dongcarl/8c7fcea174108d3a101ab0eb33179a05 18:09 < andytoshi> cool, looks great .. except, can you add `Multi` (multisig) and make `And` binary? 18:12 < dongcarl> andytoshi: Fo sho 18:12 < andytoshi> and i need to think about this KeyDescriptor trait... it's not really usable by a compiler right now because where is it supposed to get the evaluation parameters from? 18:12 < andytoshi> maybe there needs to be another intermediate type somewhere 18:13 < dongcarl> Yeah intermediary type seems to be the ansewr 18:15 < dongcarl> andytoshi: gist updated 18:16 < andytoshi> awesome, thanks 18:18 < dongcarl> andytoshi: lots of BIP32 stuff involved here... Will be making ChildNumber consistent first, then come back to the parsing/formatting of these structs. Lmk if you need anything. 18:18 < andytoshi> ok, sounds good 18:18 < andytoshi> i think i'm good .. i just wanted to be working from a Descriptor struct somewhat consistent with yours 18:19 < dongcarl> :thumbsup: 18:51 < dongcarl> Naming the constructor for ChildNumbers from their index ChildNumber::from_hardened_idx(index: u32) which will panic if index & (1 << 31) != 0 18:51 < dongcarl> A little verbose but... gets the message across 18:52 < andytoshi> i'm not sure there's a use for this function 18:52 < andytoshi> the ChildNumber should have a .is_hardened() method .. and anything that wants to panic on non-hardened keys can use that 18:54 < andytoshi> like, i can't see why anyone would use a function which is identical to From except that it panics 18:54 < dongcarl> andytoshi: so in tests I'll write a convienience function? We have tests like so: https://pastebin.com/6tJYAVNv 18:55 < andytoshi> oh i see 18:55 < andytoshi> it's not identical 18:55 < dongcarl> right 18:55 < andytoshi> yeah, i think that's good to have 18:55 < andytoshi> and a from_normal_idx as well i guess 18:55 < dongcarl> Yup, have both 18:55 < andytoshi> even though that really _is_ the same as from:: 18:56 < dongcarl> Haha yeah the normal one definitely is 18:59 < andytoshi> still struggling with the descriptor API ... i guess the compiler should somehow take a hashmap from keydescriptors to their indices 19:04 < dongcarl> andytoshi: Urgh... 19:04 < dongcarl> andytoshi: that sucks... 19:04 < dongcarl> but will probably work 19:05 < andytoshi> heh, well, it's hard to make it type-sensible 19:05 < andytoshi> and there's other weirdness, like the need to have secret keys available for paths that have hardened steps in them 19:09 < andytoshi> i think we should go back to only supporting one type of key in descriptors ... and the script compiler will only work on Descriptor ... and there will be another function instantiate(): Descriptor -> Descriptor 19:09 < andytoshi> and instantiate() will take a descriptor along with a hashmap of keys to auxilliary data 19:09 < andytoshi> i can code this up, gimme 15 19:09 < dongcarl> Does the key origin stuff not solve this? As in //xpub<...>/ 19:10 < dongcarl> andytoshi: cool lemme take a look 19:10 < andytoshi> dongcarl: that solves the specific problem of needing secret key data 19:10 < andytoshi> but none of the other problems about how to turn these things into secp256k1::PublicKeys 19:11 < dongcarl> right 19:17 < dongcarl> eh... from_hardened_idx is counted as dead code as it's only used in tests... do I annotate with allow? 19:17 < andytoshi> it should be pub 19:17 < dongcarl> andytoshi: right, my bad 19:31 < andytoshi> dongcarl: here is roughly what i'm thinking for the descriptor type https://0bin.net/paste/qjg-KITeAUbNuYHn#V03fXwVLeFqSjB2s+-K+NvAtFKjiQs7vfEi9d8CeVQl 19:31 < andytoshi> then i guess we also add a FromStr and Display bound on the public key trait ... though secp256k1::PublicKey doesn't actually support either of those 19:33 < andytoshi> dongcarl: so we'll still have your enum type .. but have it implement this trait 19:33 < andytoshi> rather than having the individual enum variants implement some trait 19:35 < andytoshi> oh, actually i think i want PublicKey::instantiate to take an Option<&Self::Aux> (which is what HashMap::get returns) and then it can decide whether missing aux data is a problem 19:46 < dongcarl> andytoshi: Love it 19:48 < andytoshi> it does mean no mixing keys, but i think that's fine. the idea is that a (non-instantiated) descriptor would only be used by a single wallet, and they can define their own key type if the Core stuff doesn't work for them 19:49 < dongcarl> andytoshi: no way to make it work with mixing keys? :-/ 19:51 < andytoshi> sure, just make an enum of all the key types that you want to support 19:51 < andytoshi> but no, if you mix keys then there's no sensible API for instantiation or for parsing 19:51 < dongcarl> andytoshi: Right, that makes sense 20:16 < dongcarl> I'm headed out but this is ready for review: https://github.com/rust-bitcoin/rust-bitcoin/pull/126 20:16 < dongcarl> ChildNumber consistency