--- Log opened Thu Apr 11 00:00:41 2019
01:18 -!- ccdle12 [~ccdle12@223.197.137.165] has quit [Read error: Connection reset by peer]
01:18 -!- ccdle12 [~ccdle12@223.197.137.165] has joined #rust-bitcoin
01:29 -!- TamasBlummer1 [~Thunderbi@p200300DD670F9D436506B7C4D522E0CB.dip0.t-ipconnect.de] has joined #rust-bitcoin
01:31 -!- TamasBlummer [~Thunderbi@p200300DD670F9D486506B7C4D522E0CB.dip0.t-ipconnect.de] has quit [Ping timeout: 264 seconds]
01:31 -!- TamasBlummer1 is now known as TamasBlummer
01:33 -!- kcalvinalvin [01d1af64@gateway/web/freenode/ip.1.209.175.100] has joined #rust-bitcoin
01:37 -!- ccdle12 [~ccdle12@223.197.137.165] has quit [Ping timeout: 245 seconds]
01:38 -!- ccdle12 [~ccdle12@223.197.137.165] has joined #rust-bitcoin
02:24 -!- willpiers [~willpiers@38.75.231.30] has joined #rust-bitcoin
02:28 -!- willpiers [~willpiers@38.75.231.30] has quit [Ping timeout: 246 seconds]
02:33 -!- ccdle12 [~ccdle12@223.197.137.165] has quit [Remote host closed the connection]
02:37 < stevenroose> funny: (u64::max_value() as f64 + 1.0 > u64::max_value() as f64) == false
02:54 -!- kcalvinalvin [01d1af64@gateway/web/freenode/ip.1.209.175.100] has quit [Ping timeout: 256 seconds]
04:04 < stevenroose> I pushed a good update to the Amount PR. nickler I hope you can agree with the approach I took for the std::ops implementations. I made them panic-on-overflow, using the checked math methods and unwraping the result. Also added tests for this.
04:10 < stevenroose> https://github.com/rust-bitcoin/rust-bitcoin/pull/252/
05:12 -!- dpc [dpcmatrixo@gateway/shell/matrix.org/x-vedqvmmkxxympznl] has quit [Ping timeout: 264 seconds]
05:23 < stevenroose> dpc: your unit test in bitcoincore_rpc::client was never tested, you annotated it as #[cfg(tests)] instead of `test`. So not it complaints..
06:26 -!- willpiers [~willpiers@38.75.231.30] has joined #rust-bitcoin
06:31 -!- willpiers [~willpiers@38.75.231.30] has quit [Ping timeout: 264 seconds]
06:37 < nickler> panic on overflow sounds good to me, thanks!
07:32 -!- willpiers [~willpiers@38.75.231.30] has joined #rust-bitcoin
07:34 -!- willpiers [~willpiers@38.75.231.30] has quit [Remote host closed the connection]
08:26 -!- willpiers [~willpiers@107-1-237-234-ip-static.hfc.comcastbusiness.net] has joined #rust-bitcoin
09:32 -!- willpiers [~willpiers@107-1-237-234-ip-static.hfc.comcastbusiness.net] has quit [Remote host closed the connection]
09:43 -!- willpiers [~willpiers@107-1-237-234-ip-static.hfc.comcastbusiness.net] has joined #rust-bitcoin
10:11 -!- willpiers [~willpiers@107-1-237-234-ip-static.hfc.comcastbusiness.net] has quit [Remote host closed the connection]
10:33 -!- willpiers [~willpiers@107-1-237-234-ip-static.hfc.comcastbusiness.net] has joined #rust-bitcoin
10:58 -!- willpiers [~willpiers@107-1-237-234-ip-static.hfc.comcastbusiness.net] has quit [Remote host closed the connection]
11:00 -!- elichai2 [uid212594@gateway/web/irccloud.com/x-cuaqqjoxrlrhvpmj] has joined #rust-bitcoin
11:07 < elichai2> andytoshi: hey, I rebased and implementing, and I'm trying to decide if SerDeserialize deref into &[u8] too (to give it all the functions of slices)
11:09 -!- DeanGuss [~dean@gateway/tor-sasl/deanguss] has quit [Ping timeout: 256 seconds]
11:26 < andytoshi> elichai2: i think it should
11:27 < andytoshi> you mean SerSignature? :)
11:28 < elichai2> Yeah lol
11:29 < elichai2> andytoshi: about the safe implementation I think if we stick to bindings then we should use bindings all the way
11:31 < elichai2> (I'm thinking of these things right now because i'm implementing a pure rust secp256k1 (for learning reasons) and I just implemented DER https://github.com/elichai/ecc-secp256k1 (right now it uses GMP for big ints but i'll replace those with arrays), So if I think of anything else interesting i'll ping you :) )
11:32 < andytoshi> elichai2: kk, agreed, let's stick with bindings
11:32 < andytoshi> i'd only be okay with safe-reimplementing the serialization functionality
11:32 < andytoshi> (and even then, i probably shouldn't be, that stuff is consensus-critical for bitcoin
11:33 < elichai2> yeah the DER serialization is pretty simple, but I don't know about the mixes. it's really nice that it's straight forward the exact same stuff as in the bitcoin code
11:34 < elichai2> I agree. it's consensus critical, if a node will fail decoding a signature it won't accept the transaction
11:46 -!- willpiers [~willpiers@107-1-237-234-ip-static.hfc.comcastbusiness.net] has joined #rust-bitcoin
12:03 < andytoshi> stevenroose: what's going on with the `Amount` type PR? why does it bring in `serde_json` with the arbitrary_precision feature?
12:03 < andytoshi> does this PR supersede the serde_json PR?
12:05 < stevenroose> yeah, it's mentined in the description
12:05 < stevenroose> I could make it independent, I think
12:06 < stevenroose> but that would mean conflicts (since it supports optional serde_json as a feature too)
12:06 < stevenroose> you can see it's two commits
12:06 < stevenroose> first commit makes serde_json just a dev-dependency for testing (IIRC) and amount adds it as normal optional dep to implement IntoBtc for Value::Number
12:07 < stevenroose> IMO that impl is not amazingly useful, was just there in my initial effort to not break too much from the bitcoin-amount crate
12:07 < stevenroose> andytoshi: ^
12:08 < andytoshi> ok, so, what's the deal with arbitrary_precision? do we need it?
12:08 < stevenroose> Decimal needs it
12:09 < stevenroose> I guess once we have Amount, we can remove Decimal and then we can take away arbitrary_prevision
12:09 < andytoshi> yeah, i think we should do that
12:09 < stevenroose> basically serde_json::Number needs arbitrary precision to do the same as strason's number
12:09 < andytoshi> right
12:09 < stevenroose> Would you prefer me to include removing Decimal in the no-strason PR?
12:10 < andytoshi> yeah
12:10 < stevenroose> Or in the Amount PR as an extra commit?
12:10 < andytoshi> how does Amount avoid neededing arbitary_precision
12:10 < stevenroose> Amount doesn't use serde_json at all
12:10 < andytoshi> i'd prefer a series of commits which (1) removes decimal, (2) adds Amount, (3) adds serde_json
12:10 < andytoshi> does it impl Deserialize or Serialize?
12:10 < andytoshi> how?
12:11 < stevenroose> I mean it just has a conversion trait between serde_json::Number and Amount, so then I guess the user decides what precision he needs
12:11 < andytoshi> eek
12:13 < andytoshi> in https://github.com/rust-bitcoin/rust-bitcoin/pull/252/commits/7ff0572adb4b9649abef7fdfd9653edc7e437224#diff-e7171a58864e41655f995922df9ce86fR620
12:13 < stevenroose> as for serde Serialize and Deserialize, we're currently using floats, since they are fine for all values from 1 satoshi to 21 million bitcoin
12:13 < andytoshi> we deserialize as an f64
12:13 < andytoshi> uhh
12:14 < andytoshi> i'm sure we've seen misparsed numbers before doing that
12:14 < andytoshi> which is why strason was created in the first place
12:14 < stevenroose> Hmm, could re-implement that using serde_json::Number, but then we'd need arbitrary precision
12:15 < andytoshi> :(
12:15 < stevenroose> In fact
12:15 < stevenroose> I don't think we do
12:15 < stevenroose> It should be possible to get the underlying string value while deserializing, right?
12:16 < andytoshi> iirc it is not
12:16 < stevenroose> And then parse. I just think it's not as easy as deserializing into String because String will expect quotes.
12:16 < gwillen> hm, it does seem like doubles should be able to handle the full precision of bitcoin
12:16 < gwillen> now I'm curious why this failed before
12:16 < gwillen> doubles have 52 bits of mantissa according to https://stackoverflow.com/questions/1848700/biggest-integer-that-can-be-stored-in-a-double
12:16 < gmaxwell> it's common for random json code to accidentally convert to float.
12:16 -!- moneyball [sid299869@gateway/web/irccloud.com/x-pvwustdvbiexpqjb] has joined #rust-bitcoin
12:17 < andytoshi> gwillen: yes, the width of f64 is not the issue
12:17 < gwillen> hmm, where does the loss of precision happen then?>
12:17 < gmaxwell> so for example, when bitcoin core used to use some external json library, in one minor update they released they managed to pass all json numbers through float...
12:17 < andytoshi> "common parsers dividing by 10 and creating rounding errors that accumulate" is the issue
12:17 < gwillen> ahh, hrm
12:17 < gwillen> also, I guess inexact representation of base 10 numbers is still bad, even if technically it's recoverable with available precision
12:19 < andytoshi> https://github.com/serde-rs/json/blob/master/src/de.rs#L478 looks legit actually
12:19 < andytoshi> oh, maybe not, i'm looking at https://github.com/serde-rs/json/blob/master/src/de.rs#L744 and there are divisions..
12:21 < stevenroose> Yeah I'm looking into serde_json's internals. It should be possible to parse the "raw" json, right?
12:21 < andytoshi> maybe. it wasn't 3 years ago
12:21 < stevenroose> Like how a String is serialized to `"something"` and an u32 to `42` without the quotes
12:22 < andytoshi> stevenroose: gwillen: reading the existing serde_json code, it would surprise me if it misparses any bitcoin satoshi amounts. it does at most one division (by the appropriate power of 10) when parsing
12:23 < gwillen> are you talking about integer satoshi amounts, or floating bitcoin amounts?
12:23 < andytoshi> which may be wrong by up to episilon, which is much smaller than a satoshi
12:23 < stevenroose> floating bitcoin amounts
12:23 < andytoshi> gwillen: floating bitcoin amounts
12:23 < stevenroose> Core uses those
12:24 < gwillen> well, not _much_ smaller
12:24 < gwillen> it's like half a satoshi or something, around 21 million?
12:24 < gwillen> this is the calculation I was just doing about mantissa size
12:25 < gwillen> it's close enough to make me wonder if the selection of these parameters for bitcoin was related
12:25 < gwillen> I guess maybe I'm mixing up epsilon with ulps
12:26 < stevenroose> andytoshi: did you look into the arbitrary_prevision implementation?
12:26 < andytoshi> gwillen: it sure feels like it was related
12:26 < andytoshi> stevenroose: yeah that's fine, it just uses String
12:26 < stevenroose> It utterly confuses me, it seems like they are not evne producing json numbers
12:26 < andytoshi> but if dpc is correct that our use of arbitrary_precision fucks up serde_json for all downstream users
12:26 < andytoshi> that's insane but it means we can't use it
12:26 < stevenroose> It seems like they are serializing as a special struct with a key "$serde_json::private::Number"
12:26 < andytoshi> stevenroose: ;)
12:27 < andytoshi> this is a hack to signal the json de/serializer that it is de/serializing a json type, using only the serde API
12:27 < stevenroose> are you sure using a feature makes all users use the feature
12:27 < andytoshi> strason did the same thing
12:27 < andytoshi> stevenroose: no, i'm just believing dpc because it matches my preconceived bias against the cargo developers
12:27 < andytoshi> lemme look this up
12:27 < stevenroose> :'D
12:28 < stevenroose> serde should allow you to serialize into something raw
12:28 < stevenroose> like decide whether you want quotes or not
12:28 < andytoshi> you mean serde_json
12:28 < andytoshi> serde itself has no notion of quotes, or ascii or bytes
12:28 < stevenroose> what if you want to create a special boolean type that is ternary and has "maybe", you can't JSONify that then?
12:28 < andytoshi> correct
12:29 < stevenroose> no serde, it has no notion of that, but it has the Serializer trait that is limited in the types it can serialize
12:29 < stevenroose> it has things like serialize_string, serialize_xxx and serde_json just fulfills those (AFAIK)
12:29 < andytoshi> correct
12:29 < andytoshi> none of those are "raw"
12:30 < stevenroose> so there should be something like serialize_raw
12:30 < andytoshi> there is not
12:30 < stevenroose> or better
12:30 < stevenroose> serde_json should serialize_str as raw and implement Strings to serialize to a string that has the quotes
12:30 < andytoshi> they broke my hack for strason, invented a new one for serde_json, and did not provide any means to do this sanely
12:30 < andytoshi> yeah, sure, they "should" have done a lot of stuff :)
12:31 < andytoshi> i can't find anything about cargo features affecting downstream users btw
12:32 < andytoshi> anyway
12:33 < andytoshi> i think we can just use serde_json without arbitrary_precision and expect it to do the right thing. we'll add a fuzz test to find numbers it screws up on
12:33 < andytoshi> 2^-52 is less than half a satoshi. (it's about 46% of a satoshi)
12:34 < andytoshi> so a single division by 10^8 should not be able to screw us up
12:34 < stevenroose> so you prefer to use serde_json::Number as the intermediate type instead of f64?
12:35 < stevenroose> I'm gonna try reach out to the serde guys though. Their arbitrary precision feature is not really an arbitrary precision feature.
12:35 < andytoshi> ?
12:35 < andytoshi> i don't understand either of your comments
12:35 < stevenroose> Like JSON doesn't limit precision, so {"test": 0.000000000000000000000000012345} is valid JSON
12:36 < stevenroose> so an "arbitrary_precision" feature on a JSON crate would be able to parse that into a number with arbitrary precision
12:36 < andytoshi> (a) yes, we can use f64 as an intermediate type because serde_json will dtrt. there is no need to have an actual serde_json dependency anywhere
12:36 < andytoshi> (b) yes, and it does, by using String as the internal representation of Number
12:36 < stevenroose> not {"test": {"$serde_json::private::Number": "0.000000000000000000000000012345"}}
12:37 -!- willpiers [~willpiers@107-1-237-234-ip-static.hfc.comcastbusiness.net] has quit [Remote host closed the connection]
12:37 < stevenroose> but it looks like that is what a struct S { pub test: Number } will be serialized to
12:37 -!- willpiers [~willpiers@107-1-237-234-ip-static.hfc.comcastbusiness.net] has joined #rust-bitcoin
12:37 < andytoshi> that should only happen if you're using the serde_json serializer
12:37 < andytoshi> which recognizes that magic field name
12:37 < andytoshi> and does the right thing
12:37 < stevenroose> with arbitrary_precision, which is broken as hell :/
12:38 < andytoshi> ...
12:38 < andytoshi> i don't believe you can produce anything that will serialize the "$serde_json::private::Number" sting
12:38 < andytoshi> string
12:38 < andytoshi> first off, no actual struct field can have a $ sign in it. this is purely a sentinel value used internally by serde_json. and serde_json will recognize it and serialize a number
12:39 < stevenroose> Let me test this :) I also can't believe it. I just quickly went over the impl
12:39 < andytoshi> if you try to serialize a serde_json::Number with a *different* json serializer maybe you'll have trouble, but i'm pretty sure not, because presumably they check somehow what serializer is being used
12:39 < andytoshi> i looked at this when they implemented it
12:40 < andytoshi> to understand how they were avoiding unsafe code, and to understand how they were abusing the serde API to do things it can't do
12:40 < andytoshi> (why the fscking *serde* developers would invent hacks like this rather than fixing the API, i don't know..)
12:47 < stevenroose> hmm, ok so it can serialize ridiculously precise numbers
12:47 < stevenroose> without the ugly map
12:47 < stevenroose> so why can't we?
12:47 < stevenroose> *grabs shovel*
12:48 < andytoshi> strason did exactly this
12:48 < andytoshi> then it was broken by a serde upgrade
12:48 < andytoshi> i just had to replace my old hack with a different one
12:48 < andytoshi> but i didn't bother, since serde_json had it already
12:48 < andytoshi> the reason, BTW, is that serde_json controls both the serializer and the value type
12:48 < andytoshi> Amount is only a value type
12:49 < andytoshi> we *could* copy the magic serde_json sentinel code into Amount
12:49 < andytoshi> so that it'd dtrt when serialized with serde_json..
12:49 < andytoshi> but i'll bet our code will be broken in minor version upgrades if we do that, and we'll get no sympathy
12:50 < stevenroose> dtrt?
12:50 < andytoshi> do the right thing
12:50 < stevenroose> hmm, k, so we settle for float?
12:50 < andytoshi> yes
12:50 < stevenroose> In fact lightning folks are already all using ints in their APIs
12:50 < andytoshi> thank god
12:50 < stevenroose> gwillen, gmaxwell any chance Core would do that in a future major release too?
12:51 < stevenroose> It's super breaking, though
12:51 < andytoshi> it wouldn't be, it would just require a flag in all the decimal-outputting RPC calls
12:51 < stevenroose> In fact just adding a "{}_sat" field for all existing amount fields would be great
12:51 < andytoshi> you just missed the bitcoin core meeting though
12:52 < instagibbs> half of it was about rust in core :P
12:52 < stevenroose> ah IRC meeting?? I don't know when they are..
12:52 < instagibbs> another 8 minutes actually
12:52 < gwillen> what is the "that" here
12:52 < stevenroose> gwillen: move from btc to satoshi amounts
12:52 < andytoshi> gwillen: outputting integers rather than json-encoded floats for decimals
12:52 < gwillen> god, I wish it would
12:53 < gwillen> I have no real pull here but I would back you on this
12:53 < stevenroose> > In fact just adding a "{}_sat" field for all existing amount fields would be great
12:53 < stevenroose> That could be a quite simple but effective PR, wouldn't it?
12:53 < andytoshi> i assume PRs are welcome to add a non-breaking flag that enables this
12:53 < andytoshi> stevenroose: no, because half the things that matter don't output structs
12:53 < andytoshi> e.g. `getbalance`
12:53 < stevenroose> ah damn
12:53 < gmaxwell> stevenroose: that kind of change is super super super super breaking. (Also because json numbers always get handled as floats in pratically everything, it still doesn't fix the problem.)
12:54 < gmaxwell> stevenroose: not just super breaking but getting it wrong can cause funds loss.
12:54 < instagibbs> jtimon has tried that type of PR in the past
12:54 < instagibbs> and yes, getting it wrong becomes very bad
12:54 < BlueMatt> ariard: yea, something like that :p
12:54 < gmaxwell> stevenroose: so it would probably have to be coupled with a change that renamed every amount field. :(
12:54 < stevenroose> > Also because json numbers always get handled as floats in pratically everything, it still doesn't fix the problem.
12:54 < stevenroose> Thought that was mostly just Javascript
12:54 < instagibbs> you'd just hope you weren't rich enough to actually send 3,000 BTC when you wanted 3,000 sat
12:54 < gwillen> how bad would it be to turn getbalance into a struct-returning RPC (and whatever else), so you can add a _sat field to everything
12:55 < gwillen> so you never ever change the meaning of a numeric field
12:55 < gwillen> but you add a bunch, and also you (eventually, with deprecation) breakingly change some numeric fields to structs
12:55 < gmaxwell> stevenroose: no, virtually every json library everwhere uses floating point to handle numbers.
12:56 < stevenroose> is Rust really the exception then?
12:56 < andytoshi> probably yes.
12:56 < instagibbs> gwillen, in general im a big fan of returning structs because you can return additional stuff later in versioned steps
12:56 < gmaxwell> stevenroose: in bitcoin land we crated our own json library specifically designed to hand number serialization to the application (univalue), just to avoid this. (too bad the model hasn't been copied by other json libraries)
12:57 < gwillen> even in floating point I expect it's easier to safely work with integer values
12:57 < gmaxwell> stevenroose: rust is the only one I've seen with arbritary precision support  (there may be others, but they're rare at least)
12:57 < andytoshi> gmaxwell: we were doing this in rust-bitcoin. the maintenance burden of keeping up with generic serialization APIs was too much for us to maintain our own library
12:57 < gwillen> like, the maximum number of satoshis is exactly representable in a double, but the maximum number of bitcoin is not
12:57 < gmaxwell> gwillen: dunno, I think just the failures shift around.  our non-integer values should be totally safe in doubles, yet they're clearly not. :)
12:57 < gwillen> welll, they sort of should be
12:57 < stevenroose> gmaxwell: I'm not talking arbitrary precision here
12:57 < gwillen> but it requires you to argue that each double value _actually_ represents the closest decimal number
12:57 < stevenroose> just if a raw json number doesn't contain a dot, use int, otherwise float
12:57 < gwillen> rather than the number it appears to represent
12:58 < stevenroose> simple as that
12:59 < gwillen> also, with integer-doubles, there is _always_ a safe path of "parse, convert to integer, do math"
12:59 < gwillen> which is easy to prove does not lose precision
12:59 < gwillen> although I guess "parse, multiply by 10**8, convert to integer, do math" should be safe in the other case
12:59 < gwillen> but it may not be totally obvious that you need to do this
13:00 < gwillen> actually, to get MORE aggressive, you could just make all the _sat values be strings.
13:00 < gwillen> then even naive json parsers couldn't fuck them up.
13:01 < andytoshi> that would irritate the hell out of me FYI
13:01 < andytoshi> so many unnecessary allocations
13:02 < andytoshi> in practice the solution we've adopted at blockstream is to just implement deserialization code for all of the bitcoin structures, and if i'm forced to use RPC then i use the hex-outputting variants, and if i'm forced to check on things that require more context then we scan the whole chain
13:02 < gmaxwell> gwillen: strings were the original proposal for dealing with this years ago.
13:02 < andytoshi> which works fine, for everything except fee estimation
13:02 < andytoshi> but we don't care about much precision there anyway..
13:03 < gmaxwell> and probably the only safe way to use a 'typical' json library.
13:03 < gmaxwell> (because even though there exists a safe way to handle integers, things don't do that.)
13:04 < stevenroose> well, for Amount in the context of Core's API, I guess we can just assume floating point will do because that's apparently what literally all other libraries interacting with Core's RPC API do
13:04 < gmaxwell> stevenroose: lots of things then also get it wrong. :(  it really depends on what you're doing.
13:04 < gmaxwell> like an exchange losing a few satoshi here and there probably is essentially irrelevant.
13:04 < stevenroose> And I'll try bugging the serde guys to allow a way to de|serialize raw json
13:05 < andytoshi> we're trying to use software that will *only* switch to float if it sees a `.`, and which will probably do the right thnig even so
13:05 < gwillen> andytoshi: when you say unnecessary allocations, you mean for parsing strings in JSON instead of integers? That seems like a really weird concern but maybe I'm misunderstanding it. Doesn't parsing JSON already involve a boatload of allocations? Aren't you already allocating a string for the parsing anyway, that you can probably reuse for the final structure if you're not numericizing it?
13:05 < andytoshi> stevenroose: yeah, that'd be awesome
13:05 < gmaxwell> but some kind of consensus system bolted onto bitcoin (like blockstreams' liquid stuff) _cannot_ just randomly lose a few satoshi here and there.
13:05 < andytoshi> gwillen: i gtg. but no
13:06 < gwillen> well, please come back and explain later :-)
13:06 < gmaxwell> stevenroose: also, if the rust json stuff is returning a field that is varriant float/int that sounds ... really ugly for performance, like is it using (the equvilent) of a vtable for every access to that field?
13:06 < gwillen> I can't imagine parsing JSON not already involving a ton of allocs for all the maps/arrays/etc. in the output
13:07 < stevenroose> gmaxwell: what kind of access to that field?
13:07 < stevenroose> the serde_json::Number type is an enum (variant) float/int, yes
13:07 < gwillen> gmaxwell: I doubt most things that make extensive use of values that come from parsing RPC are leaving anything in the RPC-result datastructure
13:08 < gwillen> they probably grab what they need once and then dump it
13:08 < gmaxwell> (for bitcoiny use, I can't imagine the performance mattering, it was just a general question)
13:08 < stevenroose> unless you enable a feature called arbitrary_precision, then it's a string
13:08 < gwillen> I assume that RPC and parsing is slow enough that a single virtual call to access the result makes no difference
13:08 < stevenroose> gmaxwell: sure, but I don't see the performance issue.  even though honestly people don't user that type
13:08 < stevenroose> people will de|serialize int and float types
13:09 < stevenroose> and they skip using that number
13:09 < gmaxwell> like for example, if you read in data in your machine learning application or something and then procede to do a lot of calculation using data stored in json numbers, you're probably going to get a 100x slowdown compared to if you first copied all the data into a vector of doubles.
13:09 < stevenroose> yeah that's what you'd do, deserialize them as f64
13:09 < stevenroose> serde_json::Number doesn't evne implement arithmetics, IIRC
13:10 < stevenroose> the serde_json::Value type is like the "native" or "nameless" way to deserialize JSON
13:11 < stevenroose> but in almost all use cases, you will be using your own structs and the serialization of those structs and their fields don't use the serde_json::Value types like Number
13:22 < andytoshi> gmaxwell: no no vtable. yes there are hi perf json parses for specific applications
13:23 < andytoshi> gwillen: parsing numbers should involve no allocaton whatsoever
13:23 < andytoshi> i think parsing an array will alloc a vec yes but thats one allocation (plus reallocs i guess)
13:25 < stevenroose> andytoshi: I think I bumped into serde_json::RawValue (behind the raw_value feature).  It will probably use some for of (at least publicly supported) hack too.
13:25 < stevenroose> So the question becomes, are we implement "serde" serialization or "serde_json" serialization.
13:25 < stevenroose> Like if someone wants to use the serde impls not for json, they might have overhead
13:26 < stevenroose> (even though those people will never use the as_sat impl instead of as_btc, though
13:26 -!- willpiers [~willpiers@107-1-237-234-ip-static.hfc.comcastbusiness.net] has quit [Remote host closed the connection]
13:42 -!- willpiers [~willpiers@107-1-237-234-ip-static.hfc.comcastbusiness.net] has joined #rust-bitcoin
13:46 < andytoshi> lets just use f64
13:48 < gmaxwell> in any case, I think bitcoin core is probably more likely to stop using json (not likely...) than it is to switch amounts to satoshis.
14:03 < andytoshi> ok. good to know (and roughly what I expected)
14:05 < andytoshi> id love to see a switch from json..
14:23 < stevenroose> gRPC? :D
14:27 < andytoshi> gwillen: arrays and maps do require allocations, yes, as do strings. no other value types require allocations
14:28 < andytoshi> allocating a string during parsing numbers, just to throw it away upon conversion to a numeric type, would result in a huge perf hit for some applications
14:28 < andytoshi> actually, if you don't use the serde_json::Value type, there is no reason for maps to require allocations
14:28 < andytoshi> you can just parse directly into a structure on the stack
14:31 < andytoshi> elichai2: very interesting that we only need `std` for the `Error` trait
14:31 < andytoshi> in rust-secp
14:32 < andytoshi> that's pretty frustrating.
14:34 -!- DeanGuss [~dean@gateway/tor-sasl/deanguss] has joined #rust-bitcoin
14:37 < andytoshi> eh, well, googling around it seems like it's a pretty common thing
14:38 < elichai2> andytoshi: yep. I'm trying to decide if that trait really gives us anything big
14:39 < andytoshi> it allows embedding secp errors in other error types
14:39 < stevenroose> can't std be a feature?
14:39 < andytoshi> if we remove it it will break rust-bitcoin and all of its downstream users
14:39 < andytoshi> stevenroose: yes, and it is
14:39 < andytoshi> but why the hell do we need a feature for something so simple?
14:39 < elichai2> andytoshi: but somehow failure can do that without std
14:40 < andytoshi> can it? https://github.com/rust-lang-nursery/failure/issues/205
14:40 < elichai2> It's really frustrating
14:40 < andytoshi> looks like  failure feature-gates https://github.com/rust-lang-nursery/failure/blob/master/src/lib.rs#L272
14:41 < elichai2> Andytoshi, yeah. You can't use their Error type for auto conversion without the error trait
14:41 < elichai2> But I have a no-std library that is used by an std one and both have failure and the std one converts them nicely
14:41 < elichai2> Although maybe it's because they add another trait called Fail
14:42 < andytoshi> hmm interesting
14:49 -!- willpiers [~willpiers@107-1-237-234-ip-static.hfc.comcastbusiness.net] has quit [Remote host closed the connection]
14:59 -!- willpiers [~willpiers@107-1-237-234-ip-static.hfc.comcastbusiness.net] has joined #rust-bitcoin
15:16 < stevenroose> andytoshi: I think I did it :D
15:18 -!- DeanGuss [~dean@gateway/tor-sasl/deanguss] has quit [Remote host closed the connection]
15:19 < stevenroose> andytoshi: https://gist.github.com/stevenroose/f1896e989f30fdacd458d16be8b252d0 (can't link the line, search "pub mod as_btc")
15:19 -!- DeanGuss [~dean@gateway/tor-sasl/deanguss] has joined #rust-bitcoin
15:19 < stevenroose> It takes the raw_value feature for serde_json. But it basically does arbitrary precision without needing the arbitrary precision feature.
15:20 < stevenroose> Under the hood it also uses the strange map structure, but at least it doesn't infest our serde_json exports.
15:21 < stevenroose> Personally, if I document in that module that that serde implementation is only meant for JSON purposes, it could be useful.
15:21 < stevenroose> (Also see the precision test at the bottom.)
15:24 < stevenroose> Damn, instant regret.. I just realize this won't work with YAML or TOML, obviously.
15:24 -!- DeanGuss [~dean@gateway/tor-sasl/deanguss] has quit [Ping timeout: 256 seconds]
15:25 < stevenroose> Night digging wasted :D
15:42 < gwillen> andytoshi: are there any applications specifically parsing _bitcoin core RPC output_, though, for which putting a small number of strings on the heap would matter performance-wise
15:42 < gwillen> I'm not advocating replacing json numbers with strings generally
15:42 < gwillen> _just_ money values in bitcoin core RPC responses
15:43 < gwillen> I would expect the RPC roundtrip, the maps and arrays, and the other overhead to totally overwhelm the cost of a single heap allocation being made anf discarded in this case
15:44 < gwillen> and I think making it easy to write correct code is more important than a relatively small hit to performance _in this specific application_
15:44 -!- willpiers [~willpiers@107-1-237-234-ip-static.hfc.comcastbusiness.net] has quit [Remote host closed the connection]
15:45 -!- willpiers [~willpiers@107-1-237-234-ip-static.hfc.comcastbusiness.net] has joined #rust-bitcoin
15:46 -!- willpiers [~willpiers@107-1-237-234-ip-static.hfc.comcastbusiness.net] has quit [Remote host closed the connection]
15:46 -!- willpiers [~willpiers@107-1-237-234-ip-static.hfc.comcastbusiness.net] has joined #rust-bitcoin
15:47 -!- willpiers [~willpiers@107-1-237-234-ip-static.hfc.comcastbusiness.net] has quit [Remote host closed the connection]
15:51 < andytoshi> gwillen: before we replaced all the structured RPC calls in liquid with hex ones, it would have been a huge hit
15:52 < andytoshi> i don't have numbers, but extrapolating from the perf hit we had from similar pointless allocations in logs, an order of magnitude wouldn't surprise me
15:52 < andytoshi> in the initial block scan
15:53 < andytoshi> stevenroose: if it helps, i'm sure many person-years have been wasted on dealing with bitcoin jsonrpc
15:54 < gwillen> o_O
15:55 < andytoshi> i believe the problem is memory fragmentation
15:55 < gwillen> I find that shocking, is the overhead of an RPC really that low?
15:55 < gwillen> doesn't every RPC almost necessarily involve a bunch of allocations?
15:55 < andytoshi> no, the overhead of thrashing the hell out of your memory is extremely high
15:55 < andytoshi> no
15:55 < gwillen> at multiple layers of the stack?
15:55 < andytoshi> afaik if there are no strings or arrays, json should involve -no- allocations
15:56 < andytoshi> so it's just the ones that are in the network message buffering
15:56 < gwillen> (or are we talking about single RPCs returning a large number of numerical values, such that it's a large ratio of allocs per RPC?)
15:57 < andytoshi> i expect in common cases numeric values would be 100% of the allocations
16:03 < andytoshi> i think there's a reasonable argument to be made that in cases where performance matters, nobody would use json
16:03 < gwillen> well, 100% of the allocations that are not network message buffering
16:03 < gwillen> heh
16:03 < andytoshi> and given that, we might as well just treat it as a trash can
16:03 < andytoshi> i have a knee-jerk reaction against stringifying things, especially in rust
16:03 < gwillen> where are we on replacing JSONRPC with something sane
16:03 < andytoshi> afaik nobody has undertaken this
16:04 < andytoshi> liquid development has been a long process of reimplementing core functionality so that we don't have to talk to core as much
16:04 < gwillen> do you think a proposal to add protobuf or capnproto rpc to core would be met with acceptance
16:04 < andytoshi> i have no intuition about that
16:04 < andytoshi> but if i were to propose something, i'd propose adding wallet functionality to REST
16:04 < andytoshi> and fee estimation
16:06 < gwillen> REST?
16:06 < andytoshi> the core REST API
16:06 < andytoshi> https://github.com/bitcoin/bitcoin/blob/master/doc/REST-interface.md
16:06 < gwillen> I was hoping one could fix the JSON problem and the HTTP dependency in one shot
16:07 < andytoshi> yeah, that'd be nice
16:07 < gwillen> REST takes care of half that I guess
16:07 < andytoshi> istr a history of security issues with capnproto and/or protobuf?
16:07 < gwillen> uhhhh
16:07 < gwillen> I doubt that without links
16:07 < gwillen> I especially doubt that as to capnproto
16:07 < andytoshi> ok
16:07 < andytoshi> i'm probably wrong then
16:08 < andytoshi> oh lol, i'm probably thinking of zmq
16:08 < gwillen> hmm, found https://www.cvedetails.com/cve/CVE-2015-5237/
16:08 < gwillen> hah
16:09 < gwillen> only exploitable if you accept and successfully attempt to parse a single message over 4GB though
16:09 < andytoshi> i mean, regardless, if we alreday had protobuf and i were suggesting we add a *HTTP server*
16:09 < andytoshi> obviously security would not be a point of argument :)
16:09 < gwillen> oh, no, reverse that, it's only when you try to _serialize_ a single message over 4GB
16:09 < gwillen> on the sending side
16:09 < gwillen> also, hahaha, yeah
16:13 < andytoshi> my guess is, if someone were to propose a protobuf IPC, and wrote the code for it, it would be met by popular conceptual support, but a ton of bikeshedding, and tired core maintainers who were worried about maintaining it
16:16 < andytoshi> stevenroose: musing further on serde implementation of Amount
16:17 < andytoshi> if we want Amount to be the type of the `value` field in TxOut
16:17 < andytoshi> i really really don't want it going to/from f64 for de/serialization
16:17 < andytoshi> because then core rpc madness is infecting every single other format that rust-bitcoin interfaces with
16:21 -!- Netsplit *.net <-> *.split quits: gwillen, harding
16:22 < andytoshi> i see serde no longer provides any facility for determining whether you're using json, and special-casing that
16:22 < gmaxwell> protobuf is kinda awful in different ways, unfortunately.
16:22 < gmaxwell> including having ongoing maintaince problem.
16:28 -!- Netsplit over, joins: harding, gwillen
16:28 < andytoshi> conversely serde provides no way for a rust-bitcoincore-rpc json parser to tell if it's dealing with `Amount` rather than an ordinary numerical type :|
16:28 < andytoshi> i don't know how this framework became so pervasive given how irritating it is
16:30 < gmaxwell> json itself isn't actually terrible (in this respect, being not generally posible to round trip the serialization in a bit exact form, and other issues are ... other issues)
16:30 < gmaxwell> like the way a 'number' is defined in json is perfectly compatible with bitcoin's usage.
16:30 < gmaxwell> But javascript handles it awfully, and everything else inherits that awfulness (or worse)
16:32 < andytoshi> that's also my view. ultimately our #rust-bitcoin troubles come from serde (the de facto rust serialization framework) giving us no way to get the ASCII representation of numbers and make sure they're not munged
16:33 < andytoshi> instead we have to trust serde_json to do it, which runs it through the f64 floating-point type, and historically has corrupted bitcoin amounts doing this (though i think it won't now)
16:33 -!- willpiers [~willpiers@38.75.231.30] has joined #rust-bitcoin
16:33 < andytoshi> alternately we can write our own bitcoin-specific json interpreter, which we did do in the past, but it's a maintenance burder and seems likely to increase the bug rate
16:34 < andytoshi> because serde_json is really popular and gets a lot of use
16:37 < andytoshi> it's kinda tempting to do this again, being even more bitcoin-specific than before, becuase we could also get away with e.g. only supporting ASCII, which would let us drop all the utf dependencies
16:37 -!- willpiers [~willpiers@38.75.231.30] has quit [Ping timeout: 252 seconds]
16:38 < andytoshi> and we could stream hex encodings instead of going through Strings..
16:38 < gmaxwell> or convince serde_json to offer what you need?
16:39 < andytoshi> gmaxwell: well, they sorta did, behind the `arbitrary_precision` feature flag ... but we still need to go through some contortions to use it
16:40 < andytoshi> also dpc suggested that could make life harder for downstream users of the library who don't want that flag (but he's not online and we can't verify this)
16:41 < andytoshi> in fact i deprecated our special-purpose library exactly because serde_json added this flag
16:42 < andytoshi> another issue is that our de/serialization code should be general, but when we do stuff like this it forces us to add serde_json-specific code
16:42 < andytoshi> which adds an explicit serde_json dependency, increases coupling, and it's not clear what will happen if people use other serializers
16:50 -!- willpiers [~willpiers@38.75.231.30] has joined #rust-bitcoin
17:38 -!- DeanGuss [~dean@gateway/tor-sasl/deanguss] has joined #rust-bitcoin
17:38 -!- elichai2 [uid212594@gateway/web/irccloud.com/x-cuaqqjoxrlrhvpmj] has quit [Quit: Connection closed for inactivity]
18:28 < ariard> BlueMatt: kinda rewrite #334 and add unit test, sigs and time-lock delays sizes variations make things tricky so follow a expected weight calculation not actual
19:50 -!- willpiers [~willpiers@38.75.231.30] has quit [Remote host closed the connection]
19:51 -!- willpiers [~willpiers@38.75.231.30] has joined #rust-bitcoin
19:55 -!- willpiers [~willpiers@38.75.231.30] has quit [Ping timeout: 246 seconds]
19:56 -!- willpiers [~willpiers@38.75.231.30] has joined #rust-bitcoin
20:46 -!- willpiers [~willpiers@38.75.231.30] has quit [Remote host closed the connection]
20:47 -!- willpiers [~willpiers@38.75.231.30] has joined #rust-bitcoin
20:51 -!- willpiers [~willpiers@38.75.231.30] has quit [Ping timeout: 250 seconds]
21:33 -!- willpiers [~willpiers@38.75.231.30] has joined #rust-bitcoin
22:35 -!- willpiers [~willpiers@38.75.231.30] has quit [Remote host closed the connection]
23:14 -!- willpiers [~willpiers@38.75.231.30] has joined #rust-bitcoin
23:19 -!- willpiers [~willpiers@38.75.231.30] has quit [Ping timeout: 246 seconds]
--- Log closed Fri Apr 12 00:00:42 2019