--- Day changed Sat Nov 10 2018 00:00 < sgeisler> num-traits isn't nightly imo, I'm using it for rust-lightning-invoice which compiles on 1.14.0 00:01 < sgeisler> the std version of checked_pow is nightly, not the num version 00:03 < sgeisler> but I'm aware that we want to reduce our dependencies, so I think u128 (if available in 1.14.0) is the better solution if you just want tocombine sat+sub-sat amounts for easier serialization 00:03 < dongcarl> andytoshi: with https://github.com/rust-bitcoin/rust-secp256k1/pull/78 merged, we can do https://github.com/rust-bitcoin/rust-secp256k1/issues/76 right? 00:05 < stevenroose> sgeisler: it's more u64 btc * 10^11 msat 00:05 < stevenroose> but I guess that should still fit in u128, no? 00:08 < sgeisler> u64*u64 should always fit into u128 00:08 < sgeisler> 10**11 < 2**64-1 and (2**64-1) * (2**64-1) < (2**128-1), so yes, that should work 00:09 < sgeisler> (at least that's what python says :P) 00:21 < stevenroose> sgeisler: until the largest denom (10^8) and the smallers denom (picosatoshi? 10^-12) are too far apart :D what does 10**20 < 2**64-1 say? 00:21 < stevenroose> False :/ :D 00:22 < stevenroose> let's say picotoshis are too far apart 00:22 < stevenroose> in fact, a feature can be added like "bitint-amounts" that easily replaces Inner with another type 00:23 < sgeisler> picotoshis :D I like the name 00:24 < sgeisler> but I think we can actually go till attoshis with u64 00:24 < sgeisler> 10**18 < 2**64-1 00:26 < sgeisler> a good estimation is that 10**(3*n) < 2**(10*n) since 2**10 ~ 1000 00:26 < stevenroose> I know that one, that's what drives the whole MiB vs MB debate :D 00:27 < stevenroose> I got "an implementation of `std::cmp::PartialEq` might be missing for `util::amount::Amount`" 00:27 < stevenroose> While I do derive those traits :/ 00:27 < sgeisler> did the derive error before that? 00:28 < sgeisler> maybe you forgot to demand PartialEq for some T 00:28 < sgeisler> and this is just the last error you see 00:29 < stevenroose> ah nvm got it 00:30 < stevenroose> if Amount's generic param has a default, does impl Eq for Amount { impl for all generics, or just default? 00:30 < stevenroose> well, can test after I finish, though 00:32 <+andytoshi> dongcarl: yep 00:32 < sgeisler> I think only for the default, how would it know the type constraints otherwise? Why not just `impl Eq for Amount {`? 00:33 < sgeisler> That sould enable you to impl Eq for all interesting T (including the default if Default: Eq) 00:45 < stevenroose> sgeisler: https://github.com/rust-bitcoin/rust-bitcoin/pull/192 feedback please 00:45 < stevenroose> I didn't convert to u128 yet, not sure if it's needed like this, but probably better, yes. 00:46 < stevenroose> `impl Eq for Amount that's basically what I did, because Inner is fixed for now. with different Inners, that might be better 01:13 < stevenroose> sgeisler: I don't think Mul allows for two different types to be multiplied. Do you suggest just dropping Mul altogether? 01:14 < sgeisler> It does: pub trait Mul { you just have to set the RHS. 01:15 < stevenroose> ah yeah just noticed 01:15 < stevenroose> Does that mean that amount / 2 and amount / amount can both be supported? neat! 01:16 < sgeisler> I don't know, associated types forbid this, but I think here it's a standard type argument with default, so possibly? 01:18 < sgeisler> But I don't know how much pain this will cause: normally Rust forbids ambiguity/function overloading, so it might be possible that this will force you to always specify the trait that you want to use when calling a function defined by it. This would make the infix syntax useless. 01:20 < sgeisler> Just test if infix operators still work. 01:20 < stevenroose> sgeisler: can you try to change the Mul for numbers? I tried impl> ops::Mul for Amount

{ but can't get it to work 01:20 < sgeisler> ok, will do 01:23 < stevenroose> sgeisler: seems like the issue is that you can't "filter" on the output type 01:28 < stevenroose> at least it was a good exercise in Rust magic 01:32 < stevenroose> btw, I think we could have used PhantomData

as well instead of the actual empty structs 01:34 < sgeisler> Yes, of course, but my initial design inluded the sub satoshi amount in the precision struct to be able to express the full range of possible values. Your design trades that for memory efficiency. 01:34 < sgeisler> That's why I didn't use PhantomData 01:36 < stevenroose> Ah, I didn't get that, that's why I asked what the u32 was for. You intended Inner + subtoshi scaled? 01:41 < stevenroose> I'm still thinking it might be a good idea to drop the Bitcoin precision and add a Denomination enum for the formatting part, to avoid confusion 01:41 < stevenroose> I can see users using the .as_precision::() and always getting 0 01:42 < sgeisler> https://gist.github.com/sgeisler/901722f38612de688fabab1cece45473 01:42 < stevenroose> oh, did I ask div? sorry 01:42 < stevenroose> ah no I didn't 01:43 < stevenroose> I already did div 01:43 < stevenroose> Wasn't getting Mul in order 01:43 < sgeisler> ok, but Mul doesn't need multiple impls, doesn't it? 01:43 < sgeisler> then I didn't understand what you were trying to do 01:45 < sgeisler> Ah, you wanted to impl it for every possible multiplication partner of Inner? 01:46 < stevenroose> yeah like that 01:46 < stevenroose> but then Output doesn't want to be casted to i64 01:47 < stevenroose> what's your thought on Denomination vs Precision? 01:48 < sgeisler> It seems useful to be able to control the rendering independently from the inner representation. +1 for enum. 01:48 < stevenroose> I like how the generic precision param can easily be overcome by having a supercrate type alias. f.e .rust-lightning can alias `type Amount = bitcoin::Amount;` 01:49 < sgeisler> yes, that's quite elegant since lightning doesn't need the full range of possible amounts anyway. 01:52 < sgeisler> regardin Mul: I don't think R: ops::Mul is really necessary since the only type from std that fits this description if Inner=i64 would be i64 imo 01:52 < sgeisler> and then you wouldn't have the casting problem 02:09 < stevenroose> good, I'll do that 02:14 < dpc> What's the easiest way to convert `TxOut.script_pubkey` into `Address`? 02:15 < stevenroose> I don't think we can do PicoToshis in i64 02:15 < stevenroose> 21*10**8*10**12 < 2**63 == False 02:15 < stevenroose> ah wait that's even wrong 02:16 < stevenroose> Msat seems lowerst you can go if you want to be able to express the total supply 02:20 < stevenroose> dpc: hmm, from a script 02:20 < stevenroose> I think I have a method like that, wait 02:20 < stevenroose> https://gist.github.com/stevenroose/627db881e652179ac864c874df1254f8 02:21 < stevenroose> not trivial 02:21 < dpc> It seems I have to call each of the `is_x` and then corresponding `Address::from_*`? 02:21 < dpc> stevenroose: Thank you. Any reason why this can't be in the library? 02:35 < stevenroose> no idea, probably to vague 02:35 < stevenroose> andytoshi: ^ ?? 02:35 < stevenroose> sgeisler: I'm gonna leave it for today. Now that we have Denomination, I'm seriously thinking the Bitcoin Precision doesn't make sense and we should remove IntoAmount 02:36 < sgeisler> I'll have a look at it again later 02:37 < stevenroose> andytoshi: came across this, you might like it: https://github.com/iqlusioninc/crates/blob/master/gaunt/README.md 12:34 <+andytoshi> stevenroose: yeah, i think your giant address_from_script thing could go in rust-bitcoin. it'll require continual updates as we support more address types. 12:34 <+andytoshi> i wonder if it should support any segwit version 12:34 <+andytoshi> probably not 12:38 <+andytoshi> my concern is that having this function would mean every segwit upgrade is a breaking change because that function's behaviour could change 12:40 < stevenroose> andytoshi: yeah that kinda of problems is what I envisioned preventing it from going in :p 12:42 < stevenroose> We could have methods like `Script::to_p2pkh_address` that does that so that the function becomes just a series of checks and converters 12:42 < stevenroose> But that would make Script quite ugly 12:42 <+andytoshi> yeah, i don't really like Script knowing about Address 12:42 < stevenroose> me neither 12:42 < stevenroose> and Address::from_p2pkh_script() could also be a bridge too far 12:44 <+andytoshi> btw gaunt requires rustc 1.27 12:45 < stevenroose> but has no huge deps? :) 12:46 < stevenroose> What exactly ties you to older Rust versions? I mean it's not for inclusion in a lib here, just as an alternative implementer of a jsonrpc HttpRoundTripper 12:46 <+andytoshi> no.. but it does depend on `failure` and it does bump the compiler version a lot for fairly unnecessary reasons (e.g. using new apis in std::time) 12:46 < stevenroose> sgeisler: https://github.com/rust-lang/rfcs/issues/1374 12:46 < stevenroose> oh, hmm yeah I don't like failure that much 12:46 <+andytoshi> stevenroose: i'm concerned that they don't have any CI checking old compiler versions, it looks like they'll just randomly break builds 12:47 <+andytoshi> and historically it looks like that's exactly what they have done 18:59 < dpc> andytoshi: adress_from_script could designed in such a way that introducing new supported addresses would not break the API, no? 19:05 <+andytoshi> dpc: by adding real_address_from_script and real_real_address_from_script variants? 19:18 < dpc> I don't really understand what exactly is the problem. There's a function `fn s2a(&Script) -> Option

` and as you add more addresses support it doesn't change the signature, so I think it's not technically a breaking change as per semver. The behavior changes, but I think it's OK. Even if it is, it's for a good reason and will be well documented. And if you really, really want to preserve it's runtime behavior, you 19:18 < dpc> can add a counter argument `fn s2a(&Script, version: u64) -> Option
` and it's very easy to support new addresses if the user has bumped the `version`. 19:29 < dpc> andytoshi does this scheme work or am I missing something? 19:40 < dpc> Another aproach would be to take some form of supported addres types as a list. 20:30 < Terr> Hi, I'm using rust-secp256k1 and rust-bitcoin to sign a (regtest) transaction in this way: https://gist.github.com/rust-play/79218ac1b3b5961e3c754fe53016a395 20:31 < Terr> however, when I submit it to my node, it errors with: "mandatory-script-verify-flag-failed (Non-canonical DER signature) (code 16)". But I think I do have a canonicalized (normalized?) signature 20:31 < Terr> anyone have any idea what it could be? 20:39 < stevenroose> Terr: wait I did something similar recently 20:39 < Terr> ah, I forgot to mention, Script::verify() says the signature is okay (but according to the docs, it's possible that it's inaccurate when used for Bitcoin) 20:39 < Terr> ah :) 20:40 < stevenroose> I halfway stopped though so never verified, but what I did differently was I just used push_slice, without pushing the length 20:40 < stevenroose> let me check which one is correct 20:41 < stevenroose> yeah so I think you don't have to do the push_int for the length 20:41 < stevenroose> push_slice does that for you 20:41 < Terr> ah, you're right, push_slice() pushes an extra opcode 20:41 < Terr> cool, I'll remove it and try again 20:41 < stevenroose> that's one thing. strange that Script::verify work sthen :/ 20:42 < Terr> that seems to be the fix, thanks a lot :) 20:42 < Terr> I don't know why bitcoind gives this error 20:42 < Terr> I would say the signature is valid, but the script is wrong 20:45 < stevenroose> Terr: but it looks at the signature that is actually (your size byte || your_signature[..=len()-2]) right? so it's incorrect 20:47 < Terr> from bitcoind's perspective, yes, but Script::verify() / Secpk2561::verify() only checks the byte input (I think? this is not my area of expertise :P) 20:48 < Terr> I just find the contents of bitcoind's error message odd 20:51 < stevenroose> Terr: yeah it's just that it looks for your signature as the wrong set of bytes, so it will of course produce a failure when trying to interpret that as a signature 21:15 <+andytoshi> dpc: in practice if you change the behaviour that's a breaking change. i disagree that "adding new functionality" is a reason to change the function output for otherwise valid inputs without a major version rev 21:16 <+andytoshi> we could add a version to the function signature, i'd be ok with that 21:16 <+andytoshi> i think you're correct that semver puts basically no obligations on us here .. but to be nice i'd like to at least give a `max_segwit_version` parameter to the function 21:17 <+andytoshi> and warn in the docs that if a user gives a high max version with no current meaning, in future we reserve the right to restrict it 21:18 <+andytoshi> e.g. segwit version 0 outputs must have a program length of 32 or 20 bytes 21:30 < gmaxwell> Indeed, other SW versions will get length constrained when they are defined. 22:03 < dpc> andytoshi: One could panic on "versions" from the future. Or version could be an "open-enum" (with #[doc(hidden)] used to high the "unnable" variant. 22:04 <+andytoshi> neither is particularly ergonomic 22:04 < gmaxwell> It's an explicit violation of the segwit specs to refuse to handle future versions, generally. 22:06 < gmaxwell> as it was explicitly construted this way so intoducing a new version would not cause another four year long new-address-type deployment delay. 23:34 < dpc> More ergonomic than copy&pasting the code. :D 23:48 <+andytoshi> yes, i was being facetious when i suggested real_address_from_script :) 23:48 <+andytoshi> but i think the segwit versioning solution is fine 23:49 <+andytoshi> where by default we will parse segwit outputs of any version up to `max_version`, and any versions above 0 we'll just accept any program (well, iirc there is some maximum length) 23:49 <+andytoshi> and document that whenevr v1 comes out, we'll enforce whatever restrictions that requires, and so on