19:00:10 #startmeeting 19:00:10 Meeting started Thu May 3 19:00:10 2018 UTC. The chair is wumpus. Information about MeetBot at http://wiki.debian.org/MeetBot. 19:00:10 Useful Commands: #action #agreed #help #info #idea #link #topic. 19:00:13 hi 19:00:26 hi 19:00:29 #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 achow101 meshcollider jnewbery maaku fanquake promag provoostenator 19:00:32 hello 19:00:37 hi. 19:00:42 hi 19:00:43 hi 19:00:56 hi 19:01:05 hi 19:01:07 lurking as usual 19:01:23 any other proposed topics? 19:01:51 logging at some point? 19:02:04 what about logging? 19:02:16 I'd like to talk about moving it into a separate thread 19:02:25 ok 19:02:37 #12934 19:02:38 +1 to moving logging to a separate thread 19:02:39 https://github.com/bitcoin/bitcoin/issues/12934 | [WIP] [net] [validation] Call ProcessNewBlock() asynchronously by skeees · Pull Request #12934 · bitcoin/bitcoin · GitHub 19:02:41 let's start with the usual topic 19:03:06 even though it will piss off BlueMatt :) 19:03:07 #topic High prioriity for review 19:03:14 https://github.com/bitcoin/bitcoin/projects/8 19:03:52 only #12979 #12560 #10757 left now 19:03:55 https://github.com/bitcoin/bitcoin/issues/12979 | Split validationinterface into paralell validation/mempool interfaces by TheBlueMatt · Pull Request #12979 · bitcoin/bitcoin · GitHub 19:03:59 https://github.com/bitcoin/bitcoin/issues/12560 | [wallet] Upgrade path for non-HD wallets to HD by achow101 · Pull Request #12560 · bitcoin/bitcoin · GitHub 19:04:05 https://github.com/bitcoin/bitcoin/issues/10757 | RPC: Introduce getblockstats to plot things by jtimon · Pull Request #10757 · bitcoin/bitcoin · GitHub 19:04:16 anything to discuss about those? 19:04:25 anything new to add? or remove? 19:04:49 I'd like #12254 to get some review :-) 19:04:52 https://github.com/bitcoin/bitcoin/issues/12254 | BIP 158: Compact Block Filters for Light Clients by jimpo · Pull Request #12254 · bitcoin/bitcoin · GitHub 19:05:22 There's a whole series of sequential PRs on wallet load/create/unload. It'd be good to start moving through those, but I don't know how high priority they are for others 19:05:42 jnewbery: if it's blocking anyone, the first one on the list should be on there at least 19:05:48 perhaps put the load one in high priority? 19:05:52 #10740 19:05:56 https://github.com/bitcoin/bitcoin/issues/10740 | [wallet] `loadwallet` RPC - load wallet at runtime by jnewbery · Pull Request #10740 · bitcoin/bitcoin · GitHub 19:06:14 It's only blocking because there are other ones queued behind it 19:06:25 jnewbery: that is the most common form of blocking 19:06:26 so it's blocking the continuation ones 19:06:33 That's basically the definition of blocking... 19:06:35 right, it's pretty much the definition of blocking 19:06:37 lol 19:06:39 I agree that we should add 10740 to the list 19:06:46 #10740 given me an unicorn though 19:06:51 https://github.com/bitcoin/bitcoin/issues/10740 | [wallet] `loadwallet` RPC - load wallet at runtime by jnewbery · Pull Request #10740 · bitcoin/bitcoin · GitHub 19:07:01 has also a unicorn, reload solved 19:07:15 yes 19:07:16 unicorns probably have a high street value 19:07:37 added 19:07:39 not any more. The market's been flooded 19:07:49 topic: 0.15.1 19:07:50 shows what I know of unicorn markets 19:07:52 err 16 19:08:12 LukeJr: yes, I"m trying to farm them and sell them, but I have more now than atoms in the knows universe so you could say the supply is more than the demand... 19:08:20 wumpus: could also add #13097 too? 19:08:22 https://github.com/bitcoin/bitcoin/issues/13097 | ui: Support wallets loaded dynamically by promag · Pull Request #13097 · bitcoin/bitcoin · GitHub 19:08:57 promag: ok 19:09:11 ty 19:09:34 #topic Delete 0.8, 0.9 and 0.10 git branches 19:09:43 Background: Those last tagged versions on those branches are EOL for more than a year now. The tags can be kept for archival reasons, but the branches are no longer required. See https://bitcoincore.org/en/lifecycle/#schedule 19:09:49 yes 19:09:53 ack 19:09:57 bye 19:10:00 ack 19:10:02 ack 19:10:17 I didn't know we still had those 19:10:18 is the latest commit of each tagged? 19:10:34 If not, maybe a 0.n_final tag is appropriate 19:10:35 LukeJr: i hope so 19:10:48 LukeJr: agree, will check that first 19:10:58 er, are they not tagged from their respective branches? 19:11:06 jonasschnelli: it wouldn't be the first time we have backported fixes and never released with them 19:11:06 LukeJr: For the 0.8 one not 19:11:23 The others are last tag == tip of branch, last time I checked 19:11:42 will create a v0.8_final for that 19:11:49 +tag 19:12:02 maybe it can be a non-object tag, to avoid confusing users with a recent date 19:12:33 yes fine with me, just to avoid losing history 19:12:48 Oh, and the 0.9 one is missing one commit (upnp) 19:13:06 back 19:13:27 MarcoFalke: so same for 0.9 then 19:14:03 #topic Moving logging to a separate thread (jamesob) 19:14:44 after working on #13099, I think it may be worthwhile to move logging into a separate thread 19:14:46 https://github.com/bitcoin/bitcoin/issues/13099 | Use thread names in logs and deadlock diagnostics by jamesob · Pull Request #13099 · bitcoin/bitcoin · GitHub 19:15:03 esp. given instances like #12970 19:15:04 ACKACKACKACKACKACKACKACKACKACK (this is a surprisingly high lag-creator for miners, at least for those with spinning-disk-backed-or-cloud-hosted machines 19:15:06 https://github.com/bitcoin/bitcoin/issues/12970 | logging: bypass timestamp formatting when not logging by theuni · Pull Request #12970 · bitcoin/bitcoin · GitHub 19:15:17 queue log messages to a ring buffer ? 19:15:26 wumpus: sure, something along those lines 19:15:32 gmaxwell has been proposing that for ages... 19:15:35 see-also commit which does this at https://github.com/bitcoinfibre/bitcoinfibre/commit/6b6a3aef0663775b63bac7d0aa07ec5fc4eb9fc9 19:15:45 I started working on a branch for this last year, but didn't get very far 19:15:51 definite concept ACK 19:15:56 yes, concept ACK 19:16:16 I did not propose it upstream as it means if we LogPrintf(); assert(false) we'll likely miss it 19:16:18 means the message processing thread can't be blocked on i/o hangs 19:16:21 and I figured folks wouldnt want that 19:16:30 ACK - should point out that it could make crash debugging substantially more complicated 19:16:39 jnewbery: it still will be a ton cause it ReadBlockFromDisk()s 19:16:40 BlueMatt: we could have special priority log calls that always make it to disk? 19:16:46 skeees: right, I think that's the only caveat I can think of 19:16:50 BlueMatt: surely there's a way to run log flushing at assert 19:16:53 BlueMatt: CriticalLogPrintf? 19:16:53 +1 BlueMatt point 19:16:56 are there any possible interactions where someone might be tailing the log and assume that it's synchronous? 19:17:04 use gdb 19:17:15 wumpus: I mean the number of times its been helpful just to see which lines actually made it to debug.log when someone submitted a crash is super helpful 19:17:21 cfields: it couldn't be? 19:17:25 and gdb isnt really an option when debugging remotely over github issues :/ 19:17:32 Does anyone know the relative performance of logging to console vs debug log file? 19:17:50 jimpo: on non-serial-consoles, console logging should be fast, serial consoles can block 19:17:54 in theory, we could fork() a logging-only process, but that feels ugly 19:17:56 (or vtt on a slow-refresh-monitor) 19:17:59 We should fork a separate process for logging, and then open a FIFO with it and pipe the log data there. If bitcoind crashes, that process hopefully survives :) 19:18:02 !hi5 LukeJr 19:18:03 Error: "hi5" is not a valid command. 19:18:17 it's mostly a win for low priority debugging messages, I guess 19:18:21 or we could just make it optional 19:18:26 if logging is low volume it's never a bottleneck 19:18:35 miners can enable it, everyone else probably doesnt need to 19:18:47 only miners that have debug=net enabled? 19:18:51 just throwing out some alternatives with a properly tuned buffer cache - logging shouldn't hit disk that often? if its still flushing frequently - could consider a more compact binary format for logs - though that means you'd need a special tool to read them 19:19:18 I feel like disabling logging to file and piping stdout through tee or something should work fine 19:19:38 skeees: well the logging is unbuffered at this moment so can't do much worse than that... 19:19:38 wumpus: I mean -debug should always mean non-async loggin, I think 19:19:50 skeees: it's not just hitting disk, it's serialization as well. Though I guess we couldn't defer serialization if references are passed in. 19:19:51 even line-buffered would be better for performance 19:20:12 wumpus: I thought fwrite was buffered? 19:20:12 BlueMatt: right 19:20:15 another option is to have debug logging compiled out 19:20:25 instead of just disabled via flag as it is currently 19:20:30 jamesob: yes, but the buffer is disabled after opening 19:20:40 I mean I dont think people who want performance dont want a debug.log 19:20:48 they just dont want it to result in 8-ms pauses 19:21:00 skeees: that's not the issue; debug logging is already not executed if it's disabled 19:21:03 * LukeJr wonders if we skip serialization when the debug log level is such that it will not be logged 19:21:08 skeees: even formatting is bypassed in that case 19:21:19 LukeJr: yes 19:21:24 LukeJr: yeah that's exactly the question i was trying to ask 19:21:26 ah ok 19:21:58 i dunno actually 19:22:02 tbh I don't think there's an actual issue here 19:22:09 because theres a lot of uint256.ToString() stuff 19:22:15 operations on incoming vars aren't skipped though, I believe. Like LogPrint("foo: %s", foo.get()) 19:22:18 the only issue here is for folks who care a shitload about small disk pauses 19:22:22 although the ring buffer would be nice because of gmaxwell's argument (log at higher debug message, but don't log the messages to disk unless a crash) 19:22:22 hence why I use it in fibre 19:22:58 when we get ReadBlockFromDisk to be async for peer handling, then it may make more sense to revisit this 19:23:11 so a crash would log the last low-priority debug messages even if debugging is disabled.. then agian, it's not possible to bypass formatting anymore in that case 19:23:25 we'd have to swap assert() for dump_log_and_crash() 19:23:36 not sure that even requires logging to be in a sepaerate thread 19:23:41 wumpus: not sure if formatting is a big deal tbh 19:23:50 BlueMatt: or handle SIGABRT? 19:23:51 no, but then I dont get to upstream my lockless ring buffer logger :p 19:23:54 LukeJr: well some messages are extremely high volume 19:24:01 luke-jr: I dont think we want to do that much work in a signal handler 19:24:03 LukeJr: try enabling *all* debugging for fun 19:24:31 matt@bitcoin-seednode:~$ ls -lha ~/.bitcoin/debug.log 19:24:31 -rw------- 1 matt matt 8.6T May 3 19:24 /home/matt/.bitcoin/debug.log 19:24:41 heh 19:24:51 how long? 19:24:54 BlueMatt: if it's plain old C code, we can probably get away with it 19:25:04 hence my proposal at some point to split up net logging into low and high volume messages 19:25:14 * jamesob buys BlueMatt a pair of socks with the logrotate manpage on them 19:25:23 sipa: 2014-11-19 00:06:55 19:25:26 #12219 19:25:28 https://github.com/bitcoin/bitcoin/issues/12219 | More granular net logging · Issue #12219 · bitcoin/bitcoin · GitHub 19:25:40 wumpus: +1. Will have a look. 19:26:02 I do like the flush-debug-ring-to-disk-on-crash idea 19:26:09 yes 19:26:09 +1 19:26:24 (would mean serializing all debug messages, though) 19:26:38 maybe even on non-crash critical errors 19:26:57 non-crash critical errors should crash :p 19:27:09 it's in the word 'critical' 19:27:55 okay so unless anyone has any objections, I'll start working on a thread-consumes-from-ring-buffer implementation in the near future 19:28:04 I think I killed the topic 19:28:05 oh 19:28:10 jamesob: ACK 19:28:21 jamesob: I think only as optional except for disabled-debug-messages 19:28:37 i.e. async logging should be opt-in? 19:28:38 jamesob: but if upstream wants to maintain by fibre patches, fine by me :p 19:28:41 yes 19:28:48 also, you may want to start with https://github.com/bitcoinfibre/bitcoinfibre/commit/6b6a3aef0663775b63bac7d0aa07ec5fc4eb9fc9 19:29:07 I'll give it a look, thanks 19:29:42 #topic 0.16.1 (BlueMatt) 19:30:06 so for those who weren't paying attention, skeees found some particularly novel races in block handling in #13092 19:30:07 https://github.com/bitcoin/bitcoin/issues/13092 | ActivateBestChain concurrency issues · Issue #13092 · bitcoin/bitcoin · GitHub 19:30:21 cause they're threading issues they almost certainly wont effect anyone except submitblock users 19:30:29 ie miners, and only rare races 19:30:42 but, still, I think given that and some of the other various fixes we've had, it may be worth backporting 19:31:11 13092 is an issue, not a PR, can't label it for backport 19:31:14 further, at least personally, I'd kinda like to see #11423 make some movement, and making that kind of policy change in a non-minor-version strikes me as weird 19:31:17 https://github.com/bitcoin/bitcoin/issues/11423 | [Policy] Make OP_CODESEPARATOR and FindAndDelete in non-segwit scripts non-std by jl2012 · Pull Request #11423 · bitcoin/bitcoin · GitHub 19:31:25 yes, it needs a pr, sdaftuar has a branch, I believe, but not open 19:31:35 skeees has a pr, one sec 19:31:46 #13023 19:31:46 #13023 19:31:48 was the skeees pr updated for the new method? 19:31:48 https://github.com/bitcoin/bitcoin/issues/13023 | Always refresh most work chain when reacquiring cs_main in ActivateBestChain() by skeees · Pull Request #13023 · bitcoin/bitcoin · GitHub 19:31:50 https://github.com/bitcoin/bitcoin/issues/13023 | Always refresh most work chain when reacquiring cs_main in ActivateBestChain() by skeees · Pull Request #13023 · bitcoin/bitcoin · GitHub 19:31:55 no we need to settle on the right fix 19:32:01 still needs a bit of work - i need to pull in some fixes @sdaftuar made 19:32:09 i think the fix i proposed works, but maybe someone has a better idea 19:32:26 ok added tags 19:32:27 ok, yes, I think that is doable, however, and I think between that and making progress on the standardness changes from jl2012 probably are worth a minor bump 19:32:37 there's also #12988 - which is a similar type of bug 19:32:40 https://github.com/bitcoin/bitcoin/issues/12988 | Hold cs_main while calling UpdatedBlockTip() signal by skeees · Pull Request #12988 · bitcoin/bitcoin · GitHub 19:33:07 yes, indeed, that too 19:33:18 agreed 19:33:30 ok 19:33:55 (so, obviously, for those following along at home, next-milestone usually gets auto-high-priority-for-review status :p) 19:34:19 re: #11423, I rebased that one for jl2012, not sure if there's further things to be done there 19:34:22 https://github.com/bitcoin/bitcoin/issues/11423 | [Policy] Make OP_CODESEPARATOR and FindAndDelete in non-segwit scripts non-std by jl2012 · Pull Request #11423 · bitcoin/bitcoin · GitHub 19:35:12 added 0.16.1 tag there too 19:35:18 I believe jl2012 wanted to add one more policy rule there, which should be done, there was a branch floating around but I cant find it atm 19:35:25 anyway, thats for discussion on that pr 19:35:47 I'll ping jl2012 since he's probably asleep 19:36:04 anyway, seems no disagreement, so next topic? 19:36:06 yes, but it is the open PR with the most (ut)ACKs so I thought it'd be almost ready for merge 19:36:34 that's why I rebased it with the idea to merge it after final review 19:36:38 but ok 19:36:54 Yeah, would need some re-ACKs first, though 19:37:19 right, but if the goalposts keep moving 19:37:41 I think we've exhausted all proposed topics 19:37:52 If there are minor fix ups, they should probably happen soon 19:38:21 oh, no, wait i had another topic 19:38:34 topic: activate segwit 19:38:38 topic 2: #12934 19:38:40 https://github.com/bitcoin/bitcoin/issues/12934 | [WIP] [net] [validation] Call ProcessNewBlock() asynchronously by skeees · Pull Request #12934 · bitcoin/bitcoin · GitHub 19:39:23 its certainly not ready for review, but we should maybe have a discussion about what concurrency across peers is gonna look like 19:39:24 #topic Call ProcessNewBlock() asynchronously (skeees) 19:39:40 there are two main approaches, but both end up requiring similar refactors for the majority of their work 19:39:51 in the past I've looked at doing ProcessMessages() in parallel 19:40:07 in this pr skeees moves the validation processing of txn/blocks into a queue and does that in a separate thread 19:40:31 how does that interact with eg sending a ping after a block, and expecting it to be processed after the pong returns? 19:40:34 in both cases, we end up building logic to "pause" processing of a peer until whatever message it just generated has been processed 19:40:36 that sounds sensible, though I'm really afraid of c++ threading 19:40:43 sipa: ^ 19:40:51 Seems like it would add more locking overhead. 19:40:55 (which would also likely be useful for eg ReadBlockFromDisk moving to be more async) 19:41:31 sound reasonable to me 19:41:32 and a ton of logic to move CNodeState out of cs_main (likely by creating a CNodeState2 during transision) 19:41:43 cfields: may have more thoughts 19:41:43 that sounds even better 19:41:48 the other side benefit of this approach is that ultimately it may simplify the concurrency model inside validation 19:41:53 though everything depends on the details... 19:42:06 gmaxwell: indeed, thats a major difference between the skeees approach and the parallel ProcessMessages approach 19:42:09 having a single thread processing from an ordered queue would have also solved the concurrency issues discussed before 19:42:21 ProcessMessages will still do the work on the processing thread, which should have a bit less locking overhead 19:42:21 BlueMatt: just reading now. at a glance, this approach sounds similar to what I had in mind as well 19:42:28 but is definitely more complicated than the skeees approach 19:42:38 which draws a much cleaner boundary between validation and net_processing 19:42:41 The biggest gain I'm aware of from concurrency is that right now when connecting multiple blocks at a time (due to out of order fetching in IBD) we stop downloading new blocks, ... fixing that would be a big gain; so it might be worth prioritizing improvements that would let that happen. 19:42:54 BlueMatt: I don't see why they'd be mutually exclusive? 19:43:07 gmaxwell: that and relaying compact block gettxn during block connection 19:43:25 which is the one big cheapish win left for block-relay-latency 19:43:29 (e.g. esp when fetching from slow peers, watching network traffic during IBD is comical... transfering at only a couple mbps for a while then nothing for seconds at a time while it connects) 19:43:34 cfields: well doing both is maybe not so useful 19:43:54 since they'd accomplish largely the same thing 19:44:13 BlueMatt: Fair enough for gettxn, though right now my own node makes almost no gettxn requests... and orphaning rates appear to be exceptionally low. 19:44:30 (so what I'm saying is that at the moment I don't think of latency improvements as that critical) 19:44:33 gmaxwell: the skeees approach is certainly better for doing things like deserialization of blocks coming in from other peers while calling ProcessNewBlock/ActivateBestChain 19:44:52 indeed, right now (with mempool ~empty) compact block performance is ~perfect 19:44:55 its not always so, however 19:45:16 hm. interesting point that making deserialization concurrent would potentially be a multi-core gain. 19:45:17 also, fibre nodes see more getblocktxn than miners 19:45:17 by "mempool ~empty" you mean "mempool is a perfect match" ? 19:45:33 which would be equally solved by miners delaying 1 second before including new txn in blocks.... 19:45:54 sipa: if each block mostly clears the mempool, miner-specific prioritizaition of transactions doesn't cause as much surprise txn. 19:46:13 BlueMatt: and also making use of the CB facility for transmitting extra txn proactively. 19:46:24 yea, that too 19:46:52 anyway, so tl;dr: I was a bigger fan of the skeees approach and wanted a little bit of concept discussion on the idea of putting a queue in front of the entire validation stuff 19:46:59 since thats a huge departure from current operation 19:47:10 but I think the potential gain is nice 19:47:17 plus all our blocks are already passed in as shared_ptrs anyway..... 19:47:21 so right my point was that speading up IBD is probably a big win, .. improving latency and what not would be nice too but I think it's less important. 19:47:42 +1 19:47:43 yes, right now that is definitely true 19:47:51 certainly this has the potential to address both, however 19:47:56 making use of more cores during sync would be nice too, e.g. if deserilization and hashing can happen on seperate threads async with connection. 19:48:13 we can do CheckBlock on the net_processing thread 19:48:27 (since, and sipa is gonna die a little bit inside, here, the result is cached in the CBlock structure) 19:48:44 that is kinda gross :) 19:48:52 its incredibly gross 19:48:59 (e.g. multiple message handling threads, which do deserialization and maybe stateless checks) 19:49:21 oh no... 19:49:24 [13bitcoin] 15practicalswift opened pull request #13163: Make it clear which functions that are intended to be translation unit local (06master...06internal-linkage) 02https://github.com/bitcoin/bitcoin/pull/13163 19:49:51 though I did a dumb hack a while back that reordered some of the block validation steps to interleave checks that operate per transaction and got a few percent sync speed improvement... we should be mindful of memory locality and not introduce even more passes over blocks. 19:50:05 gmaxwell: dunno about multiple message handling threads being doable in the immediate future, but doing CheckBlock on the message processing thread (which does merkle root verification, which is a good chunk of the work) may be worth a chunk 19:50:27 cool - so if no strong objections or concerns with the approach i'll continue this and come back when its more ready for review 19:51:07 Funny, I would have thought handling seperate peers in seperate threads would almost just work now, vs stuff that handled messages from the same peer concurrently, which violates protocol assumptions about ordering without special work to add processing barriers. 19:51:25 gmaxwell: CNodeState requires cs_main, so....not even close :( 19:51:40 (almost just work, because the critical data structues have locks ... oh CNodeState) 19:51:45 I've got some old branches that do that, if you want to look 19:51:48 cs_main lite. 19:51:50 but they're....not small changes 19:52:03 and cfields hates them, though he's probably right 19:52:26 anyway, so ["DIE", "XML"]? 19:52:29 in particular our most commons messages are transaction invs and get datas, and those should only need mempool stuff for much of their activity. 19:52:36 k. 19:52:44 gmaxwell: yes, I did all that in an old branch 19:52:51 that in particular is much closer than it used to be 19:53:00 cause now the HandleGetData stuff cant take cs_main at the top 19:53:10 IIRC that refactor landed for 0.16 19:54:08 https://github.com/TheBlueMatt/bitcoin/commits/2017-07-paralell-processmessages-redux 19:55:13 Thanks. 19:55:44 (it got a bunch of time in helgrind, too, and got all the issues fixed, so I'm at least kinda confident in it, but its essentially unreviewable, even by my standards) 19:57:40 #endmeeting