--- Day changed Wed Mar 04 2020 01:04 -!- jonatack [~jon@2a01:e0a:53c:a200:bb54:3be5:c3d0:9ce5] has joined #bitcoin-core-pr-reviews 01:09 -!- jonatack [~jon@2a01:e0a:53c:a200:bb54:3be5:c3d0:9ce5] has quit [Ping timeout: 272 seconds] 01:11 -!- jonatack [~jon@37.171.110.92] has joined #bitcoin-core-pr-reviews 01:12 -!- tylerlevine6 [~hardforkt@li120-195.members.linode.com] has joined #bitcoin-core-pr-reviews 01:13 -!- tylerlevine [~hardforkt@li120-195.members.linode.com] has quit [Read error: Connection reset by peer] 01:29 -!- Zenton [~user@unaffiliated/vicenteh] has joined #bitcoin-core-pr-reviews 02:00 -!- jonatack_ [~jon@37.167.199.192] has joined #bitcoin-core-pr-reviews 02:03 -!- jonatack [~jon@37.171.110.92] has quit [Ping timeout: 258 seconds] 03:03 -!- jonatack_ [~jon@37.167.199.192] has quit [Ping timeout: 265 seconds] 03:04 -!- jonatack_ [~jon@213.152.161.25] has joined #bitcoin-core-pr-reviews 03:05 -!- Trenton36Nienow [~Trenton36@ns334669.ip-5-196-64.eu] has joined #bitcoin-core-pr-reviews 03:11 -!- Trenton36Nienow [~Trenton36@ns334669.ip-5-196-64.eu] has quit [Ping timeout: 256 seconds] 03:54 -!- belcher [~belcher@unaffiliated/belcher] has joined #bitcoin-core-pr-reviews 04:20 -!- jonatack_ [~jon@213.152.161.25] has quit [Ping timeout: 256 seconds] 04:22 -!- jonatack_ [~jon@37.167.220.16] has joined #bitcoin-core-pr-reviews 04:35 -!- slivera_ [~slivera@217.138.204.73] has joined #bitcoin-core-pr-reviews 04:36 -!- slivera [~slivera@217.138.204.73] has quit [Read error: Connection reset by peer] 04:48 -!- slivera_ [~slivera@217.138.204.73] has quit [Ping timeout: 258 seconds] 04:54 -!- ethzero [sid396973@gateway/web/irccloud.com/x-uqphkiisxuzltphm] has quit [Ping timeout: 260 seconds] 04:55 -!- ethzero [sid396973@gateway/web/irccloud.com/x-uzblexnxdpzdtzeh] has joined #bitcoin-core-pr-reviews 05:18 -!- jb55 [~jb55@gateway/tor-sasl/jb55] has quit [Remote host closed the connection] 05:20 -!- jb55 [~jb55@gateway/tor-sasl/jb55] has joined #bitcoin-core-pr-reviews 05:24 -!- TheRec [~toto@drupal.org/user/146860/view] has quit [Ping timeout: 258 seconds] 05:25 -!- TheRec [~toto@drupal.org/user/146860/view] has joined #bitcoin-core-pr-reviews 06:58 -!- andrewtoth_ [~andrewtot@gateway/tor-sasl/andrewtoth] has joined #bitcoin-core-pr-reviews 07:17 -!- andrewtoth_ [~andrewtot@gateway/tor-sasl/andrewtoth] has quit [Ping timeout: 240 seconds] 07:21 -!- pinheadmz [~matthewzi@45.83.89.68] has quit [Quit: pinheadmz] 07:51 -!- jonatack_ [~jon@37.167.220.16] has quit [Quit: jonatack_] 07:51 -!- jonatack [~jon@37.167.220.16] has joined #bitcoin-core-pr-reviews 08:33 -!- emilengler [~emilengle@stratum0/entity/emilengler] has joined #bitcoin-core-pr-reviews 09:01 -!- halfass [310ff659@49.15.246.89] has joined #bitcoin-core-pr-reviews 09:04 -!- halfass [310ff659@49.15.246.89] has quit [Remote host closed the connection] 09:04 -!- lightlike [~lightlike@p200300C7EF0EC400657752C6D67C6B18.dip0.t-ipconnect.de] has joined #bitcoin-core-pr-reviews 09:07 -!- Talkless [~Talkless@hst-227-49.splius.lt] has joined #bitcoin-core-pr-reviews 09:11 < jnewbery> We'll get started in about 50 minutes 09:32 -!- michaelfolkson [~textual@2a00:23c5:be01:b201:55fc:f43c:fed0:4fc6] has joined #bitcoin-core-pr-reviews 09:39 -!- rafalcpp_ [~racalcppp@ip-178-211.ists.pl] has quit [Ping timeout: 255 seconds] 09:41 -!- rafalcpp_ [~racalcppp@ip-178-211.ists.pl] has joined #bitcoin-core-pr-reviews 09:50 < jnewbery> 10 minutes 09:50 -!- LarryRuane [cda8418a@205-168-65-138.dia.static.qwest.net] has joined #bitcoin-core-pr-reviews 09:51 -!- Aleru [sid403553@gateway/web/irccloud.com/x-ecdcawxikpgbcnro] has joined #bitcoin-core-pr-reviews 09:57 -!- okkkk [aed11c6e@110.sub-174-209-28.myvzw.com] has joined #bitcoin-core-pr-reviews 10:00 < emilengler> hi 10:00 < jnewbery> #startmeeting 10:00 < jnewbery> Hi folks! Welcome to Bitcoin Core PR Review Club. Feel free to say hi to let everyone know you're at keyboard. 10:00 < kanzure> hi 10:00 < jonatack> hi 10:00 < lightlike> hi 10:00 < fjahr> hi 10:00 < LarryRuane> hi 10:00 < okkkk> Hello! 10:00 < jnewbery> As always, we'll have a host to guide discussion, but feel free to jump in at any time to ask questions. 10:00 < amiti> hi 10:00 < michaelfolkson> hi 10:00 < ajonas> hi 10:00 < emzy> hi 10:00 < jnewbery> Today's PR is #16981. Notes and questions are here: https://bitcoincore.reviews/16981.html 10:01 < willcl_ark> hi 10:01 < jnewbery> lightlike is hosting for us today. Thanks lightlike! We also have the PR author, LarryRuane here. Thanks for joining us, Larry 10:01 < jnewbery> ok, I'll pass over to lightlike 10:01 -!- Guest83 [~textual@cpc103056-sgyl39-2-0-cust1444.18-2.cable.virginm.net] has joined #bitcoin-core-pr-reviews 10:01 < lightlike> Ok so today's PR is about improving the performance of reindexing. 10:01 < jkczyz> hi 10:01 < lightlike> Did everyone get a chance to review the PR? ( y/n ) 10:01 < emilengler> y 10:01 < jkczyz> n 10:01 < emzy> n 10:01 < Guest83> y 10:01 < jnewbery> 0.3y 10:02 < amiti> ~.2 y 10:02 < willcl_ark> brief code review, but not tested 10:02 < fjahr> y 10:02 < michaelfolkson> ~.1 y 10:02 < ajonas> read comments and code but not tested 10:02 < lightlike> So for those who did get a chance to look at it, what is your opinion on the PR? Concept ACK, approach ACK, tested ACK, or NACK? 10:02 < emilengler> To summarize it up: It does a more low level job when reading the reindexing plus it only reads the block header instead fo the entire block 10:02 -!- theme [~theme@195.181.160.175.adsl.inet-telecom.org] has joined #bitcoin-core-pr-reviews 10:02 < emilengler> Correct me if I missed something 10:03 -!- pbleam [4a487bd3@cpe-74-72-123-211.nyc.res.rr.com] has joined #bitcoin-core-pr-reviews 10:03 < emilengler> lightlike: I did a Concept ACK. Was going to test once my chain is fully synced but I reviewed the code 10:03 < willcl_ark> if implemented correctly, seems like a win with no downsides 10:03 < jnewbery> Concept ACK. Better is better 10:04 < fjahr> Concept ACK but i only skimmed the test so far and did not have time to test 10:04 < lightlike> emilengler: I wouldn't really say that the level really changed ( we still use CBufferedFile, which is pretty low-level). 10:04 < lightlike> But first to some more general questions: 10:04 < lightlike> In which situations is reindex useful? When is it better to use reindex-chainstate? 10:05 < instagibbs> reindex is when block index is fubar, right? 10:05 < instagibbs> block data is ok, but block index broken 10:05 < fjahr> both rebuild the utxo set, full reindex rebuilds the block index as well 10:05 < elichai2> Hi 10:06 < LarryRuane> yes or for example, isn't it needed if you enable `txindex`? 10:06 < willcl_ark> I think if you enable txindex (e.g. ElectrumX) then you _require_ a reindex also 10:06 < fjahr> so it depends on what you need 10:06 < elichai2> about note 2.2, is reindex really fully validating? why? and does it have the same logic as IBD?(ie assumevalid) 10:06 < lightlike> instagibbs, fjahr: yes. 10:06 < jnewbery> instagibbs: that's how I understand it. reindex-chainstate only rebuilds the UTXO set. reindex also rebuilds the block index 10:06 < LarryRuane> reindex is in a way similar to initial block sync except it can be done completely offline 10:06 < michaelfolkson> In which situations is reindex useful? Data corruption yes. Anything else? 10:06 < lightlike> elichai2: yes, I think it does, didn't think of that until later today. 10:07 < jnewbery> willcl_ark: that used to be true. Since jimpo refactored the indexing code, I think that building a txindex doesn't require a -reindex 10:07 < instagibbs> right, incremental indexing is a thing now AFAIU :) 10:08 < willcl_ark> jnewbery: that's good news! I recall doing it myself a while ago for that 10:08 < emilengler> michaelfolkson: I'm not sure but maybe for verification 10:08 < emilengler> For example if you transferred the data from a disk to another 10:08 < michaelfolkson> As always there's good sipa StackExchange answer: https://bitcoin.stackexchange.com/questions/60709/when-should-i-use-reindex-chainstate-and-when-reindex 10:08 < lightlike> I also think reindexing might possibly be needed if the format of the index db would ever be changed in the future between releases 10:08 < willcl_ark> although, as IBD is usually saturated at CPU, perhaps --reindex is not actually that useful (unless you are bandwidth/data constrained in some way for a db error) 10:08 < nehan_> hi 10:09 < michaelfolkson> Yeah makes sense emilengler 10:09 < jnewbery> willcl_ark: PR 13033 I think 10:09 < fjahr> michaelfolkson: definitely when you were running pruned node 10:09 < fjahr> and now you switch to fully validating 10:10 < raj_> hi 10:10 < jnewbery> fjahr: not sure I understand. A pruned node _is_ fully validating 10:11 < michaelfolkson> Yeah that's good one fjahr. You switch to unpruned I assume you mean 10:11 < luke-jr> indeed 10:11 < fjahr> yeah, sorry, i meant non pruned node %) 10:11 < lightlike> just noting that in some situations (only utxo db is broken), reindex-chainstate is sufficient. 10:11 < lightlike> ok, next q: 10:11 < lightlike> What does the block index database look like? What information is stored there? 10:12 < lightlike> it's quite a general question, just name some of the most important infos... 10:12 -!- okkkk [aed11c6e@110.sub-174-209-28.myvzw.com] has quit [Remote host closed the connection] 10:13 -!- cprkrn [aed11c6e@110.sub-174-209-28.myvzw.com] has joined #bitcoin-core-pr-reviews 10:13 < Guest83> Indexed on header? 10:13 < willcl_ark> block hash, tx hashes, block file names 10:13 < michaelfolkson> Where block is stored on disk 10:14 < LarryRuane> i was just reading sipa's reply on the stackexchange (linked above).. "You should use -reindex only when you were running in pruning mode..." should that be non-pruning mode? to reindex requires that you have all 300+ gb of blocks, right? 10:14 < lightlike> I think a great overview is on https://bitcoin.stackexchange.com/questions/28168/what-are-the-keys-used-in-the-blockchain-leveldb-ie-what-are-the-keyvalue-pair (again by Pieter Wuille :-)) 10:14 < emilengler> Yes reindexing is not possible if pruning 10:15 < emilengler> I think it throws a runtime error then 10:15 < willcl_ark> LarryRuane: does --reindex init a full IBD if it's missing the data? 10:15 < LarryRuane> good question, i don't know 10:16 < fjahr> LarryRuane: if you were running it pruned and you switch to non pruning I think you have to start the node with -reindex if i remember correctly 10:16 < lightlike> so willcl_ark: I think that it would sync up the last block we can connect from disk, and then switch to IBD for the rest of the chain 10:17 < willcl_ark> that would seem logical 10:17 < jnewbery> I'm looking in init.cpp, and there appears to be logic for when using -reindex and -prune, so I guess it'll just redownload blocks from the network 10:17 < jnewbery> (from here: https://github.com/jnewbery/bitcoin/blob/a50f956e304adc7c75428ec391f1a76a99c1ddde/src/init.cpp#L628) 10:18 < molly> once i tested reindex on a laptop a few years ago, it took a week to fully sync, so i wouldn't use reindex if i have a corrupt database, i would resync the node from scratch, it's faster 10:19 < willcl_ark> maybe not after this PR :) 10:19 < jnewbery> molly: I'd be surprised if that were true in general, even before this PR 10:19 < lightlike> willcl_ark: I think that is what happened to me once when I had a corrupt block file (and slow internet connection): I would reindex up until the corrupt block, and then download all blocks beyond that from peers 10:19 < molly> jnewbery, i haven't looked at this PR 10:20 < LarryRuane> for me it took slightly less time to reindex than IBD last time i checked, several months ago ... but also reindexing puts less traffic on the network on load on peers 10:21 < lightlike> ok, next question: Can you explain why this PR reduces the runtime of reindexing considerably? 10:21 < emilengler> LarryRuane: If you use the same hardware it jsut takes longer becasue of the growing size 10:21 < LarryRuane> (do you want me to answer? :) ) 10:21 < nehan_> it avoids deserializing blocks that will need to be deserialized later 10:21 -!- andrewtoth_ [~andrewtot@gateway/tor-sasl/andrewtoth] has joined #bitcoin-core-pr-reviews 10:22 < emilengler> AFAIK because it reads the header instead of the entire block 10:22 < lightlike> LarryRuane: maybe give others a chance first, but feel free to add your view on anything :-) 10:22 < theme> reduced read operations? 10:23 < jnewbery> it allows us to seek through the block file for headers rather than reading everything 10:23 < LarryRuane> nehan_ that's correct, a lot of CPU is spent deserializing blocks unnecessarily (throwing that work away) 10:23 < cprkrn> Yup. Doesn't require reading all of the data 10:23 < lightlike> emilengler: yes, but just at the first encounter, because based on the header we can decide if wa want to deserialize the block already now (or need it only later) 10:23 < raj_> by checking into memory for a chilld block instead of reading it from the disk when the parent is found. 10:24 < LarryRuane> so... i made a comment on the PR yesterday that explains in more detail but it doesn't actually reduce reads from disk (either num of reads or length of reads), but it only saves CPU time spent deserializing 10:25 < cprkrn> Gotcha 10:25 < LarryRuane> lightlike yes that is correct 10:25 < jnewbery> https://bitcoin.stackexchange.com/questions/28168/what-are-the-keys-used-in-the-blockchain-leveldb-ie-what-are-the-keyvalue-pair is excellent, but a little out of date now. I believe the transaction index record is now in its own database structure, and the chain state databse is per-txout rather than per-tx 10:26 < lightlike> yes, I was not precise there in my notes: The reading from disk, in my understanding, is done in the Fill() method, which happens also if we skip the block 10:26 < willcl_ark> jnewbery: so thats how #13033 did it 10:27 < lightlike> jnewbery: do you know if a good documentation exists somewhere that is up to date? 10:27 < MarcoFalke> Ideally the documentation about Bitcoin Core should be in the source code :) 10:28 < jnewbery> lightlike: I do not. Someone should suggest edits to sipa's SE answer 10:28 < MarcoFalke> So someone should copy-past the useful parts into our code base 10:29 < jnewbery> MarcoFalke: for design documentation, is https://github.com/bitcoin-core/bitcoin-devwiki/wiki better? 10:30 < lightlike> I think it is also helpful to mention the observation by Larry in the PR, that this change is only an improvement because IBD typically saves block out of order. If everything was in order, this change would make things (slightly) slower. 10:32 < nehan_> LarryRuane: I added a comment. You are now holding cs_main while doing a disk read (i think) in Skip(). might that affect performance? 10:32 < raj_> just one tangent question. While reindexing, is transaction validation process again repeated? 10:32 < LarryRuane> yes because when we encounter a block and its parent has already been seen, it backs up (in the memory buffer) by 80 bytes (header) and deserializes the entire block (incuding header, again) .. so header is deserialized twice .. but since it's only 80 bytes probably not much impact 10:33 < michaelfolkson> That's just used for release notes jnewbery? achow101 said that got vandalized recently as no merge restrictions 10:33 < LarryRuane> raj_ i believe yes, similar to IBD, all checks are performed, only difference is blocks come from disk instead of peers 10:34 < lightlike> raj_: although as elichai2 noted earlier, with the same restrictions as in IBD (afaik not every signature of very old blocks is validated, but I don't know the details there) 10:36 < lightlike> ok, next q: How about test coverage of the reindexing (functional and unit tests)? Do you think it can be improved? 10:36 < LarryRuane> nehan_ thank you, i'll reply there, but today, where of course there is no Skip(), instead the deserialization (`blkdat >> block;`) can trigger the same disk read, so there is no difference 10:37 < nehan_> LarryRuane: I think that's outside the cs_main lock though 10:37 < jnewbery> raj_: yes, exactly the same as for IBD. You can imagine we're treating the blk files as untrusted and revalidating everything again. assumevalid means that we don't check scripts/signatures before a certain height 10:38 -!- Guest83 is now known as docallag 10:39 < LarryRuane> nehan_ you're right! good catch, i'll investigate how to improve that (or try to see if it may be acceptable) 10:40 < willcl_ark> jnewbery: which also might give us another --reindex use-case: recieving an offline copy of the block files from a friend (or torrent download?) which you might want to reindex before trusting 10:40 < nehan_> LarryRuane: I'm having a lot of trouble figuring out how nRewind is updated given that it moved around. I'm still trying to get my head around CBufferedFile semantics... 10:41 < LarryRuane> _nehan_ this lock can be dropped before calling Skip() .. Is there a way to drop a LOCK() besides it going out of scope? i don't think i've ever seen that 10:41 < jonatack> michaelfolkson: there are several different things in https://github.com/bitcoin-core/bitcoin-devwiki/wiki... mempool, p2p design, wallet structure 10:41 < jnewbery> nehan_: good catch! In practice, I don't think it matters too much. If we're doing reindex then the critical path is single-threaded in this thread (the ThreadImport thread) 10:41 < raj_> jnewbery: Thanks. So how much is actuallly the difference between a fresh IBD and reindexing in terms of time? 10:41 < jnewbery> definitely worth benching performance though 10:43 < LarryRuane> nehan_ yes that rewind stuff is somewhat obtuse... basic idea of rewind is it marks a point in the stream that we can reposition to if deserialization throws 10:43 < raj_> nehan_: I cant seem to find where cs_main is held in read(). :( 10:43 < jnewbery> raj_: depends on bandwidth/quality of peers/disk access speed/CPU 10:43 < lightlike> raj_: that would depend a lot on the speed of your internet connection (and that of your peers). 10:44 < nehan_> raj_: it's not. It's held while this is called, which calls read: https://github.com/bitcoin/bitcoin/pull/16981/commits/a50f956e304adc7c75428ec391f1a76a99c1ddde#diff-24efdb00bfbe56b140fb006b562cc70bR4698 10:44 < michaelfolkson> Re tests. Any additional tests would be separate PR not related to this specific performance improvement? 10:45 < raj_> ok makes sense. So in % term what kind of reduction we are talking about here? 10:45 < lightlike> michaelfolkson: yes, plus this PR adds unit tests for the new Skip() functionality. 10:45 < raj_> Oh thanks nehan_ 10:45 < jnewbery> in general, you'd expect reading files from disk to be faster than downloading that data from peers, but exact quantitative difference depends on those factors 10:45 < nehan_> LarryRuane: oh so that's another thing, read() could throw while holding the lock. does that matter? 10:45 < lightlike> but in general I think that while the unit tests of CBufferedFile are really great, the validation code is tested quite poorly in the functional tests: 10:46 < LarryRuane> just for those who may not be that familiar with this code ... (this wasn't at all clear to me initially) ... deserialization (`blkdat >> ....`) can throw an exception, and then we magically end up the catch and continue that top-level loop 10:46 < LarryRuane> nehan_ nope, the compiler (semantics of LOCK) deal with that correctly, one aspect of its magic 10:46 < nehan_> LarryRuane: cool thanks 10:47 < lightlike> feature_reindex.py exists but seems quite rudimentary. 10:48 < lightlike> Last question: Can you think of further optimizations of reindexing beyond this PR that are possible/worthwhile? 10:48 < jnewbery> lightlike: I agree that feature_reindex.py is rudimentary. What kind of additional tests would you like to see? 10:49 < LarryRuane> lightlike yes it's tiny, tbh i didn't even really look at it while doing this PR .. yes, my question too, should it do more? 10:49 < LarryRuane> i could look into that 10:49 < lightlike> LarryRuane: That was in no way meant a criticism - it was like that for years, and you added lots of units tests :-) 10:50 < lightlike> jnewbery: I think having multiple block files, and also full blocks with blocks being serialized out of order, would be nice. 10:50 < jnewbery> LarryRuane: also, you don't change functionality, so there shouldn't need to be changes to functional tests! 10:50 < lightlike> *full block files (MB) 10:50 < LarryRuane> the testing for this PR isn't really great, it does have a nice test for the new Skip method, but there really aren't any (new) tests for the changes to validation.cpp .... oh no, not taken that way! 10:50 < jnewbery> but improving functional test coverage in a separate PR seems like it'd be worthwhile 10:51 < LarryRuane> yes i agree, i could take that on if people like that idea ... since i have learned a little about this area already 10:52 < jonatack> +1 on that LarryRuane 10:52 < jnewbery> lightlike: to answer your question about further optimizations, it seems like a lot of the performance degredation is due to blocks being out of order in the blk files. Maybe this is too wacky, but could bitcoind sort those blocks in the background, so that on reindex we're less likely to have to skip forwards and backwards in those files? 10:53 < LarryRuane> ok i would love to ... what's the convention, make a ticket? or just a PR okay? 10:54 < docallag> How often would you expect to reindex? 10:54 < lightlike> jnewbery: do you mean on the fly during IBD, or some kind of reorganization script that can be run on demand? 10:54 < jnewbery> docallag: basically never 10:55 < LarryRuane> that is an interesting idea jnewbery.... i like it .. i suggested a different way to improve this at the end of the last comment i made on the PR (yesterday), maybe take a look at that too .. but my suggestion would create more files in `blocks/` 10:55 < jonatack> LarryRuane: maybe ask for input on #bitcoin-core-dev, could add it as a weekly meeting topic 10:55 < jnewbery> lightlike: I was thinking in the background after IBD, but could also be on demand 10:55 < michaelfolkson> Why do the blocks go out of order in the first place? 10:56 < LarryRuane> headers-first IBD 10:56 < jnewbery> I don't necessarily think it's worth it, since it would be changing mainline behavior to optimize for something we expect never to do 10:56 < LarryRuane> @jnew 10:56 < jnewbery> michaelfolkson: pieter talks about it here: https://podcast.chaincode.com/2020/01/27/pieter-wuille-1.html 10:56 < LarryRuane> jnewbery good point 10:57 < fjahr> jnewbery: I was just going to ask, such on the fly optimization might be hard to get in if it slows down iBD even slightly? 10:57 < nehan_> also crazy on the fly optimizations often increase bug surface area :) 10:57 < jnewbery> fjahr: right, if such an idea was considered, it should be in the background after IBD 10:57 < lightlike> yes, it seems a bit pessimistic to think too much about reindexing during IBD (plus, It's just an hour) 10:57 < fjahr> ah, I see, you wrote after ibd earlier 10:57 < jnewbery> nehan_: +1 10:58 < lightlike> ok, we are almost at the end of the hour. Any questions left? 10:59 < jnewbery> I don't know the serving-blocks-to-other-peers-doing-IBD logic well enough to know whether having blocks serialized on disk in order would be an improvement there too 10:59 < docallag> How would you know you needed to reindex? 10:59 < jnewbery> docallag: https://bitcoin.stackexchange.com/questions/60709/when-should-i-use-reindex-chainstate-and-when-reindex 11:00 < LarryRuane> jnewbery i think *probably* not, because i think the indices record the exact start (and length) of each block (to serve to peers during IBD) 11:00 < docallag> Sorry I meant would Core fall over and then you'd know to reindex? 11:00 < lightlike> let's wrap it up, thanks all for participating! 11:00 < michaelfolkson> Use cases covered earlier in the meeting docollag if that's your question. Basically something goes wrong or you want to do something you can't do 11:00 < LarryRuane> thank you everyone! been great 11:00 < emilengler> thanks for hosting :) 11:01 < nehan_> thank you! 11:01 < emilengler> and thanks to LarryRuane for the PR 11:01 < jnewbery> Great meeting. Thanks for hosting lightlike, and thanks for joining us LarryRuane! 11:01 < lightlike> and thanks LarryRuane for answering questions 11:01 < willcl_ark> thanks lightlike + LarryRuane 11:01 < emilengler> And also thanks to anyone else for his/her feedback 11:01 < emzy> thank you! 11:01 < docallag> tks 11:01 < michaelfolkson> Thanks all. And congrats jonatack :) 11:02 < jnewbery> LarryRuane: I think you're right, but it seems possible that we could be more efficient in serving blocks if they were serialized in order 11:02 < LarryRuane> ah i see, perhaps less seeking 11:02 < jonatack> Thanks lightlike, LarryRuane, jnewbery and everyone! (Thank you, michaelfolkson) 11:02 < jnewbery> like we could have a new P2P message to serve a range of blocks(?) 11:03 < jnewbery> sorry - probably way off topic! 11:03 < LarryRuane> makes sense 11:14 < emilengler> Is this meeting over now? Maybe it is time to do hashtag end meeting 11:15 -!- cprkrn [aed11c6e@110.sub-174-209-28.myvzw.com] has quit [Remote host closed the connection] 11:16 < lightlike> right 11:16 < lightlike> #endmeeting 11:16 < lightlike> (though I'm not sure if there are any bots here that would listen to it) 11:17 < emilengler> Well I didn't wanted to trigger any ;) 11:17 -!- pbleam [4a487bd3@cpe-74-72-123-211.nyc.res.rr.com] has quit [Remote host closed the connection] 11:21 -!- emilengler [~emilengle@stratum0/entity/emilengler] has quit [Quit: Leaving] 11:22 -!- emilengler [~emilengle@stratum0/entity/emilengler] has joined #bitcoin-core-pr-reviews 11:23 -!- Talkless [~Talkless@hst-227-49.splius.lt] has quit [Quit: Konversation terminated!] 11:27 -!- emilengler [~emilengle@stratum0/entity/emilengler] has quit [Client Quit] 11:28 -!- emilengler [~emilengle@stratum0/entity/emilengler] has joined #bitcoin-core-pr-reviews 11:29 -!- emilengler [~emilengle@stratum0/entity/emilengler] has quit [Client Quit] 11:40 -!- vasild_ [~vd@gateway/tor-sasl/vasild] has joined #bitcoin-core-pr-reviews 11:43 -!- vasild [~vd@gateway/tor-sasl/vasild] has quit [Ping timeout: 240 seconds] 11:48 -!- theme [~theme@195.181.160.175.adsl.inet-telecom.org] has quit [Quit: theme] 11:52 -!- LarryRuane [cda8418a@205-168-65-138.dia.static.qwest.net] has left #bitcoin-core-pr-reviews [] 11:58 -!- vasild_ is now known as vasild 12:06 -!- jonatack [~jon@37.167.220.16] has quit [Read error: Connection reset by peer] 12:12 -!- jonatack [~jon@2a01:e0a:53c:a200:bb54:3be5:c3d0:9ce5] has joined #bitcoin-core-pr-reviews 12:18 -!- slivera [~slivera@217.138.204.68] has joined #bitcoin-core-pr-reviews 12:43 -!- docallag [~textual@cpc103056-sgyl39-2-0-cust1444.18-2.cable.virginm.net] has quit [Quit: Textual IRC Client: www.textualapp.com] 12:44 -!- ncantu [~ncantu@37.170.175.201] has joined #bitcoin-core-pr-reviews 12:45 -!- docallag [~~docallag@cpc103056-sgyl39-2-0-cust1444.18-2.cable.virginm.net] has joined #bitcoin-core-pr-reviews 15:23 -!- michaelfolkson [~textual@2a00:23c5:be01:b201:55fc:f43c:fed0:4fc6] has quit [Quit: Sleep mode] 16:18 -!- provoostenator [~quassel@provoostenator.sprovoost.nl] has quit [Remote host closed the connection] 16:19 -!- provoostenator [~quassel@provoostenator.sprovoost.nl] has joined #bitcoin-core-pr-reviews 16:22 -!- provoostenator [~quassel@provoostenator.sprovoost.nl] has quit [Remote host closed the connection] 16:28 -!- provoostenator [~quassel@provoostenator.sprovoost.nl] has joined #bitcoin-core-pr-reviews 16:33 -!- harrigan_ [~harrigan@ptr-93-89-242-235.ip.airwire.ie] has joined #bitcoin-core-pr-reviews 16:35 -!- andrewtoth_ [~andrewtot@gateway/tor-sasl/andrewtoth] has quit [Remote host closed the connection] 16:35 -!- andrewtoth_ [~andrewtot@gateway/tor-sasl/andrewtoth] has joined #bitcoin-core-pr-reviews 16:36 -!- harrigan [~harrigan@ptr-93-89-242-235.ip.airwire.ie] has quit [Ping timeout: 240 seconds] 16:44 -!- pinheadmz [~matthewzi@45.83.89.68] has joined #bitcoin-core-pr-reviews 16:57 -!- TheRec [~toto@drupal.org/user/146860/view] has quit [Read error: Connection reset by peer] 16:58 -!- TheRec [~toto@84-75-225-47.dclient.hispeed.ch] has joined #bitcoin-core-pr-reviews 16:58 -!- TheRec [~toto@84-75-225-47.dclient.hispeed.ch] has quit [Changing host] 16:58 -!- TheRec [~toto@drupal.org/user/146860/view] has joined #bitcoin-core-pr-reviews 16:59 -!- pinheadmz [~matthewzi@45.83.89.68] has quit [Quit: pinheadmz] 17:06 -!- anditto [~anditto@240d:1a:29:b700:940f:87df:b1e:52f7] has joined #bitcoin-core-pr-reviews 17:25 -!- pinheadmz [~matthewzi@45.83.89.68] has joined #bitcoin-core-pr-reviews 17:27 -!- kallewoof [~quassel@240d:1a:759:6000:a7b1:451a:8874:e1ac] has joined #bitcoin-core-pr-reviews 18:14 -!- pinheadmz [~matthewzi@45.83.89.68] has quit [Quit: pinheadmz] 18:29 -!- pinheadmz [~matthewzi@45.83.89.68] has joined #bitcoin-core-pr-reviews 18:40 -!- belcher [~belcher@unaffiliated/belcher] has quit [Quit: Leaving] 18:40 -!- pinheadmz [~matthewzi@45.83.89.68] has quit [Quit: pinheadmz] 19:26 -!- coinsureNZ [67e75b73@103.231.91.115] has joined #bitcoin-core-pr-reviews 19:26 < coinsureNZ> testing testing hello_world 19:30 < kallewoof> hi :) 19:36 < coinsureNZ> thanks for organizing this :) 19:37 -!- akionak [~akionak@2407:c800:3f32:ffee:e5ff:9ca:fc25:ccf] has joined #bitcoin-core-pr-reviews 19:37 -!- akionak [~akionak@2407:c800:3f32:ffee:e5ff:9ca:fc25:ccf] has quit [Client Quit] 19:53 < kallewoof> thanks for participating! hopefully we get more people 19:56 < jnewbery> Sorry they're a bit late, but I've just posted logs from today's meeting: https://bitcoincore.reviews/16981.html 19:57 < kallewoof> thanks jnewbery! 19:57 < meshcollider> A fellow NZer I assume from the username :) 19:58 < meshcollider> Wait did I miss it? Thought it was 5pm NZ time? 19:58 < aj> meshcollider: jnewbery means the one that was at 1am-ish NZ time 19:58 < jnewbery> meshcollider: sorry, I meant logs from the 18:00 UTC meeting 19:58 < kallewoof> That's right. We haven't started yet 19:58 < meshcollider> Ok phew :p 19:58 < luke-jr> why is it here anyway? 19:59 < aj> meshcollider: oh whoops, wrong direction, 7am NZ time 19:59 < kallewoof> luke-jr: the meeting? it's a follow-up PR review meeting, so this seems appropriate. 19:59 < luke-jr> oh 20:00 -!- akionak [~quassel@240d:1a:759:6000:a7b1:451a:8874:e1ac] has joined #bitcoin-core-pr-reviews 20:00 -!- felixfoertsch23 [~felixfoer@92.117.39.73] has joined #bitcoin-core-pr-reviews 20:00 < meshcollider> I think the normal one is 6am atm for me aj 20:01 < kallewoof> meeting time! i've never done one of these (or any meetings for that matter) before, so advice/help/assistance would be great :) 20:01 < jnewbery> hi! 20:01 < kallewoof> #startmeeting 20:01 < aj> hey 20:01 < meshcollider> hi :) 20:01 < akionak> Hi! 20:01 < kallewoof> If you're here, say hi so we know 20:01 -!- felixfoertsch [~felixfoer@i6DFA6845.versanet.de] has quit [Ping timeout: 240 seconds] 20:01 < coinsureNZ> yep thats the one meshcollider , albiet expat now 20:01 < jnewbery> it's 11pm here so I'm not going to stick around for too long. I just wanted to be here at the start of the first one :) 20:01 < anditto> hi! ^_^ 20:01 < kallewoof> or to quote our illustrious leader, < jnewbery> Hi folks! Welcome to Bitcoin Core PR Review Club. Feel free to say hi to let everyone know you're at keyboard. 20:02 < kallewoof> jnewbery: thanks for sticking around :) 20:02 < fanquake> hi 20:02 < meshcollider> Night John 20:02 < kallewoof> I'm going to guide myself through the other log so forgive some amount of copy-pasting. 20:03 < kallewoof> Today's PR is #16981. Notes and questions are here: https://bitcoincore.reviews/16981.html 20:03 < kallewoof> Did everyone get a chance to review the PR? ( y/n ) 20:03 < kallewoof> y 20:03 < coinsureNZ> y 20:03 < jnewbery> kallewoof: I'll post meeting logs sooner in future. I had to run out straight after the meeting today 20:03 < aj> ish 20:03 < coinsureNZ> not the code- just the gist of the nodes 20:03 < fanquake> 1/2 y 20:03 < coinsureNZ> *notes 20:04 < kallewoof> jnewbery: No worries. I am still wondering if it's better to NOT look at logs to reduce bias, but we'll use it this time as I've never done one of these before 20:04 < kallewoof> What are people's general opinion on the PR? 20:05 < kallewoof> Concept/code/approach/etc. 20:05 -!- lightlike_ [~lightlike@p200300C7EF140100657752C6D67C6B18.dip0.t-ipconnect.de] has joined #bitcoin-core-pr-reviews 20:06 -!- kcalvinalvin [~kcalvinal@ec2-52-79-199-97.ap-northeast-2.compute.amazonaws.com] has joined #bitcoin-core-pr-reviews 20:06 < fanquake> Concept ACK improving performance. +1 new test. I can't actually bench it atm though. 20:06 < fanquake> *test code 20:07 < fanquake> I don't think I've actually run a --reindex in quite a while. 20:07 -!- lightlike [~lightlike@p200300C7EF0EC400657752C6D67C6B18.dip0.t-ipconnect.de] has quit [Ping timeout: 240 seconds] 20:08 < jnewbery> goodnight all. Have fun! 20:08 < kallewoof> fanquake: why can't you bench it? 20:08 < kallewoof> jnewbery: night night 20:08 < RubenSomsen> hey guys, and goodnight John :) 20:09 < kallewoof> hi Ruben :) 20:09 < fanquake> I nuked my datadir, and need to redownload block data 20:09 < aj> i wonder how the performance changes depending on the layout of the blocks. i think you get different layouts depending on the network speed of the peers you do IBD from -- if you've got 1 peer that gives you a block per second (~10Mbps), and another that gives you a block per minute (~133kbps), then I think you'll tend to have sets of 60 out of order blocks (from the fast peer) when you're 20:09 < aj> expecting a block from the slow peer 20:10 < kallewoof> aj: does this PR impact that? 20:11 < aj> i think the PR's performance improvement should rely on the "60 out of order blocks" size matching the choice of memory size constant? 20:13 < kallewoof> aj: I don't think I follow. Can you point out file/line no? 20:13 < aj> no not really, maybe i'm way off 20:13 < kallewoof> i don't know :) 20:14 < kallewoof> to summarize what the PR does, IIUC, is to instead of reading every block one at a time, it reads only the block header, then skips over the block if it's not the desired one 20:15 < kcalvinalvin> Does it keep that skipped block in memory? 20:15 < kallewoof> No, I don't think so: https://github.com/bitcoin/bitcoin/blob/a50f956e304adc7c75428ec391f1a76a99c1ddde/src/streams.h#L797-L800 20:17 < aj> ah, the bits i'm thinking of are in the second change which was removed from this PR https://github.com/bitcoin/bitcoin/pull/16981#issuecomment-589791783 20:17 < meshcollider> aj: that's an interesting thought anyway 20:17 < kallewoof> aj: ahh, okay 20:18 < aj> https://github.com/bitcoin/bitcoin/pull/16981#issuecomment-542020115 they were getting an additional ~10% so almost noise by comparison 20:18 < kcalvinalvin> Little bit of a basic question but what data does the txindex=1 save vs when you do txindex=0? 20:19 < kallewoof> aj: I didn't realize there was a 'keep in memory' component. I only looked at this PR yesterday. 20:19 < aj> kallewoof: well there's not any more, so you're right! 20:19 < kallewoof> kcalvinalvin: I think it stores the block hash for each transaction only. Would have to check 20:21 < kallewoof> Any other general opinions on the concept / idea of this code change? Or if someone reviewed it and have questions about the code or such, we can go through that too. 20:23 < aj> kcalvinalvin: txindex is run as a separate process, that triggers off new blocks being accepted, and stores the index data in a separate ldb in .bitcoin/indexes/txindex 20:23 < aj> separate thread, not process i suppose 20:24 < kcalvinalvin> Is reindex also a separate thread? 20:25 < aj> reindex is updating the chain state which is the most important thing, so it's kind of the main thread? 20:26 < fanquake> and it's also using the loadblk thread 20:29 < kallewoof> I'm gonna steal a question from the IRC log, as I was actually unsure about this one myself: In which situations is reindex useful? When is it better to use reindex-chainstate? 20:31 < aj> https://bitcoin.stackexchange.com/questions/60709/when-should-i-use-reindex-chainstate-and-when-reindex -- only when you were pruning or you think your disk might be corrupted? 20:37 < kallewoof> so reindex recreates the index pointing out where in the blk files the blocks are located 20:37 < kallewoof> and also does what reindex-chainstate does. 20:39 < kallewoof> aj: seems you also do it when you enable txindex 20:42 < kallewoof> The notes say "Reindex uses the CBufferedFile stream, introduced in #1962, which has a buffer to allow “rewinding” the stream position to an earlier position without additional disk access." -- does that mean it keeps all the read data from the blk file in memory until it's destroyed? 20:45 < aj> i think it's a fixed size buffer, size declared as second arg to constructor, so 8MB currently 20:47 < aj> kallewoof: i'm pretty sure the new txindex code lets you enable txindex *without* doing a reindex? PR#13033 20:48 < kallewoof> ohh, didn't know that. nice. 20:51 < kallewoof> Does anyone have any other questions about the PR? I think these meetings are partially meant to mentor people through the review process, so don't hesitate to speak up even if you feel unsure of yourself. 20:51 < kallewoof> Me and AJ can keep going for days unless somebody stops us. :P 20:53 < fanquake> I'd be interested if someone on Linux could benchmark this with and without the thread prioritization from SCHED_BATCH, to see how much difference it actually makes 20:54 < fanquake> (unrelated to the changeset though) 20:54 < fanquake> macOS doesn't support SCHED_BATCH so I can't check easily. 20:54 < kallewoof> fanquake: I have a linux full node synced up. How do I toggle SCHED_BATCH, though? 20:54 < fanquake> Easiest way is probably just to comment out the call to ScheduleBatchPriority() 20:55 < fanquake> https://github.com/bitcoin/bitcoin/blob/master/src/init.cpp#L672 20:55 < kallewoof> Ahh, gotcha 20:56 < kallewoof> So if I get this right, the entire reindex part is in init.cpp's ThreadImport function. That while loop runs until it gets to the end and then it continues on with the rest. 20:59 < kallewoof> Yeah that seems to be the case. The author said he put an assert(0) after the "Reindexing finished" and used time to benchmark it. Guess I'll do that, with and without the ScheduleBatchPriority part. 21:02 < kallewoof> Anyway, unless people have more questions or opinions on the PR, I think we can end the meeting. 21:02 < meshcollider> Yeah it all seems sane to me and I don't have any specific questions, I'd like to test it tomorrow when I get back to uni though 21:03 < meshcollider> That seems the most useful thing to do 21:03 < fanquake> SGTM 21:03 < anditto> I don't remember last time I used -reindex & never thought about it, so for this particular PR I'm trying to learn as much as I can. It's also nice to have this meeting in an accessible timezone. Thanks everyone. 21:03 < kallewoof> meshcollider: cool, yeah i'm going to test it on my linux machine with/without the sched batch tweak 21:04 < kallewoof> I'm up for talking about more general bitcoin core related things, but maybe we need to switch to another channel (#bitcoin-core-dev?) 21:04 < meshcollider> Thanks for hosting btw kallewoof :) 21:05 < aj> +1 21:05 < fanquake> Thanks kallewoof 21:05 < kallewoof> my pleasure! I think I'm deviating a bit from the original concept, but I'll read up and do better in the future, if we do these again. 21:05 < anditto> Thanks kallewoof. 21:05 < akionak> Thanks kallewoof 21:06 < kallewoof> I know people were talkign about doing this & a more general bitcoin core related meeting (like the one that is happening tonight in #bitcoin-core-dev). Since we're a smaller crowd it may make sense to combine the two into one. 21:09 -!- ncantu [~ncantu@37.170.175.201] has quit [Ping timeout: 268 seconds] 21:10 -!- ncantu [~ncantu@212.129.39.4] has joined #bitcoin-core-pr-reviews 21:10 < coinsureNZ> thanks guys, didnt have much to add just eager to sit in on these discussions for a while and understand how PRs are evaluated. Its been informative and I appreciate being able to added in a workable time slot 21:10 < RubenSomsen> Thanks kalle :) 21:10 < kallewoof> coinsureNZ: Thanks for joining! 21:11 < kallewoof> #endmeeting 21:11 < kallewoof> (not sure if that does anything but hey) 21:12 < aj> it doesn't do anything, except make it easy for jnewbery to find where to copy and paste logs to the website 21:12 < aj> coinsureNZ: yeah, lurking is great! 21:15 -!- asdsadasd [999c4b41@p4948065-ipngnfx01marunouchi.tokyo.ocn.ne.jp] has joined #bitcoin-core-pr-reviews 21:16 < kallewoof> aj: ah okay. that's good then :) 21:16 < coinsureNZ> hope to be found hiding in the bushes more frequently in the future :') :') :') :') :') :') 21:16 -!- pinheadmz [~matthewzi@45.83.89.68] has joined #bitcoin-core-pr-reviews 21:26 -!- coinsureNZ [67e75b73@103.231.91.115] has quit [Ping timeout: 260 seconds] 21:32 -!- anditto [~anditto@240d:1a:29:b700:940f:87df:b1e:52f7] has quit [Remote host closed the connection] 21:32 -!- anditto [~anditto@240d:1a:29:b700:940f:87df:b1e:52f7] has joined #bitcoin-core-pr-reviews 21:37 -!- anditto [~anditto@240d:1a:29:b700:940f:87df:b1e:52f7] has quit [Ping timeout: 256 seconds] 21:59 -!- soju [uid403160@gateway/web/irccloud.com/x-dsbyvqasqhkaetyq] has joined #bitcoin-core-pr-reviews 22:06 -!- pinheadmz [~matthewzi@45.83.89.68] has quit [Quit: pinheadmz] 22:09 -!- asdsadasd [999c4b41@p4948065-ipngnfx01marunouchi.tokyo.ocn.ne.jp] has quit [Remote host closed the connection] 22:22 -!- anditto [~anditto@240d:1a:29:b700:940f:87df:b1e:52f7] has joined #bitcoin-core-pr-reviews 22:36 -!- ncantu [~ncantu@212.129.39.4] has quit [Ping timeout: 240 seconds] 22:36 -!- ncantu [~ncantu@37.165.58.31] has joined #bitcoin-core-pr-reviews 23:05 -!- andrewtoth_ [~andrewtot@gateway/tor-sasl/andrewtoth] has quit [Ping timeout: 240 seconds] 23:11 -!- anditto [~anditto@240d:1a:29:b700:940f:87df:b1e:52f7] has quit [] 23:40 -!- vasild_ [~vd@gateway/tor-sasl/vasild] has joined #bitcoin-core-pr-reviews 23:43 -!- vasild [~vd@gateway/tor-sasl/vasild] has quit [Ping timeout: 240 seconds]