1 2016-10-31 00:34:47	0|BlueMatt|does anyone care about a 0.5ms speedup to CheckBlock?
  2 2016-10-31 00:35:32	0|gmaxwell|is it a simple clean change?
  3 2016-10-31 00:35:47	0|gmaxwell|I had speedups at that levle (somewhat faster) by merging loops over all the txn.
  4 2016-10-31 00:36:32	0|BlueMatt|well half of it is (https://github.com/bitcoinfibre/bitcoinfibre/commit/e4884c9f706881fb33ed49f7098e1cea58807e87) the other half is only bareless less so (https://github.com/bitcoinfibre/bitcoinfibre/commit/8bd42b8f763fb736bfde4fba7390d49b9f3cccc6) though i havent benchmarked individually so not sure which is helping
  5 2016-10-31 00:36:44	0|BlueMatt|i think it might actually be the first one, which is 2 line diff
  6 2016-10-31 00:37:32	0|sipa|do we even need that check?
  7 2016-10-31 00:37:52	0|BlueMatt|which? the transaction-length one? no, it is 100% redundant
  8 2016-10-31 00:38:04	0|BlueMatt|the duplicate-inputs one? unclear, probably not but we added it for a reason
  9 2016-10-31 00:38:07	0|BlueMatt|dont recall what it was
 10 2016-10-31 00:38:24	0|BlueMatt|i do remember that we had a reason, however
 11 2016-10-31 00:38:25	0|gmaxwell|was it related to bip30?
 12 2016-10-31 00:40:22	0|BlueMatt|hum...i dont see why? :/
 13 2016-10-31 00:40:57	0|BlueMatt|lol, all these half-empty blocks are fucking with my benchmarking :/
 14 2016-10-31 00:42:00	0|sipa|https://github.com/bitcoin/bitcoin/pull/443
 15 2016-10-31 00:42:03	0|sipa|you added this.
 16 2016-10-31 00:42:38	0|BlueMatt|yes, and i recall having a reason :(
 17 2016-10-31 00:43:02	0|gmaxwell|the pr explains, they were getting relayed.
 18 2016-10-31 00:43:25	0|gmaxwell|(or so says the pr)
 19 2016-10-31 00:43:54	0|BlueMatt|hum, that seems strange to me?
 20 2016-10-31 00:44:21	0|BlueMatt|i mean that was a long time ago, though, i guess before CCoins
 21 2016-10-31 00:44:27	0|BlueMatt|CCoinsViewCache stuff should have fixed this?
 22 2016-10-31 00:45:00	0|sipa|yes
 23 2016-10-31 00:45:12	0|sipa|i believe the ConnectInputs code at the time may not have been able to catch this
 24 2016-10-31 00:45:22	0|BlueMatt|yes, this sounds correct to me
 25 2016-10-31 00:46:37	0|sipa|it had an fBlock which was false for mempool txn, and it disabled part of the checks
 26 2016-10-31 00:49:53	0|gmaxwell|removal trumps optimizing, though if you want to optimize there, I think I found a 1ms-scale speedup by fusing varrious loops over every transaction in the block into a single loop.
 27 2016-10-31 00:52:27	0|BlueMatt|you mean in CheckBlock?
 28 2016-10-31 00:52:38	0|BlueMatt|theres only two over-tx loops
 29 2016-10-31 00:52:53	0|BlueMatt|no, 3
 30 2016-10-31 00:53:19	0|gmaxwell|there are three, coinbase check, checktransaction and sigops check.
 31 2016-10-31 00:53:46	0|BlueMatt|yea, should merge that, though the sigops loop is ~free
 32 2016-10-31 00:54:22	0|BlueMatt|well, its well under 1ms
 33 2016-10-31 00:54:24	0|BlueMatt|well, well under
 34 2016-10-31 00:54:59	0|gmaxwell|IIRC I benchmarked it an it was about a 1ms change.
 35 2016-10-31 00:55:12	0|BlueMatt|hum, that would surprise me (or your system was slower than mine :p)
 36 2016-10-31 00:55:39	0|gmaxwell|well slower probably, -- probably benchmarked it on my laptop.
 37 2016-10-31 00:58:50	0|gmaxwell|BlueMatt: or I flubbed the meaurement.
 38 2016-10-31 00:59:30	0|gmaxwell|But I think the opterations can basically be fused into CheckTransaction... e.g. takes a flag that indicates if its allowed to be a coinbase, and returns the sigops count.
 39 2016-10-31 03:15:38	0|gmaxwell|BlueMatt: in 9045 what initilizes data_hash?
 40 2016-10-31 03:16:03	0|BlueMatt|gmaxwell: default-initialized to null
 41 2016-10-31 03:16:09	0|BlueMatt|its a class
 42 2016-10-31 03:26:32	0|BlueMatt|gmaxwell: anyway, i'll look at optimizing it a bit tomorrow...see if i can write a bench_bitcoin thinggy for it
 43 2016-10-31 03:27:10	0|BlueMatt|CheckBlock is not-insignificant in terms of time from wire to udp broadcast (which is right before disk write in AcceptBlock - same place compact block announcement will end up)
 44 2016-10-31 03:27:54	0|BlueMatt|ofc actual block deserialization is really the largest time sink
 45 2016-10-31 03:29:52	0|gmaxwell|I don't believe you. http://0bin.net/paste/KLQCR8JpdMu5i-x2#VitVZ6nLLdJSu+EnHlCy70e95NLkWxxGgoeNtjY7JmZ
 46 2016-10-31 03:36:46	0|gmaxwell|BlueMatt: ^
 47 2016-10-31 03:37:46	0|sipa|gmaxwell: ints are default not initialized
 48 2016-10-31 03:40:01	0|gmaxwell|oh, matt was saying the uint256 constructor is initing it. I thought he was saying because CNetMessage is a class all its members are initilized.
 49 2016-10-31 03:40:24	0|sipa|yes, it is
 50 2016-10-31 03:40:46	0|sipa|CNetMessage is a class, so all its members have their default constructor called
 51 2016-10-31 03:41:31	0|gmaxwell|Yes, I'd just missed that the hash was a uint256. (or rather that a uint256 behaves differently from a primitive type. :) )
 52 2016-10-31 03:41:52	0|sipa|we could in theory not initialize the member chars in a uint5
 53 2016-10-31 03:41:57	0|sipa|*uintw56
 54 2016-10-31 03:42:06	0|sipa|**uint256
 55 2016-10-31 03:42:17	0|sipa|but i think that would surprise people
 56 2016-10-31 07:31:22	0|sipa|BlueMatt: https://github.com/bitcoin/bitcoin/pull/8930/commits/37aefff5fcf7169a1b07ff8939850f630640f7e7 <- i remember there was some discussion about the reintroduction of #ifdef ENABLE_WALLET there, but i don't know where it was or what was said
 57 2016-10-31 08:12:15	0|jonasschnelli|sipa: https://github.com/bitcoin/bitcoin/pull/8977/files fixes BlueMatt's #ifdef ENABLE_WALLET
 58 2016-10-31 10:00:12	0|wumpus|gmaxwell: can you take a look here? https://github.com/bitcoin-dot-org/bitcoin.org/pull/1401 (re: final alert stuff)
 59 2016-10-31 10:01:14	0|wumpus|I think it can be merged but as you're going to send it, it makes sense if you sign off on it
 60 2016-10-31 12:15:25	0|achow101|ping gmaxwell
 61 2016-10-31 12:18:09	0|Victorsueca|try to CTCP ping him? that usually shows on other tabs
 62 2016-10-31 12:32:37	0|rabidus_|try ddos
 63 2016-10-31 12:32:50	0|rabidus_|ehm... i didn't really say that
 64 2016-10-31 12:32:53	0|rabidus_|that's not an advice
 65 2016-10-31 12:34:35	0|luke-jr|…
 66 2016-10-31 12:34:53	0|rabidus_|:'(
 67 2016-10-31 13:03:18	0|luke-jr|https://www.reddit.com/r/Bitcoin/comments/5ac1a4/error_in_bitcoind_munmap_chunk_invalid_pointer/
 68 2016-10-31 13:31:27	0|BlueMatt|sipa: I prefer adding an ifdef than having a segfault (also that was already merged in another pr)
 69 2016-10-31 14:47:02	0|GitHub117|[13bitcoin] 15sdaftuar opened pull request #9048: [0.13 backport] Fix handling of invalid compact blocks (060.13...06fix-invalid-cb-ban-0.13) 02https://github.com/bitcoin/bitcoin/pull/9048
 70 2016-10-31 15:20:22	0|sipa|BlueMatt: 8977 it seems
 71 2016-10-31 16:50:54	0|BlueMatt|gmaxwell: so i went and combined everything into one loop of txn in a block (and even combined the sigops count loops pushed into checktransaction, etc...also removed the duplicative GetSerializeSize check in CheckTransaction (which is duplicative of CheckBlock) and the only part of that whole excersize that made any noticeable performance difference was removing the duplicate-input check, which made for a significant performance
 72 2016-10-31 16:50:54	0|BlueMatt|increase (of around 1.5ms in the 3-4ms function...)
 73 2016-10-31 16:54:47	0|BlueMatt|nvm, 0.7ms
 74 2016-10-31 16:55:08	0|BlueMatt|on my laptop: 8ms to deserialize, 11.1 -> 10.5ms for deserialize + checkblock
 75 2016-10-31 17:16:53	0|sipa|BlueMatt: cool, so let's just get rid of the duplicates check
 76 2016-10-31 17:17:09	0|BlueMatt|sipa: not so easy due to network partition risk, as sdaftuar and I just found out :(
 77 2016-10-31 17:18:02	0|sipa|uh?
 78 2016-10-31 17:18:17	0|sipa|ah.
 79 2016-10-31 17:18:26	0|BlueMatt|oops, no, nvm, just makes it tricky-er to do pre-relay of compact blocks
 80 2016-10-31 17:18:40	0|BlueMatt|since then CheckBlock becomes network-ban-partition-consensus
 81 2016-10-31 17:19:12	0|BlueMatt|(0.13.1 will already ban you for this, so it cant get worse...)
 82 2016-10-31 17:20:54	0|BlueMatt|sipa: re: #9026
 83 2016-10-31 17:22:02	0|gmaxwell|bump the protocol version number. then you can decide on your behavior based on it.?
 84 2016-10-31 17:26:25	0|BlueMatt|true, probably need to do that, though still really want to do for 0.13.2
 85 2016-10-31 17:26:54	0|BlueMatt|and then the problem is we dont want to change too often because you would just skip pre-relay for people not on your protocol version
 86 2016-10-31 18:10:13	0|sipa|BlueMatt: i'm really curious what typo you made that got corrected into "I did not Rome the hashing"
 87 2016-10-31 18:10:41	0|BlueMatt|dunno how my phone converted benchmark to rome, but it appears to have
 88 2016-10-31 18:11:25	0|gmaxwell|.query eragmus
 89 2016-10-31 18:11:27	0|gmaxwell|oops
 90 2016-10-31 18:50:00	0|GitHub133|13bitcoin/06master 147f61b49 15matthias: Change all instance of 'GMT epoch' to 'Unix epoch'
 91 2016-10-31 18:50:00	0|GitHub133|[13bitcoin] 15MarcoFalke pushed 2 new commits to 06master: 02https://github.com/bitcoin/bitcoin/compare/d2143dc937e3...3d69ecb4edeb
 92 2016-10-31 18:50:01	0|GitHub133|13bitcoin/06master 143d69ecb 15MarcoFalke: Merge #9041: keypoololdest denote Unix epoch, not GMT...
 93 2016-10-31 18:50:15	0|GitHub16|[13bitcoin] 15MarcoFalke closed pull request #9041: keypoololdest denote Unix epoch, not GMT (06master...06patch-8) 02https://github.com/bitcoin/bitcoin/pull/9041
 94 2016-10-31 18:52:50	0|GitHub112|[13bitcoin] 15TheBlueMatt opened pull request #9049: Remove duplicatble duplicate-input check from CheckTransaction (06master...062016-10-bench-checkblock) 02https://github.com/bitcoin/bitcoin/pull/9049
 95 2016-10-31 18:57:39	0|sipa|duplicatble duplicate-input ?
 96 2016-10-31 22:03:02	0|GitHub106|[13bitcoin] 15theuni opened pull request #9050: net: make a few values immutible, and use deterministic randomness for the localnonce (06master...06connman-const) 02https://github.com/bitcoin/bitcoin/pull/9050
 97 2016-10-31 22:09:41	0|gmaxwell|cfields_: I haven't looked yet, but does that fix the current race condition, where if you connect to yourself then someone else connects to you then you get your own nonce, you'll not detect the connect to self?
 98 2016-10-31 22:10:36	0|cfields_|gmaxwell: i believe that was fixed a while back with the initial CConnman merge.
 99 2016-10-31 22:10:43	0|gmaxwell|Ah. good
100 2016-10-31 22:11:11	0|sipa|yes, that was fixed afaik
101 2016-10-31 22:12:38	0|cfields_|gmaxwell: if we're speaking of the same race, this should've fixed it: https://github.com/bitcoin/bitcoin/commit/960cf2e4058a9c195bf64e1aecb46024f9ef022a
102 2016-10-31 22:13:43	0|gmaxwell|yup! that looks good. Okay, I'd missed that.
103 2016-10-31 22:16:35	0|gmaxwell|cfields_: whats the motivation for making the nonce determinstic?
104 2016-10-31 22:18:16	0|cfields_|gmaxwell: none really, other than it's one of the few (the only?) users of Rand in there. Eliminating them could ease replay for testing.
105 2016-10-31 22:19:04	0|gmaxwell|uh. wait... you mean this is completely determinstic?
106 2016-10-31 22:19:12	0|cfields_|gmaxwell: really not much though, i just figured if it was immutible and per-connection, may as well make it deterministic if possible
107 2016-10-31 22:19:20	0|cfields_|gmaxwell: heh, no. It's seeded at startup
108 2016-10-31 22:19:41	0|cfields_|wait, i hope...
109 2016-10-31 22:19:59	0|cfields_|yes, sorry. It's a per-connman seed.
110 2016-10-31 22:20:13	0|cfields_|you made me doubt myself for a sec :)
111 2016-10-31 22:23:41	0|gmaxwell|K. yes, I see that.  Just the 'replay for testing' made me wonder!
112 2016-10-31 22:24:31	0|gmaxwell|so if someone manages to make enough connections to wrap the peer_id ... it'll repeat. OTOH if someone manages to do that lots of other things will probably break. :P
113 2016-10-31 22:27:07	0|cfields_|heh, yes. NodeId is signed, so things could go wonky way before that
114 2016-10-31 22:31:42	0|cfields_|err
115 2016-10-31 22:36:17	0|gmaxwell|sipa: cfields_ just had a spurrious travis failure caused by the overly jumpy rand_bits test in libsecp256k1.
116 2016-10-31 22:37:43	0|gmaxwell|sipa: IMO we should ifdef out the RNG tests and just run them once per release as part of a prerelease checklist or something.
117 2016-10-31 22:39:01	0|sipa|iirc i started a replacement for that at some point, but it remained too spurious
118 2016-10-31 22:41:41	0|BlueMatt|cfields_: hum, hashing isnt much of a bottleneck here...
119 2016-10-31 22:42:40	0|gmaxwell|sipa: I believe its the only rest with spurrious failures, and the rng tests are the only ones which _must_ have spurrious failures.
120 2016-10-31 22:43:30	0|sipa|right, but it should be doable to bring that down to 1 in a billion runs or so
121 2016-10-31 22:43:50	0|BlueMatt|well, the compact blocks stuff is somewhat spurious
122 2016-10-31 22:44:15	0|BlueMatt|though i suppose it could effect regtest during semi-normal usage
123 2016-10-31 22:44:56	0|sipa|BlueMatt: this is about libsecp256k1
124 2016-10-31 22:45:01	0|BlueMatt|oh, sorry
125 2016-10-31 22:45:05	0|BlueMatt|get your own channel!
126 2016-10-31 22:45:06	0|BlueMatt|(jk)
127 2016-10-31 22:48:00	0|cfields_|BlueMatt: i just can't think of any good reason for handling it on the socket thread, which lags more with each added connection, as opposed to on the message thread(/pool), potentially lazily, and only as needed
128 2016-10-31 22:48:15	0|BlueMatt|cfields_: oops, see my respond on github
129 2016-10-31 22:49:13	0|BlueMatt|its a latency question...our current hasher should (mostly) be able to keep up with 1Gbps of shit (ok, maybe more like 500Mbps), but putting it in the processing logic adds latency to block processing
130 2016-10-31 22:51:42	0|gmaxwell|also we do no other computation in the thread that handles the incoming data... so this should better let it run concurrently with processing given the current setup.
131 2016-10-31 22:52:38	0|cfields_|hmm
132 2016-10-31 22:53:11	0|sipa|i would expect that in the future it's also easier to parallellize networking than it is to parallellize processing
133 2016-10-31 22:55:11	0|sipa|and the bip151 hasher should be a few times faster
134 2016-10-31 22:55:13	0|cfields_|sipa: heh, i argued the exact opposite in the PR. At the select/epoll/etc. level anyway. Though I suppose you could split the nodes into batches.
135 2016-10-31 22:55:15	0|BlueMatt|cfields_: if you're bored, #8969 can probably get another ack and a merge, then we're only about 3 commits from splitting main, altough the next one will probably have to wait until the compact block fix goes in
136 2016-10-31 22:56:24	0|cfields_|BlueMatt: you can save the poke, i owe you an ack on that one already :)
137 2016-10-31 22:56:30	0|BlueMatt|heh, ok
138 2016-10-31 22:56:48	0|BlueMatt|just trying to move things in since its gonna be a painful merge cycle to get this all in for 0.4
139 2016-10-31 22:58:48	0|sipa|cfields_: also, 8708 mentions constness... i'm addressing that to some extent in #8580, which now is queued after #9039 :)
140 2016-10-31 22:59:02	0|BlueMatt|lol, so much to review
141 2016-10-31 22:59:38	0|sipa|9039 grew a bit beyond what i originally planned to address, but i think the result is worth it
142 2016-10-31 23:00:24	0|BlueMatt|yea, i saw there was some back-and-forth, but, indeed, I'll bet thats worth it
143 2016-10-31 23:00:34	0|BlueMatt|serialization can always use cleaning
144 2016-10-31 23:00:44	0|cfields_|sipa: ack. Your serialization changes are next on my list to review. As it relates to 8708, i removed the const hack that i justified with "but we already do this elsewhere!", so my changes won't undo your fixes.
145 2016-10-31 23:01:07	0|BlueMatt|we need an irc bot that posts pr titles after numbers are mentioned :p
146 2016-10-31 23:01:32	0|sipa|BlueMatt: yes!
147 2016-10-31 23:01:41	0|BlueMatt|where is nanotube when you need him?
148 2016-10-31 23:01:43	0|cfields_|BlueMatt: heh, yes, and links to commit ids
149 2016-10-31 23:01:46	0|BlueMatt|gribble: help me out bro
150 2016-10-31 23:02:11	0|cfields_|BlueMatt: i used that in another channel, iirc it was easy enough to setup
151 2016-10-31 23:02:29	0|BlueMatt|I'm sure, but I'm lazy...prefer if someone who's already running a bot in here does it :p
152 2016-10-31 23:02:49	0|BlueMatt|I mean its easy...4 digit number starting with a # and link to github.com/bitcoin/bitcoin/issue/#
153 2016-10-31 23:03:12	0|BlueMatt|cfields_: I'll try to get you another review on 8708 tonight, otherwise tomorrow morning
154 2016-10-31 23:03:35	0|sipa|BlueMatt, cfields_: can you two fight over the order in which 8708 and 8969?
155 2016-10-31 23:03:53	0|BlueMatt|should be minimal conflicts?
156 2016-10-31 23:03:57	0|BlueMatt|or at least easy-to-resolve ones?
157 2016-10-31 23:03:57	0|sipa|ok
158 2016-10-31 23:04:02	0|sipa|so much easier :)
159 2016-10-31 23:04:08	0|sipa|i assumed they'd conflict a lot
160 2016-10-31 23:04:28	0|BlueMatt|8969 is some random trivial cleanups in preparation for the big split (tm)
161 2016-10-31 23:04:34	0|BlueMatt|so souldnt conflict with much of anything
162 2016-10-31 23:05:21	0|sipa|cfields_: 9039 touches pretty much every line that deals with serialization, so it'll conflict with the beginning of 8708
163 2016-10-31 23:06:26	0|BlueMatt|sipa: I think 8708 is further along?
164 2016-10-31 23:06:36	0|cfields_|sipa: np, i don't mind rebasing that one. It'll need one more set of changes on top, in fact, so i'll take the burden there
165 2016-10-31 23:06:46	0|BlueMatt|I think cfields_ and I probably agree on it now, just need to review
166 2016-10-31 23:06:59	0|BlueMatt|heh, ok
167 2016-10-31 23:07:04	0|sipa|ok
168 2016-10-31 23:07:22	0|sipa|i'll review 8708 in more detail too hen
169 2016-10-31 23:07:23	0|cfields_|yea, it should be ready, i think, but it'll touch PushMessage again.
170 2016-10-31 23:07:23	0|sipa|then
171 2016-10-31 23:08:37	0|cfields_|sipa: if it makes more sense to get the serialization changes in first, I could go ahead and make the other change in 8708, rather than breaking it out, that way it can be reviewed in parallel, then just rebase on the serialization changes
172 2016-10-31 23:08:50	0|cfields_|either way, doesn't matter at all to me.
173 2016-10-31 23:09:10	0|sipa|cfields_: i think it'll just boil down to removing "nType, nVersion, " everywhere in your patch
174 2016-10-31 23:09:37	0|cfields_|sipa: ah ok, that's trivial then.
175 2016-10-31 23:12:45	0|cfields_|BlueMatt: btw, i'm just a few steps away from splitting CConnman out of net, but I'll wait for the main split :)
176 2016-10-31 23:12:56	0|BlueMatt|cfields_: cool
177 2016-10-31 23:13:11	0|BlueMatt|if we get all this shit in 0.14 I'm gonna be really impressed
178 2016-10-31 23:19:27	0|BlueMatt|cfields_: you missed one of my nits - you need to move the assert in EndMessage up above the first WriteLE32
179 2016-10-31 23:20:34	0|BlueMatt|cfields_: while you're at it, just change it to assert size >= HEADER_SIZE
180 2016-10-31 23:23:18	0|cfields_|BlueMatt: hm, thought I moved it. sorry about that. will make that change.
181 2016-10-31 23:23:56	0|BlueMatt|cfields_: while you're at it, you changed the LogPrint on sending messages
182 2016-10-31 23:24:04	0|BlueMatt|it used to read "sending: ..." you removed the sending
183 2016-10-31 23:24:15	0|BlueMatt|and you got rid of the SanitizeString() call there, though i doubt that matters
184 2016-10-31 23:26:11	0|cfields_|BlueMatt: grr, not sure why those got dropped. will fix.
185 2016-10-31 23:26:25	0|cfields_|crap, got to go help with dinner. bbl.
186 2016-10-31 23:26:47	0|BlueMatt|cfields_: ok, I'll queue them in review...gotta run as well, so might have to finish tomorrow