19:00:20 <wumpus> #startmeeting
19:00:20 <lightningbot> Meeting started Thu Apr  5 19:00:20 2018 UTC.  The chair is wumpus. Information about MeetBot at http://wiki.debian.org/MeetBot.
19:00:20 <lightningbot> Useful Commands: #action #agreed #help #info #idea #link #topic.
19:00:24 <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:00:26 <jonasschnelli> hi
19:00:29 <sipa> hi
19:00:31 <Murch> hi
19:00:35 <jamesob> yo
19:00:36 <BlueMatt> This week's high-priority-for-review stats: 11857 got a few rounds of review (me, ryanofsky and sjors), 12560 went horribly under-reviewed (with only two comments from me and one from jimpo this week, no real reviews!), 11775 got one round of review from jimpo (which I missed until today, sorry about that!). MVP: jimpo. Overall rating: Needs Significant Improvement (for everyone except jimpo, for jimpo: Good Job!).
19:00:36 <phantomcircuit> wat
19:00:39 <wumpus> hi
19:01:03 <meshcollider> Hi
19:01:08 <wumpus> #topic high priority for review
19:01:18 <cfields> hi
19:02:30 <BlueMatt> I mean no point nominating new things if its not gonna get any additional review. Might as well just directly ping jimpo and ask him to take a look.
19:02:36 <wumpus> well, I guess we need to keep it at the current list then, if the current ones don't get enough review we certainly shouldn't add more :)
19:02:39 <jnewbery> hi
19:02:57 <jamesob> This PR fixes a null pointer deref that's currently in master: https://github.com/bitcoin/bitcoin/pull/12836
19:03:16 <MarcoFalke> ^ Needs rebase
19:03:30 <achow101> hi
19:04:17 <wumpus> if something fixes an important issue such as a null pointer dereference (an existing one, not a potential one), please mention that in the PR title!
19:04:30 <instagibbs> achow101, if you rebase psbt I'd nominate it for high prio, not sure you have the time to carry it right now
19:04:41 <wumpus> "Make WalletInitInterface and DummyWalletInit private" really doesn't communicate that
19:04:51 <MarcoFalke> Also, those fixes should go in without having them to put on high-prio
19:04:53 <achow101> instagibbs: I'll try to do that later today or tomorrow
19:05:06 <wumpus> yes, apart from needing rebase it seems to have enough review to go in
19:05:18 <jonasschnelli> indeed
19:05:29 <meshcollider> wumpus: maybe he wasn't aware it fixed that
19:05:41 <wumpus> but please, don't hide fixes in refactor PRs
19:05:43 <kanzure> hi.
19:05:45 <wumpus> meshcollider: right , okay
19:06:18 <jnewbery> I think he wasn't aware of the bug that he fixed when he opened the PR
19:06:57 <wumpus> I see MarcoFalke already improved the title
19:07:16 <wumpus> #topic Slow unit test/Run unit tests in parallel
19:07:17 <cfields> jimpo: thanks for the reviews
19:07:49 <MarcoFalke> I thought that running the unit tests in parallel (similar to how the functional tests are run in parallel) is a free win
19:08:03 <jonasschnelli> MarcoFalke: is that possible with boost?
19:08:07 <MarcoFalke> Seems like they can be parallelized even on a single core
19:08:17 <jonasschnelli> https://www.boost.org/doc/libs/1_57_0/libs/test/doc/html/open-issues.html
19:08:18 <BlueMatt> yea, most of them use our globals in them
19:08:19 <cfields> MarcoFalke: i must admit, I kinda grumbled looking at your PR. Seems like it's really just a huge failure of the boost framework
19:08:26 <BlueMatt> we're a *long way* off from being able to do that, no?
19:08:26 <MarcoFalke> jonasschnelli: I adapted the google parallel tests wrapper
19:08:37 <wumpus> I hope it won't cause any ugly race conditions and such
19:08:44 <wumpus> we have so many intermittent travis failures as is :/
19:08:47 <MarcoFalke> BlueMatt: It works for me
19:08:51 <wumpus> at this point I'd prefer more stable tests to faster ones
19:08:52 <jonasschnelli> AFAIK boost test can't be run in parallel...
19:08:53 <MarcoFalke> at least locally
19:09:17 <MarcoFalke> You spin up different processes of course
19:09:20 <BlueMatt> oh, sorry, i didnt realize they were separate processes, was thinking no way in hell separate threads works
19:09:25 <wumpus> ohh smart
19:09:27 <cfields> jonasschnelli: iirc MarcoFalke's PR creates a wrapper that runs them individually, in parallel
19:09:34 <jnewbery> > I'd prefer more stable tests to faster ones
19:09:40 <jonasschnelli> PR #?
19:09:43 <MarcoFalke> like test_bitcoin -t wallet/t1 & test_bitcoin -t wallet/t2
19:09:48 <jnewbery> We need faster too! Travis PR builds are timing out all over the place
19:10:15 <MarcoFalke> jnewbery: that is a wine issue. Not sure if we can do much about it
19:10:21 <jonasschnelli> Yes. The amount of tests we added during the last year made SAS CI pretty hard
19:10:29 <MarcoFalke> I looked to realize I know not enough of wine to be of any use
19:10:32 <jamesob> not to mention the Travis backlog has been pretty deep lately
19:10:34 <wumpus> jnewbery: I was afraid of some race condition fest, but he spawns multiple processes, so that concern is gone
19:10:55 <achow101> what pr number?
19:11:07 <cfields> #12831
19:11:07 <wumpus> #12831
19:11:09 <gribble> https://github.com/bitcoin/bitcoin/issues/12831 | [WIP] Run unit tests in parallel by MarcoFalke · Pull Request #12831 · bitcoin/bitcoin · GitHub
19:11:11 <gribble> https://github.com/bitcoin/bitcoin/issues/12831 | [WIP] Run unit tests in parallel by MarcoFalke · Pull Request #12831 · bitcoin/bitcoin · GitHub
19:11:18 <MarcoFalke> Oh Chaincode Labs is willing to sponsor us additional 10 jobs for travis
19:11:32 <MarcoFalke> I hope that goes through until next week
19:11:37 <jonasschnelli> MarcoFalke: nice!
19:11:38 <sipa> pulling in parallel seems like huge overkill though
19:11:57 <jonasschnelli> That's what I just thought
19:12:12 <wumpus> MarcoFalke: nice, but does offering travis more money help? afaik what cfields said, it doens't
19:12:29 <cfields> didn't jeremy start on a replacement for boost_test at one point?
19:12:36 <cfields> yes, I've tried before, but by all means try again!
19:12:36 <wumpus> yes...
19:12:38 <MarcoFalke> wumpus: No, it will only increase the number of jobs
19:12:45 <MarcoFalke> So it clears the backlog faster
19:12:58 <jonasschnelli> wumpus: I guess money means going away from free OS travis to private support which seems to be slower even if you pay a lot? I may be wrong though.
19:12:59 <MarcoFalke> It doesn't increase the quality or anything
19:13:17 <MarcoFalke> jonasschnelli: No it is a out-of-band update
19:13:30 <wumpus> #8650
19:13:32 <gribble> https://github.com/bitcoin/bitcoin/issues/8650 | Make tests much faster by replacing BOOST_CHECK with FAST_CHECK by JeremyRubin · Pull Request #8650 · bitcoin/bitcoin · GitHub
19:13:36 <jonasschnelli> MarcoFalke: Okay. Good to know.
19:13:42 <jamesob> $$$ = more parallelism at the travis job level
19:13:47 <cfields> MarcoFalke: the issue that we had before is that they had no option for extra-paid open-source projects. Just paid and free. Maybe that's changed recently?
19:14:04 <MarcoFalke> cfields: I contacted the support
19:14:14 <MarcoFalke> They don't have anything listed on the public website/plans
19:14:34 <cfields> MarcoFalke: huh. I guess it's new then. Great :)
19:14:50 <MarcoFalke> apache or someone did it a few years ago, I am just trying the same
19:14:59 <meshcollider> Cool :)
19:15:03 <wumpus> great
19:15:06 <phantomcircuit> jamesob, i seem to remember the threshold for payed support being better than the free support for oss being pretty high
19:15:08 <sipa> hell yes, go for it
19:15:26 <jonasschnelli> 8650 looks after a huge win.
19:15:31 <MarcoFalke> Doing a wholesale replacement of the test framework seems not a short term solution and perpendicualr to running the tests in parallel
19:15:50 <jtimon> thanks chaincode for the travis jobs!
19:15:58 <wumpus> jtimon: +1
19:16:24 <MarcoFalke> 8650 seems like WIP
19:16:26 <wumpus> MarcoFalke: agree, would be a longer-term concern, if it can be done with boost test that's preferable
19:16:49 <cfields> MarcoFalke: I only mentioned it because it'll probably be done at some point anyway. And if so, we'd want to write it with parallelism in mind.
19:16:54 <wumpus> for now at least
19:16:59 <wumpus> 8650 loses boost test features
19:17:06 <sipa> MarcoFalke: i can't believe that what we need from parallel can't be done with 20 lines of bash
19:17:11 <wumpus> e.g. logging what values mismatch
19:17:34 <wumpus> sipa: yes - just list the test suites, then distribute them over processes
19:17:41 <jonasschnelli> agree
19:17:58 <MarcoFalke> I can't write bash, so someone else has to volunteer
19:18:00 <wumpus> sounds fairly doable in bash, or at least python
19:18:07 <sipa> MarcoFalke: i hereby volunteer
19:18:13 <MarcoFalke> the current thing is python
19:18:18 <aj> 20 lines of python sounds preferable...
19:18:23 <wumpus> python is preferable to me
19:18:27 <wumpus> at least I can help review and maintain it
19:18:28 <BlueMatt> ugh, y'all bash-haters
19:18:33 <MarcoFalke> aj: It has nice features such as a cache for the run times
19:18:34 <sipa> aj hereby volunteered :p
19:18:52 <MarcoFalke> sot the sorting would be done automatically and based on your specs
19:19:27 <sipa> MarcoFalke: ok, 22 lines of bash :)
19:19:34 <jonasschnelli> IMO the whole testing system is already pretty complex. I wouldn't set the burden higher
19:19:42 <MarcoFalke> sipa: Pull requests very welcome :)
19:19:50 <sipa> anyway, i'll see what i can do
19:20:05 <wumpus> ok, so we should look at 12831
19:20:18 <MarcoFalke> And the one sipa proposes
19:20:18 <sipa> #12831
19:20:21 <gribble> https://github.com/bitcoin/bitcoin/issues/12831 | [WIP] Run unit tests in parallel by MarcoFalke · Pull Request #12831 · bitcoin/bitcoin · GitHub
19:20:28 <MarcoFalke> #?????
19:20:34 <MarcoFalke> tba
19:20:41 <jamesob> at what grain does 12831 do parallelism? per file? boost test case?
19:20:52 <MarcoFalke> jamesob: Whatever you like
19:20:52 <sipa> jamesob: one test case per process
19:20:58 <MarcoFalke> Currently ^
19:20:58 <wumpus> per test suite, which is the only parallelism that makes sense
19:21:37 <jonasschnelli> I guess finer (case) would result in concurrency issue
19:21:43 <sipa> no...
19:21:47 <MarcoFalke> ^
19:21:56 <sipa> they're all in separate processes
19:22:04 <jonasschnelli> Have we made sure there are no dependencies between cases?
19:22:06 <sipa> concurrency doesn't even come into the pictire
19:22:09 <wumpus> that sounds like a ton of overhead
19:22:19 <wumpus> launcing a process for every test case
19:22:23 <sipa> wumpus: 250 process creations.
19:22:24 <achow101> cases should be independent of each other
19:22:25 <sipa> ?
19:22:27 <wumpus> yes
19:22:42 <aj> test suite is the file, test case is the function (and each case has many checks)
19:22:45 <jonasschnelli> achow101: Yes. But are they (ex. wallet test)?
19:23:06 <jonasschnelli> But however, suite is what we want not cases
19:23:11 <jonasschnelli> *suites
19:23:21 <cfields> so, the tests can be built as a library...
19:23:23 <MarcoFalke> The savings from --jobs=2 eat all the overhead from running in 250 processes
19:23:30 <wumpus> sounds like a better granularity to me too
19:23:42 <wumpus> in any case we need to get rid of the txt file with all the test cases
19:23:45 <wumpus> and generate that automatically
19:23:46 <meshcollider> Agree
19:23:49 <jonasschnelli> Yes.
19:24:02 <sipa> that seems easy
19:24:09 <jonasschnelli> (same should be done for the functional test IMO, *OT* though9
19:24:17 <wumpus> too easy to forget a test now
19:24:19 <sipa> we can grep for test cases/suites
19:24:46 <MarcoFalke> wumpus: If we keep the list it would be linted on travis of course. *ducks*
19:24:56 <cfields> not sure how it works, but if boost provides a reasonable api that let us fork() into each suite, we could write our own test_main.cpp to do so, no?
19:25:02 <wumpus> jonasschnelli: yes, there were plans for that too, embedding some metadata in a header at the top of the py files, and automatically generating the lists, but orthogonal :)
19:25:10 <wumpus> MarcoFalke: :-(
19:25:19 <jonasschnelli> +1 :(
19:25:36 <MarcoFalke> Fine
19:26:03 <jtimon> wumpus: if we have one dir with all the cases and only that, it should be simple, perhaps we want to maintain the list of extra ones to skip the slow suites by default
19:26:25 <wumpus> jtimon: there are some other parameters: sort order, and which list it goes into
19:26:40 <wumpus> jtimon: but anyhow off topic now
19:26:42 <MarcoFalke> Other topics
19:26:44 <jonasschnelli> topic proposal: multiwallet merge (luke-jr brought this up last time while I was not here)
19:26:44 <MarcoFalke> ?
19:26:54 <wumpus> #topic multiwallet GUI
19:27:03 <jonasschnelli> But I guess luke-jr is not here now
19:27:19 <jtimon> right, mhmm, I guess you can rename the tests starting with numbers to keep the order, but that's kind of ugly
19:27:33 <wumpus> cfields: I don't think doing it that way would change the challenges
19:27:37 <jonasschnelli> I heard that the merge of #12610 was done while it was still controversial...
19:27:39 <gribble> https://github.com/bitcoin/bitcoin/issues/12610 | Multiwallet for the GUI by jonasschnelli · Pull Request #12610 · bitcoin/bitcoin · GitHub
19:27:47 <wumpus> jonasschnelli: I'm happy that you merged it
19:27:55 <jonasschnelli> If I did so, my appologies. I only wanted to make progress.
19:27:58 <cfields> ok
19:27:59 <wumpus> if there's anything to be fixed, file a new PR
19:28:12 <jonasschnelli> PRs to fix design mistakes are welcome
19:28:17 <jonasschnelli> yes
19:28:29 <wumpus> luke-jr overblows that part imo
19:28:52 <meshcollider> Yeah it's good that something has been put in, it's been weeks of small disagreement holding it up
19:28:53 <jnewbery> jonasschnelli: better to get it in now so it has time to soak. Rough edges can be fixed in future PRs
19:29:14 <jonasschnelli> I overhauled luke-jr's version mainly because of things like this: https://github.com/bitcoin/bitcoin/pull/11383/files#diff-2c15c5b52f35ea388ebab757eaab0f1cR506
19:29:28 <wumpus> but in any case, the idea of open source software is collaborative development, we can't make progress with something like this if it stays a PR forever, and it had a quite lot of review IIRC
19:29:32 <jonasschnelli> Erm this: https://github.com/bitcoin/bitcoin/pull/11383/files#diff-2c15c5b52f35ea388ebab757eaab0f1cR903
19:29:55 <wumpus> yes
19:29:58 <jonasschnelli> Yes. I took also care to keep luke-jr authorship in commits during my overhaul.
19:30:39 <jonasschnelli> Okay. Done with that topic then. Thanks
19:30:42 <wumpus> so it's ok, any other topics?
19:31:08 <sipa> let me think
19:31:26 <jnewbery> I had a topic: merge #10244
19:31:32 <jnewbery> but wumpus already did
19:31:32 <gribble> https://github.com/bitcoin/bitcoin/issues/10244 | Refactor: separate gui from wallet and node by ryanofsky · Pull Request #10244 · bitcoin/bitcoin · GitHub
19:31:39 <jamesob> woo!
19:31:40 <sipa> i don't have anything
19:31:48 <wumpus> yes, congrats :)
19:31:53 <jnewbery> \o/
19:31:55 <sipa> yay
19:31:58 <jonasschnelli> nice! Yes.
19:32:01 <MarcoFalke> Good to see that in
19:32:05 <BlueMatt> dont forget to review High-Priority PRs this week!
19:32:14 <BlueMatt> </meeting>?
19:32:21 <MarcoFalke> #action  dont forget to review High-Priority PRs this week!
19:32:44 <wumpus> #endmeeting