1 2017-06-08 00:14:39	0|achow101|is there any particular reason to be gathering coverage data for leveldb?
  2 2017-06-08 00:14:55	0|achow101|cfields_: ^^ (you added it 4 years ago)
  3 2017-06-08 00:18:24	0|gmaxwell|achow101: when reporting you can easily filter it out by passing through lcov -r
  4 2017-06-08 00:18:54	0|gmaxwell|I think it might be of interest at some point to know figures for how much of leveldb that our tests cover.
  5 2017-06-08 00:19:33	0|gmaxwell|though kinda odd to do it by default
  6 2017-06-08 00:19:58	0|achow101|lcov -r is really slow, but I have a workaround for that :)
  7 2017-06-08 00:21:01	0|achow101|I don't think having it is necessary so we can just skip gathering data for it altogether
  8 2017-06-08 00:32:07	0|gmaxwell|faster computer. :P
  9 2017-06-08 00:32:48	0|gmaxwell|the way the lcov stuff is setup in our build seems kinda convoluted to me, but I'm not maintaining it.  I normally just do it with CFLAGS/CCFLAGS overriding and buildings.
 10 2017-06-08 00:34:22	0|sipa|gmaxwell: achow101 reimplemented lcov -r in python, making it run 60x faster
 11 2017-06-08 00:34:29	0|sipa|(20 minutes -> 20 sec)
 12 2017-06-08 00:36:39	0|gmaxwell|lol
 13 2017-06-08 00:40:44	0|bitcoin-git|[13bitcoin] 15jtimon closed pull request #9579: Net: Trivial-review: Make SendMessages easier to review (06master...060.15-split-sendmessages) 02https://github.com/bitcoin/bitcoin/pull/9579
 14 2017-06-08 00:42:54	0|jtimon|can https://travis-ci.org/bitcoin/bitcoin/jobs/239728492 be restarted ?
 15 2017-06-08 01:03:12	0|bitcoin-git|[13bitcoin] 15achow101 opened pull request #10552: [Test] Tests for zmqpubrawtx and zmqpubrawblock (06master...06zmq-raw-tests) 02https://github.com/bitcoin/bitcoin/pull/10552
 16 2017-06-08 03:24:40	0|Chris_Stewart_5|bitcoind in -regtest activates all soft forks (+ segwit) and enforces all Policy flags after generating ~500 blocks right?
 17 2017-06-08 06:46:26	0|bitcoin-git|[13bitcoin] 15fanquake closed pull request #10477: Use C++ initializer to initialze map and implement map comparator as const (06master...06master) 02https://github.com/bitcoin/bitcoin/pull/10477
 18 2017-06-08 06:59:59	0|kallewoof|fanquake: I think you got the @name wrong in the comment on #10477 btw. (cg31 opened, you said @benma)
 19 2017-06-08 07:00:01	0|gribble|https://github.com/bitcoin/bitcoin/issues/10477 | Use C++ initializer to initialze map and implement map comparator as const by cg31 · Pull Request #10477 · bitcoin/bitcoin · GitHub
 20 2017-06-08 07:10:01	0|wumpus|hhm thinking whether to add statistics such as number of incoming/outgoing connections, connections per bind point, to getnetworkinfo or just make a statistics script based on getpeerinfo on top for myself
 21 2017-06-08 07:10:25	0|wumpus|the # in/out connections is in the GUI, so in principle it's make sense to add it to RPC
 22 2017-06-08 07:18:37	0|fanquake|kallewoof oops, corrected.
 23 2017-06-08 07:43:32	0|bitcoin-git|13bitcoin/06master 140abc588 15practicalswift: [tests] Remove printf(...)
 24 2017-06-08 07:43:32	0|bitcoin-git|[13bitcoin] 15laanwj pushed 2 new commits to 06master: 02https://github.com/bitcoin/bitcoin/compare/e801084decf4...6c2d81f34dc4
 25 2017-06-08 07:43:33	0|bitcoin-git|13bitcoin/06master 146c2d81f 15Wladimir J. van der Laan: Merge #10524: [tests] Remove printf(...)...
 26 2017-06-08 07:44:07	0|bitcoin-git|[13bitcoin] 15laanwj closed pull request #10524: [tests] Remove printf(...) (06master...06u-for-unsigned-int) 02https://github.com/bitcoin/bitcoin/pull/10524
 27 2017-06-08 08:49:15	0|bitcoin-git|[13bitcoin] 15practicalswift opened pull request #10553: Simplify "bool x = y ? true : false". Remove unused function and trailing semicolon. (06master...06minor-cleanups) 02https://github.com/bitcoin/bitcoin/pull/10553
 28 2017-06-08 10:40:06	0|bitcoin-git|13bitcoin/06master 14227ae9b 15practicalswift: [tests] Use FastRandomContext instead of boost::random::{mt19937,uniform_int_distribution}
 29 2017-06-08 10:40:06	0|bitcoin-git|[13bitcoin] 15laanwj pushed 2 new commits to 06master: 02https://github.com/bitcoin/bitcoin/compare/6c2d81f34dc4...71ab6e553856
 30 2017-06-08 10:40:07	0|bitcoin-git|13bitcoin/06master 1471ab6e5 15Wladimir J. van der Laan: Merge #10547: [tests] Use FastRandomContext instead of boost::random::{mt19937,uniform_int_distribution}...
 31 2017-06-08 10:40:49	0|bitcoin-git|[13bitcoin] 15laanwj closed pull request #10547: [tests] Use FastRandomContext instead of boost::random::{mt19937,uniform_int_distribution} (06master...06remove-boost-random-dependency) 02https://github.com/bitcoin/bitcoin/pull/10547
 32 2017-06-08 10:45:58	0|bitcoin-git|13bitcoin/06master 14246a02f 15practicalswift: Use std::unordered_{map,set} (C++11) instead of boost::unordered_{map,set}
 33 2017-06-08 10:45:58	0|bitcoin-git|[13bitcoin] 15laanwj pushed 2 new commits to 06master: 02https://github.com/bitcoin/bitcoin/compare/71ab6e553856...35e7f13f68f9
 34 2017-06-08 10:45:59	0|bitcoin-git|13bitcoin/06master 1435e7f13 15Wladimir J. van der Laan: Merge #10548: Use std::unordered_{map,set} (C++11) instead of boost::unordered_{map,set}...
 35 2017-06-08 10:46:36	0|bitcoin-git|[13bitcoin] 15laanwj closed pull request #10548: Use std::unordered_{map,set} (C++11) instead of boost::unordered_{map,set} (06master...06unordered_map) 02https://github.com/bitcoin/bitcoin/pull/10548
 36 2017-06-08 11:37:09	0|bitcoin-git|[13bitcoin] 15laanwj pushed 3 new commits to 06master: 02https://github.com/bitcoin/bitcoin/compare/35e7f13f68f9...9c248e39f280
 37 2017-06-08 11:37:10	0|bitcoin-git|13bitcoin/06master 145b75c47 15Andrew Chow: Add a valid opcode sanity check to CScript...
 38 2017-06-08 11:37:10	0|bitcoin-git|13bitcoin/06master 14ac4e438 15Andrew Chow: Sanity check transaction scripts in DecodeHexTx...
 39 2017-06-08 11:37:11	0|bitcoin-git|13bitcoin/06master 149c248e3 15Wladimir J. van der Laan: Merge #10481: Decodehextx scripts sanity check...
 40 2017-06-08 11:37:39	0|bitcoin-git|[13bitcoin] 15laanwj closed pull request #10481: Decodehextx scripts sanity check  (06master...06decodehextx-sanity) 02https://github.com/bitcoin/bitcoin/pull/10481
 41 2017-06-08 13:22:12	0|bitcoin-git|[13bitcoin] 15somdoron opened pull request #10554: ZMQ: add publishers for wallet transactions. (06master...06zmq_wallet_tx) 02https://github.com/bitcoin/bitcoin/pull/10554
 42 2017-06-08 14:47:44	0|bitcoin-git|[13bitcoin] 15jnewbery opened pull request #10555: [tests] various improvements to zmq_test.py (06master...06zmqtestimprovements) 02https://github.com/bitcoin/bitcoin/pull/10555
 43 2017-06-08 15:04:47	0|bitcoin-git|[13bitcoin] 15jnewbery opened pull request #10556: Move stop/start functions from utils.py into BitcoinTestFramework (06master...06testframeworkstopstart) 02https://github.com/bitcoin/bitcoin/pull/10556
 44 2017-06-08 15:32:12	0|bitcoin-git|[13bitcoin] 15morcos opened pull request #10557: Make check to distinguish between orphan txs and old txs more efficient. (06master...06dontcheckoutputs) 02https://github.com/bitcoin/bitcoin/pull/10557
 45 2017-06-08 16:16:43	0|bitcoin-git|[13bitcoin] 15morcos opened pull request #10558: Address nits from per-utxo change (06master...0610195nits) 02https://github.com/bitcoin/bitcoin/pull/10558
 46 2017-06-08 16:22:45	0|jtimon|travis didn't run for https://github.com/bitcoin/bitcoin/pull/9176
 47 2017-06-08 17:57:57	0|bitcoin-git|[13bitcoin] 15achow101 closed pull request #9522: [RPC] Fix decoderawtransaction decoding of segwit txs (06master...06fix-decoderawtx) 02https://github.com/bitcoin/bitcoin/pull/9522
 48 2017-06-08 18:09:16	0|arubi|vindicated at last :) https://github.com/bitcoin/bitcoin/pull/8837#issuecomment-250542708
 49 2017-06-08 18:11:19	0|arubi|luke-jr made a comment about changing that 0x00 to 0xff.  in case segwit is retried, this is very good input imo..
 50 2017-06-08 18:13:41	0|sipa|yeah we should have used 0xff
 51 2017-06-08 18:13:55	0|sipa|or even better... we should never have used normal transaction encoding for partial transactions
 52 2017-06-08 18:15:08	0|arubi|right, I'd love for a blob of tx data to tell me what it wants to me to do with it
 53 2017-06-08 18:16:04	0|arubi|haha, make the wallet even fatter
 54 2017-06-08 18:16:16	0|Chris_Stewart_5|arubi: so some sort of marker that says 'needs signature'?
 55 2017-06-08 18:16:50	0|arubi|the scriptsig area itself can describe what it wants without hurting current layout
 56 2017-06-08 18:17:12	0|arubi|"oops, this is unsigned input" or maybe a prevtxid "oops, not funded..."
 57 2017-06-08 18:17:29	0|arubi|I mean, it depends at what stage it's parsed..  hard to tell for "decoderawtransaction" :\
 58 2017-06-08 18:27:06	0|bitcoin-git|13bitcoin/06master 143fb81a8 15practicalswift: Use list initialization (C++11) for maps/vectors instead of boost::assign::map_list_of/list_of
 59 2017-06-08 18:27:06	0|bitcoin-git|[13bitcoin] 15laanwj pushed 2 new commits to 06master: 02https://github.com/bitcoin/bitcoin/compare/9c248e39f280...29f80cd230c3
 60 2017-06-08 18:27:07	0|bitcoin-git|13bitcoin/06master 1429f80cd 15Wladimir J. van der Laan: Merge #10545: Use list initialization (C++11) for maps/vectors instead of boost::assign::map_list_of/list_of...
 61 2017-06-08 18:27:49	0|bitcoin-git|[13bitcoin] 15laanwj closed pull request #10545: Use list initialization (C++11) for maps/vectors instead of boost::assign::map_list_of/list_of (06master...06list_of) 02https://github.com/bitcoin/bitcoin/pull/10545
 62 2017-06-08 18:50:47	0|bitcoin-git|[13bitcoin] 15sdaftuar reopened pull request #10357: Allow setting nMinimumChainWork on command line (06master...062017-05-chainwork-commandline) 02https://github.com/bitcoin/bitcoin/pull/10357
 63 2017-06-08 18:50:51	0|gmaxwell|Meeting in ten minutes.
 64 2017-06-08 19:00:01	0|sipa|DOINK
 65 2017-06-08 19:00:04	0|lightningbot|Meeting started Thu Jun  8 19:00:04 2017 UTC.  The chair is wumpus. Information about MeetBot at http://wiki.debian.org/MeetBot.
 66 2017-06-08 19:00:04	0|wumpus|#startmeeting
 67 2017-06-08 19:00:05	0|lightningbot|Useful Commands: #action #agreed #help #info #idea #link #topic.
 68 2017-06-08 19:00:08	0|wumpus|#bitcoin-core-dev Meeting: wumpus sipa gmaxwell jonasschnelli morcos luke-jr btcdrak sdaftuar jtimon cfields petertodd kanzure bluematt instagibbs phantomcircuit codeshark michagogo marcofalke paveljanik NicolasDorier jl2012 instagibbs
 69 2017-06-08 19:00:11	0|jonasschnelli|hi
 70 2017-06-08 19:00:13	0|sdaftuar|hi
 71 2017-06-08 19:00:15	0|sipa|hi
 72 2017-06-08 19:00:16	0|achow101|hi
 73 2017-06-08 19:00:29	0|wumpus|proposed topics?
 74 2017-06-08 19:00:50	0|kanzure|hi.
 75 2017-06-08 19:00:51	0|sipa|UI interaction with pertxout upgrade?
 76 2017-06-08 19:00:58	0|gmaxwell|#bitcoin-core-dev Meeting: wumpus sipa gmaxwell jonasschnelli morcos luke-jr btcdrak sdaftuar jtimon cfields petertodd kanzure bluematt instagibbs phantomcircuit codeshark michagogo marcofalke paveljanik NicolasDorier
 77 2017-06-08 19:01:08	0|jonasschnelli|sipa: ack
 78 2017-06-08 19:01:33	0|wumpus|ok, let's do high priority for review first, then that
 79 2017-06-08 19:01:38	0|wumpus|#topic high priority for review
 80 2017-06-08 19:01:55	0|sipa|#10148
 81 2017-06-08 19:01:57	0|gribble|https://github.com/bitcoin/bitcoin/issues/10148 | Use non-atomic flushing with block replay by sipa · Pull Request #10148 · bitcoin/bitcoin · GitHub
 82 2017-06-08 19:01:59	0|luke-jr|I've rebased multiwallet etc
 83 2017-06-08 19:02:09	0|wumpus|luke-jr: great!
 84 2017-06-08 19:02:13	0|sipa|(^ will double our effectively available dbcache)
 85 2017-06-08 19:02:30	0|wumpus|luke-jr: sae there were some new review comments, did you address them yet?
 86 2017-06-08 19:03:01	0|luke-jr|wumpus: I believe all review comments are addressed or answered
 87 2017-06-08 19:03:13	0|wumpus|added 10148
 88 2017-06-08 19:03:16	0|sipa|thanks!
 89 2017-06-08 19:03:52	0|jtimon|still waiting on my current one, thanks wumpus for benchmarking
 90 2017-06-08 19:03:59	0|jonasschnelli|#8694 now passes gitian, will re-utack soon
 91 2017-06-08 19:04:02	0|gribble|https://github.com/bitcoin/bitcoin/issues/8694 | Basic multiwallet support by luke-jr · Pull Request #8694 · bitcoin/bitcoin · GitHub
 92 2017-06-08 19:04:14	0|wumpus|jtimon: a very nice gain!
 93 2017-06-08 19:04:39	0|luke-jr|oh, there was a comment asking for tests..
 94 2017-06-08 19:04:53	0|luke-jr|I didn't write any, although I agree they would be nice
 95 2017-06-08 19:04:56	0|wumpus|jtimon: unfortunately bitcoind nuked my log so I can't get the timings, but 26% savings in block hash operations is noce
 96 2017-06-08 19:05:07	0|jtimon|wumpus: nice
 97 2017-06-08 19:05:37	0|jtimon|we're talking about #10339 in case someone is lost
 98 2017-06-08 19:05:40	0|gribble|https://github.com/bitcoin/bitcoin/issues/10339 | Optimization: Calculate block hash less times by jtimon · Pull Request #10339 · bitcoin/bitcoin · GitHub
 99 2017-06-08 19:05:43	0|wumpus|luke-jr: well I'd say add those later, this is only the first in a series towards multiwallet after all
