19:01:13 <wumpus> #startmeeting
19:01:13 <lightningbot> Meeting started Thu Feb 15 19:01:13 2018 UTC.  The chair is wumpus. Information about MeetBot at http://wiki.debian.org/MeetBot.
19:01:13 <lightningbot> Useful Commands: #action #agreed #help #info #idea #link #topic.
19:01:34 <wumpus> #bitcoin-core-dev Meeting: wumpus sipa gmaxwell jonasschnelli morcos luke-jr btcdrak sdaftuar jtimon cfields petertodd kanzure bluematt instagibbs phantomcircuit codeshark michagogo marcofalke paveljanik NicolasDorier jl2012 achow101 meshcollider jnewbery maaku fanquake promag provoostenator
19:02:03 <cfields> hi
19:02:18 <wumpus> we're almost ready to tag rc4, the still open PRs could still use some review https://github.com/bitcoin/bitcoin/pulls?q=is%3Aopen+is%3Apr+milestone%3A0.16.0
19:02:32 <promag> hi
19:02:39 <kanzure> hi.
19:03:20 <luke-jr> hi
19:03:45 <cfields> I'm not sure if I'm happy to see people testing the RCs, or scared of how many things keep cropping up :\
19:03:55 <BlueMatt> heh
19:04:02 <BlueMatt> yea
19:04:05 <wumpus> I'd like to discuss #12349
19:04:07 <gribble> https://github.com/bitcoin/bitcoin/issues/12349 | shutdown: fix crash on shutdown with reindex-chainstate by theuni · Pull Request #12349 · bitcoin/bitcoin · GitHub
19:04:20 <BlueMatt> i hate that fix, but it should be sufficient :(
19:04:37 <wumpus> #topic reindex-chainstate crash
19:04:44 <sipa> present
19:04:57 <wumpus> so we committed a fix for that in an earlier rc, but apparently the issue still exists
19:05:17 <BlueMatt> yes, the fix was for a highly-related bug
19:05:52 <BlueMatt> well, ok, essentially, either the same crash triggered by doing reindex, or the crash triggered by simply quickly quitting
19:06:01 <BlueMatt> so its already fixed for fast-quit
19:06:06 <wumpus> is it clear what the issue is in the first place? it seems it's causing workarounds to be spread over the code to 'fix' it
19:06:16 <wumpus> I'm not really happy to see that
19:06:33 <BlueMatt> yes, the issue is that we call Flush during shutdown before we've loaded genesis block
19:06:36 <cfields> wumpus: I just commented on the PR, I'll re-paste here for a little context:
19:06:42 <cfields> "As a small data point, though, we believed this to be qt only and couldn't hit it with rc3. It was @eklitzke's bitcoind backtrace that made it easy to reproduce."
19:06:51 <BlueMatt> so the flush assert(false)s because we have no "tip block" which we are flushing towards
19:07:03 <BlueMatt> huh? no?
19:07:14 <BlueMatt> I did not believe it to only be bitcoin-qt?
19:07:19 <BlueMatt> not sure where you got that
19:07:28 <wumpus> oh, and the flush assumes that the genesis block is there?
19:08:00 <BlueMatt> the flush assumes there is a tip block, including gensis
19:08:09 <promag> bitcoind crashed too
19:08:25 <MarcoFalke> ^
19:08:35 <cfields> BlueMatt: huh, I was definitely working under that impression
19:08:46 <BlueMatt> there are several largely-unrelated bugs in qt
19:08:56 <BlueMatt> and there were other init-fast-shutdown-crash bugs in bitcoind
19:09:02 <cfields> not anymore ofc, now that we can hit it with bitcoind.
19:09:06 <BlueMatt> that trying to reproduce this and other issues dug up
19:09:22 <cfields> right, ok
19:09:58 <wumpus> there's no automatic test coverage for these paths, so problems are only found incidentally
19:10:17 <BlueMatt> yes, one thing I did during testing was just make ShutdownRequested() start shutdown after being called N times
19:10:21 <BlueMatt> and just restart with an incrementing N
19:10:31 <BlueMatt> this is something we could automate, but would largely not have caught the qt issues
19:10:42 <wumpus> nice idea
19:10:55 <wumpus> no, testing qt is a whole different can of worms
19:11:19 <promag> true
19:11:55 <wumpus> we solved some long-running bugs there recently :)
19:12:07 <promag> but do we care to flush if shutdown is requested?
19:12:11 <cfields> BlueMatt: I like that too. We'd just want to remember to grind hard on the tests before release
19:12:18 <promag> (on init)
19:12:19 <BlueMatt> to make it actually have good coverage we need to drop all direct accesses to fShutdownRequested and replace with ShutdownRequested()
19:12:25 <BlueMatt> which is why I missed the current incantation of the bug
19:12:34 <BlueMatt> cfields: its super fast, you could run it in travis
19:12:41 <MarcoFalke> I can do that
19:13:23 <wumpus> drop all direct accesses to fShutdownRequested and replace with ShutdownRequested() -> that sounds like a good idea for encapsulation in any case
19:13:29 <BlueMatt> yes
19:13:47 <cfields> can optimize the atomic too :p
19:14:01 <promag> what does that solve?
19:14:09 <BlueMatt> anyway, as for the current bug, I think #12349 is likely fine for 0.16, though I'd prefer it skips more of the FlushStateToDisk codepaths in the case that we've clearly never written anything we need to flush (morcos just pointed out it'd be nice to skip the wallet best chain setting, though unlikely thats a bug)
19:14:11 <gribble> https://github.com/bitcoin/bitcoin/issues/12349 | shutdown: fix crash on shutdown with reindex-chainstate by theuni · Pull Request #12349 · bitcoin/bitcoin · GitHub
19:14:23 <promag> maybe we should replace atomic var with a mutex? so that some blocks can hold the mutex and prevent others to continue?
19:14:52 * BlueMatt has non-x86 hardware coming soon (I think there will likely be a dev box or two available for people who want to test/work on core on powerpc at that time as well), it'd be great to see more atomic relaxations =D
19:15:13 <BlueMatt> promag: wat, why would you do that?
19:15:23 <wumpus> looks like problem that has spooked us for a long time, that much of the code cannot cope without genesis block set
19:15:23 <cfields> promag: that's a different long discussion :). It was really just a joke in this context
19:15:52 <wumpus> it should only be a problem in shutdown() because we don't exit AppInit() succesfully in that ase
19:16:03 <wumpus> AppInitMain()
19:16:10 <BlueMatt> wumpus: yes, one other thing to be done is to move fucking ThreadImport up inside init, but  regular old reindex is hard to work with to fix this issue
19:16:22 <BlueMatt> unless you want to re-introduce the bug where we write a copy of genesis every time we load.......
19:16:26 <wumpus> but other sneaky things come up all the time
19:16:40 <promag> BlueMatt: because once you request the shutdown other threads can start the tear down, which can mess other stuff
19:16:57 <BlueMatt> promag: shutdown can happen at any point, you shouldnt be able to block shutdown by taking a lock.....
19:17:01 <wumpus> BlueMatt: let's at least have a test for that, that if someone regresses that we'd at least detect it
19:17:05 <BlueMatt> subsystem separation should work.......
19:17:34 <BlueMatt> wumpus: yes, agreed....didnt MarcoFalke just say he'd build it? =D
19:17:36 <cfields> BlueMatt: yea, my earlier iterations skipped more of FlushStateToDisk, but I figured it was best to not tangle that logic up with init's
19:17:41 <wumpus> promag: we don't really have that issue afaik
19:18:14 <wumpus> but ok, then we keep #12349 for 0.16
19:18:15 <BlueMatt> cfields: hmm, ugh
19:18:16 <gribble> https://github.com/bitcoin/bitcoin/issues/12349 | shutdown: fix crash on shutdown with reindex-chainstate by theuni · Pull Request #12349 · bitcoin/bitcoin · GitHub
19:18:31 <BlueMatt> cfields: cant we literally skip the entire function in that case?
19:18:36 <BlueMatt> we clear the dbs on load themselves
19:18:40 <wumpus> please pray let it be fixed now
19:18:43 <BlueMatt> so its not like we have anything at all whatsoever to flush
19:18:46 <wumpus> :P
19:18:50 <BlueMatt> lol
19:19:06 <cfields> BlueMatt: yes, but that involves locking changes
19:19:14 <cfields> (possibly only one teeny tiny one)
19:19:20 <wumpus> noooooo
19:19:37 <BlueMatt> ugh, I still want to go back to my CChainState::fWeveDoneAnythingAtAll idea
19:19:39 <wumpus> we saw what happened last time we tried "locking changes" :-)
19:19:39 <cfields> right, hence _this_ approach :)
19:19:42 <BlueMatt> and just insert that and skip flush if its false
19:19:50 <promag> next topic suggestion #11913
19:19:52 <gribble> https://github.com/bitcoin/bitcoin/issues/11913 | Avoid cs_main during ReadBlockFromDisk Calls by TheBlueMatt · Pull Request #11913 · bitcoin/bitcoin · GitHub
19:19:57 <BlueMatt> wumpus: yea, we ended up with a super-long 0.16 release process.....sorry :(
19:20:01 <cfields> BlueMatt: +1 for master. Completely agree.
19:20:19 <wumpus> BlueMatt: it's not your fault, the code is just completely crazy in that regard
19:20:34 <BlueMatt> wumpus: no, I meant the validationinterface queue garbage, that had a few last-minute bugs as well
19:21:08 <wumpus> promag: sure
19:21:15 <wumpus> #topic Avoid cs_main during ReadBlockFromDisk Calls
19:21:30 <BlueMatt> what about it?
19:21:49 <wumpus> yes, what about it
19:21:51 <BlueMatt> I mean needs a ton of rebase due to merging of a dep, but I can do that whenever folks are ready to review
19:22:09 <wumpus> it's obviously a concept ack
19:22:13 <promag> is this going forward?
19:22:24 <promag> I mean, it's in priority review
19:22:27 <BlueMatt> I dont see why not, I mean my name's on the tin, but...
19:22:54 <wumpus> it's not going backward at least!
19:22:57 <BlueMatt> oh, you're saying I should rebase
19:22:58 <BlueMatt> yea, k
19:23:06 <BlueMatt> I mean IIRC it shoudl be reviewable if you just ignore the first few commits
19:23:10 <BlueMatt> which are since merged
19:23:11 <BlueMatt> but whatever
19:23:23 <promag> like that, there are others that avoid cs_main
19:23:24 <BlueMatt> it appears I have two things in high-priority though, which is not ok
19:23:25 <wumpus> if some commits are merged you can say it has been going forward
19:23:48 <BlueMatt> anyway, next topic?
19:23:57 <wumpus> any other topics?
19:24:15 <BlueMatt> Bitcoin!
19:24:19 <sipa> Bitcoin!
19:24:19 <achow101> encrypting wallets without restarting
19:24:39 <wumpus> what about it?
19:25:00 <cfields> wumpus: It's this new cryptocurrency...
19:25:07 <sipa> LOL
19:25:07 <wumpus> aren't bitcoins those big, golden coins with 1's and 0's on them?
19:25:25 <wumpus> I think they're too heavy to ever be practical
19:25:42 <cfields> haha
19:25:44 <wumpus> #topic encrypting wallets without restarting (achow101)
19:25:57 <achow101> relevant PR is #11678
19:26:00 <gribble> https://github.com/bitcoin/bitcoin/issues/11678 | [wallet] Dont shut down after encrypting the wallet by achow101 · Pull Request #11678 · bitcoin/bitcoin · GitHub
19:26:04 <BlueMatt> yea, you have to carry around a computer to spend them....arent those those things that take up like a whole basement and you have to hire a few people to maintain it?
19:26:46 <achow101> there's a really really really ugly hack in that pr to make it work from the rpcconsole
19:27:21 <achow101> but I'm kinda stuck on a better way to let people encrypt from the rpcconsole and not have the gui get all screwed up
19:28:19 <promag> achow101: with dynamic wallet loading this should be easier?
19:28:23 <wumpus> BlueMatt: a computer?! oh no, I heard there wil never be demand for more than 10 of them in the whole world
19:28:27 <achow101> promag: I don't think so
19:28:28 <kanzure> what is the problem with a restart?
19:28:54 <achow101> I noted in the dynamic reloading pr that it actually has a similar problem if you try to reload the wallet that is currently used by the gui
19:28:56 <wumpus> kanzure: for multiwallet it can be kind of annoying, I guess
19:29:13 <wumpus> kanzure: if you want to encrypt 10 wallets and need to restart 10 times
19:29:22 <BlueMatt> I mean, yea, question is how it gets handled in dynamic multiwallet gui, handle it the same way........
19:29:38 <BlueMatt> all that code is gonna have to get written one way or another
19:29:48 <BlueMatt> de-init'ing the whole gui wallet stuff and re-init'ing it is gonna have to exist
19:29:52 <achow101> BlueMatt: the thing is, dynamic multiwallet doesn't handle it at all
19:29:55 <BlueMatt> not that I have any opinion on *how*
19:30:04 <cfields> maybe an unpopular opinion, but forcing the inconvenience of a shutdown for this doesn't seem terrible to me...
19:30:08 <BlueMatt> achow101: well that just sounds downright broken
19:30:11 <kanzure> wumpus: perhaps just do it all at once and do only one restart.
19:30:20 <BlueMatt> cfields: see wumpus note on encrypting multiple wallets
19:30:21 <wumpus> cfields: I don't think it's particularly urgent either, no
19:30:36 <cfields> in fact, seems like it should really be handled by a tool outside of bitcoind
19:30:39 <wumpus> but I never encrypt wallets so don't listen to me
19:30:52 <BlueMatt> sounds good, lets remove encrypted wallet support!
19:31:13 <promag> achow101: it needs fixing then, between dyn wallet, your pr and #11383
19:31:15 <wumpus> (well obviously I encrypt backups, but I don't use bitcoin's encrypted wallet stuff)
19:31:17 <gribble> https://github.com/bitcoin/bitcoin/issues/11383 | Basic Multiwallet GUI support by luke-jr · Pull Request #11383 · bitcoin/bitcoin · GitHub
19:31:23 <sipa> cfields: i think that we're struggling to do so is probably a sign of some problems internally, regardless of whether it's a desired feature or not
19:31:31 <achow101> My plan for doing wallet encryption by default needed to do something like this, so I went and did it as a separate thing
19:31:47 <wumpus> but yes, once we have dynamic wallet unloading and loading, it shoudl be straightforward
19:31:48 <BlueMatt> sipa: that was my point...we're gonna have to handle it wrt dynamic multiwallet
19:31:50 <wumpus> so focus on that first
19:31:53 <achow101> but I kinda got stuck at how the hell to get the rpcconsole to tell the gui to reload the wallet
19:32:00 <sipa> BlueMatt: yup, agree
19:32:04 <wumpus> just unload the wallet ,resilver/rewrite it, then reopen it, voila
19:32:07 <cfields> sipa: fair point.
19:32:15 <promag> wumpus: +1
19:32:22 <achow101> wumpus: that's what I thought. but then qt got in the way
19:32:23 <promag> the gui should react
19:32:31 <promag> some signals etc
19:32:32 <wumpus> achow101: GUI should subscribe to wallet unload events
19:32:36 <achow101> wumpus: qt and the rpcconsole donn't interact particularly well
19:32:40 <achow101> apparently
19:32:42 <wumpus> achow101: and delete the associated state from the GUI
19:32:50 <wumpus> promag: yes exactly
19:33:04 <BlueMatt> achow101: I'm confused, so this is an open issue to solve for dynamic multiwallet gui, no?
19:33:12 <achow101> BlueMatt: it is
19:33:24 <BlueMatt> ok, so the question is how to handle it properly in that case?
19:33:27 <wumpus> wallets have an associated GUI object (walletmodel) which then needs to be deleted, as well as the tabs/other stuff associated with that wallet
19:33:27 <promag> what's the deal with rpcconsole? because of the target wallet of wallet commands?
19:34:07 <achow101> BlueMatt: the issue that needs to be handled is how to get the gui to reload a wallet from actions that happened over rpc
19:34:08 <wumpus> how is the console a problem? does it care about wallets?
19:34:13 <promag> I think #11383 deals with changing wallets, it's a matter to change to null wallet
19:34:16 <gribble> https://github.com/bitcoin/bitcoin/issues/11383 | Basic Multiwallet GUI support by luke-jr · Pull Request #11383 · bitcoin/bitcoin · GitHub
19:34:17 <wumpus> it just interprets commands and sends them to the RPC interface IIRC
19:34:28 <wumpus> oh the wallet selectbox thing
19:34:38 <wumpus> yes, that needs to listen to wallet removal events too, then...
19:34:41 <achow101> wumpus: things that happen over rpc need to be reflected in the gui
19:35:06 <wumpus> so I'd say first implement it in the RPC
19:35:08 <kanzure> are the relevant qt signal hooks not implemented..?
19:35:12 <wumpus> then later make a PR to do it for the GUI
19:35:13 <promag> both gui and rpc should load/unload wallets in the same place, and only then the gui reacts
19:35:23 <wumpus> that's usually the order in which we do things
19:35:44 <achow101> wumpus: but then people may use the rpcconsole which just does the rpc things, and that will just break the gui.
19:35:47 <achow101> it segfaults
19:35:49 <wumpus> GUI is usually a hairier business, and needs different reviewers
19:36:03 <wumpus> oh yes it should certainly not segfault..
19:36:32 <achow101> e.g. for the encrypting the wallet without restarting, it works over rpc and in the gui if you encrypt from gui
19:36:40 <achow101> but if you call encryptwallet from the rpcconsole, it will segfault
19:37:06 <achow101> because rpc cannot tell gui to reload the wallet
19:37:12 <wumpus> so that's because we don't have the hooks in place for dynamic wallet loading/unloading yet
19:37:14 <wumpus> we need that first
19:37:16 <achow101> yes
19:37:35 <promag> rpc and gui don't know each other
19:37:38 <achow101> I guess the question is really *how*
19:37:46 <achow101> since rpc and gui don't really talk to each other
19:37:49 <wumpus> ay other topics?
19:37:58 <wumpus> achow101: using signals, conencting the GUI to them
19:37:59 <promag> achow101: let's discuss this later
19:38:18 <wumpus> achow101: the same as it works for other ClientModel. WalletModel signals
19:38:33 <jnewbery> dynamic wallet loading (#10740) is very much a work in progress. I haven't touched it for a while because it felt like we would want multiwallet in the GUI to go in first
19:38:36 <gribble> https://github.com/bitcoin/bitcoin/issues/10740 | [WIP] [wallet] dynamic loading/unloading of wallets by jnewbery · Pull Request #10740 · bitcoin/bitcoin · GitHub
19:39:02 <wumpus> so there's a signal WalletAboutToBeUnloaded, and a signal NewWallet, the GUI can handle those and take appropriate action
19:39:04 <promag> jnewbery: right, that was the last conclusion iirc
19:40:10 <wumpus> other topics?
19:40:14 <kanzure> please send me your topic suggestions for my collection for march things, including short-term (like specific issues or merge requests) and long-term (future stuff)
19:40:59 <wumpus> #action please send kanzure your topic suggestions for my collection for march things, including short-term (like specific issues or merge requests) and long-term (future stuff)
19:41:28 <kanzure> i could also just randomly assign issues :-)
19:41:38 <cfields> kanzure: thanks for doing that
19:41:55 <luke-jr> #12208 should still be in 0.16, somehow not tagged/merged yet
19:41:57 <gribble> https://github.com/bitcoin/bitcoin/issues/12208 | GUI: Rephrase Bech32 checkbox texts, and enable it with legacy address default by luke-jr · Pull Request #12208 · bitcoin/bitcoin · GitHub
19:42:15 <achow101> any news on the mpc rsa signing key?
19:43:07 <wumpus> luke-jr: doing a string change in a rc would be really unwise, we can tag it 0.16.1 though
19:43:21 <luke-jr> wumpus: more unwise than having confusing strings?
19:43:27 <wumpus> luke-jr: yes
19:43:40 <wumpus> it's too late, translators have no chance to even look at it anymore
19:43:54 <luke-jr> could copy the translations to it, but whatever
19:44:18 <luke-jr> long-term, I guess that could result in confusing translations persisting XD
19:44:54 <cfields> I'm sure we'll learn a lot about bech32 confusion with 0.16.0. We'll be in better shape to clean it up for 0.16.1 with that feedback, I think.
19:45:01 <wumpus> &
19:45:04 <wumpus> ^
19:45:29 <luke-jr> well, as-is, it seems to affirm the myth that there is a from address
19:45:44 * booyah can translate whatever string to PL if anyone needs quickly just ping me
19:46:22 <booyah> if it's like 1 string I bet on #bitcoin + reddit community has someone ready for any lang
19:46:48 <achow101> meta topic: meeeting notes
19:47:02 <wumpus> #topic meeting notes
19:47:18 <achow101> the person I got to write the meeting notes got bored with it so they aren't happening anymore :(
19:47:35 <sipa> can't blame them, i guess..
19:47:36 <wumpus> oh :(
19:48:06 <cfields> sex.
19:48:15 <cfields> there, fixed. next topic?
19:48:29 <wumpus> heh
19:48:36 <wumpus> I think we're out of topics
19:48:53 <wumpus> #endmeeting