19:00:30 <wumpus> #startmeeting
19:00:30 <lightningbot> Meeting started Thu May 23 19:00:30 2019 UTC.  The chair is wumpus. Information about MeetBot at http://wiki.debian.org/MeetBot.
19:00:30 <lightningbot> Useful Commands: #action #agreed #help #info #idea #link #topic.
19:00:41 <sipa> ohai
19:00:50 <wumpus> #bitcoin-core-dev Meeting: wumpus sipa gmaxwell jonasschnelli morcos luke-jr sdaftuar jtimon cfields petertodd kanzure bluematt instagibbs phantomcircuit codeshark michagogo marcofalke paveljanik NicolasDorier jl2012 achow101 meshcollider jnewbery maaku fanquake promag provoostenator aj Chris_Stewart_5 dongcarl gwillen jamesob ken281221 ryanofsky gleb moneyball kvaciral
19:00:58 <achow101> hi
19:01:01 <promag> hi
19:01:05 <fanquake> Hi
19:01:08 <achow101> only here for 30 min or so (if the meeting even goes that long)
19:01:15 <meshcollider> hi
19:01:15 <jamesob> hi
19:01:47 <gwillen> hi
19:01:49 <wumpus> proposed topics? (none on moneyball 's list)
19:02:10 <phantomcircuit> hi
19:02:38 <jonasschnelli> hi
19:02:46 <Chris_Stewart_5> hello
19:03:03 <wumpus> hello
19:03:23 <wumpus> #topic High priority for review
19:03:32 <wumpus> https://github.com/bitcoin/bitcoin/projects/8
19:04:16 <wumpus> current PRs: #15427 #15741 #15759 #15024 #15649
19:04:20 <gribble> https://github.com/bitcoin/bitcoin/issues/15427 | Add support for descriptors to utxoupdatepsbt by sipa · Pull Request #15427 · bitcoin/bitcoin · GitHub
19:04:21 <moneyball> hi
19:04:23 <gribble> https://github.com/bitcoin/bitcoin/issues/15741 | Batch write imported stuff in importmulti by achow101 · Pull Request #15741 · bitcoin/bitcoin · GitHub
19:04:26 <gribble> https://github.com/bitcoin/bitcoin/issues/15759 | [p2p] Add 2 outbound blocks-only connections by sdaftuar · Pull Request #15759 · bitcoin/bitcoin · GitHub
19:04:27 <gribble> https://github.com/bitcoin/bitcoin/issues/15024 | Allow specific private keys to be derived from descriptor by meshcollider · Pull Request #15024 · bitcoin/bitcoin · GitHub
19:04:30 <gribble> https://github.com/bitcoin/bitcoin/issues/15649 | Add ChaCha20Poly1305@Bitcoin AEAD by jonasschnelli · Pull Request #15649 · bitcoin/bitcoin · GitHub
19:04:36 <kanzure> hi
19:04:37 <jamesob> can I ask that #15976 be added? it's prone to rebase conflicts and pretty close to ack-threshold for merge (I think)
19:04:44 <gribble> https://github.com/bitcoin/bitcoin/issues/15976 | refactor: move methods under CChainState (pt. 1) by jamesob · Pull Request #15976 · bitcoin/bitcoin · GitHub
19:04:54 <jamesob> (and blocks the assumeutxo project)
19:04:55 <wumpus> jamesob: of course
19:05:13 <jamesob> wumpus: thanks
19:05:16 <fanquake> done
19:05:20 <dongcarl> Could we add #16059? Small but without it linux gitian builds are broken right now
19:05:22 <gribble> https://github.com/bitcoin/bitcoin/issues/16059 | configure: Fix thread_local detection by dongcarl · Pull Request #16059 · bitcoin/bitcoin · GitHub
19:06:02 <wumpus> dongcarl: sure
19:06:17 <wumpus> fanquake: thanks
19:06:18 <jnewbery> hi
19:06:26 <fanquake> I think that can be merged quite soon. I'm testing it at the moment.
19:06:37 <fanquake> Have added to high prio.
19:06:40 <dongcarl> thank you
19:06:45 <wumpus> why are we using thread_local again btw?
19:06:55 <jamesob> for thread names
19:06:56 <wumpus> I thought that was removed at some point
19:07:12 <jamesob> cory and I came up with a way to do without at some point but it was pretty convoluted
19:08:19 <fanquake> It's also linux and openbsd? only atm. Broken on win, freebsd and we can't use it on macOS until we are using a newer sdk.
19:08:34 <wumpus> doesn't pthread have a way to keep track of thread names?
19:09:13 <wumpus> I thought that was how we did that, anyway
19:09:17 <jonasschnelli> fanquake: you mean SDK for the depends build?
19:09:21 <wumpus> that'll also show up in top and such
19:09:28 <jamesob> there's some posix key-value store API we were using with the pthreads id to avoid thread_local, but it ended up being a lot of code IIRC
19:09:49 <wumpus> pthread_setname or such
19:10:11 <wumpus> pthread_getname_np
19:10:16 <fanquake> jonasschnelli yes. Although now I think about it, we are building against 10.11 which I think means we can use it. Have to double check.
19:10:40 <jonasschnelli> Also since this is nice for debugging, the depends build matters not too much
19:11:23 <sipa> wumpus: https://github.com/bitcoin/bitcoin/blob/master/src/util/threadnames.cpp
19:11:56 <wumpus> do we have a whole util to keep track of thread names? wow
19:11:57 <ryanofsky> #16059 is a straightforward fix, simpler than rewriting thread names imo...
19:12:00 <gribble> https://github.com/bitcoin/bitcoin/issues/16059 | configure: Fix thread_local detection by dongcarl · Pull Request #16059 · bitcoin/bitcoin · GitHub
19:12:11 <jamesob> ryanofsky: agree
19:12:26 <wumpus> I had no idea it was such a big issue, anyhow, any other topics?
19:12:40 * luke-jr for one finds thread names useful sometimes
19:12:47 <jamesob> we avoided doing pthread_getname because there are supposedly implementation problems (https://stackoverflow.com/questions/2369738/how-to-set-the-name-of-a-thread-in-linux-pthreads/7989973#7989973). see also the notes at the bottom of this PR description: https://github.com/bitcoin/bitcoin/pull/13099
19:12:48 <gribble> https://github.com/bitcoin/bitcoin/issues/7989973 | HTTP Error 404: Not Found
19:14:17 <wumpus> jamesob:thanks
19:15:36 <meshcollider> Not a topic, but I won't be able to attend tomorrows wallet meeting so could someone please volunteer to host it?
19:15:54 <wumpus> anyonw?
19:15:57 <sipa> i'll be here, if there's interest
19:16:15 <fanquake> Maybe #15993 for a topic? Has been through a few iterations but seems to be more ready now?
19:16:17 <jnewbery> I'll be there
19:16:18 <gribble> https://github.com/bitcoin/bitcoin/issues/15993 | net: Drop support of the insecure miniUPnPc versions by hebasto · Pull Request #15993 · bitcoin/bitcoin · GitHub
19:16:39 <wumpus> fanquake:I think that should just be merged?
19:16:49 <wumpus> fanquake: is there anything to discuss about it?
19:17:26 <luke-jr> did configure finally get updated?
19:17:38 <luke-jr> not that I see..
19:17:48 <wumpus> I don't know...
19:17:51 <MarcoFalke> Seems like it is just shuffling things around and not actually dropping support
19:17:59 <fanquake> wumpus I haven't tested any of the changes, just seemed there was discussion about which versions to drop support for, and wether other versions had actually been patched.
19:18:20 <wumpus> it's dropping support for <10, which is fine, the only thing controversial was <14
19:18:21 <luke-jr> looks like it would detect older miniupnpc libraries, then fail to compile
19:18:36 <wumpus> because that's still in debian table
19:18:38 <wumpus> stable
19:19:30 <fanquake> ok.
19:19:42 <fanquake> One other topic is suggestions for 0.18.1 backporting that aren't already in #16035
19:19:44 <gribble> https://github.com/bitcoin/bitcoin/issues/16035 | 0.18.1: Backports by MarcoFalke · Pull Request #16035 · bitcoin/bitcoin · GitHub
19:19:46 * wumpus would prefer to drop support for miniUPnP completely but, don't feel like having that discussion
19:20:00 <wumpus> #topic backport suggestions for 0.18.1
19:21:23 <wumpus> anything?
19:21:47 <promag> maybe #14984?
19:21:49 <gribble> https://github.com/bitcoin/bitcoin/issues/14984 | rpc: Speedup getrawmempool when verbose=true by promag · Pull Request #14984 · bitcoin/bitcoin · GitHub
19:22:17 <wumpus> it's not really a bugfix
19:22:19 <MarcoFalke> It is not strictly a bugfix.
19:22:22 <fanquake> Apparently that isn't actually a clean backport, and maybe not worth it if it's just perfomance gain.
19:22:23 <wumpus> unless performance is *really* horrible right now
19:22:52 <MarcoFalke> Only when the mempool is large ;)
19:22:53 <wumpus> if it's non-trivial to backport too then better not to, I think
19:23:15 <promag> right, large mempool, about 30% faster
19:23:27 <promag> no problem, just asked
19:23:38 <wumpus> no problem, thanks for suggesting something
19:24:14 <MarcoFalke> I am mostly waiting on those to get merged: https://github.com/bitcoin/bitcoin/milestone/41
19:24:28 <wumpus> are we planning on doing a 0.18.1 soon btw?
19:24:38 <wumpus> I mean, is there anything motivating it?
19:25:10 <MarcoFalke> nothing urgent, no
19:25:11 <promag> ops, forgot about #15453
19:25:12 <gribble> https://github.com/bitcoin/bitcoin/issues/15453 | Starting bitcoin-qt with -nowallet and then opening a wallet does not show the wallet · Issue #15453 · bitcoin/bitcoin · GitHub
19:25:46 <fanquake> A couple of bug fixes for potentially confusing behaviour, like #15952, but nothing catastrophic
19:25:48 <gribble> https://github.com/bitcoin/bitcoin/issues/15952 | Cant open wallet · Issue #15952 · bitcoin/bitcoin · GitHub
19:25:49 <MarcoFalke> oh, maybe the gcc compile bug?
19:26:05 <wumpus> MarcoFalke:yes maybe that
19:26:13 <MarcoFalke> But it seemed to only affect the tests, so ... flip a coin?
19:26:16 <wumpus> would be good to have that out of the way
19:26:23 <wumpus> we don't *know*
19:26:36 <MarcoFalke> jup
19:26:39 <fanquake> #15983
19:26:41 <gribble> https://github.com/bitcoin/bitcoin/issues/15983 | build with -fstack-reuse=none by MarcoFalke · Pull Request #15983 · bitcoin/bitcoin · GitHub
19:27:25 <fanquake> Maybe wait another week or two then, assuming outstanding PRs are merged, cut an rc1 ?
19:27:33 <wumpus> that definitely needs backport to 0.18
19:27:55 <MarcoFalke> It is the first thing in #16035
19:27:57 <gribble> https://github.com/bitcoin/bitcoin/issues/16035 | 0.18.1: Backports by MarcoFalke · Pull Request #16035 · bitcoin/bitcoin · GitHub
19:27:59 <wumpus> it's labeled for 0.17.2 but that's not very likely  to happen
19:28:01 <wumpus> oh okay
19:28:04 <sipa> has anyone done general benchmarks to see the affect of -fstack-reuse=none ?
19:28:10 <fanquake> jamesob
19:28:32 <fanquake> sipa https://github.com/bitcoin/bitcoin/pull/15983#issuecomment-490700321
19:28:34 <jamesob> I tried but I don't think I was doing it right
19:28:35 <MarcoFalke> sipa: We couldn't find any significant changes on our hardware
19:28:46 <wumpus> I doubt it affects performance at all
19:28:47 <sipa> MarcoFalke: that's not surprising, but good to confirm
19:28:48 <wumpus> just stack use
19:29:48 <gmaxwell> My only concern was that it would break some other performance optimization, sounds like it doesn't.
19:30:00 <wumpus> anyhow, upgrade to a compiler that doesn't have the bug (when there's one)
19:30:10 <wumpus> performance loss is preferable to random unpredictable corruption
19:30:22 <wumpus> certainly for something like bitcoin
19:30:29 <MarcoFalke> right
19:31:24 <wumpus> (I mean it's obviously different if say, validation becomes two times as slow or something extreme like that, but we'd have noticed)
19:31:28 <gmaxwell> if the performance loss were truly significant though, we might want to look for other alternative workarounds, good that we don't need to.
19:31:34 <wumpus> of course
19:31:56 <sipa> agree
19:32:01 <wumpus> any other topics?
19:32:45 <promag> is this worth it? https://github.com/bitcoin/bitcoin/pull/16065#issuecomment-494853746
19:33:54 <wumpus> avoiding hashing is always the best sha256 optimization :)
19:34:34 <jonasschnelli> ;-)
19:34:43 <wumpus> (but if it's worth it depends on what slice of the total the 2.44s is, if it's on the whole initial sync it's neglible)
19:35:05 <fanquake> If anyone can reproduce #16027 that might be handy. Has come up a couple times now.
19:35:06 <gribble> https://github.com/bitcoin/bitcoin/issues/16027 | client 0.18.0 crashes when computer wakes up from hibernation · Issue #16027 · bitcoin/bitcoin · GitHub
19:35:14 <MarcoFalke> Yeah, sync is ~hours, so a second doesn't matter at all
19:35:28 <wumpus> MarcoFalke: right
19:35:29 <promag> wumpus: right, agree at the moment saving a couple of seconds isn't worth it
19:35:31 <gmaxwell> that sounds like it maybe more more relevant to propagation at the tip.
19:35:46 <wumpus> and that's within the measuring noise isn't it?
19:36:06 <gmaxwell> e.g. time from getting a block to sending the first non-HB peer.
19:36:31 <wumpus> gmaxwell: yes, that latency would be useful to measure
19:36:41 <wumpus> if it makes a significant difference there then it could still be worth it
19:36:48 <gmaxwell> the fact that we're rehashing in general suggests we've got something kinda wrong, layout wise.
19:36:55 <wumpus> true
19:37:27 <promag> ok, I'll keep doing those profiles
19:37:40 <MarcoFalke> Could just cache the hash in the data structure?
19:37:40 <wumpus> depends also on how much more complicated it makes the code, or whether it increases memory use, etc
19:37:58 <wumpus> how much it increases memory use, of course caching increases memory use
19:38:03 * MarcoFalke ducks
19:38:07 <sipa> jamesob: mr profiling... do we have any way to generally benchmark block-propagation-speed-at-synced-tip ?
19:38:39 <jamesob> mmm short of parsing verbose debug.log, I don't think so. not built into bitcoinperf yet, at any rate :)
19:38:41 <promag> MarcoFalke: the hash is only necessary while processing/validating
19:39:07 <sipa> jamesob: i think it would be very valuable to have; especially for scenarios where the block's transactions have already been validated in the mempool
19:39:25 <sipa> as there are very relevant performance improvements there that'd generally be dwarfed by script validation
19:39:41 <MarcoFalke> sipa: Would that require two nodes?
19:39:53 <MarcoFalke> or are you talking about a micro benchmark
19:39:56 <jamesob> sipa: yeah, agree that's a metric worth tracking
19:40:05 <jamesob> happy to build something for it
19:40:22 <sipa> perhaps we should do some brainstorming about that, but maybe outside the meeting
19:40:22 <wumpus> jamesob: cool!
19:40:46 <fanquake> If anyone is interested in guix building, dongcarl has been doing a lot of work in #15277. Would be good to get some more builds to compare hashes with.
19:40:48 <gribble> https://github.com/bitcoin/bitcoin/issues/15277 | [Help Wanted] contrib: Enable building in Guix containers by dongcarl · Pull Request #15277 · bitcoin/bitcoin · GitHub
19:41:00 <MarcoFalke> sipa: If it is for bitcoinperf: https://github.com/chaincodelabs/bitcoinperf/issues
19:41:03 <gmaxwell> sipa: there are tests in bitcoinfibre.
19:41:07 <wumpus> fanquake: if there's clear instructions to test, I'm happy to try
19:41:16 <promag> ah btw, one thing that takes a while is base_uint<BITS>& base_uint<BITS>::operator>>=(unsigned int shift)
19:41:37 <dongcarl> wumpus: I'll update and link you
19:41:44 <wumpus> dongcarl: great !
19:41:46 <sipa> promag: inside the division logic for retargetting that's expected
19:41:51 <sipa> most of the time is in shifts
19:42:14 <gmaxwell> MarcoFalke: The logical place to cache hashes in block objects themselves, doing so there is irrelevant memory use wise (32 bytes in a 1.2MB object), but complicates constness.
19:42:48 <fanquake> I have a setup/container guide for a quick Guix setup here as well: https://github.com/fanquake/core-review/tree/master/guix
19:43:03 <gmaxwell> promag: if thats actually taking a non-trivial amount of some real usecase, the division could be made faster... it uses a fairly naieve algorithim right now.
19:43:05 <wumpus> agree it's irrelevant on block objects, it's mostly CBlockIndex where it counts because so many of them are permanently in memory
19:43:28 <jamesob> fanquake: cool!
19:44:08 <wumpus> fanquake:nice
19:45:32 <wumpus> any other topics?
19:47:00 <wumpus> #endmeeting