100 2017-06-08 19:06:03	0|luke-jr|indeed, it might not be possible to test right now since it's not exposed to RPC
101 2017-06-08 19:06:03	0|sipa|wumpus: status on account to label conversion?
102 2017-06-08 19:06:07	0|BlueMatt|wumpus: "in block hash operations" hmm? can you be more specific?
103 2017-06-08 19:06:08	0|gmaxwell|Related to hashing, does anyone here have AMD ryzen yet?
104 2017-06-08 19:06:12	0|BlueMatt|whats that compared to total runtime?
105 2017-06-08 19:06:33	0|wumpus|BlueMatt: I don't know about time, I just instrumented the block header hash function
106 2017-06-08 19:06:34	0|gmaxwell|BlueMatt: it's tiny vs total runtime, I assume-- but it should impact latency on block handling somewhat.
107 2017-06-08 19:06:39	0|luke-jr|gmaxwell: no, but I'd been holding out for SHA2 acceleration before upgrading, so I might get one if AMD is competing with Intel again
108 2017-06-08 19:06:56	0|sipa|related topic: sha2/crc32 hw accel?
109 2017-06-08 19:07:08	0|wumpus|as I said, I lost the timings (blame bitcoind nuking the log if it's too large)
110 2017-06-08 19:07:12	0|gmaxwell|luke-jr: Ryzen has the sha2 instructions... so I'm asking to find out if anyone will be able to test support for them. :)
111 2017-06-08 19:07:24	0|luke-jr|gmaxwell: right, that's why ;)
112 2017-06-08 19:07:49	0|luke-jr|I just haven't had time to investigate the pros/cons otherwise yet
113 2017-06-08 19:07:59	0|wumpus|sipa: no progress on that yet
114 2017-06-08 19:08:03	0|gmaxwell|In any case, 98% of the work needed to support sha2 instructions is the same as using SSE4 SHA2 which will itself be a non-trival speedup.
115 2017-06-08 19:08:30	0|wumpus|crc speedup should be possible after the leveldb upgrade that sipa PRed
116 2017-06-08 19:08:40	0|luke-jr|gmaxwell: we removed SSE4 stuff years ago, but I'm not sure if it was used for non-mining
117 2017-06-08 19:08:50	0|sipa|luke-jr: it was only used for mining at the time
118 2017-06-08 19:08:51	0|gmaxwell|BlueMatt: IIRC I instrumented and measured accepting a block at tip hashed the header ~6 times or so.
119 2017-06-08 19:08:54	0|gmaxwell|luke-jr: it was mining only.
120 2017-06-08 19:09:01	0|wumpus|luke-jr: that was a different one, the N-way-parallel
121 2017-06-08 19:09:12	0|gmaxwell|and it was a vector SHA2 that had to work on 4 elements at a time.
122 2017-06-08 19:09:12	0|sipa|yup 4-way parallel SSE2, iirc
123 2017-06-08 19:09:37	0|gmaxwell|What we should be implementing now is the one at a time SIMD sha2.  I believe matt uses it in fibre but without autodetection.
124 2017-06-08 19:09:37	0|jtimon|gmaxwell: that's before or after the patch?
125 2017-06-08 19:09:39	0|sipa|it seems we've strayed from the topic?
126 2017-06-08 19:09:42	0|BlueMatt|gmaxwell: yea, but if its not measurable on the total, unlikely to be worth the effort and complexity.....an alternative - do we have CBlockIndex*es in most of those places? pblockindex->GetBlockHash() is free and simpler than passing hashes around
127 2017-06-08 19:09:42	0|gmaxwell|jtimon: before.
128 2017-06-08 19:09:42	0|luke-jr|I wonder if it'd be worth using 4-way for merkle trees etc
129 2017-06-08 19:09:45	0|jonasschnelli|Will have access to a Ryzen in a couple of days via Hetzner
130 2017-06-08 19:10:38	0|morcos|Without doing a thorough review of 10339, is the speedup worth it as opposed to the tradeoff on making the code a little more complicated/involved.  Who was it that said the primary point of source code is to communicate between developers
131 2017-06-08 19:10:51	0|jonasschnelli|#10339
132 2017-06-08 19:10:54	0|gribble|https://github.com/bitcoin/bitcoin/issues/10339 | Optimization: Calculate block hash less times by jtimon · Pull Request #10339 · bitcoin/bitcoin · GitHub
133 2017-06-08 19:10:54	0|morcos|It just seems weird to pass a block and separately it's hash into a bunch of functions
134 2017-06-08 19:11:07	0|jtimon|BlueMatt: what's wrong with passing hashes around?
135 2017-06-08 19:11:26	0|BlueMatt|DoPartOfConnectBlock(42 args)
136 2017-06-08 19:11:28	0|wumpus|#topic Optimization: Calculate block hash less times by jtimon
137 2017-06-08 19:11:36	0|BlueMatt|we already have too many args in many of those functions
138 2017-06-08 19:11:39	0|instagibbs|"fewer" :P
139 2017-06-08 19:11:55	0|wumpus|well if hashing is a bottleneck, the obvious optimization is to just to it less
140 2017-06-08 19:11:57	0|sipa|instagibbs: i didn't want to say anything :)
141 2017-06-08 19:12:05	0|BlueMatt|wumpus: is it?
142 2017-06-08 19:12:09	0|wumpus|if hashing is not a bottleneck, then going after SSE and such makes no sense either
143 2017-06-08 19:12:26	0|sipa|BlueMatt: i believe that in all places where we already have CBlockIndex, no new block hashes are computed
144 2017-06-08 19:12:35	0|sipa|all of the cases in 10339 are before having a CBlockIndex
145 2017-06-08 19:12:40	0|wumpus|so if #10339 is not an improvement then we can leave hashing alone
146 2017-06-08 19:12:42	0|gribble|https://github.com/bitcoin/bitcoin/issues/10339 | Optimization: Calculate block hash less times by jtimon · Pull Request #10339 · bitcoin/bitcoin · GitHub
147 2017-06-08 19:12:45	0|gmaxwell|Well my suggestion on that hash thing was just to cache the hash in the block object, but Pieter said he prefered this.
148 2017-06-08 19:12:46	0|jtimon|instagibbs: calculate fewer times? thanks
149 2017-06-08 19:13:00	0|gmaxwell|(I also implemented that, though in a way that wasn't correct for the mining code.)
150 2017-06-08 19:13:07	0|BlueMatt|gmaxwell: I highly prefer that, though making CBlock const ala CTransaction is a bit more involved
151 2017-06-08 19:13:10	0|sipa|gmaxwell: if we create another CBlock for the cases where that's not needed
152 2017-06-08 19:13:18	0|sipa|(like GetTransaction and mining code)
153 2017-06-08 19:13:27	0|jtimon|Node also that we're sometimes getting the hash from the index and sometimes from the CBlock
154 2017-06-08 19:13:39	0|wumpus|right, calculating it eagerly can also result in overhead
155 2017-06-08 19:13:44	0|morcos|I'd just like to see some evidence, even a little bit, that this optimization is worth it.  26% fewer hashing operations results in what savings?  Or do people not agree it makes the code a bit more cumbersome
156 2017-06-08 19:14:07	0|sipa|i don't see much problem in passing a hash along with a block if you know it
157 2017-06-08 19:14:22	0|BlueMatt|passing in the hash as a part of ProcessNewBlock? yuck
158 2017-06-08 19:14:22	0|sipa|it's certainly a bit of complication, but a trivial one
159 2017-06-08 19:14:23	0|wumpus|morcos: sure, my point is just that if 26% fewer hashing operations isn't worth it, then using special intstructions to gain a few % is netiher worth it
160 2017-06-08 19:14:26	0|CodeShark|seems like there are probably lower hanging fruit in optimizations but meh
161 2017-06-08 19:14:28	0|BlueMatt|I just spent a bunch of time getting fewer args there
162 2017-06-08 19:14:46	0|morcos|wumpus: well hashing occurs a lot more often than in the calculations of these block hashes
163 2017-06-08 19:14:53	0|BlueMatt|to separate the consensus code from net_processing, now we're adding more coupling :(
164 2017-06-08 19:14:54	0|jtimon|it seems to me that the reserve is mostly a question of style
165 2017-06-08 19:14:55	0|gmaxwell|morcos: My reason for bringing it up is that I believe the repeated hashing is on the latency critical path for block propagation to the tune of maybe a millisecond-ish.
166 2017-06-08 19:15:09	0|jtimon|I mean calculating it eagerly can penalize in some cases, right
167 2017-06-08 19:15:23	0|gmaxwell|wumpus: these optimizations just prevent repeated hashing of the headers, in terms of total bytes hashed while syncing thats pretty small even though we do have a high repetition factor.
168 2017-06-08 19:15:36	0|gmaxwell|jtimon: what case would computing it early peanalize?
169 2017-06-08 19:15:38	0|CodeShark|BlueMatt: my preference is to sacrifice a little performance for better architecture ;)
170 2017-06-08 19:15:38	0|wumpus|gmaxwell: ok...
171 2017-06-08 19:15:42	0|sdaftuar|i will try to benchmark the effect on validation speed for this
172 2017-06-08 19:15:52	0|wumpus|CodeShark: I think that's a fair argument
173 2017-06-08 19:15:56	0|sipa|wumpus: transaction hashing accounts for far more than block header hashing, but not on the critical path
174 2017-06-08 19:16:02	0|jtimon|gmaxwell: when the validation fails for some other reason before you need the hash?
175 2017-06-08 19:16:12	0|BlueMatt|passing it in to ProcessNewBlock is.....ugh
176 2017-06-08 19:16:37	0|gmaxwell|jtimon: I don't think there is any validation failure path where we don't hash... after all, we'll want to know _what_ block hash failed to validate.
177 2017-06-08 19:16:39	0|jtimon|BlueMatt: you could have said that when each function had a separated commit, but I can leave that out
178 2017-06-08 19:16:47	0|BlueMatt|or, really, chnging the signatures of stuff in validation.h for this without a benchmark showing its a real win :(
179 2017-06-08 19:16:59	0|jtimon|gmaxwell: yeah, fair enough
180 2017-06-08 19:17:23	0|wumpus|ok, clear, better to close it I guess, would have made sense to have this discussion earlier
181 2017-06-08 19:17:25	0|jtimon|so we come back to only the "ugh" argument
182 2017-06-08 19:17:41	0|gmaxwell|But I still don't understand why sipa prefered to do this jtimon patch instead of making the object cache it.   FWIW, just monkey patching it 'works' and the only obvious issue is mining.
183 2017-06-08 19:17:57	0|sipa|gmaxwell: and every other read from disk
184 2017-06-08 19:18:47	0|jtimon|I preferred this because I think it's simpler than the cache, even if it's "ugh"
185 2017-06-08 19:18:50	0|gmaxwell|sipa: I don't understand your comment.
186 2017-06-08 19:18:55	0|sipa|i consider adding more arguments to validation-specific functions less invasively than changing a primitive datastructure
187 2017-06-08 19:18:58	0|sipa|we can do both, if needed
188 2017-06-08 19:19:17	0|wumpus|sipa: I tend to agree
189 2017-06-08 19:19:27	0|sipa|gmaxwell: i believe we often read blocks from disk without caring about its hash, because we already know it
190 2017-06-08 19:19:36	0|wumpus|sipa: just passing extra arguments is easier to reason about than extending primitive datastructures
191 2017-06-08 19:19:40	0|gmaxwell|sipa: the read funcion computes it!
192 2017-06-08 19:19:46	0|sipa|gmaxwell: ?
193 2017-06-08 19:20:02	0|sipa|currently, yes, as a checksum
194 2017-06-08 19:20:18	0|sipa|if we care about the checksum functionality, we should add a crc
195 2017-06-08 19:20:20	0|gmaxwell|// Check the header
196 2017-06-08 19:20:21	0|gmaxwell|if (!CheckProofOfWork(block.GetHash(), block.nBits, consensusParams))
197 2017-06-08 19:20:21	0|wumpus|jtimon: I agree, caching is always somewhat risky and error prone, this is more explicit and clear
198 2017-06-08 19:20:23	0|gmaxwell|return error("ReadBlockFromDisk: Errors in block header at %s", pos.ToString());
199 2017-06-08 19:20:49	0|wumpus|then again if it's not worth it, performanec wise, we shouldn't do it at all
200 2017-06-08 19:20:52	0|wumpus|not in another way either
201 2017-06-08 19:21:02	0|sipa|making the block hash part of CBlock makes us unable to skip computing that hash
202 2017-06-08 19:21:10	0|sipa|even in places where we don't care about it
203 2017-06-08 19:21:14	0|gmaxwell|I think I previously gave figures on the latency impact of caching.
204 2017-06-08 19:21:18	0|jtimon|how can I benchmark this to convince morcos?
205 2017-06-08 19:21:35	0|gmaxwell|sipa: Where are those places?
206 2017-06-08 19:21:35	0|wumpus|I don't know, I tried
207 2017-06-08 19:22:10	0|sipa|gmaxwell: every time we read a block from disk, we already have its hash (because that's how we look it up!)
208 2017-06-08 19:22:27	0|sdaftuar|sipa: don't we also hash all the transactions when we read a block from disk?
209 2017-06-08 19:22:28	0|sipa|gmaxwell: recomputing it there serves as nothing more than a checksum
210 2017-06-08 19:22:31	0|sdaftuar|surely that dominates all this
211 2017-06-08 19:22:35	0|morcos|sipa: i know i said i'd shut up, but you just read from DISK, who cares if you hash 80 bytes?
212 2017-06-08 19:22:42	0|wumpus|might be time for next topic, if this is not a perf issue, might be better to discuss it after meeting
213 2017-06-08 19:22:58	0|sipa|sdaftuar: good point.
214 2017-06-08 19:23:02	0|gmaxwell|sdaftuar: we don't. I used to think we did but we do not.
215 2017-06-08 19:23:15	0|sipa|yes, we do
216 2017-06-08 19:23:24	0|gmaxwell|sipa: where?
217 2017-06-08 19:23:33	0|sipa|deserializing a transaction computes its hash
218 2017-06-08 19:23:50	0|gmaxwell|oh right, what we don't do is validate the hashroot.
219 2017-06-08 19:23:54	0|sipa|let's discuss this after the meeting
220 2017-06-08 19:23:59	0|gmaxwell|k
221 2017-06-08 19:24:46	0|sipa|next topic?
222 2017-06-08 19:24:57	0|wumpus|#topic UI interaction with pertxout upgrade
223 2017-06-08 19:25:17	0|jtimon|in any case, BlueMatt morcos if you already knew you didn't liked this, I would have appreaciated knowing it earlier, maybe a provisional concept NACK or something
224 2017-06-08 19:25:45	0|sipa|so, since 10195, we'll have a possibly lengthy (minutes on decent hardware, possibly much longer elsewhere) upgrade process from the old per-txid utxo db to the per-txout one
225 2017-06-08 19:25:52	0|morcos|jtimon: yes sorry, i only looked at the PR during todays meeting, also why i said i'd stop arguing about it
226 2017-06-08 19:26:05	0|sipa|that needs some GUI interaction, i believe
227 2017-06-08 19:26:08	0|wumpus|jtimon: same here, this should have been clear sooner, otherwise I wouldn't have spent as much time on it
228 2017-06-08 19:26:20	0|sipa|or perhaps even in bitcoind
229 2017-06-08 19:26:37	0|morcos|wumpus: in general perf improvements shoudl include data in the PR request when possible I think
230 2017-06-08 19:26:41	0|wumpus|jtimon: it's kind of disappointing to discover this does what it says on the tin, save ~1/4th of block hash operations, just to discover that's not even important
231 2017-06-08 19:26:45	0|jonasschnelli|Can you not just use uiInterface.Progress() like in VerifyDB?
232 2017-06-08 19:26:46	0|wumpus|jtimon: that kind of annoys me
233 2017-06-08 19:26:58	0|sipa|jonasschnelli: that doesn't let you interrupt
234 2017-06-08 19:27:08	0|morcos|sipa: yeah i noticed that it doesn't even output to debug log that its doing an upgrade right?
235 2017-06-08 19:27:15	0|sipa|it should log
236 2017-06-08 19:27:19	0|sipa|"Upgrading database..."
237 2017-06-08 19:27:21	0|luke-jr|jonasschnelli: I'd think users should be able to send txs during the upgrade.
238 2017-06-08 19:27:22	0|morcos|oh
239 2017-06-08 19:27:41	0|jonasschnelli|luke-jr: but RPC is also not ready in this case? RIght?
240 2017-06-08 19:27:45	0|gmaxwell|morcos: it logs, but you won't see it if you have leveldb logging turned on. :P
241 2017-06-08 19:27:47	0|sipa|nothing works during the upgrade
242 2017-06-08 19:27:52	0|sipa|there is no full node available
243 2017-06-08 19:28:06	0|jonasschnelli|why would you then need interruption?
244 2017-06-08 19:28:11	0|wumpus|it should at least show a progress indicator and be interruptible indeed, ideally
245 2017-06-08 19:28:14	0|sipa|because maybe it takes too long
246 2017-06-08 19:28:20	0|sipa|and they want to continue another time
247 2017-06-08 19:28:37	0|wumpus|yes, because the user may want to do something else with the computer, having not expected to have to spend a long time upgrading
248 2017-06-08 19:28:45	0|gmaxwell|by interruption this means 'crash', not continue running without the upgrade.
249 2017-06-08 19:28:45	0|jtimon|anyway, I would still like to benchmark it if anyone can help me (not sure what to do)
250 2017-06-08 19:28:51	0|jonasschnelli|Ah.. the update can be done in multiple steps..
251 2017-06-08 19:28:58	0|jonasschnelli|well that would also need communication then
252 2017-06-08 19:28:58	0|sipa|yes
253 2017-06-08 19:29:00	0|wumpus|gmaxwell: well 'exit' not crash
254 2017-06-08 19:29:08	0|gmaxwell|same difference
255 2017-06-08 19:29:11	0|morcos|sipa: yeah i missed that somehow, sorry
256 2017-06-08 19:29:14	0|sipa|crashing "should" be fine
257 2017-06-08 19:29:30	0|gmaxwell|it better be! :) (also, I've tested it a fair bit)
258 2017-06-08 19:29:31	0|jonasschnelli|[Updating 0%] \n You can shutdown and continue later [Shutdown-Button] <-- something like that?
259 2017-06-08 19:29:37	0|wumpus|jtimon: benchmarking the reindex chainstate maybe?
260 2017-06-08 19:29:40	0|gmaxwell|jonasschnelli: like that.
261 2017-06-08 19:29:40	0|luke-jr|what if you crash, run the old version, and then the new one again?
262 2017-06-08 19:29:47	0|wumpus|jtimon: I still wonder why -stopatblock doesn't work
263 2017-06-08 19:30:18	0|gmaxwell|luke-jr: you get to keep both pieces? (I believe the old version will tell you the database is corrupt and stop there.... but I haven't tested that, so it's a good question.)
264 2017-06-08 19:30:21	0|sipa|wumpus: no; that's dominated by tx validation/deserialization... a good benchmark tests block validation given fully populated mempool and sigcache etc
265 2017-06-08 19:30:30	0|jonasschnelli|How about logging? Can we log similar to the VerifyDB [the non-newline % steps]?
266 2017-06-08 19:30:39	0|sipa|jonasschnelli: sure
267 2017-06-08 19:30:40	0|wumpus|sipa: ok...
268 2017-06-08 19:30:49	0|jonasschnelli|I can work on that. Seems to fit my skillset. :)
269 2017-06-08 19:30:51	0|jtimon|sipa: perhaps it could be as simple as using -upgradeutxodb=1 once or something of the sort?
270 2017-06-08 19:30:51	0|luke-jr|gmaxwell: IMO users may get tired of waiting and prefer to start the upgrade over in exchange for being able to use the old version in the meantime
271 2017-06-08 19:30:53	0|wumpus|sipa: good luck doing that in a deterministic way, though
272 2017-06-08 19:30:54	0|sipa|maybe VerifyDB or something like that can pass a bool saying "interruptible" ?
273 2017-06-08 19:31:05	0|gmaxwell|luke-jr: sounds like a case we should handle.
274 2017-06-08 19:31:05	0|sipa|jtimon: you can't not do it
275 2017-06-08 19:31:30	0|sipa|jtimon: i mean, -upgradeutxodb=0 would mean just exiting immediately :)
276 2017-06-08 19:31:38	0|wumpus|no, the upgrade needs to be done
277 2017-06-08 19:31:43	0|luke-jr|we could prompt before starting to warn the user, but IMO we'd probably want to begin ASAP, even if we hold off destroying backward compat until the user OKs it
278 2017-06-08 19:31:51	0|gmaxwell|meh.
279 2017-06-08 19:32:02	0|wumpus|sure, you could warn...
280 2017-06-08 19:32:07	0|gmaxwell|Keep in mind that on a reasonably fast computer the upgrade does not take long.
281 2017-06-08 19:32:08	0|jtimon|no, I mean once you set -upgradeutxodb=1 once it doesn't matter what you set anymore
282 2017-06-08 19:32:10	0|sipa|it's a new major release, i don't care about backward compatibility; the user can always reindex if they really need to
283 2017-06-08 19:32:13	0|morcos|so just to be clear, right now if someone went back to an old version, there is some chance they'd screw themselves right?
284 2017-06-08 19:32:24	0|morcos|(and need to reindex)
285 2017-06-08 19:32:33	0|wumpus|morcos: yes, it simply doesn't work. We should warn in the release notes.
286 2017-06-08 19:32:38	0|sipa|morcos: i believe that with very high probability the startup sanity check will fail
287 2017-06-08 19:32:39	0|luke-jr|pruned nodes cant reindex
288 2017-06-08 19:32:45	0|gmaxwell|morcos: old version rejects with a "your database is corrupted"  though luke was asking an interesting question about what happens if you do this mid-conversion.
289 2017-06-08 19:33:03	0|wumpus|we coudl have added logic to 0.14.2 to detect the new format and give a more specific error
290 2017-06-08 19:33:04	0|morcos|sipa: yeah that was my question, how far do you have to get along in the upgrade before your sanity check would fail with high probability
291 2017-06-08 19:33:07	0|gmaxwell|sipa: I did test that case, how could it not fail?
292 2017-06-08 19:33:26	0|sdaftuar|we only go back 6 blocks now in the startup sanity check i think
293 2017-06-08 19:33:27	0|luke-jr|wumpus: IMO it'd be better to destroy the upgrade so it starts over, and work (for incomplete upgrades)
294 2017-06-08 19:33:32	0|sdaftuar|isn't it possible you don't come across any bad entries?
295 2017-06-08 19:33:35	0|gmaxwell|wumpus: hm? I don't think that would be the right way, the right way would be to make the new version more incompatible.
296 2017-06-08 19:33:54	0|wumpus|gmaxwell: ok...
297 2017-06-08 19:34:03	0|sipa|there is a trivial change we can make that guarantees old versions will treat it as an empty db
298 2017-06-08 19:34:25	0|luke-jr|change the dir name?
299 2017-06-08 19:34:28	0|sipa|haha
300 2017-06-08 19:34:32	0|wumpus|gmaxwell: I just mean a specific error like 'the utxo database is in a newer format than supported by this version' would be clearer than a general sanity check error
301 2017-06-08 19:34:34	0|sipa|yes, i've considered that too
302 2017-06-08 19:34:35	0|morcos|empty db == corrupt and leave it so the new version can continue
303 2017-06-08 19:34:40	0|gmaxwell|no there is just the height index entry.
304 2017-06-08 19:34:40	0|morcos|wumpus +1
305 2017-06-08 19:34:46	0|gmaxwell|wumpus: oh, sure that would have been okay!
306 2017-06-08 19:34:46	0|wumpus|but I don't know ,sorry for suggesting it
307 2017-06-08 19:34:52	0|morcos|thats at least somethign we shoudl do  for future changes
308 2017-06-08 19:34:52	0|sipa|wumpus: unfortunately it doesn't have a version number
309 2017-06-08 19:34:55	0|jtimon|luke-jr: we could move the main datadir to ./bitcoin/main maybe
310 2017-06-08 19:34:56	0|sipa|agree
311 2017-06-08 19:34:58	0|luke-jr|using a new db would also mean users from pre-obfuscation get that enabled..
312 2017-06-08 19:35:10	0|sipa|a new db is a possibility
313 2017-06-08 19:35:19	0|sipa|though i'd prefer to not need 2x disk storage during the upgrade
314 2017-06-08 19:35:19	0|wumpus|sipa: we could have added one! but yes, too late.
315 2017-06-08 19:35:25	0|sipa|not too late
316 2017-06-08 19:35:28	0|morcos|sipa: what was the trivial change you were referring to
317 2017-06-08 19:35:45	0|sipa|morcos: change the record name for the 'best block hash' entry
318 2017-06-08 19:35:53	0|gmaxwell|the database stores a record to indicate the current tip; we can just change that.
319 2017-06-08 19:35:56	0|sipa|and set the old one to something invalid
320 2017-06-08 19:36:23	0|luke-jr|[19:35:19] <sipa> though i'd prefer to not need 2x disk storage during the upgrade <-- IMO unavoidable if the "user gets tired of waiting and runs old version" is desired to work okay
321 2017-06-08 19:36:33	0|gmaxwell|FWIW the non-atomic flushing change already changes those records around.
322 2017-06-08 19:36:39	0|sipa|yup
323 2017-06-08 19:36:47	0|sipa|well, in a backward compatible way
324 2017-06-08 19:36:48	0|gmaxwell|I don't think we should support user gets tired of waiting.
325 2017-06-08 19:36:59	0|gmaxwell|too high a cost for too low a benefit.
326 2017-06-08 19:37:00	0|sipa|it's even easier if it doesn't need to be backward compatible
327 2017-06-08 19:37:16	0|morcos|gmaxwell: yes we shouldn't support it, but it might be nice to support they tried to do it and didn't corrupt their database by trying so they now have to reindex
328 2017-06-08 19:37:42	0|gmaxwell|morcos: yes, in practice that is the case but it isn't guarenteed. We could make it guarenteed.
329 2017-06-08 19:37:45	0|wumpus|gmaxwell: well just exiting and continuing the upgrade later would be ok
330 2017-06-08 19:38:02	0|gmaxwell|wumpus: yea, that works except we don't have an exit button other than kill -9 :)
331 2017-06-08 19:38:04	0|sipa|wumpus: also, i don't understand why the CompactRange doesn't work for you
332 2017-06-08 19:38:22	0|sipa|if it's not guaranteed (or even not likely) to work, maybe the 2x disk storage is inevitable
333 2017-06-08 19:38:28	0|sipa|and we're better off explicitly creating a new db
334 2017-06-08 19:38:37	0|wumpus|sipa: I don't know either, could try it again if you think it's a fluke...
335 2017-06-08 19:38:49	0|sipa|i'll test it myself again too
336 2017-06-08 19:38:51	0|gmaxwell|I tested an earlier range compacting patch and it did work.
337 2017-06-08 19:39:01	0|gmaxwell|I'll also test.
338 2017-06-08 19:39:22	0|sipa|monitor disk usage during upgrade, with and without compactrange patch
339 2017-06-08 19:39:24	0|sipa|would be nice
340 2017-06-08 19:39:37	0|sipa|and maybe this discussion depends on the outcome there
341 2017-06-08 19:39:59	0|gmaxwell|if indeed compacting isn't reliable we should change to create a new database. IMO
342 2017-06-08 19:40:06	0|sipa|agree
343 2017-06-08 19:40:31	0|wumpus|it might have to do with my test framework, I'll copy the database directory next time instead of using my usual hardlink hack
344 2017-06-08 19:41:03	0|gmaxwell|(FWIW, the reason I checked compacting before is because I was going to suggest changing to a new database if it didn't work.)
345 2017-06-08 19:41:07	0|wumpus|other topics?
346 2017-06-08 19:41:27	0|sipa|crc32 leveldb 1.20?
347 2017-06-08 19:41:32	0|gmaxwell|^
348 2017-06-08 19:41:41	0|wumpus|#topic crc32 leveldb 1.20
349 2017-06-08 19:41:42	0|jtimon|gmaxwell: agreed on cost/benefit for supporting interruption, much easier to require confirmation with a warning "this may take a while and cannot be interrupted" or something
350 2017-06-08 19:42:07	0|gmaxwell|sipa: Why is travis failing?
351 2017-06-08 19:42:10	0|sipa|i personally really dislike the approach they're using (which seems to be the advised way even), which requires a separate object compiled with different flags
352 2017-06-08 19:42:26	0|sipa|gmaxwell: i assume incompatibility with our own build system integration of leveldb
353 2017-06-08 19:42:35	0|wumpus|compiling a separate object with special flags is  pretty much the standard way of doing it
354 2017-06-08 19:42:57	0|sipa|they're even abusing it... they're calling the new object without knowing that the CPU supports it
355 2017-06-08 19:43:26	0|wumpus|ok, that's not good
356 2017-06-08 19:43:28	0|gmaxwell|Compiling a new object with different flags is standard and correct,  calling it without knowing, is wrong.
357 2017-06-08 19:43:34	0|wumpus|yes
358 2017-06-08 19:43:46	0|wumpus|the detection function should not be in that object
359 2017-06-08 19:43:47	0|sipa|the approach in #10377 is so much easier
360 2017-06-08 19:43:48	0|gribble|https://github.com/bitcoin/bitcoin/issues/10377 | Use rdrand as entropy source on supported platforms by sipa · Pull Request #10377 · bitcoin/bitcoin · GitHub
361 2017-06-08 19:43:49	0|wumpus|simple as that
362 2017-06-08 19:44:35	0|wumpus|that only works because you don't spell out the instruction but put it in binary, if you spelled out the instruction it would have to be compiled with a special flag
363 2017-06-08 19:44:57	0|wumpus|you can't use e.g. sse4.2 instructions if the object isn't compiled with that
364 2017-06-08 19:45:00	0|gmaxwell|IIRC MSVC has banned inline ASM, if we don't care about MSVC then we're free to do other things.  However, you need to do the object thing if you want to use code that accesses simd via intrensics, for something like rdrand or clmul it's no big deal to use asm.
365 2017-06-08 19:45:08	0|sipa|ok ok
366 2017-06-08 19:45:37	0|wumpus|gmaxwell: yes, intrinsics are more compatible
367 2017-06-08 19:45:58	0|cfields_|here!
368 2017-06-08 19:46:32	0|gmaxwell|so we need to fix leveldb to now call into that object without having done the detection. Anything else needed there?
369 2017-06-08 19:46:42	0|wumpus|I don't think leveldb's way is wrong, except for putting the detection function in the instruction-set specific object
370 2017-06-08 19:47:15	0|BlueMatt|oh, i was supposed to mention cfields_ would be late
371 2017-06-08 19:47:25	0|cfields_|heh, thanks
372 2017-06-08 19:47:35	0|jtimon|maybe we can open an issue on their repo or something?
373 2017-06-08 19:47:38	0|BlueMatt|you're welcome :)
374 2017-06-08 19:47:48	0|sipa|jtimon: the issue is in our own build system integration, i'm sure
375 2017-06-08 19:47:51	0|sipa|not upstream
376 2017-06-08 19:48:06	0|sipa|oh, you mean for calling the machine specific object? yeah
377 2017-06-08 19:48:06	0|wumpus|wouldn't upstream have the same problem then?
378 2017-06-08 19:48:09	0|gmaxwell|jtimon might have been referring to the detection in the wrong object, thats just wrong.
379 2017-06-08 19:48:14	0|sipa|yeah, agree
380 2017-06-08 19:48:18	0|sipa|if they ever look at issues...
381 2017-06-08 19:48:19	0|jtimon|gmaxwell: sipa right
382 2017-06-08 19:48:22	0|gmaxwell|we should submit a fix, it should be trivial.
383 2017-06-08 19:48:28	0|cfields_|that's done already: https://github.com/theuni/bitcoin/commit/2cb7dda13884e44105f33c16e7e2c1a9aed46295
384 2017-06-08 19:48:33	0|wumpus|yes better to submit a fix, submitting an issue likely won't help
385 2017-06-08 19:48:36	0|cfields_|or are you guys talking about something else?
386 2017-06-08 19:48:36	0|sipa|cfields_: oh!
387 2017-06-08 19:48:57	0|sipa|probably not
388 2017-06-08 19:48:59	0|sipa|let's try
389 2017-06-08 19:49:09	0|gmaxwell|okay, we have actions here.
390 2017-06-08 19:49:34	0|wumpus|lol <long discussion> oh, cfields did it already
391 2017-06-08 19:49:38	0|cfields_|that enables the runtime detection and separate object, but iirc there's still a little generic code that may end up with the wrong instructions
392 2017-06-08 19:49:46	0|sipa|cfields_: yup...
393 2017-06-08 19:50:16	0|gmaxwell|that just needs to be moved into another file, I expect.
394 2017-06-08 19:50:22	0|sipa|yup
395 2017-06-08 19:50:26	0|sipa|let's do that independently
396 2017-06-08 19:50:29	0|wumpus|yes, move all the code out of it except for the function itself
397 2017-06-08 19:50:43	0|gmaxwell|should be a trivial patch we can take locally and send upstream.
398 2017-06-08 19:50:51	0|cfields_|heh, sorry I was late. Movers showed up at 3:00 exactly :\
399 2017-06-08 19:50:51	0|cfields_|i even linked it on the PR! :)
400 2017-06-08 19:51:20	0|gmaxwell|We'll need to do the same thing for SSE4 (and sha2 instruction) sha2.
401 2017-06-08 19:51:41	0|jtimon|cfields_: that doesn't seem related the meeting started at 21:00 :p
402 2017-06-08 19:52:00	0|wumpus|from what I remember that's actual assembly code from intel, in .s format, not using intrinsics
403 2017-06-08 19:52:07	0|gmaxwell|ah. okay!
404 2017-06-08 19:52:18	0|luke-jr|jtimon: 3:00 is meeting time for east USA
405 2017-06-08 19:52:30	0|jtimon|luke-jr: sure, bad joke, sorry
406 2017-06-08 19:52:39	0|cfields_|i suspect I missed this discussion too, but I have intrinsics done for sha2
407 2017-06-08 19:52:44	0|gmaxwell|I usually expect corporate code to be intrinsics because having good performance is not enterprise enough. :P
408 2017-06-08 19:52:56	0|wumpus|https://github.com/laanwj/bitcoin/commit/7e3e717892d96cae7f05dd1b6742425a298cc12e
409 2017-06-08 19:53:05	0|wumpus|it's even in yasm format
410 2017-06-08 19:53:20	0|cfields_|wumpus: ah, i'll shut up :)
411 2017-06-08 19:53:36	0|gmaxwell|I'm very exicited about getting the faster hash in, with all our other improvements, I'm now expecting a 8% IBD speedup.
412 2017-06-08 19:53:47	0|gmaxwell|if not more.
413 2017-06-08 19:53:56	0|wumpus|cfields_: you mean sha-specific instructions? the stuff I quote is from before that
414 2017-06-08 19:54:15	0|cfields_|wumpus: yea, intrinsics for intel sha1/2
415 2017-06-08 19:54:27	0|wumpus|it just implements a SHA using SSE4/AVX, so it works on older architectures..
416 2017-06-08 19:54:29	0|sipa|jtimon: ha, cool
417 2017-06-08 19:54:45	0|sipa|wumpus: no license issues?
418 2017-06-08 19:55:00	0|jtimon|sipa: not cool, I should have learned with the at & t notation, no?
419 2017-06-08 19:55:02	0|gmaxwell|sipa: it's permissively licensed IIRC.
420 2017-06-08 19:55:10	0|cfields_|wumpus: ah, nice. I suspect that's where the leveldb discussion came from. We can re-use the build-time logic for sure.
421 2017-06-08 19:55:10	0|wumpus|sipa: it's either MIT or BSD
422 2017-06-08 19:55:20	0|sipa|jtimon: just swap the argument order :)
423 2017-06-08 19:55:53	0|wumpus|spot-the-license https://github.com/laanwj/bitcoin/blob/7e3e717892d96cae7f05dd1b6742425a298cc12e/src/crypto/sha256_avx1.asm#L10
424 2017-06-08 19:55:56	0|gmaxwell|Before the metting closes, I'd like to remind people that Matt's caching change #10192 is a 31% block connection speedup.  It's done, rebased current, and needs review.
425 2017-06-08 19:56:00	0|gribble|https://github.com/bitcoin/bitcoin/issues/10192 | Cache full script execution results in addition to signatures by TheBlueMatt · Pull Request #10192 · bitcoin/bitcoin · GitHub
426 2017-06-08 19:56:24	0|jtimon|sipa: yeah, I think that's mostly it, and some minor things I think, but I was too lazzy to translate the programs I had
427 2017-06-08 19:56:24	0|wumpus|#topic cache full script execution
428 2017-06-08 19:57:42	0|gmaxwell|I don't know that there is too much to say about 10192. It saves a lot of operations when checking if an already validated tx is cached.  It's fairly straight forward.
429 2017-06-08 19:58:15	0|sipa|yeah, let's do it
430 2017-06-08 19:58:28	0|wumpus|seems it had a lot of review, no (ut)ACKs though
431 2017-06-08 19:58:58	0|gmaxwell|I'm working on an ACK now. Just want other people to look, it sat for a long time needing rebase.
432 2017-06-08 19:59:16	0|wumpus|right
433 2017-06-08 19:59:19	0|morcos|on the list
434 2017-06-08 19:59:21	0|sdaftuar|+1
435 2017-06-08 19:59:49	0|wumpus|it's already on the high priority for review list
436 2017-06-08 19:59:58	0|gmaxwell|The non-atomic flush stuff is also still outstanding, but I think it might get slightly changed based on todays per txo discussion.
437 2017-06-08 19:59:59	0|sipa|cfields_: yours commit (after trivial cherry pick) works
438 2017-06-08 20:00:33	0|wumpus|#endmeeting
439 2017-06-08 20:00:34	0|lightningbot|Log:            http://www.erisian.com.au/meetbot/bitcoin-core-dev/2017/bitcoin-core-dev.2017-06-08-19.00.log.html
440 2017-06-08 20:00:34	0|lightningbot|Meeting ended Thu Jun  8 20:00:33 2017 UTC.  Information about MeetBot at http://wiki.debian.org/MeetBot . (v 0.1.4)
441 2017-06-08 20:00:34	0|lightningbot|Minutes:        http://www.erisian.com.au/meetbot/bitcoin-core-dev/2017/bitcoin-core-dev.2017-06-08-19.00.html
442 2017-06-08 20:00:34	0|lightningbot|Minutes (text): http://www.erisian.com.au/meetbot/bitcoin-core-dev/2017/bitcoin-core-dev.2017-06-08-19.00.txt
443 2017-06-08 20:00:37	0|gmaxwell|wumpus: thanks!
444 2017-06-08 20:01:28	0|luke-jr|#10512's travis failure is being weird :/
445 2017-06-08 20:01:29	0|gribble|https://github.com/bitcoin/bitcoin/issues/10512 | Rework same-chain from abusing DoS banning, to explicit checks by luke-jr · Pull Request #10512 · bitcoin/bitcoin · GitHub
446 2017-06-08 20:01:42	0|luke-jr|it seems like adding debugging logging is fixing the test
447 2017-06-08 20:02:01	0|sipa|ah, a heisenbu
448 2017-06-08 20:02:11	0|sipa|+g
449 2017-06-08 20:02:22	0|kanzure|heisentypu
450 2017-06-08 20:02:24	0|gmaxwell|aside, I have a fair amount of distaste for intrensics; they're slightly more portable but for a long time they had poor performance because they left register allocation up to the compliler and it usually did it wrong.    But newer compilers have been much better about register allocation around simd code.
451 2017-06-08 20:02:25	0|cfields_|luke-jr: double the debug logging, close as fixed.
452 2017-06-08 20:04:37	0|luke-jr|oh wait, I'm wrong. *rubs eyes*
453 2017-06-08 20:04:42	0|luke-jr|forgot I had started a bisect last night
454 2017-06-08 20:04:51	0|jtimon|I have the feeling that some who didn't like #10339 may not like #8498 either for similar reasons
455 2017-06-08 20:04:53	0|gribble|https://github.com/bitcoin/bitcoin/issues/10339 | Optimization: Calculate block hash less times by jtimon · Pull Request #10339 · bitcoin/bitcoin · GitHub
456 2017-06-08 20:04:55	0|gribble|https://github.com/bitcoin/bitcoin/issues/8498 | Optimization: Minimize the number of times it is checked that no money... by jtimon · Pull Request #8498 · bitcoin/bitcoin · GitHub
457 2017-06-08 20:04:55	0|wumpus|for ARM32, intrinsics were pretty bad, apparently there were some changes in ARM64 arch making them better suited for optimization by compilers
458 2017-06-08 20:05:24	0|jtimon|BlueMatt: morcos ^^
459 2017-06-08 20:05:53	0|gmaxwell|Sorry about earlier with jtimon caching patch,  I thought I had expressed the view that I thought the change was adding complexity-- on reread of my comment I can see that my effort to be more constructive (in the form of offering "why not just cache") backfired.
460 2017-06-08 20:06:12	0|gmaxwell|I don't really think the complexity it adds is that big a deal.
461 2017-06-08 20:06:39	0|wumpus|but if reducing the number of header hashing operations isn't worth it in the bigger scheme of things, it's a boondoggle
462 2017-06-08 20:06:50	0|jtimon|gmaxwell: I think you were clear: you preferred a cache, but since there were more comments and you didn't say anything else, I assumed it wasn't a strong preference
463 2017-06-08 20:06:50	0|sipa|wumpus: i'm not convinced it's not worth it
464 2017-06-08 20:07:08	0|cfields_|sipa: i can patch up leveldb to move all ops to that file, if you're not already working on it
465 2017-06-08 20:07:41	0|jtimon|is there a benchmarking tutorial somewhere ?
466 2017-06-08 20:07:42	0|sipa|wumpus: but it will (if at all) only be relevant for blocks with pre-cached utxos, pre-checked sigs, already loaded...
467 2017-06-08 20:07:57	0|sipa|which is what matters for relay, not reindex
468 2017-06-08 20:08:26	0|wumpus|sipa: well for what it's worth it also helped with the number of hashes in reindex
469 2017-06-08 20:08:53	0|sipa|wumpus: i very much doubt it will be significant if you could all SHA256 operations (as opposed to just block header hashes)
470 2017-06-08 20:08:58	0|sipa|*count
471 2017-06-08 20:08:59	0|jtimon|yeah, it should reduce the number of hashes when validating a block
472 2017-06-08 20:09:08	0|jnewbery|luke-jr: I've noticed an issue with zmq_test.py not exiting correctly. #10555 fixes that and also adds a timeout for individual tests (which means travis should show more useful output if one of the tests hangs). Try rebasing your PR on that and see if it helps.
473 2017-06-08 20:09:09	0|gribble|https://github.com/bitcoin/bitcoin/issues/10555 | [tests] various improvements to zmq_test.py by jnewbery · Pull Request #10555 · bitcoin/bitcoin · GitHub
474 2017-06-08 20:09:20	0|wumpus|well the point of the pull was to reduce header hashes, so that's what I counted
475 2017-06-08 20:09:48	0|morcos|jtimon: I
476 2017-06-08 20:09:49	0|sipa|i'm sure it reduces header hashes (and thanks for the concrete evidence for that)... but there are probably 2 orders of magnitude more hashes in computing merkle trees
477 2017-06-08 20:09:50	0|wumpus|but I don't care, won't spend any more time on it at least
478 2017-06-08 20:10:10	0|sipa|:)
479 2017-06-08 20:10:22	0|sipa|cfields_: that should go in our leveldb repo then
480 2017-06-08 20:10:26	0|sipa|... or upstream
481 2017-06-08 20:10:32	0|morcos|jtimon: I'd need to look at 8498 more closely, but that one doesn't not seem to me to add that much complication.. it might be a nicer layout..  but i need to spend more time thinking it through
482 2017-06-08 20:10:43	0|cfields_|sipa: sure, first one, then the other
483 2017-06-08 20:11:03	0|gmaxwell|jtimon: it wasn't a strong preference, I'm okay with that PR if pieter is okay with it.  Though morcos seems to have stronger views.
484 2017-06-08 20:11:04	0|jtimon|morcos: thank you for the fast look, that already helps
485 2017-06-08 20:12:18	0|jtimon|gmaxwell: yeah, that's why I should learn to benchmark these things, I'm sure it isn't hard and it's just that I've never tried
486 2017-06-08 20:12:20	0|morcos|like i said, i personally don't love the direction that 10339 goes, but if other people have already done the review and like it, i don't think its worth further debate to stop it
487 2017-06-08 20:13:12	0|morcos|it seems like premature optimization that makes the code less easy to use to me, but not something terrible
488 2017-06-08 20:13:34	0|sipa|right - perhaps there is no noticable performance gain, in which case it's obviously not worth it
489 2017-06-08 20:14:08	0|sipa|i soft of assumed there would be, and if that's the case, i think passing along a hash of an object with the object itself is a very reasonable thing to do
490 2017-06-08 20:14:19	0|sipa|but perhaps all of this is premature
491 2017-06-08 20:14:52	0|wumpus|right
492 2017-06-08 20:15:00	0|wumpus|it's all up to finding a way to benchmark it then
493 2017-06-08 20:15:26	0|sipa|i believe we should perhaps postpone this until after #10192
494 2017-06-08 20:15:29	0|gribble|https://github.com/bitcoin/bitcoin/issues/10192 | Cache full script execution results in addition to signatures by TheBlueMatt · Pull Request #10192 · bitcoin/bitcoin · GitHub
495 2017-06-08 20:15:38	0|sipa|and then use the benchmark strategy that BlueMatt used there
496 2017-06-08 20:16:24	0|sipa|or use some replay that feeds transactions and blocks into bitcoind over p2p...
497 2017-06-08 20:18:21	0|jtimon|I'll go look that benchmarking stratedy
498 2017-06-08 20:25:33	0|gmaxwell|I think there is a simpler way to argue the performance: benchmark hashing a header, and then count how many hashes the change saves.  Total saved time is no more than the product.
499 2017-06-08 20:25:52	0|wumpus|the playback would work using morcos's data
500 2017-06-08 20:26:23	0|jtimon|so how did you counted it was about 6 ?
501 2017-06-08 20:27:57	0|wumpus|jtimon: https://github.com/laanwj/bitcoin/commit/fcfd20ea01f413dd2aa19b0013fe2b7aa2e47ad8
502 2017-06-08 20:45:56	0|jtimon|wumpus: thanks
503 2017-06-08 21:05:34	0|bitcoin-git|[13bitcoin] 15morcos opened pull request #10559: Change semantics of HaveCoinInCache to match HaveCoin (06master...06haveunspentincache) 02https://github.com/bitcoin/bitcoin/pull/10559
504 2017-06-08 21:26:34	0|gmaxwell|Am I alone in thinking we should have more comments in the code and commit message vs PRs?   E.g. when I wanted to cite the multiply/shift hash trick for the mailing list, I first looked at the source-- no explination of what its doing,  then the commit message that added the change,  all it said was allow non-power-of-two cache sizes, -- the PR text had the citation I wanted https://github.com/
505 2017-06-08 21:26:40	0|gmaxwell|bitcoin/bitcoin/pull/9533
506 2017-06-08 21:28:36	0|wumpus|no, more (useful) source code comments would be nice
507 2017-06-08 21:28:59	0|wumpus|I'm always happy if people add them, or other documentation for that matter
508 2017-06-08 21:29:05	0|morcos|agreed, i've at least started to incororate most of my PR messages in the commits
509 2017-06-08 21:29:38	0|morcos|but more source code commentary would be good too, although it sometimes gets a bit stale when a major PR changes a lot of pieces
510 2017-06-08 21:30:16	0|wumpus|yes that's the drawback, keeping comments up to date is extra maintenance work
511 2017-06-08 21:30:23	0|wumpus|ideal would be self-illustrating code
512 2017-06-08 21:30:27	0|wumpus|but uh, yeah
513 2017-06-08 21:30:37	0|luke-jr|gmaxwell: I agree
514 2017-06-08 21:31:17	0|luke-jr|instead of answering questions on PRs, we should interpret it as a bug that it's underdocumented ;)
515 2017-06-08 21:31:23	0|wumpus|I've tried, there's many times I've encouraged people to add a code comment instead of just mentioning the rationale in a PR/commit message
516 2017-06-08 21:32:57	0|wumpus|in any case being able to find it in the commit message or even PR is still better than if it was some discussion on IRC :)
517 2017-06-08 21:34:41	0|luke-jr|heh
518 2017-06-08 22:08:22	0|bitcoin-git|[13bitcoin] 15practicalswift closed pull request #10343: Remove redundant on-the-same-line-repetition of type names (DRY): RepeatedTypeName foo = static_cast<RepeatedTypeName>(bar) (06master...06auto) 02https://github.com/bitcoin/bitcoin/pull/10343
519 2017-06-08 23:21:53	0|bitcoin-git|[13bitcoin] 15practicalswift opened pull request #10560: Remove non-existing "force safe mode" (-testsafemode). Remove unused constants. (06master...06noop-modes) 02https://github.com/bitcoin/bitcoin/pull/10560
520 2017-06-08 23:35:05	0|bitcoin-git|[13bitcoin] 15practicalswift opened pull request #10561: Remove duplicate includes (06master...06remove-duplicate-includes) 02https://github.com/bitcoin/bitcoin/pull/10561