19:00:21 <meshcollider> #startmeeting
19:00:21 <lightningbot> Meeting started Fri Aug  2 19:00:21 2019 UTC.  The chair is meshcollider. Information about MeetBot at http://wiki.debian.org/MeetBot.
19:00:21 <lightningbot> Useful Commands: #action #agreed #help #info #idea #link #topic.
19:00:27 <meshcollider> #bitcoin-core-dev Wallet 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
19:00:32 <provoostenator> hi\
19:00:32 <achow101> hi
19:00:35 <sipa> hi
19:01:06 <ariard> hi
19:01:07 <meshcollider> any topics? :)
19:01:09 <jonatack> hi
19:01:27 <jnewbery> hi
19:01:39 <achow101> Big news is #16528 opened last night for native descriptor wallets
19:01:42 <gribble> https://github.com/bitcoin/bitcoin/issues/16528 | [WIP] Native Descriptor Wallets (take 2) by achow101 · Pull Request #16528 · bitcoin/bitcoin · GitHub
19:01:52 <achow101> only 107 commits :p
19:01:53 <ariard> working on a rework of rescan logic https://gist.github.com/ariard/89f9bcc3a7ab9576fc6d15d251032cfa
19:02:14 <sipa> achow101: wow
19:02:17 <ariard> thanks to feedbacks design from ryanosfky
19:02:40 <sipa> ariard: tip for remembering his nickname: ryan of sky
19:02:42 <ariard> s/ryanosfky/ryanofsky/g
19:02:53 <ariard> sipa: already heard this one
19:03:12 <meshcollider> achow101: The 107 commits include the legacy rework PR right :p
19:03:18 <achow101> <provoostenator> For todays wallet meeting, it would be good to brainstorm if there's a way to avoid all the [ci skip] commits in #16341.
19:03:21 <gribble> https://github.com/bitcoin/bitcoin/issues/16341 | Introduce ScriptPubKeyMan interface and use it for key and script management (aka wallet boxes) by achow101 · Pull Request #16341 · bitcoin/bitcoin · GitHub
19:03:22 <achow101> meshcollider: yes
19:03:29 <kanzure> hi
19:03:41 <achow101> I think provoostenator has the only topic suggestion today
19:03:48 <provoostenator> Yes, maybe it's impossible, but's pretty daunting to review otherwise
19:04:03 <meshcollider> Let's start with that then
19:04:26 <achow101> I had a look at doing that, and it gave me a headache
19:04:47 <meshcollider> #topic Avoiding the [ci skip]s in #16341
19:04:49 <gribble> https://github.com/bitcoin/bitcoin/issues/16341 | Introduce ScriptPubKeyMan interface and use it for key and script management (aka wallet boxes) by achow101 · Pull Request #16341 · bitcoin/bitcoin · GitHub
19:05:11 <achow101> the problem is that a lot of functions are dependent on other ones, so there would need to be a pretty big commit just to add those
19:05:39 <meshcollider> Yeah I thought the whole reason you put them there in the first place was because we decided it was better than one massive commit that doesn't break stuff
19:05:41 <achow101> then as you add them into the wallet, things start becoming inconsistent if not all of the things that can effect each other are also added at the same time. these inconsistencies also result in test failures
19:06:52 <achow101> One idea I had was to combine implementation of the function in LegacyScriptPubKeyMan and replacement in CWallet, but still leave the CWallet stuff there to operate in parallel
19:07:50 <provoostenator> That's what I was thinking as well
19:07:51 <achow101> but that seemed kind of pointless because the new code wasn't really doing anything, and we would still need to have the gigantic "delete all the old things" commit at the end
19:08:07 <provoostenator> Maybe start with a bunch dummy methods in the LegacyScriptPubKeyMan and then move stuff over?
19:08:38 <provoostenator> Then in the end you still need to delete some calls, but no implementations.
19:08:39 <achow101> there is also some consistency issues with doing that because the wallet files could be overwritten by two different code paths and become inconsistent, also failing tests
19:08:41 <meshcollider> It's the moving which is the problem
19:08:43 <elichai2> crazy idea. PR into PR? (that way they could be reviewed seperatly but merged into master together)
19:09:31 <sipa> we must go deeper.
19:09:39 <achow101> elichai2: they're all separate commits, so it shouldn't actually be that hard to review separately
19:10:44 <achow101> provoostenator: I don't believe that there is a way to avoid having [ci skip] commits because tests will fail
19:11:29 <meshcollider> Ease of review > Travis passing on every single commit, as long as the tests pass at the end of the PR I think it's fine :)
19:11:40 <provoostenator> meshcollider: agree
19:11:58 <achow101> a lot of the commits are really small too
19:12:21 <provoostenator> Currently the dimmed-zebra can't help
19:12:36 <provoostenator> Which makes it super tedious to check if the new function bodies are the same
19:12:41 <sipa> having intermediary commits that pass CI is helpful for review, as you can be sure about things like "this method changes, did all call sites get updated too?" without needing much work
19:13:12 <achow101> having them all pass tests would be nice, makes bisect not suck
19:13:19 <sipa> yeah
19:13:32 <sipa> not saying it's a strict requirement, though it's certainly very helpful as a policy
19:13:47 <achow101> I spent a lot of time finding a bug that was introduced in one of the [ci skip] commits but bisect wasn't as helpful since the tests wouldn't get to the part where the bug was triggered
19:14:54 <achow101> but even then, having them all pass tests wouldn't be helpful since the new code isn't the one that's actually doing the work. it would still just be the old stuff and bisecting would just point you to the gigantic "change everything over" commit
19:15:24 <jonatack> at least the [ci skip] comments signal when the failure is to be expected, so there's that... but from a review perspective hygienic commits are much better
19:15:58 <jonatack> commit messages could contain the steps
19:17:53 <meshcollider> achow101: can CWallet just have an instance of a LegacyScriptPubkeyManager inside it right from the start and call methods inside it as they're moved into it?
19:18:24 <meshcollider> And then do some inheritance refactoring at the end
19:18:54 <meshcollider> Then the massive "move everything over" commit would just be deleting methods like foo(){ legacy.foo(); }
19:19:25 <achow101> no because methods are dependent on other ones
19:20:44 <achow101> for example, I thought that AddKey would be easy, it should be mostly by itself, right? Nope. Requres AddCryptedKey to be implemented, which requires Encryption stuff to be implemented. For some reason it also requires watch only things so you need to have setWatchOnly, HaveWatchOnly, RemoveWatchOnly
19:21:13 <achow101> and you need AddKey for loading keys in the first place!
19:21:32 <sipa> fun.
19:22:44 <meshcollider> Yeah... I think the [ci skip] are here to stay then
19:23:06 <meshcollider> Does anyone else want to discuss anything this week?
19:24:07 <achow101> (If you couldn't tell, I was very annoyed by this and spent a while cursing the person who wrote this code, which was probably sipa :p)
19:24:16 * sipa hides.
19:25:31 <ariard> meshcollider: if anyone has thoughts on this https://gist.github.com/ariard/89f9bcc3a7ab9576fc6d15d251032cfa it would be welcomed
19:25:53 <ariard> (not necessarily now)
19:26:25 <achow101> it would be nice for rescan (and other wallet things) to not need cs_main
19:26:31 <meshcollider> #action Read and feedback on ariard's proposal
19:26:39 <ariard> main idea is just to use a common thread between index and ChainClient to provide blocks in one sequence instead of redoing the work multiple times
19:26:57 <achow101> there's been a lot of complaints I've seen recently about RPCs taking a while, probably because cs_main is held by ~everything
19:26:58 <ariard> achow101: yes that's the end of goal, not relying at all on cs_main
19:27:15 <ariard> but will need multiple PRs to get there to avoid to big diff
19:27:52 <ariard> first step move all logic in a thread, future steps  worker thread pool + cache what we need to avoid locking
19:28:47 <ariard> hope to submit code next week, people will get a better idea
19:29:03 <emilengler> By the way, what does the LOCK and cs_main mean? I see this nearly everywhere when I look in the code
19:29:05 <meshcollider> ariard: sounds good!
19:29:09 <emilengler> And why it needs to be in the curly brackets
19:30:03 <achow101> emilengler: cs_main is one of the locks, while one thread holds it, no other thread can acquire it. it is used to protect things from concurrency issues like race conditions. LOCK(cs_main) is the way to acquire cs_main, if it is held by someone else, the thread will wait
19:30:04 <sipa> emilengler: RAII
19:30:17 <meshcollider> emilengler: cs_main is something which only one thread can "hold" at once, making sure stuff which it protects isn't written twice at the same time or something which would result in race conditions
19:30:26 <sipa> emilengler: it introduces a lock in the scope it's defined at; it automatically unlocks when it goes out of scope
19:30:36 <sipa> emilengler: it's a general concept in C++ called RAII
19:30:39 <ariard> have a look in sync.h and threadsafety.h
19:31:13 <meshcollider> Anyway I think this wraps up the meeting
19:31:35 <emilengler> Ok so it is more a C++ thing? I usually don't work that often with multiple threads
19:31:47 <meshcollider> We can read the proposal and PR outside
19:31:49 <meshcollider> #endmeeting